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
cmd/vet: warn about slice being compared to nil #33329
Comments
https://play.golang.org/p/MwAH4Y57HL3 Good catch! Could be a regression from earlier go versions though. |
@av86743 there is no regression here, everything is working as expected. @rillig I don't think that this is something that vet can flag without causing false positives. Empty slices and nil slices are semantically different. A lot of places do not (or should not) care about the distinction, but some places do. For example, a nil slice can denote the difference between an empty list and the absence of a list, or a message with an empty body vs a message with no body. |
This comment has been minimized.
This comment has been minimized.
Yes, I know that this is difficult to decide. But there should at least be some tool that can flag this. For humans this is difficult since The point is that this oversight caused some real bugs, and there are more bugs of this kind lying around, waiting to be discovered. |
I quickly hacked together the nilslice check (based on nilfunc), which found these possible issues:
The one in line 1277 is the one from #33103. I don't know how to trigger the other comparisons, but these examples alone look like it would be worthwhile to apply this check to the whole go tree, at least once and manually. |
On the topic of #33103, I do not believe that your fix in go/printer is correct. Rather, this looks like a bug in go/parser, as the list really should be nil, not empty. In a similar vein, all the comparisons with nil in nodes.go should be correct (assuming no other bugs in go/parser.) nodes.go:76 is a false positive independent of the AST parsing issue, it lazily allocates a slice, it really does mean to check for nil. Edit: I've started looking into this. It seems likely that |
So you do have AST invariants (for golang.) Do you also have a tool/package which validates that these are satisfied? |
What version of Go are you using (
go version
)?go version go1.12.7 windows/amd64
Does this issue reproduce with the latest release?
yes
What did you do?
I reported bugs #33103, #33104 and #33105. In the changes to fix these bugs, I noticed a common pattern: a slice is checked for being nil instead of checking its length.
I had thought that
len(slice) == 0
would be the canonical way of testing a slice for emptiness, and I was surprised to find nothing about this topic in Effective Go.I was also surprised that this bug has occurred in go/printer, which is written by the core go team. Is there a specific reason for using the nil check instead of the len check?
To prevent this kind of bugs in the future, there should be some tool that warns about this situation. My first idea was to integrate this check into vet. Since vet reports suspicious constructs, this seems to fit perfectly.
The text was updated successfully, but these errors were encountered: