x/tools/gopls: add the unusedwrite analysis

ainar-g commented 3 years ago

CL 287034 added the new unusedwrite analysis pass. I think it would be nice to have in gopls.

I'm not sure if it should be among the default analysers, but either way, if there are no objections to this, I am ready to send a CL.

stamblerre commented 3 years ago

We'd be happy to accept a CL--if you think it should be off by default, we can start with that first, and then enable it later on?

stamblerre commented 3 years ago

Ah, looks like it uses SSA, so let's keep it off by default to start.

gopherbot commented 3 years ago

Change https://golang.org/cl/295172 mentions this issue: internal/lsp/source: add the unusedwrite analyzer

ainar-g commented 3 years ago

Thanks for reviewing!

adonovan commented 5 months ago

SSA is now enabled by default (for nilness), so the marginal cost of enabling unusedwrite is very small. I just ran a very quick experiment and found that 10 out of 10 findings I randomly sampled in kubernetes were genuine opportunities for simplification. So let's do a proper evaluation and perhaps enable it in gopls.

adonovan commented 5 months ago

I should run a job over the Module Mirror corpus to get results that we can share, but in the meantime Googlers can inspect a list of findings of this analyzer on Google source code at go/unusedwrite-diagnostics-b1c6ea0fd8b46c. The false positive rate is zero in my manual sampling so far. I think this might be a good feature for gopls.

adonovan commented 5 months ago

I ran unusedwrite over the Module Mirror corpus of 22867 modules. It reported 2976 unique diagnostics, of which 100 picked at random are shown below. I analyzed a dozen or so by hand, and all were true positives, indicating that the code was unnecessarily complicated and confusing, though potentially still correct; but in three cases, the excessive complexity was clear evidence of a bug, for example updating a field of the receiver struct before returning from a method with a non-pointer receiver.

I think we should enable this analyzer by default in gopls.

The complete list is here: unusedwrite.txt

timothy-king commented 5 months ago

I took a quick look. Most of the reports in non-test code look really good.

https://go-mod-viewer.appspot.com/honnef.co/go/tools@v0.4.7/pattern/parser.go#L304

FYI there are some false positives due to struct literals:

The common pattern is x := struct{f, g int}{f: 0, g: 1}; println(x.f). The implicit x.g = 1 write is being reported as it is never used. The solution is likely just better representation of struct literal initialization at the SSA level and not reporting these. This could be something like what staticcheck does in its ir, which is to have a struct literal Instruction, or better 'debug info' on the write instructions.

Just something to keep in mind. Unclear if it is urgent.

adonovan commented 5 months ago

FYI there are some false positives due to struct literals:

They're not bugs, but they do indicate that a field of a struct is assigned but never used, which is a sign that the code is building a bigger data structure than it actually needs.

golang / go

x/tools/gopls: add the unusedwrite analysis #44461