Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adhere to the widespread comment directive format #1658

Open
mvdan opened this issue Jan 19, 2021 · 23 comments
Open

adhere to the widespread comment directive format #1658

mvdan opened this issue Jan 19, 2021 · 23 comments
Assignees
Labels
area: nolint Related to nolint directive and nolintlint enhancement New feature or improvement
Milestone

Comments

@mvdan
Copy link

mvdan commented Jan 19, 2021

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 ....

@mvdan mvdan added the bug Something isn't working label Jan 19, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Jan 19, 2021

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

@mvdan
Copy link
Author

mvdan commented Jan 19, 2021

For the sake of documenting this better upstream, I've raised golang/go#43776.

@ldez ldez added enhancement New feature or improvement and removed bug Something isn't working labels Jan 19, 2021
@kortschak
Copy link
Contributor

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
Copy link
Contributor

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

@ldez
Copy link
Member

ldez commented Jan 20, 2021

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 #1566 (comment)

@mvdan
Copy link
Author

mvdan commented Jan 20, 2021

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
Copy link
Contributor

AlekSi commented Mar 25, 2021

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

@ldez
Copy link
Member

ldez commented Mar 25, 2021

@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
Copy link
Contributor

AlekSi commented Mar 25, 2021

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
Copy link
Contributor

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
Copy link
Contributor

scop commented Mar 21, 2022

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
Copy link
Author

mvdan commented Jul 13, 2022

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
Copy link
Member

ldez commented Jul 14, 2022

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
Copy link

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
Copy link
Member

ldez commented Jul 14, 2022

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
Copy link

dominikh commented Jul 14, 2022

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
Copy link
Member

ldez commented Jul 14, 2022

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
Copy link
Author

mvdan commented Jul 14, 2022

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
Copy link

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
Copy link
Member

ldez commented Jul 14, 2022

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
Copy link
Member

ldez commented Jul 14, 2022

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
Copy link

My linters are colliding already (see: mvdan/gofumpt#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.

@ldez ldez mentioned this issue Sep 9, 2022
4 tasks
ungtb10d pushed a commit to blckhodl/gitaly that referenced this issue Sep 30, 2022
With recent changes `gofmt` [1] started reformatting godoc comments.
This causes a problem wherein it reformats `//nolint: staticcheck` to
`// nolint: staticcheck`.

But it does ignore directives [2]. So let's change all our nolint to
directive format. This avoids the conflict with `gofmt`.

This fix was done by running:
`grep -r --include="*.go" -E "//nolint: .*" -l | xargs sed -i 's/nolint:
stylecheck/nolint:stylecheck/g'`
as such, we can skip it from review.

[1]:
golangci/golangci-lint#1658 (comment)
[2]:
golangci/golangci-lint#3098 (comment)
This was referenced Aug 29, 2023
@ldez ldez added the area: nolint Related to nolint directive and nolintlint label Mar 21, 2024
@owenwaller
Copy link

As can been seen in my comment in issue #2671, which has been closed as its a duplicate of this issue which I hadn't seen, this is still very much a live issue.

#2671 (comment)

From reading the comments above it looks there is no solution at present.

Is this still the case?

Owen

owenwaller added a commit to owenwaller/vugu that referenced this issue Aug 27, 2024
Using a golangci-lint directive of the form:

//nolint:staticcheck // reason why staticcheck has been disabled

We can disable the use of the staticcheck linter completely at the blocks
or lines where is is currently reporting the use of the deprecated
ast.Package stuct.

This approach is documented here:
https://golangci-lint.run/usage/false-positives/#nolint-directive

At present golangci-lint will ignore staticheck //lint:ignore style
directives. This issues has been raised with the golangci-lint team. See:

golangci/golangci-lint#2671 (comment)
and here:
golangci/golangci-lint#1658 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nolint Related to nolint directive and nolintlint enhancement New feature or improvement
Projects
None yet
Development

No branches or pull requests

8 participants