adhere to the widespread comment directive format

[mvdan]

In Go, //go:* is reserved for the Go toolchain. See https://golang.org/cmd/compile/.

It is encouraged that other Go tools use a similar "namespace" prefix for their comment directives. For example, //gccgo:*, //lint:* for staticcheck, //go-sumtype:*, and so on. See https://groups.google.com/g/golang-dev/c/r4rdPdsH1Fg/m/tNZazPenX5cJ.

golangci-lint kind of does this, but not really. https://golangci-lint.run/usage/false-positives/#nolint gives this example:

var bad_name int //nolint

That's no good, because it clearly does not adhere to the widespread format. It is not formally defined, but in practice it follows something like ^[a-z]+:[a-z]+.

A possible fix would be //nolint:all, which is also clearer. Another option would be to follow staticcheck's https://staticcheck.io/docs/configuration/#line-based-linter-directives, which already follows the format with //lint:ignore ....

[boring-cyborg[bot]]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[mvdan]

For the sake of documenting this better upstream, I've raised https://github.com/golang/go/issues/43776.

[kortschak]

Moving to the approach used by staticcheck would also have the advantage that file-wide directives will work properly; currently the way to do a file-wide directed is to attach the comment to the package decl, which in turn results in a line in the godoc.

See https://staticcheck.io/docs/#file-based-linter-directives for how staticcheck does it.

[kortschak]

Also, @ldez given that this is a deviation from the Go standard, I think this is a bug and not an enhancement.

[ldez]

Today, nolint can have two syntax:

• just a simple comment: // nolint
• a directive syntax //nolint

In the code base of golangci-lint, it's used as a simple comment syntax. So from my point of view, there is no deviation.

But my main concern, it's the resulting breaking change of the syntax change.

currently the way to do a file-wide directed is to attach the comment to the package decl, which in turn results in a line in the godoc.

currently the nolint directives are ignored in the godoc https://github.com/golangci/golangci-lint/issues/1566#issuecomment-749094833

[mvdan]

just a simple comment: // nolint

To be honest, I don't think this is a good solution. All golangci-lint directives should clearly look like directives, not plain English comments.

there is no deviation.

I don't follow - the docs clearly point towards //nolint, which does not use a space and does not adhere to the format.

the resulting breaking change

The fix could be rolled out over time, or come with an automated way to fix up older comments like https://github.com/ipld/specs/pull/353/files#diff-ed097ee1bca871446cf885a8fabf9722c54921cf7bae59f780cd14a6f305c3d5.

[AlekSi]

I would propose for golangci-lint to complain about all forms with spaces: // nolint, //nolint: name, etc.

[ldez]

@AlekSi you can already do that:

linters-settings:
  nolintlint:
    # Disable to ensure that nolint directives don't have a leading space. Default is true.
    allow-leading-space: false

[AlekSi]

And I think the default should be changed to false in some future version with some warnings in between.

Edit: also, the current code doesn't handle //nolint: name (with space) case.

[kortschak]

It would be very nice if golangci-lint respected the linter directives of supported linters.

$ staticcheck .                                                          
$ golangci-lint run    
main.go:7:6: `foo` is unused (deadcode)
func foo() {}
     ^
$ cat main.go   
package main

func main() {
}

//lint:ignore U1000 Included for demonstration purposes.
func foo() {}

[scop]

It would be very nice if golangci-lint respected the linter directives of supported linters.

A bit tangential here, but I opened #2671 about that. For some linters such as revive and gosec it already works, maybe staticcheck needs special handling or changes.

Note that the example here is a bit broken, it has a lint:ignore but the error comes from deadcode which to my knowledge doesn't respect those directives. #2671 has a valid one for staticcheck.

[mvdan]

See the upstream bug report above; users are starting to run into this problem now that gofmt reformats godoc comments. I would say that fixing this bug before 1.19 is released in three weeks is very important - otherwise the number of incoming bug reports will multiply.

[ldez]

I will create a PR to drop allow-leading-space option and add the support of nolint:all.

It will not be the real fix for this issue but a fix for go1.19 gofmt.

In parallel, I started to work on a real fix for this issue with a new syntax. We cannot use exactly the same option as staticcheck (//lint:ignore) because it will conflict, so I think we will use //lint:skip linter1[,linter2,...,linterN] reason. This will be introduced in a v2 of golangci-lint.

[dominikh]

We cannot use exactly the same option as staticcheck (//lint:ignore) because it will conflict

I explicitly designed the semantics of that directive to be usable by more tools than just Staticcheck (which is why the prefix is lint:). In particular, Staticcheck complains about unused ignore directives, but only if it actually has those checks enabled. That means other linters are free to use the same directive with their own check names.

[ldez]

I'm talking about the "option" part of the directive: ignore. I said that we will use skip instead of ignore.

As we check the linter name inside golangci-lint and not the rules inside a linter, we cannot use ignore because we will have a problem with directives defined for staticcheck (//lint:ignore SA4000).

[dominikh]

I don't understand what the problem is? If you saw //lint:ignore SA4000 you would ignore the directive, the same way that Staticcheck would ignore a directive like //lint:ignore somelinter. The two uses of //lint:ignore can coexist.

[ldez]

I think that adding code related to staticcheck inside the core system of golangci-lint is not a good idea: every linter can use this syntax, if I start with excluding staticcheck directives, I will have to ignore those from other linters. So maybe, in the end, I will not follow your design to manage that, currently I don't know. I'm working and thinking on it, so no strong decision for now.

[mvdan]

I think Dominik means that golangci-lint should ignore directives that it doesn't understand, like //lint:ignore SA4000, because SA4000 is not a linter name that it knows. This is how many directives in Go work: for example, the Go compiler obeys //go:noinline, but will ignore //go:foobar. And encoding/json will obey Field string `json:",omitempty", but will ignore Field string `json:",foobar". Not just for better interoperability with other implementations, but also for fowards compatibility with newer versions adding new directives.

[dominikh]

I think that adding code related to staticcheck inside the core system of golangci-lint is not a good idea: every linter can use this syntax, if I start with excluding staticcheck directives, I will have to ignore those from other linters.

My point wasn't to ignore Staticcheck's checks specifically, but to simply skip directives concerning checks you do not know. That is, only process those //lint:ignore directives that refer to names you recognize. If all tools do that, then there is little potential for conflict.

[ldez]

Currently, we report nolint directives that use non-existing linters, and I think we will keep that.

The difference between the 2 following lines is only on the content (check name vs linter name):

//lint:ignore SA4000
//lint:ignore staticcheck

Also, I think that users will not understand the point and they will mix both (check name and linter name) in the same line.

[ldez]

For now, no strong decision, I'm just thinking about it.

I have to see what really needs to be kept from the past, and how the behavior can be extended to manage the specific constraints of golangci-lint.

The current implementation of nolint (which was created before I was maintainer) is too complicated due to the previous lax syntax, so I don't want to reproduce the same errors.

So I will not take a decision quickly, and currently, it's not my top priority. I'm working a bit on several topics to be able to have the time to think about them in the background.

[mvrahden]

My linters are colliding already (see: https://github.com/mvdan/gofumpt/issues/241) and I do not believe that waiting for a v2 can actually be the go-to option here, except the v2 release is right around the corner.

Why not use the auto-fix option that was proposed previously in this thread?
I think lint:skip sounds like a good plan to go forward with, there's no need to serve both linters staticcheck and golangci-lint under one directive. As people will either use golangci-lint linter (with staticcheck,unused,stylecheck... as a plugin) or staticcheck as standalone solution.

The fact that golangci-lint reports non-existing linters is actually a good thing, because you'll get feedback about the tool setup, otherwise you end up having nolint directives which will only perform half of what they are configured to be doing.

Anyway, golangci-lint is not the same use case as staticcheck here, because staticcheck maintains a catalogue of checks, which is less volatile than the linters being added and deprecated within this project. So it actually golangci-lint deserves this sanitization feature and therefore its configuration should live under its own directive domain.

PS: A workaround to not break CI pipelines is to enable the fix flag.