proposal: x/tools/go/analysis/passes/shadow: deprecate it

[adonovan]

Background: The shadow analyzer aims to report mistakes related to shadowing of local variables, which is a not infrequent source of mistakes in Go. However, the shadow analyzer has at no point in its history gotten remotely close to the signal-to-noise ratio we expect of our static analysis tools, and that's why it was never enabled by default in the old cmd/vet, nor even included in the vet suite once vet was modularized using the Go analysis framework. Yet we still receive occasional CLs trying to improve the algorithm, and they take time to review.

If anyone wishes to develop their own shadow analyzer, either based on our code or from whole cloth, they are free to do so and we wish them luck; and if they succeed at eliminating false positives (for example, demonstrating a <5% false positive rate on the goalng.org/x repos), we could discuss replacing the current implementation and perhaps even adding it to vet. Until then:

Proposal: We propose to formally deprecate the shadow analyzer on the grounds that it is a failed experiment, and to summarily reject CLs to incrementally improve its algorithm.

@findleyr

[findleyr]

I guess this qualifies as a Proposal, since we typically treat deprecations as API changes (albeit reversible ones).

Fully agree. We have somewhat different standards for gopls, where hint level diagnostics are less prominent, and even there it is disabled by default.

[gabyhelp]

Related Issues

• proposal: x/tools/.../shadow: add flag to skip reporting about "err"/specified identifiers #61566 (closed)
• x/tools/gopls: add the shadow analyser #43245 (closed)

Related Documentation

• Go 1.12 Release Notes > Tools > go tool vet no longer supported

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[mateusz834]

Does that also mean removal from gopls? I have this analyzer enabled all the time in gopls. I don’t really use it in the sense of renaming variables based on its results, but rather as a hint, so I can spot such cases more easily while debugging. I’m fine with all the false positives.
The description of the analyzer mentions that "check for possible unintended shadowing of variables", which i have been not aware of. I thought that it reports all shadowed variables.
Perhaps an inlay hint (similar to the recently added ignoredError) would have been more helpful in such cases.

[adonovan]

Does that also mean removal from gopls?

That was my intent, yes; I should have been more explicit.

I have this analyzer enabled all the time in gopls. I don’t really use it in the sense of renaming variables based on its results, but rather as a hint, so I can spot such cases more easily while debugging. I’m fine with all the false positives.

Huh, interesting. And it's not even configured at level protocol.SeverityHint as it should have been, so presumably it pollutes the VS Code problems pane with non-problems?

[mateusz834]

so presumably it pollutes the VS Code problems pane with non-problems?

No idea how it is displayed in VS Code, in neovim i see it being displayed as warnings.

main.go|6 col 3-4 warning| declaration of "a" shadows declaration at line 4