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.

[thebeline]

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

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

In which case, as all functions you mentioned are in fixer.go I am not sure the behaviour is related.

I have to do some more research, but I did notice some very strange behaviour when I started doing A/B tests.

I'll have a nice little spreadsheet for you tomorrow. :-)

[ldez]

Ok so I think it's off-topic, you can continue in https://github.com/golangci/golangci-lint/discussions/1875

[sheldonhull]

I recently ran into this as well, and finally found this issue. Thought I was conflicting with other tooling but I've confirmed this behavior is occurring directly when invoked. I tried running golangci-lint run --fix --enable gci,gofumpt --fast --allow-parallel-runners=false to see if it would help with duplicates but no results. Will have to revert to manually invoking commands.

[nightlyone]

Hi @ldez isnt this fixed by PR #2653?

After merging PR #2653 both linters should be compatible.

[ldez]

no #2653 will not fix this issue. Since v1.45.0, the support of the autofix with gci has been dropped. #2653 just restore the previous behavior and then the bug.

[yngvark]

While this is being fixed, my workaround to remove this error is to put the following in my .golangci.yaml:

linters-settings:
    gci:
        sections:
            - standard
            - default
            - prefix(github.com/ossf/scorecard) # Replace with your module name

I found this fix here: https://github.com/ossf/scorecard/pull/1664/files.

I'm using mvdan.cc/[email protected] and [email protected].

The error I'm getting without the fix above:

gofumpt -l -w ./main.go ./pkg/hello/hello_test.go ./pkg/hello/hello.go
golangci-lint run ./...
pkg/hello/hello_test.go:7:1: Expected '\t', Found '\n' at pkg/hello/hello_test.go[line 7,col 1] (gci)

[Lpaydat]

I'm not sure if this will help you all, but I added the mentioned flags to the VSCode settings and the warning has gone away.

  "go.formatTool": "gofumpt",
  "go.formatFlags": ["-s"],
  "go.lintTool": "golangci-lint",
  "go.lintFlags": ["--skip-generated", "-s standard,default"],

P.S. I used gci and gofumpt

[kluevandrew]

Same for me, but i don't have any small example app

[Crocmagnon]

gci + gofumpt OK

I'm getting conflicts between these two in version 1.53.3

golangci-lint run --disable-all --enable gci,gofumpt --fix

in:

import (
	"github.com/Crocmagnon/snippetbox/ui"
	"net/http"
)

out:

import (
	"net/http"

	"net/http"

	"github.com/Crocmagnon/snippetbox/ui"
)

[jalaziz]

Running into this issue as well 😞. It seems like the correct thing to do here is to enforce an ordering of linters, applying fixes before running the next linter. gci recommends running goimports before gci.

Unfortunately, it doesn't seem like there's a way to enforce linter ordering with golangci-lint.

[piepmatz]

Workaround: golangci-lint v1.57.0 introduced the --enable-only flag (https://github.com/golangci/golangci-lint/pull/4438). After fixing only the imports first (e.g. using gci), the next run with the usual config works.

golangci-lint run --enable-only gci --fix
golangci-lint run --fix

[sheldonhull]

What I do is disable both of those and use gofumpt only as it includes import sort already try that and maybe you'll no longer have a competing format.