1.44.2 gci fails after a comment in import

[didrocks]

Welcome

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc).
• Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/)

Description of the problem

With latest version of gci (1.44.1 was already regressing in imports, but differently), there are some errors if the import contains a comment on the next line.

For instance:

import (
	"bufio"
	"bytes"
	"context"

	// embed gpolist python binary.
	_ "embed"
	"errors"

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version v1.44.2 built from (unknown, mod sum: "h1:MzvkDt1j1OHkv42/feNJVNNXRFACPp7aAWBWDo5aYQw=") on (unknown)

Configuration file

$ cat .golangci.yml
# This is for the IDE. We need to duplicate it because the --fast flag doesn’t filter manually enabled linters:
# https://github.com/golangci/golangci-lint/issues/1909.
# The idea was then to duplicate it, rename to -ide.yaml and reference this file in settings.json. That way, default
# manualy golangci-lint run would run all linters as CI. IDE will only run our fast list.
# However, we can’t use variables like ${workspaceRoot} in settings.json. https://github.com/microsoft/vscode/issues/2809.

# Please keep the list in sync.

linters:
  fast: true
  # linters to run in addition to default ones
  enable:
    - dupl
    #- durationcheck
    #- errname
    #- errorlint
    #- exportloopref
    - forbidigo
    - forcetypeassert
    - gci
    - godot
    - gofmt
    #- gosec
    - ifshort
    - misspell
    - revive
    #- thelper
    #- tparallel
    #- unconvert
    #- unparam
    #- wastedassign
    #- whitespace
    ##- wrapcheck # To think properly about it

# Get all linter issues, even if duplicated
issues:
  exclude-use-default: false
  max-issues-per-linter: 0
  max-same-issues: 0
  exclude:
  # EXC0001 errcheck: most errors are in defer calls, which are safe to ignore and idiomatic Go (would be good to only ignore defer ones though)
  - 'Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked'
  # EXC0008 gosec: duplicated of errcheck
  - (G104|G307)
  # EXC0010 gosec: False positive is triggered by 'src, err := ioutil.ReadFile(filename)'
  - Potential file inclusion via variable
  fix: true # we want this in IDE.

nolintlint:
  require-explanation: true
  require-specific: true

linters-settings:
   # Forbid the usage of deprecated ioutil and debug prints
  forbidigo:
    forbid:
      - ioutil\.
      - ^print.*$
  staticcheck:
    # Should be better for it to be autodetected
    # https://github.com/golangci/golangci-lint/issues/2234
    go: "1.17"

Go environment

$ go version && go env
go version go1.17 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/didrocks/.cache/go-build"
GOENV="/home/didrocks/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/didrocks/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/didrocks/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/didrocks/work/adsys/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1942294817=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run -v
INFO [config_reader] Config search paths: […] 
INFO [config_reader] Used config file .golangci.yaml 
INFO [lintersdb] Active 10 linters: [dupl forbidigo forcetypeassert gci godot gofmt ifshort ineffassign misspell revive] 
INFO [loader] Go packages loading at mode 575 (compiled_files|imports|name|types_sizes|deps|exports_file|files) took 421.806683ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 11.415105ms 
INFO [linters context/goanalysis] analyzers took 5.177104602s with top 10 stages: the_only_name: 1.130106249s, dupl: 1.08408315s, gofmt: 946.349403ms, forbidigo: 865.344122ms, misspell: 383.950135ms, godot: 213.834899ms, gci: 200.008117ms, ifshort: 178.977388ms, inspect: 81.084045ms, ineffassign: 63.196245ms 
INFO [runner] Issues before processing: 39, after processing: 3 
INFO [runner] Processors filtering stat (out/in): uniq_by_line: 3/3, diff: 3/3, severity-rules: 3/3, filename_unadjuster: 32/32, exclude-rules: 4/4, nolint: 3/4, identifier_marker: 4/4, max_from_linter: 3/3, source_code: 3/3, path_prefixer: 3/3, sort_results: 3/3, cgo: 32/39, path_prettifier: 32/32, skip_files: 32/32, path_shortener: 3/3, autogenerated_exclude: 4/32, exclude: 4/4, max_same_issues: 3/3, skip_dirs: 32/32, max_per_file_from_linter: 3/3 
INFO [runner] processing took 2.97261ms with stages: nolint: 2.297018ms, autogenerated_exclude: 236.045µs, path_prettifier: 136.869µs, identifier_marker: 108.777µs, source_code: 85.031µs, skip_dirs: 55.648µs, exclude: 26.393µs, filename_unadjuster: 10.432µs, cgo: 8.69µs, path_shortener: 2.549µs, uniq_by_line: 2.179µs, max_same_issues: 830ns, skip_files: 564ns, max_per_file_from_linter: 448ns, severity-rules: 247ns, diff: 235ns, sort_results: 205ns, exclude-rules: 192ns, max_from_linter: 169ns, path_prefixer: 89ns 
INFO [runner] linters took 426.751587ms with stages: goanalysis_metalinter: 423.684196ms 
INFO fixer took 0s with no stages                 
internal/testutils/pythoncoverage.go:5:1: Expected '\t', Found '\n' at internal/testutils/pythoncoverage.go[line 5,col 1] (gci)

^
internal/ad/ad.go:7:1: Expected '\t', Found '\n' at internal/ad/ad.go[line 7,col 1] (gci)

^
internal/ad/internal_test.go:5:1: Expected '\t', Found '\n' at internal/ad/internal_test.go[line 5,col 1] (gci)

^
INFO File cache stats: 139 entries of total size 788.6KiB 
INFO Memory: 10 samples, avg is 75.2MB, max is 121.1MB 
INFO Execution took 867.18245ms                   

Code example or link to a public repository

On https://github.com/ubuntu/adsys repo, in https://github.com/ubuntu/adsys/runs/5247363658, we get:

run golangci-lint
  Running [/github/home/golangci-lint-1.44.2-linux-amd64/golangci-lint run --out-format=github-actions --config .golangci-ci.yaml] in [] ...
  Error: Expected '\t', Found '\n' at internal/testutils/pythoncoverage.go[line 5,col 1] (gci)
  Error: Expected '\t', Found '\n' at internal/ad/ad.go[line 7,col 1] (gci)
  Error: Expected '\t', Found '\n' at internal/ad/internal_test.go[line 5,col 1] (gci)
  
  Error: issues found
  Ran golangci-lint in 46195ms

Here are the files header creating the issues:

package ad

import (
	"context"

	// #nosec: G501: we are using it only for comparing directory tree content in tests.
	"crypto/md5"
	"flag"

(failure is on the line before // #nosec:)

Another file:

import (
	"bufio"
	"bytes"
	"context"

	// embed gpolist python binary.
	_ "embed"
	"errors"

Error is again, on the line before the comment.

Comments in imports are correctly formatted by go fmt, and gopls supports them.

[ldez]

Hello,

you checked "Yes, I've tried with the standalone linter if available.", but have you really tried to use gci as a binary?

When you are using gci as a standalone binary, do you have the same problem?
If yes, you can open an issue on gci

[didrocks]

Yes, I did try:
gci print internal/ad/ad.go with both v0.3.0 and v0.3.1 (which is the one you are using it seems) and no error was reporting on such a file. Should I try anything else in the way golangci-lint runs it?

[ldez]

$ git clone git@github.com:ubuntu/adsys.git
Cloning into 'adsys'...
remote: Enumerating objects: 13693, done.
remote: Counting objects: 100% (4760/4760), done.
remote: Compressing objects: 100% (1988/1988), done.
remote: Total 13693 (delta 2644), reused 4295 (delta 2319), pack-reused 8933
Receiving objects: 100% (13693/13693), 3.99 MiB | 5.10 MiB/s, done.
Resolving deltas: 100% (7815/7815), done.
$ cd adsys

$ gci -v
gci version 0.3

$ gci diff internal/ad/ad.go
2022/02/18 15:55:50 Loaded File: internal/ad/ad.go
--- internal/ad/ad.go
+++ internal/ad/ad.go
@@ -4,7 +4,6 @@
        "bufio"
        "bytes"
        "context"
-
        // embed gpolist python binary.
        _ "embed"
        "errors"

$ gci diff internal/testutils/pythoncoverage.go
2022/02/18 16:00:40 Loaded File: internal/testutils/pythoncoverage.go
--- internal/testutils/pythoncoverage.go
+++ internal/testutils/pythoncoverage.go
@@ -2,7 +2,6 @@
 
 import (
        "bufio"
-
        // blank embed import for python3-mock.in.
        _ "embed"
        "fmt"

$ gci diff internal/ad/internal_test.go        
2022/02/18 16:01:15 Loaded File: internal/ad/internal_test.go
--- internal/ad/internal_test.go
+++ internal/ad/internal_test.go
@@ -2,7 +2,6 @@
 
 import (
        "context"
-
        // #nosec: G501: we are using it only for comparing directory tree content in tests.
        "crypto/md5"
        "flag"

[didrocks]

Yes, it seems now that gci removes this empty line (print gave the same). Ok. I think I read backward the error printed in CI: it wanted to find a \n and now has a \t (maybe it should print something like the gci diff output directly rather? This helps with context and more). Ok, changing this then, thanks for the feedback and sorry for the trouble!

[ldez]

The error message comes from gci itself, so inside golangci-lint we don't have a lot of possibilities to improve this message.
I recommend opening an issue on gci or creating a PR on gci to improve that.