x/tools/gopls: unusedparams should ignore params that shadow variables #64753
Comments
gopherbot
added
Tools
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). |
|
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. |
|
Change https://go.dev/cl/550355 mentions this issue: |
|
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. |
This change is a substantial rewrite of the unusedparams analyzer
designed to eliminate essentially all false positives, based on
the rule that unless we can see all uses of a function, we can't
tell whether it is safe to alter its signature.
The new algorithm computes the set of address-taken functions,
that is, functions that are referenced other than in the
CallExpr.Fun position of a function call. For soundness we must
assume that such values may be required to conform to some func
type, so we cannot alter their signature.
Exported functions and methods must also be treated
conservatively, as they may be address-taken in another package.
And unexported methods are ignored if they match the name of an
interface method declared in the same package, since they may be
required to conform to it.
Anonymous functions that are immediately assigned to a var are
treated like named functions, with the var symbol standing for
the function name.
Since the false positive rate is now close to zero, this change
also enables the analyzer by default in gopls.
Number of findings:
Repo Before After Comments
x/tools 86 26 100% true positives
k8s 2457 1987 Only 14% are in non-generated
files, and of those, all that
I sampled were true positives.
Updates golang/go#64753
Change-Id: I35dfc116a97bb5e98a2b098047fce176f2c1a9d6
Reviewed-on: https://go-review.googlesource.com/c/tools/+/550355
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
I have recently been enabling the gopls
unusedparamsanalyzer, 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 wrongtis used. In other words, shadowing is itself useful, and I think we shouldn't consider these parameters unused.CC @adonovan @timothy-king
The text was updated successfully, but these errors were encountered: