x/tools/gopls: should not issue lint-style warnings on generated code

[mgabeler-lee-6rs]

What version of Go are you using (go version)?

$ go version
go version go1.14.7 linux/amd64

$ gopls version
golang.org/x/tools/gopls v0.5.0
    golang.org/x/tools/gopls@v0.5.0 h1:XEmO9RylgmaXp33iGrWfCGopVYDGBmLy+KmsIsfIo8Y=

Does this issue reproduce with the latest release?

This is the latest release of gopls as far as I can tell

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/mgabeler-lee/.cache/go-build"
GOENV="/home/mgabeler-lee/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mgabeler-lee/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.14"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.14/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/mgabeler-lee/src/noneofyourbusiness/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-build340320309=/tmp/go-build -gno-record-gcc-switches"

What did you do?

• Made a project that has generated code from go-bindata
• Loaded the project in vscode with the go extension and gopls enabled
• OR: Just ran gopls check .../foo.go for all the files in the project

What did you expect to see?

• No style/format warnings for a file with a standard generated code header (// Code generated for package foo by go-bindata DO NOT EDIT. (@generated)

What did you see instead?

• .../foo.go:306:27-35: redundant type from array, slice, or map composite literal
  • The vscode warning adds that this is from the simplifycompositelit check
• I do see those warnings, and can't get rid of them since ... it's generated code

[stamblerre]

Thanks for the report! Our current standard has been to offer vet and other analysis diagnostics for generated files, but to avoid any automatically applied suggested fixes. I do agree that there's an argument to be made that we shouldn't offer any purely cosmetic diagnostics for generated files.

[mgabeler-lee-6rs]

My argument would be that, since this is generated code, the user seeing those warnings ... cannot fix them. The next time the code is re-generated, they'll come right back. If they are to be fixed, they need to be fixed in the templates used by the generating tool.

Also from what I can see, many other go linting tools overall know to ignore these files when run on a module. E.g. golangci-lint ignores files with several "standard" header comments indicating generated code:
https://github.com/golangci/golangci-lint/blob/eeff3902d440513f5598391d6589c779d39af796/pkg/result/processors/autogenerated_exclude.go#L69-L72

And FWIW, when trying to trace the source of which tool was showing these warnings to me in vscode, it seemed like most other go linting tools behave similarly.

[stamblerre]

My argument would be that, since this is generated code, the user seeing those warnings ... cannot fix them. The next time the code is re-generated, they'll come right back. If they are to be fixed, they need to be fixed in the templates used by the generating tool.

I understand this, but analyzers like go vet and staticcheck may be able to actually find bugs in the generated code, which users might want to fix in the generators. This was the reasoning behind offering diagnostics but not suggested fixes. I agree that anything that is not a bug should not be flagged.

[mgabeler-lee-6rs]

I agree that anything that is not a bug should not be flagged

That sounds like a good compromise 👍

(thinking out loud...) Cases like the example I hit here may be present in templates to support older versions of Go (not saying that's the case here, but it could be for similar cases, if new "shorthand" syntax is added in future versions of Go, but templates want to support older versions too). Differentiating that from things like errcheck or other signs of coding errors as opposed to style/simplicity makes sense to me.

[matthewmueller]

I'd like to keep linting, though I do understand the temptation to remove it.

My go-bindata currently generates lint errors. It's annoying enough that I'll submit a PR to the repository to fix it soon.

I think this is a subtle nudge to encourage better code generators.

[stamblerre]

I asked about this on Twitter (https://twitter.com/stamblerre/status/1308898546809266176?s=20), and it seems that there's enough diversity of opinion that I'd rather not make any changes right now. It'd be great to hear more opinions from users on this issue, so let's conduct an ad-hoc poll. Below, cosmetic lint errors refers to any errors that do not relate to the correctness of your code, only its readability. The simplifycompositelit analysis mentioned above is a good example.

• Please 👍 this comment if you WANT cosmetic lint errors flagged in generated files
• Please 👎 this comment if you DO NOT WANT cosmetic lint errors flagged in generated files

[mvdan]

I'd like to loop in @ianthehat and @dominikh, because I think this is a rather general issue with go/analysis. There just isn't any guidance as to when should analyzers be used on generated code. Staticcheck has also struggled with this dilemma, and you can still see the open issue with similarly opposing arguments.

I generally agree that any warnings that could be bugs should show up for generated code, and that one should fix the generator. The generator being under someone else's control isn't a good argument there, because ultimately you're the one choosing what generator you run.

I also agree that cosmetic fixes are generally not worth it for generated code, since humans should generally not have to read or maintain the code. That being said, any automated formatting changes would still be good there, for the sake of consistency with the rest of the code. This includes plain gofmt, and optional extras like gofmt -s, and if you ask me, even automated suggested fixes via go/analysis if they still belong in the "consistent formatting" category.

So what I think is missing is some way to categorize analyzers. To me, there are at least three major categories:

• Those which find likely bugs (vet, staticcheck)
• Those which enforce consistent formatting (gofmt, gofumpt)
• Those which make code quality suggestions (the former gosimple, gocyclo, dupl)

You definitely don't want to run the third on generated code, but I think you do want to run the first two there.

[myitcv]

I generally concur with the points made by @mvdan in #41436 (comment), but draw a slightly different conclusion.

I also agree that cosmetic fixes are generally not worth it for generated code, since humans should generally not have to read or maintain the code.

Agreed. I think the word "generally" is key here. Code generated by code generators should be able to be read by humans who want to understand what is going on. Some people might choose/prefer to read that code formatted/simplified/linted to their taste, others not. But this is not a matter of correctness, unlike go vet.

You definitely don't want to run the third on generated code, but I think you do want to run the first two there.

My conclusion is therefore that only the first category in @mvdan's list should ever show as errors in gopls (emphasis key because that's the focus of this description). Because "generally" speaking people don't regularly read/care about the output of a code generator. If they want to fix the formatting/linted-ness of generated code they are going to be doing that outside of analyser suggestions in any case by adding a go:generate directive to format code or by fixing the code generator in some way.

[ianthehat]

I kind of disagree, there is one specific class of user that wants to see all analyzers on generated code, the generator author, and if we don't allow for that we can expect the overall quality of generated code to be lower, which is not a good thing for the ecosystem.

For all standard analyzers, I would hesitate to trust and use a generator that did not generate clean code anyway. It gets more complex when you start having different people running different analyzers though (which is probably why all non standard linters have the ability to skip these files), as you then are running tests that the generator author does not.

I would rather think about why it is a problem to show these kinds of errors, and what we can do to solve that problem.
Most times I have seen people complain about these kinds of things it is because the problems show up and clutter some kind of list (either command line output or a problems panel in an editor). It might be a sufficient compromise to (optionally?) suppress diagnostics for generated files that are not actively being looked at (so either open in the editor or mentioned on the command line). This would still allow you to see and fix them, but not nag you if you are just working on nearby code.
This would imply that it is a presentation issue rather than an analysis issue (generate all the results and then suppress the unwanted ones). Doing it however requires understanding which files are generated, which is a go concept and should probably not be pushed to the editor, which means gopls would be the right place to implement it.

[myitcv]

I kind of disagree, there is one specific class of user that wants to see all analyzers on generated code, the generator author, and if we don't allow for that we can expect the overall quality of generated code to be lower, which is not a good thing for the ecosystem.

I just re-read my reply - I missed out the key words "by default". So I completely agree with your analysis, @ianthehat. I particularly like you suggestion of the option on whether these diagnostics are shown or not being controlled by whether the generated file is open or not.

Doing it however requires understanding which files are generated, which is a go concept and should probably not be pushed to the editor, which means gopls would be the right place to implement it.

Agreed.

[mgabeler-lee-6rs]

there is one specific class of user that wants to see all analyzers on generated code, the generator author

This is a very good point, and I agree is a slam dunk argument for why it should be possible to show the linting results even on generated files.

I would hesitate to trust and use a generator that did not generate clean code anyway

A generator that generates code with minor cosmetic issues would not influence my trust at all. A generator that, say, put everything on a single line, or didn't indent any lines, or other crimes against my eyes would be a different story of course 😉

I would hesitate to trust and use a generator that did not generate clean code anyway

This seems to be a key point here. I don't think it's reasonable to expect every "high quality" code generator to pass everyone's possible linting rules. Heck, this might not even be possible, if there are corners where there are competing contradictory linting rules across tools (admittedly unlikely in the Go ecosystem). A related use case for this is what if the generator is targeting support for a wide range of Go versions, and some of the cosmetic / simplification suggestions are not supported in the older versions of Go? In that case, the generator maintainer would be entirely reasonable to reject a proposed fix to the style, as it would break a set of users they are intentionally trying to support. (This is admittedly a more hypothetical issue, but it seems plausible to me)

I would rather think about why it is a problem to show these kinds of errors, and what we can do to solve that problem.
Most times I have seen people complain about these kinds of things it is because the problems show up and clutter some kind of list

This was my fundamental motivation behind filing this issue.

A close corollary of this is that, in most contexts, the reports are also not actionable. To clear the warning I would have to file an issue / PR against the generator, wait some indeterminate amount of time for them to fix the issue / accept my fix, and then update my dependencies. Or fork and vendor the generator.

It might be a sufficient compromise to (optionally?) suppress diagnostics for generated files that are not actively being looked at ... not nag you if you are just working on nearby code.

👍

Doing it however requires understanding which files are generated, which is a go concept and should probably not be pushed to the editor, which means gopls would be the right place to implement it

I don't understand your rationale behind this statement.

1. Understanding Go concepts seems like a pretty proper responsibility of a Go language server / linter
2. Other linters and go source tools seem to think that recognizing generated code is a reasonable responsibility
3. The standard way to do this was an accepted proposal: proposal: build: define standard way to recognize machine-generated files #13560 (the final accepted proposal, hidden deep in github's collapsing of stuff, seems to be proposal: build: define standard way to recognize machine-generated files #13560 (comment)), and the commit/issue references from there are a positive swarm of code generators and linters updating to follow the standard.

[dominikh]

First a note about Staticcheck's behavior: we implement the idea that bugs should be flagged, because you don't want buggy generated code, but that stylistic issues shouldn't be flagged, because you might have no control over the generator. To that end, each analysis decides whether its diagnostics should be hidden for generated code, on a per-diagnostic basis. This applies to most (but not all) style checks, for example. There is currently no way for gopls to overwrite this behavior in Staticcheck, either.

This comes down to a design decision in go/analysis. Analyses have no control over most aspects of the UI, and can not convey intent. They can't set severities (error, warning, ...) nor how to handle generated files. The official opinion IIRC is that an analysis can't make this decision and that it is up to the specific tool running the analysis. The downside of that, however, is that the decision can generally only be made on a per-analysis basis. For Staticcheck, that wasn't good enough, which is why we filter diagnostics for generated code at diagnostic creation time, with the downside that tools like gopls no longer have any control over the decision.

I still think that the go/analysis.Diagnostic abstraction would benefit from a way of conveying the analysis's default intent for a diagnostic. "I (the analysis) believe this diagnostic should be hidden for generated code, unless you explicitly decide otherwise."

Now my responses to various other points made in this thread, in no particular order:

I don't understand your rationale behind this statement.

You seem to be in violent agreement. Ian said this responsibility should exist in gopls, not the editor, which seems to be the thing you're arguing for as well.

Do note, however, that this isn't as trivial as it seems. The LSP protocol is somewhat limited when it comes to two-way communication between the editor and the language server. There's no good way, either in terms of protocol or the editors' UIs, to easily toggle between displaying and hiding diagnostics for generated code.

I don't think it's reasonable to expect every "high quality" code generator to pass everyone's possible linting rules. Heck, this might not even be possible,

Strictly speaking, it only has to follow the rules that everyone has agreed on. The checks that gopls runs should correspond to that set of rules.

A related use case for this is what if the generator is targeting support for a wide range of Go versions, and some of the cosmetic / simplification suggestions are not supported in the older versions of Go?

This is not an insurmountable problem. Analyses can be written to support multiple Go versions. In particular for "features only available in newer Go versions", the analysis can simply be a no-op when targeting older Go versions. Staticcheck already contains analyses doing exactly that.

[dominikh]

Prior issues that seem relevant:

• x/tools/gopls: stylistic diagnostics emitted for generated code #36914
• x/tools/gopls: hide gofmt -s diagnostics (and others?) in generated files #38467

[zikaeroh]

I don't personally care one way or the other if the diagnostics are shown; they don't bother me. What does bother me is when they have suggested fixes that get applied on save. #38467 was about fixing that, specifically gofmt -s. I realize this is against @mvdan's opinion (#41436 (comment)), but I believe any generated file should be treated as effectively read-only.

IMO, if you want the generated code to be formatted in a specific way and a code generator doesn't do that, you should run gofmt or goimports or gofumpt in a go:generate directive immediately after code gen. I do this when a generator I'm using outputs something that goimports would change. Otherwise, if I go and run go generate ./... the repo will change, which to me is just as bad as leaving go.mod/go.sum untidy. I always set up CI checks that enforce that my repos are tidy, formatted, and go generate clean.

There's no good way, either in terms of protocol or the editors' UIs, to easily toggle between displaying and hiding diagnostics for generated code.

Unless I'm mistaken, this is "easy" from the POV that the user will change a setting, the client will send a didChangeConfiguration (which will either contain the config, or trigger a workspace config query from the server to fetch the config), then the server is free to re-send diagnostics afterward. This is how other LSs let you say, hide all warnings, or similar. The server would need to store some "raw" set of diagnostics to filter on send, which is more complicated than just sending them on the fly, yes.

[myitcv]

There's no good way, either in terms of protocol or the editors' UIs, to easily toggle between displaying and hiding diagnostics for generated code.

In addition to the configuration approach that @zikaeroh mentioned, there is this suggestion from @ianthehat:

It might be a sufficient compromise to (optionally?) suppress diagnostics for generated files that are not actively being looked at (so either open in the editor or mentioned on the command line)

@ianthehat's comment seems to strike the right note about the separation of the presentational aspect of diagnostics from their calculation. By way of analogy, we do not show stylistic errors for dependencies, only vet errors; again this feels like principally a presentational decision.

[myitcv]

@stamblerre - what's the latest on this issue? Does @ianthehat's comment in #41436 (comment) provide us a way forward here?

[stamblerre]

We can certainly move forward with only showing certain analyzer results on generated files when they are open, but I do think we are missing the clear categorization of analyzers. We probably need to put some work into clearing up our analysis logic before we can move forward with that (cc @heschik who mentioned analyzer categorization today).