nolintlint does not report unused issue when disabled linter is disabled in configuration

[invidian]

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

I'm not sure if that's actually a bug or this behavior might make sense and be by design, but it's definitely surprising and confusing and I couldn't find a report for it, so I'm reporting it even to save someone's time when they hit the same issue.

So it seems, when //nolint comment disables check for a disabled linter, in the example here gomnd, it is not being reported as unused, even though with the current linter configuration, it is indeed unused (comment is no-op, removing it does not change the output of the linter).

I understand, that as long as gomnd linter is disabled, you cannot say for sure that this statement is actually unused or not.

I got there, as the following commands were returning me different output and it took me a while to figure out why:

golangci-lint run ./... --enable-all -v --no-config
golangci-lint run ./... --enable=nolintlint -v --no-config

Even though in both cases the issue in the code is actually there.

I wonder if this could be something configurable in nolintlint to either produce a warning by default in case like this or configurably make it an error.

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version 1.46.2 built from a3336890 on 2022-05-17T11:31:29Z

Configuration file

$ cat .golangci.yml
cat: .golangci.yml: No such file or directory # :)

Go environment

$ go version && go env
go version go1.18.3 linux/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/invidian/.cache/go-build"
GOENV="/home/invidian/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/invidian/.cache/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/invidian/.cache/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/invidian/data/workspaces/golangci-lint-bug/go.mod"
GOWORK=""
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-build1003932841=/tmp/go-build -gno-record-gcc-switches"

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run --enable=nolintlint -v --no-config ./...
INFO [lintersdb] Active 11 linters: [deadcode errcheck gosimple govet ineffassign nolintlint staticcheck structcheck typecheck unused varcheck]
INFO [loader] Go packages loading at mode 575 (name|types_sizes|exports_file|imports|files|compiled_files|deps) took 117.924935ms
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 131.397µs
INFO [linters context/goanalysis] analyzers took 1.87227819s with top 10 stages: buildir: 982.588523ms, fact_purity: 256.963435ms, SA5012: 227.884816ms, typedness: 218.836184ms, inspect: 52.024229ms, nilness: 44.380142ms, ctrlflow: 30.189933ms, printf: 26.880722ms, fact_deprecated: 24.682668ms, errcheck: 434.432µs
INFO [runner] Issues before processing: 1, after processing: 0
INFO [runner] Processors filtering stat (out/in): autogenerated_exclude: 1/1, exclude-rules: 1/1, cgo: 1/1, filename_unadjuster: 1/1, identifier_marker: 1/1, exclude: 1/1, path_prettifier: 1/1, skip_files: 1/1, skip_dirs: 1/1, nolint: 0/1
INFO [runner] processing took 254.481µs with stages: exclude-rules: 84.629µs, identifier_marker: 67.287µs, path_prettifier: 38.56µs, autogenerated_exclude: 38.523µs, skip_dirs: 11.467µs, nolint: 3.473µs, cgo: 2.427µs, filename_unadjuster: 1.306µs, max_same_issues: 1.249µs, max_from_linter: 800ns, uniq_by_line: 755ns, skip_files: 588ns, source_code: 486ns, severity-rules: 451ns, exclude: 449ns, diff: 442ns, max_per_file_from_linter: 439ns, path_shortener: 437ns, sort_results: 431ns, path_prefixer: 282ns
INFO [runner] linters took 1.338939845s with stages: goanalysis_metalinter: 1.338544234s
INFO File cache stats: 0 entries of total size 0B
INFO Memory: 16 samples, avg is 149.9MB, max is 184.6MB
INFO Execution took 1.4707078s

Code example or link to a public repository

package main

import (
        "errors"
        "fmt"
        "strings"
        "testing"
)

func combineErrors(msg string, errs []error) error {
        msg = fmt.Sprintf("%s (%d attempts)\n", msg, len(errs))

        for attempt, err := range errs {
                attemptLine := fmt.Sprintf("\t%d. %s\n", attempt+1, err) //nolint:gomnd
                msg += attemptLine
        }

        return errors.New(strings.TrimRight(msg, "\n"))
}

func Test_Foo(t *testing.T) {
        _ = combineErrors("", nil)
}

[stevenh]

Had the same when we disabled nonamedreturns, existing nolint directives were not flagged as unused.

[rostislaved]

This is very strange behavior, making the linter half useless. I can confirm that when a //nolint comment disables a check for an already disabled linter, it is not reported as unused. As a result, your code can be full of unused nolint directives that are never flagged

[rostislaved]

if I add here:
https://github.com/golangci/golangci-lint/blob/3eecab1ebde99a3c7205f09230c43a3c026a0074/pkg/golinters/nolintlint/internal/nolintlint.go

on line 22:
NeedsAll = NeedsMachineOnly | NeedsSpecific | NeedsExplanation | NeedsUnused

on line: 47
needs: needs | NeedsMachineOnly | NeedsUnused,

it seems to be working, but some tests failing

[ldez]

This will create a regression.

The nolint directive code is considered frozen because there are several design problems.

The directives will be replaced by another directive system. #1658

[rostislaved]

Thanks for the update! Do you have any rough timeline on when the new directive system might be introduced? Just trying to get an idea of what to expect.

Appreciate your work on this!

[ldez]

For now, I'm focused on the v2, so after the v2 release.