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: append with a single argument should be a go vet warning #21482
Comments
Originally posted here: https://groups.google.com/forum/#!topic/golang-nuts/YXKJeuI8t_k |
The frequency criterion is probably the questionable one here. We probably need some analysis of how often this occurs. |
As hinted at by cespare@, you should demonstrate the three points of cmd/vet/README: correctness, frequency, and precision. Correctness and precision look good by construction, but frequency is likely hard to demonstrate. Until now, I have never seen this issue in the wild. |
Because making this mistake would likely make your code run clearly wrong - I think it is likely to not show up in committed code much in the wild - it is an issue people hunt around for and figure out before committing - vet could still save them time. |
3 matches in the Go corpus. |
For frequency (there are a few more): Appears to be intentional: Possible edge case: |
I think we can close this. It doesn't seem to be frequent enough, or otherwise uncatchable enough, to merit adding a special check to vet. But I will leave it open a little longer in case anyone has a compelling counterargument. |
Another possibility, triggered by a suggestion from the mailing list: The compiler already gives an error if append is called and the result thrown away. It is conceivable to add this case as a second error. I may be imaginative enough but I haven't come up with a case where append with one argument achieves something otherwise impossible or even difficult. Educate me. |
I had thought about making it a compile error too: I think not allowing to leave out the variadic argument could violate the current language spec: So i think append would become inconsistent with other variadic functions and would need a definition change which i guess is not worth catching these cases. |
The check failed the three criteria for addition to vet. I would think that backwards incompatible compiler changes that add more special cases to the language would at least require passing the vet criteria. |
I don't think it's worth changing the compiler and language spec, especially since as others have said, it doesn't appear to be a common mistake. |
These recent comments appear to miss the point I was making, although not making well. Append is already special in the compiler. It could be more special and this problem would go away, at essentially zero cost. I am not arguing that we should do it, I'm just saying it would be easy and I think safe. |
Upvoted robpike's suggestion. I was recently bitten by this one, so I am a bit sensitized. But I won't be terribly disappointed if it stayed as is. |
This seems low frequency because almost no one is going to write this code and not notice that it doesn't work. But if vet were on by default as part of Note that go-corpus-0.01/src/go.uber.org/zap/zbark/debark_test.go has
which looks weird but then elsewhere in the file it says:
so it looks like maybe a placeholder. go-corpus-0.01/src/github.com/openshift/origin/pkg/auth/ldaputil/query_test.go has:
and
but again it looks like a kind of placeholder for things that might be appended later (or were appended before). People already get mad about taking unused imports out. Do we really want to start complaining about empty appends too? At first I thought other code looked like maybe the author thinks append unconditionally copies its arguments, but each of the ones I looked at turned out to be like the above. (And in any case, that mistake can be made about both |
No updates since last comment, but the examples in the previous comment seem to suggest that this (append(x)) comes up in reasonable code and should not be rejected out of hand. Closing per discussion with proposal review and comments above. |
What version of Go are you using (
go version
)?go 1.9rc2
What operating system and processor architecture are you using (
go env
)?OSX 64 bit
What did you do?
https://play.golang.org/p/wJZDoNLZeN
While this is not exactly surprising, it is a line of code that can be hard to catch in code reviews, and basically never makes sense to write. It is semantically equivalent to
a = b
What did you expect to see?
it would be nice if this were caught with go vet
What did you see instead?
no warning from go vet
The text was updated successfully, but these errors were encountered: