-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
proposal: x/tools/.../shadow: specific cases that are unlikely to have false positives #58917
Comments
CC @adonovan @timothy-king @golang/tools-team |
I agree, this case seems like a pretty straightforward readability improvement. But since the analyzer is not reporting (with high confidence) an actual mistake, it's not the kind of thing we would enable by default in vet.
If I suspect the only way to improve the shadowing analysis is to do an empirical study of a large body of code. |
I don't see why. When would you declare an var err error
if party {
// ... Preparations...
err = sliceAPizza()
} else {
// ... Preparations...
err = sliceACucumber()
}
if err != nil {
return err
} but there, an inner- We could possibly even restrict it to the zero-valued case of a := []int{1} // Different.
if party {
a := sliceAPizza()
} would not be considered a shadowing. Edit: Additionally, initially, perhaps it could be limited to the case where the outer- (The lack of the ternary operator is probably why I think this lint check is important in Go.) |
I said that I didn't think it would be that uncommon for an outer err var to be both referenced and shadowed by a block, for example:
But all of this is speculation. I think the right thing to do is build a prototype and test it on some large open-source repositories and demonstrate that it has a high signal-to-noise ratio. |
Thanks for the clarification and example. (I don't think this case should be flagged, since the inner It sounds like this (building a narrower I'll see what I can do. Thanks. |
If the check was to flag this example it would be a false positive:
You will need some way of concluding that the above is different from the given example:
Here is an approach that could distinguish these. You can do dead write inference on the To get something into cmd/vet, I would recommend collecting more real world examples to narrow down what the correctness issues you are trying to solve is. (Shadowing is not itself a correctness problem in Go.) This might even be more valuable than proposing a prototype at this stage. (For this particular example, it might be easier to warn on dead writes in vet? Unclear.) |
Thanks for the input. Did you mean to use (Assuming Perhaps looking at this as a dead write problem is indeed the better approach. I did run some tests with a very hacky declaration-before-if detector. I ran it on some big Go projects (Go, Kubernetes, Juju, Gogs) and it looked generally fine. I think I saw 4-5 flagged issues. I caught one thing that could have been a bug, but the function returned early, so it wasn't a problem. The rest were false positives, but could also be a source of confusion. Sadly I don't remember where I put the results, and I broke the implementation since, so I can't tell you more. |
I realize this issue is about detecting special cases of shadowing that could be fine, to make the check always correct so it can be included in vet by default. I think the suggestion below is relevant, but I can take it to a new issue. I run shadow for additional checks in my project. But lots of "err" is flagged. I believe it is very common in Go code to shadow err. It would already help a lot if I can tell shadow to ignore shadowed "err" (and perhaps other variables, "ctx" comes to mind, though I've renamed those in my code). "err" is also used in this discussion as a reason to improve shadow. Perhaps it's enough to solve just that case? I currently filter out warnings about "err" in a separate Makefile target, because it fails. See https://github.com/mjl-/mox/blob/main/Makefile#L32. If there is already an option to skip warnings for certain shadowed variable names, I couldn't find it. I'm also curious if projects exist with a policy to never shadow "err". Perhaps not warning about about "err" could even be the default, with an option to enable checking for it. It wouldn't be enough to run "shadow" by default, but it could make it more useful for the typical case. |
@mjl- This almost sounds like the inverse of what I'm suggesting here. You have a codebase that is already shadow-friendly and want to be able to configure exceptions. I'd suggest a new issue. |
I propose discussing more specific cases of variable shadowing, to allow making
shadow
an analyzer that can be enabled by default.Background
I did the common thing of refactoring this code:
into
The problem here being that
:=
and=
are somewhat difficult to tell apart. This is a clear case of unintended shadowing. I ran the shadow analyzer, and it came up with lots of false positives forerr
, as one would expect. So enabling that is out of the question for me.Proposal
This made me wonder if this pattern of
is something a shadow analyzer could flag without the risk of false positives.
a
not being used infor
, but an inner-a
is,) moving the outer declaration to after the block probably improves readability.a
are used, that could be detected by checking if outer-a
is being used at all in the block, before flagging the shadowing.Are there other situations where shadowing is more clear than the general case currently implemented in the analyzer?
The text was updated successfully, but these errors were encountered: