nolintlint: "allow no explanation" broken since v1.31.0 #1566

pierrre · 2020-12-18T16:34:44Z

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.

The text was updated successfully, but these errors were encountered:

ldez · 2020-12-20T23:05:28Z

Hello,

I don't see any changes on nolintlint between v1.30.0 and v1.31.0:
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 · 2020-12-21T08:11:06Z

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 · 2020-12-21T16:51:13Z

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 · 2020-12-21T17:11:37Z

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 · 2020-12-21T17:13:51Z

I think I find the root cause...

ldez · 2020-12-21T17:14:59Z

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

ldez · 2020-12-21T17:20:12Z

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

golang/go@5a550b6#diff-f56160fd9fcea272966a8a1d692ad9f49206fdd8dbcbfe384865a98cd9bc2749R158

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

golang/go#37974

ashanbrown · 2020-12-21T17:41:05Z

@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 · 2020-12-21T17:46:29Z

I already fixed the problem in #1571

pierrre · 2020-12-22T07:27:27Z

thank you very much ! 👍

pierrre added the bug Something isn't working label Dec 18, 2020

ldez added the feedback required Requires additional feedback label Dec 20, 2020

ashanbrown self-assigned this Dec 21, 2020

ldez mentioned this issue Dec 21, 2020

fix: nolintlint comment analysis. #1571

Merged

ldez added linter: update version Update version of linter and removed feedback required Requires additional feedback linter: update version Update version of linter labels Dec 21, 2020

ldez closed this as completed in #1571 Dec 22, 2020

ldez mentioned this issue Jan 20, 2021

adhere to the widespread comment directive format #1658

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nolintlint: "allow no explanation" broken since v1.31.0 #1566

nolintlint: "allow no explanation" broken since v1.31.0 #1566

pierrre commented Dec 18, 2020

ldez commented Dec 20, 2020 •

edited

Loading

pierrre commented Dec 21, 2020 •

edited by ldez

Loading

ldez commented Dec 21, 2020 •

edited

Loading

pierrre commented Dec 21, 2020

ldez commented Dec 21, 2020

ldez commented Dec 21, 2020

ldez commented Dec 21, 2020 •

edited

Loading

ashanbrown commented Dec 21, 2020

ldez commented Dec 21, 2020

pierrre commented Dec 22, 2020

nolintlint: "allow no explanation" broken since v1.31.0 #1566

nolintlint: "allow no explanation" broken since v1.31.0 #1566

Comments

pierrre commented Dec 18, 2020

ldez commented Dec 20, 2020 • edited Loading

pierrre commented Dec 21, 2020 • edited by ldez Loading

ldez commented Dec 21, 2020 • edited Loading

pierrre commented Dec 21, 2020

ldez commented Dec 21, 2020

ldez commented Dec 21, 2020

ldez commented Dec 21, 2020 • edited Loading

ashanbrown commented Dec 21, 2020

ldez commented Dec 21, 2020

pierrre commented Dec 22, 2020

ldez commented Dec 20, 2020 •

edited

Loading

pierrre commented Dec 21, 2020 •

edited by ldez

Loading

ldez commented Dec 21, 2020 •

edited

Loading

ldez commented Dec 21, 2020 •

edited

Loading