Golangci-lint: nolintlint: "allow no explanation" broken since v1.31.0

[pierrre]

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version 1.33.0 built from b90551c on 2020-11-23T05:15:36Z

Config file

$ cat .golangci.yml
run:
  timeout: "10m"
linters:
  disable-all: true
  enable:
    - "asciicheck"
    - "bodyclose"
    - "deadcode"
    - "depguard"
    - "errcheck"
    - "exportloopref"
    # - "gocognit"
    - "gocritic"
    - "gocyclo"
    - "godot"
    - "goerr113"
    - "gofmt"
    - "gofumpt"
    - "goimports"
    - "golint"
    - "govet"
    - "ineffassign"
    - "megacheck"
    - "misspell"
    - "nakedret"
    # - "nestif"
    - "nolintlint"
    - "structcheck"
    # - "stylecheck"
    - "unconvert"
    - "unparam"
    - "varcheck"
linters-settings:
  depguard:
    list-type: blacklist
    include-go-root: true
    packages-with-error-message:
      - errors: "use github.com/xxx/golang-libraries/errors"
      - reflect: "shouldn't be used by most application"
      - unsafe: "it's not safe"
      - github.com/pkg/errors: "use github.com/xxx/golang-libraries/errors"
      - golang.org/x/net/context: "use context"
      - golang.org/x/sync/singleflight: "use github.com/xxx/golang-libraries/singleflight"
  errcheck:
    check-type-assertions: true
  gocritic:
    enabled-tags:
      - diagnostic
      - style
      - performance
      - opinionated
      - experimental
    disabled-checks:
      - commentFormatting # TODO enable in order to follow the Go convention.
      - hugeParam # Too many problem for now. And maybe it's not a real issue.
      - paramTypeCombine # Too many false positive.
      - ptrToRefParam # TODO evaluate if this check can be enabled, and disable with nolint for specific cases.
      - rangeValCopy # TODO evaluate if this should be fixed or not.
      - unnamedResult # TODO fix projects.
      - whyNoLint # It's the developer/reviewer responsibility.
  gocyclo:
    min-complexity: 10
  govet:
    enable-all: true
    settings:
      printf:
        funcs:
          - "(github.com/xxx/golang-libraries/errors).Newf"
          - "(github.com/xxx/golang-libraries/errors).WithMessagef"
          - "(github.com/xxx/golang-libraries/errors).Wrapf"
  nolintlint:
    allow-unused: false
    allow-leading-space: false
    allow-no-explanation:
      - errcheck
      - misspell
    require-explanation: true
    require-specific: true
issues:
  exclude-use-default: false
  max-issues-per-linter: 0
  max-same-issues: 0

Go environment

$ go version && go env
go version go1.16beta1 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/pierre/.cache/go-build"
GOENV="/home/pierre/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/pierre/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/pierre/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/pierre/.gimme/versions/go1.16beta1.src"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/pierre/.gimme/versions/go1.16beta1.src/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16beta1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/pierre/gosrc/github.com/xxx/golang-libraries/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-build2834066088=/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: [./ /home/pierre/gosrc/github.com/xxx/golang-libraries /home/pierre/gosrc/github.com/xxx /home/pierre/gosrc/github.com /home/pierre/gosrc /home/pierre /home /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 26 linters: [asciicheck bodyclose deadcode depguard errcheck exportloopref gocritic gocyclo godot goerr113 gofmt gofumpt goimports golint gosimple govet ineffassign misspell nakedret nolintlint staticcheck structcheck unconvert unparam unused varcheck] 
INFO [loader] Go packages loading at mode 575 (types_sizes|compiled_files|exports_file|files|imports|name|deps) took 336.576226ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 22.169636ms 
INFO [linters context/goanalysis] analyzers took 1m11.186014705s with top 10 stages: buildir: 18.196055998s, buildssa: 5.653807045s, godot: 3.979265239s, unconvert: 2.767148051s, goimports: 2.207601655s, golint: 1.950709086s, unparam: 1.913974376s, gocritic: 1.875576756s, inspect: 1.846274477s, gofumpt: 1.832033599s 
INFO [linters context/goanalysis] analyzers took 4.729446694s with top 10 stages: buildir: 4.073888816s, U1000: 655.557878ms 
INFO [runner] Issues before processing: 85, after processing: 0 
INFO [runner] Processors filtering stat (out/in): cgo: 85/85, skip_files: 85/85, skip_dirs: 85/85, identifier_marker: 85/85, path_prettifier: 85/85, exclude-rules: 85/85, exclude: 85/85, filename_unadjuster: 85/85, autogenerated_exclude: 85/85, nolint: 0/85 
INFO [runner] processing took 14.906659ms with stages: nolint: 12.62204ms, identifier_marker: 1.016951ms, path_prettifier: 633.51µs, autogenerated_exclude: 459.141µs, skip_dirs: 157.489µs, cgo: 9.877µs, filename_unadjuster: 3.974µs, uniq_by_line: 905ns, max_same_issues: 672ns, diff: 257ns, skip_files: 251ns, source_code: 229ns, max_from_linter: 225ns, severity-rules: 224ns, exclude: 194ns, sort_results: 164ns, exclude-rules: 162ns, max_per_file_from_linter: 153ns, path_shortener: 150ns, path_prefixer: 91ns 
INFO [runner] linters took 4.724833786s with stages: goanalysis_metalinter: 4.116987836s, unused: 592.85491ms 
INFO File cache stats: 367 entries of total size 750.1KiB 
INFO Memory: 52 samples, avg is 484.1MB, max is 748.3MB 
INFO Execution took 5.088920461s

The "allow no explanation" feature of nolintlint is broken since v1.31.0.
With v1.30.0 it's reporting "nolint" with no explanation.
With v1.31.0, it's not reporting anymore.

[ldez]

Hello,

I don't see any changes on nolintlint between v1.30.0 and v1.31.0:
https://github.com/golangci/golangci-lint/compare/v1.30.0...v1.31.0

I tried to create the report with v1.30.0 but there is no report. (I also tried with v1.29.0)

Could you provide a code example?

[pierrre]

I don't see any changes on nolintlint between v1.30.0 and v1.31.0:

Yes, that's why I'm reporting this issue

Here is a code sample:

package foo

import "sync"

func foo() { //nolint:unused,deadcode
    var mu sync.Mutex
    mu.Lock()
    mu.Unlock() //nolint:staticcheck
}

With golangci-lint v1.30.0:

$ /home/pierre/go/pkg/golangci-lint/v1.30.0/golangci-lint -v run
INFO [config_reader] Config search paths: [./ /home/pierre/gosrc/github.com/xxx/golang-libraries /home/pierre/gosrc/github.com/xxx /home/pierre/gosrc/github.com /home/pierre/gosrc /home/pierre /home /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 26 linters: [asciicheck bodyclose deadcode depguard errcheck exportloopref gocritic gocyclo godot goerr113 gofmt gofumpt goimports golint gosimple govet ineffassign misspell nakedret nolintlint staticcheck structcheck unconvert unparam unused varcheck] 
INFO [loader] Go packages loading at mode 575 (deps|types_sizes|compiled_files|exports_file|files|imports|name) took 339.61536ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 28.848484ms 
INFO [linters context/goanalysis] analyzers took 55.070695101s with top 10 stages: buildir: 20.120179537s, buildssa: 6.73736716s, unconvert: 2.2801639s, gocritic: 1.732114317s, inspect: 1.591059712s, fact_deprecated: 1.564588276s, goimports: 1.435186059s, gofumpt: 1.431137375s, unparam: 1.093742404s, findcall: 815.953598ms 
INFO [linters context/goanalysis] analyzers took 4.790057642s with top 10 stages: buildir: 4.134919134s, U1000: 655.138508ms 
INFO [runner] Issues before processing: 185, after processing: 5 
INFO [runner] Processors filtering stat (out/in): filename_unadjuster: 185/185, path_prettifier: 185/185, skip_dirs: 185/185, max_same_issues: 5/5, cgo: 185/185, autogenerated_exclude: 185/185, identifier_marker: 185/185, nolint: 5/185, uniq_by_line: 5/5, severity-rules: 5/5, skip_files: 185/185, exclude-rules: 185/185, diff: 5/5, source_code: 5/5, exclude: 185/185, max_per_file_from_linter: 5/5, max_from_linter: 5/5, path_shortener: 5/5, path_prefixer: 5/5, sort_results: 5/5 
INFO [runner] processing took 18.857354ms with stages: nolint: 11.774195ms, identifier_marker: 4.243026ms, path_prettifier: 1.233804ms, autogenerated_exclude: 1.025312ms, skip_dirs: 425.223µs, cgo: 74.976µs, filename_unadjuster: 46.004µs, source_code: 26.916µs, uniq_by_line: 2.715µs, path_shortener: 1.862µs, max_same_issues: 968ns, max_per_file_from_linter: 698ns, exclude: 315ns, skip_files: 290ns, diff: 260ns, severity-rules: 178ns, sort_results: 177ns, max_from_linter: 173ns, exclude-rules: 169ns, path_prefixer: 93ns 
INFO [runner] linters took 4.982791242s with stages: goanalysis_metalinter: 4.337452436s, unused: 626.391297ms 
foo/foo.go:5:14: directive `//nolint:unused,deadcode` should provide explanation such as `//nolint:unused,deadcode // this is why` (nolintlint)
func foo() { //nolint:unused,deadcode
             ^
foo/foo.go:8:14: directive `//nolint:staticcheck` should provide explanation such as `//nolint:staticcheck // this is why` (nolintlint)
    mu.Unlock() //nolint:staticcheck
                ^
INFO File cache stats: 370 entries of total size 752.6KiB 
INFO Memory: 55 samples, avg is 479.2MB, max is 745.6MB 
INFO Execution took 5.355913258s

With golangci-lint v1.31.0:

$ /home/pierre/go/pkg/golangci-lint/v1.31.0/golangci-lint -v run
INFO [config_reader] Config search paths: [./ /home/pierre/gosrc/github.com/xxx/golang-libraries /home/pierre/gosrc/github.com/xxx /home/pierre/gosrc/github.com /home/pierre/gosrc /home/pierre /home /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 26 linters: [asciicheck bodyclose deadcode depguard errcheck exportloopref gocritic gocyclo godot goerr113 gofmt gofumpt goimports golint gosimple govet ineffassign misspell nakedret nolintlint staticcheck structcheck unconvert unparam unused varcheck] 
INFO [loader] Go packages loading at mode 575 (files|deps|exports_file|imports|name|types_sizes|compiled_files) took 336.069307ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 10.685933ms 
INFO [linters context/goanalysis] analyzers took 1m38.268075127s with top 10 stages: buildir: 20.327705409s, buildssa: 7.189830674s, unconvert: 2.708765339s, gocritic: 2.117544207s, gofmt: 2.116390408s, gofumpt: 1.899475029s, golint: 1.816105463s, fact_deprecated: 1.669929366s, inspect: 1.669397434s, misspell: 1.472912402s 
INFO [linters context/goanalysis] analyzers took 5.10636342s with top 10 stages: buildir: 4.287906332s, U1000: 818.457088ms 
INFO [runner] Issues before processing: 90, after processing: 0 
INFO [runner] Processors filtering stat (out/in): filename_unadjuster: 90/90, skip_files: 90/90, skip_dirs: 90/90, cgo: 90/90, exclude-rules: 90/90, nolint: 0/90, identifier_marker: 90/90, path_prettifier: 90/90, autogenerated_exclude: 90/90, exclude: 90/90 
INFO [runner] processing took 15.241814ms with stages: nolint: 12.836075ms, identifier_marker: 1.016901ms, path_prettifier: 605.119µs, autogenerated_exclude: 597.325µs, skip_dirs: 165.807µs, cgo: 11.412µs, filename_unadjuster: 4.808µs, max_same_issues: 1.101µs, uniq_by_line: 607ns, diff: 432ns, source_code: 317ns, max_from_linter: 302ns, skip_files: 259ns, sort_results: 256ns, path_shortener: 235ns, severity-rules: 210ns, exclude-rules: 199ns, max_per_file_from_linter: 179ns, exclude: 179ns, path_prefixer: 91ns 
INFO [runner] linters took 5.29990061s with stages: goanalysis_metalinter: 4.611727345s, unused: 672.396532ms 
INFO File cache stats: 368 entries of total size 750.9KiB 
INFO Memory: 58 samples, avg is 479.9MB, max is 749.2MB 
INFO Execution took 5.651591992s

Same config file as the one I shared above.

[ldez]

After a lot of tries, I found a lead:

• if I use the binary from the release, I'm able to reproduce
• if I build and run a binary from the tag, I'm not able to reproduce.

Currently, I use go1.15.6, but if I use go1.14.13 when I'm building a binary from the tag, I'm able to reproduce.

I also tried with go1.15 and I had the same problem as go1.15.6

So it seems related to the go version.

Maybe it's not related to nolintlint.

[pierrre]

I'm using the binary from the release for both versions.
My install script: curl -vfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOLANGCI_LINT_DIR) $(GOLANGCI_LINT_VERSION)
Is that the correct way ?

Currently my Go version is 1.16beta1, but I got the same issue with 1.15.6.

Maybe it's not related to nolintlint.

I didn't noticed any other issue.
Only nolintlint seems broken for me.

[ldez]

I think I find the root cause...

[ldez]

I talk about go version used to compile golangci-lint.

[ldez]

I find the root cause, now I'm sure at 100%:

https://github.com/golang/go/commit/5a550b695117f07a4f2454039a4871250cd3ed09#diff-f56160fd9fcea272966a8a1d692ad9f49206fdd8dbcbfe384865a98cd9bc2749R158

The //nolint comments are seen as a directive so they are excluded from the AST during the call of CommentGroup.Text()

https://github.com/golang/go/issues/37974

[ashanbrown]

@ldez Thank you for identifying the root cause. I can put up a fix for this if you like but if you'd rather create one yourself, I'm happy to leave it to one of you. Just let me know either way. Thanks.

[ldez]

I already fixed the problem in #1571

[pierrre]

thank you very much ! :+1: