I have recently been enabling the gopls unusedparams analyzer, and while it is useful it produces a lot of noise.
For our codebase, I think the largest source of noise is parameters that intentionally shadow outer parameters, such as a t *testing.T. In these cases, we really don't want to e.g. rename the parameter to _, because doing so invites later bugs when, for example, the wrong t is used. In other words, shadowing is itself useful, and I think we shouldn't consider these parameters unused.
CC @adonovan @timothy-king
Comment From: adonovan
For our codebase, I think the largest source of noise is parameters that intentionally shadow outer parameters, such as a t *testing.T. In these cases, we really don't want to e.g. rename the parameter to _, because doing so invites later bugs when, for example, the wrong t is used. In other words, shadowing is itself useful, and I think we shouldn't consider these parameters unused.
I agree, shadowing is useful, and perhaps good practice: by intentionally redeclaring the same name, creating a hole in the scope of an outer declaration, we avoid accidental references to it.
Perhaps the analyzer should also exempt functions that are address-taken (or are exported and thus potentially address-taken in another package) since the unused parameters may be necessary to conform to func type. Tests are of course all exported (and address-taken).
Comment From: adonovan
I spent a few minutes playing with the ideas in the previous note (see https://go.dev/cl/550355), and they reduce the noise considerably.
Comment From: gopherbot
Change https://go.dev/cl/550355 mentions this issue: gopls/internal/analysis/unusedparams: skip exported and &-taken funcs
Comment From: findleyr
I agree about locally address-taken functions, but I'm not sure about exported functions.
We should measure. The analyzer is currently too noisy to be useful for most users, IMO. I tend to turn it on for a time, when I'm working on tightening up, and then turn it off again.
Comment From: adonovan
I agree about locally address-taken functions, but I'm not sure about exported functions. We should measure. The analyzer is currently too noisy to be useful for most users, IMO. I tend to turn it on for a time, when I'm working on tightening up, and then turn it off again.
Exported means "potentially address-taken", and is thus a large source of noise. The changes in the CL above cause the number of findings to be significantly reduced, which seems bad only until you realize that it threw out all of the false positives, thereby making the tool actually useful and good enough to be enabled unconditionally.
Comment From: adonovan
Marking this as fixed by https://go.dev/cl/550355.