-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: cmd/vet: warn when shadowing variable with slices.Delete and friends #65931
Comments
Per cmd/vet/README, vet checks have three criteria: correctness, frequency, and precision. Is this a mistake that is frequent enough to be worth a vet check? Separately, if we do this, what about |
For the frequency, I have yet to collect evidence data. For
|
I don't think it's as clear as that. A slice has value beyond it's content, you might need to keep the original slice around because you are reusing it's underlying backing array to reduce allocations in your code. |
@Jorropo brings up a good point that one can reuse allocations. We would never want to warn on functions that takes a slice as a buffer to use:
Is function scope so different than shadowing in blocks? Well yes, but it is reason to be cautious about precision here.
|
Formally, the caller of It seems for any useful purpose If the original |
Proposal Details
When calling
slices.Delete
, shadowing the slice variable is almost certainly a bug:Playground
Why
Shadowing comes with surprises when used accidentally. In general, vet cannot warn about it because it does not know if a specific use of shadowing is intentional and legit, or not.
Shadowing keeps the original var intact, so it can be reused outside of the current inner scope where the new var is declared. In the specific case of Delete (and friends), we know that reusing the original value is not desirable:
"When calling these functions (Delete. etc.) we must consider the original slice invalid"
I am not aware of any such shadowing that would be legit and useful. If they do exist, the vet warning would be easy to work around by choosing a different var identifier (no shadowing).
On the other hand, I am very aware of the nasty bugs caused by accidental shadowing in general, and by accidental shadowing in the case of slices.Delete and friends.
Suggestion
vet warning:
Same for the friends DeleteFunc, Compact, CompactFunc, Replace.
Like #62729, the goal is to minimize the risks associated with incorrectly using the
slices
package API.The text was updated successfully, but these errors were encountered: