Go: cmd/vet: warn about slice being compared to nil

[rillig]

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]

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

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

[dominikh]

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

@rillig welcome to golang.

[rillig]

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

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

~~~plain
$ ~/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]

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]

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