gci+goimports = duplicated imports

[howardjohn]

Config:

linters:
  disable-all: true
  enable:
  - goimports
  - gci
  fast: false

linters-settings:
  goimports:
    local-prefixes: istio.io/

Input:

package main

import (
	"fmt"
	"istio.io/istio/pkg/test/framework/resource"
	"os"

	"istio.io/istio/pkg/test/framework/resource"
)

var (
	_ = resource.Cluster
	_ = fmt.Println
	_ = os.Stdout
)

Output:

package main

import (
	"fmt"
	"os"

	"istio.io/istio/pkg/test/framework/resource"
	"istio.io/istio/pkg/test/framework/resource"

	"istio.io/istio/pkg/test/framework/resource"
)

var (
	_ = resource.Cluster
	_ = fmt.Println
	_ = os.Stdout
)

Output seems nondeterministic

cc @daixiang0

[howardjohn]

Input:

import (
	"fmt"
	"os"

	"istio.io/istio/pkg/test/framework/resource"
	"istio.io/istio/pkg/test/framework/resource"
	"istio.io/istio/pkg/test/framework/components/prometheus"
)

Output:

import (
	"fmt"
	"os"

	"istio.io/istio/pkg/test/framework/components/prometheus"
)

[howardjohn]

Also can only reproduce through golangci-lint, not direct calls on goimports -local istio.io -w main.go; gci -local istio.io -w main.go

[ldez]

I tried with different cases:

import (
	"io/ioutil"
	"github.com/golangci/sandbox/pkg/render"
	"github.com/ghodss/yaml"
	"log"
	"encoding/json"

	"github.com/golangci/sandbox/pkg/router"
)

import (
	"github.com/golangci/sandbox/pkg/render"
	"github.com/golangci/sandbox/pkg/router"
	"encoding/json"
	"io/ioutil"
	"log"
	"github.com/ghodss/yaml"
)

and all the linters that handle the imports (gofmt, goimports, gci, gofumpt).

Results:

linters	results
gofmt + goimports	FAIL
gofmt + gci	FAIL
goimports + gci	FAIL
goimports + gofumpt	FAIL
gofmt + gci + gofumpt	FAIL
gofmt + goimports + gci + gofumpt	FAIL
gci + gofumpt	OK
gofmt + gofumpt	OK

I think that the problem is not related to a specific linter but to how the fixes are applied.

[dbraley]

I'm also seeing this problem. I'll add that with a larger set of imports and more randomization, I'm able to reproduce the failure with the gci + gofumpt and gofmt + gofumpt combinations as well.

[danielhochman]

I've started debugging this issue. I'll try to post more as I have time to dig into the results.

Input

import (
	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/status"
	"context"
	"errors"
	"fmt"

	"github.com/uber-go/tally"
	"go.uber.org/zap"
	"google.golang.org/grpc"
	"google.golang.org/protobuf/proto"
	"google.golang.org/protobuf/types/known/anypb"

	auditv1 "github.com/lyft/clutch/backend/api/audit/v1"
	"github.com/lyft/clutch/backend/gateway/log"
	"github.com/lyft/clutch/backend/gateway/meta"
	"github.com/lyft/clutch/backend/middleware"
	"github.com/lyft/clutch/backend/service"
	auditservice "github.com/lyft/clutch/backend/service/audit"
	"github.com/lyft/clutch/backend/service/authn"
)

Expected Output

import (
	"context"
	"errors"
	"fmt"

	"github.com/uber-go/tally"
	"go.uber.org/zap"
	"google.golang.org/grpc"
	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/status"
	"google.golang.org/protobuf/proto"
	"google.golang.org/protobuf/types/known/anypb"

	auditv1 "github.com/lyft/clutch/backend/api/audit/v1"
	"github.com/lyft/clutch/backend/gateway/log"
	"github.com/lyft/clutch/backend/gateway/meta"
	"github.com/lyft/clutch/backend/middleware"
	"github.com/lyft/clutch/backend/service"
	auditservice "github.com/lyft/clutch/backend/service/audit"
	"github.com/lyft/clutch/backend/service/authn"
)

Actual Output

Debug Output

I added some extra log statements in a custom build to figure out what's happening internally, so you can see what the linter returns, how golangci-lint parses the diff, and how it merges the diffs.

$ golangci-lint run --fix
DEBU [importissue] goimports diff:
--- github.com/lyft/clutch/backend/middleware/audit/audit.go.orig 2021-03-09 10:57:17.396813013 -0600
+++ github.com/lyft/clutch/backend/middleware/audit/audit.go      2021-03-09 10:57:17.396813013 -0600
@@ -5,12 +5,13 @@
 // <!-- END clutchdoc -->
 
 import (
-       "google.golang.org/grpc/codes"
-       "google.golang.org/grpc/status"
        "context"
        "errors"
        "fmt"
 
+       "google.golang.org/grpc/codes"
+       "google.golang.org/grpc/status"
+
        "github.com/uber-go/tally"
        "go.uber.org/zap"
        "google.golang.org/grpc" 
        
DEBU [importissue] goimports: pos 8     range 8-9
replacement:
- NeedOnlyDelete true
- InlineFix: <nil>
- NewLines: []
 
DEBU [importissue] goimports: pos 13    range <nil>
replacement:
- NeedOnlyDelete false
- InlineFix: <nil>
- NewLines: ['','       "google.golang.org/grpc/codes"','       "google.golang.org/grpc/status"','']

DEBU [importissue] gci diff:
--- github.com/lyft/clutch/backend/middleware/audit/audit.go
+++ github.com/lyft/clutch/backend/middleware/audit/audit.go
@@ -5,8 +5,6 @@
 // <!-- END clutchdoc -->
 
 import (
-       "google.golang.org/grpc/codes"
-       "google.golang.org/grpc/status"
        "context"
        "errors"
        "fmt"
@@ -14,6 +12,8 @@
        "github.com/uber-go/tally"
        "go.uber.org/zap"
        "google.golang.org/grpc"
+       "google.golang.org/grpc/codes"
+       "google.golang.org/grpc/status"
        "google.golang.org/protobuf/proto"
        "google.golang.org/protobuf/types/known/anypb"
  
DEBU [importissue] goimports: pos 8     range 8-9
replacement:
- NeedOnlyDelete true
- InlineFix: <nil>
- NewLines: []
 
DEBU [importissue] goimports: pos 13    range <nil>
replacement:
- NeedOnlyDelete false
- InlineFix: <nil>
- NewLines: ['','       "google.golang.org/grpc/codes"','       "google.golang.org/grpc/status"','']

> Fixer
DEBU [importissue] fixer: Fix issue &result.Issue{FromLinter:"gci", Text:"File is not `gci`-ed with -local github.com/lyft/clutch/backend", Severity:"", SourceLines:[]string{"\t\"google.golang.org/grpc/codes\"", "\t\"google.golang.org/grpc/status\""}, Replacement:(*result.Replacement)(0xc01f0707d0), Pkg:(*packages.Package)(0xc002729b00), LineRange:(*result.Range)(0xc01f0707c0), Pos:token.Position{Filename:"middleware/audit/audit.go", Offset:0, Line:8, Column:0}, HunkPos:0, ExpectNoLint:false, ExpectedNoLintLinter:""} with range 8-9 
DEBU [importissue] fixer: Fix issue &result.Issue{FromLinter:"goimports", Text:"File is not `goimports`-ed with -local github.com/lyft/clutch/backend", Severity:"", SourceLines:[]string{""}, Replacement:(*result.Replacement)(0xc047117210), Pkg:(*packages.Package)(0xc002729b00), LineRange:(*result.Range)(nil), Pos:token.Position{Filename:"middleware/audit/audit.go", Offset:0, Line:13, Column:0}, HunkPos:0, ExpectNoLint:false, ExpectedNoLintLinter:""} with range 13-13 
DEBU [importissue] fixer: Fix issue &result.Issue{FromLinter:"gci", Text:"File is not `gci`-ed with -local github.com/lyft/clutch/backend", Severity:"", SourceLines:[]string{"\t\"google.golang.org/grpc\""}, Replacement:(*result.Replacement)(0xc01f070850), Pkg:(*packages.Package)(0xc002729b00), LineRange:(*result.Range)(nil), Pos:token.Position{Filename:"middleware/audit/audit.go", Offset:0, Line:16, Column:0}, HunkPos:0, ExpectNoLint:false, ExpectedNoLintLinter:""} with range 16-16

[danielhochman]

So far, I'm pretty sure the issue is related to one or all of these code paths:

• extractIssuesFromPatch, which analyzes the diff from the linter output to determine what the affected lines are and what action is necessary on those lines
• mergeLineIssues (fixer call, func), where the fixer tries to merge all issues from all linters together for a single file
• findNotIntersectingIssues (fixer call, func), where the fixer tries to break fixes up into multiple passes? not 100% sure on that yet

[thebeline]

@danielhochman, trying to understand:

This is in fixer.go, would erroneous behavior in those three functions trigger if I weren't using --fix?

[thebeline]

Ignore this comment, something is weirder, I have to do more research

[ldez]

the fixer is only used when --fix is called.

edit: I removed my previous comment because I misread your comment.