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

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

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.

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.

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.

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.

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

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.