Skip to content
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

Open
rillig opened this issue Jul 28, 2019 · 7 comments
Open

cmd/vet: warn about slice being compared to nil #33329

rillig opened this issue Jul 28, 2019 · 7 comments
Labels

Comments

@rillig
Copy link
Contributor

@rillig rillig commented Jul 28, 2019

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.

@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Jul 28, 2019

https://play.golang.org/p/MwAH4Y57HL3

Good catch! Could be a regression from earlier go versions though.

@dominikh

This comment has been minimized.

Copy link
Member

@dominikh dominikh commented Jul 28, 2019

@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.

@av86743

This comment was marked as off-topic.

Copy link

@av86743 av86743 commented Jul 28, 2019

@rillig welcome to golang.

@rillig

This comment has been minimized.

Copy link
Contributor Author

@rillig rillig commented Jul 28, 2019

@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.

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 expr == nil needs type analysis to see whether expr is a slice or not. And even if the tool merely says "hey, dear human, have a look at these few code snippets, they might be wrong", that would already help.

The point is that this oversight caused some real bugs, and there are more bugs of this kind lying around, waiting to be discovered.

@rillig

This comment has been minimized.

Copy link
Contributor Author

@rillig rillig commented Jul 28, 2019

I quickly hacked together the nilslice check (based on nilfunc), which found these possible issues:

$ ~/git/go/bin/go vet
# go/printer
.\nodes.go:76:5: comparison of slice comments == nil should be len(comments) == 0
.\nodes.go:395:16: comparison of slice Names == nil should be len(Names) == 0
.\nodes.go:1231:6: comparison of slice Results != nil should be len(Results) > 0
.\nodes.go:1277:6: comparison of slice List != nil should be len(List) > 0
.\nodes.go:1393:6: comparison of slice Values != nil should be len(Values) > 0
.\nodes.go:1429:5: comparison of slice Values != nil should be len(Values) > 0
.\nodes.go:1511:6: comparison of slice Values != nil should be len(Values) > 0
.\printer.go:1081:5: comparison of slice comments != nil should be len(comments) > 0
.\printer.go:1120:22: comparison of slice comments == nil should be len(comments) == 0

The one in line 1277 is the one from #33103.
The one in line 395 looks a lot like #33104.
The one in line 1511 is the one from #33105.

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.

@dominikh

This comment has been minimized.

Copy link
Member

@dominikh dominikh commented Jul 28, 2019

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 a -> a is incorrectly turning the nil slice into an empty slice, thus breaking an invariant of the AST. So, really, rewriting needs to be fixed instead.

@av86743

This comment has been minimized.

Copy link

@av86743 av86743 commented Jul 28, 2019

... It seems likely that a -> a is incorrectly turning the nil slice into an empty slice, thus breaking an invariant of the AST. ...

@dominikh

So you do have AST invariants (for golang.) Do you also have a tool/package which validates that these are satisfied?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.