proposal: Go 2: ban calling append with a single argument

[kaedys]

So, this is the second time I've ran into this issue in the last couple months. Ya, it's a silly mistake, but it definitely falls into the "probably incorrect" category that vet is designed to check for:

var list []myType
for v := range someChannel {
    list = append(list)
}

The append call is missing the actual thing to append. While this is a perfectly syntactically-valid function call, it also is completely useless, and I can't think of a single valid situation where append with no variadic arguments is valid (note that this is distinct from appending a defined but empty slice, which has valid use cases). In every situation I've ever seen this, it was a programmer mistake where they simply forgot to include what they actually wanted to append, and since a variadic argument can include 0 arguments, it doesn't generate a compile failure and is fairly annoying to debug, since the slice in question just seems to be empty for no readily apparent reason.

I think this definitely meets the correctness and precision requirements for vet features, and at least in my experience is at least as common a mistake as something like unreachable code, passing locks by value, or redundant boolean expressions. A previous similar issue (#15117) noted that this is mechanically a no-op, but this is a no-op that is essentially always a programming error, not simply a no-op expression, which I think is what lets it fulfill the correctness requirement.

[kaedys]

@robpike tagging you in this, since you were the last word in the previously linked issue.

[robpike]

Same rules apply. See cmd/vet/README for adoption criteria.

I have never seen this in practice. I don't think it's worth doing a vet check.

Maybe the compiler could define it away.

[kaedys]

I think that was my point, I have seen it a number of times in practice. I just made the mistake myself, in fact, and spent perhaps 30 minutes debugging it. And I feel like I addressed the adoption criteria. The correctness and precision requirements seem self-obviously fulfilled. An empty append has no valid use cases that I can think of, so it seems like assuming it to be an error to be generally correct and lacking in false positives. So it's really a question of frequency.

Now, I've not dug into how vet actually does its checks, but since the frequency criteria seems to be based on the cost of such checks, I think it's relevant to look at here. For something like unreachable code, vet needs to analyze the actual flow of the code and check if there's any non-terminating case that could reach that code. For an append check, it's a simple syntax analysis, any append without at least 2 arguments automatically fails. Perhaps my understanding of vet is insufficient, but that check seems to me to be fairly cheap, as far as things go, and at least in my experience, the frequency of this error is on level with some of the rarer checks that vet already handles.

[josharian]

Maybe the compiler could define it away.

As an optimization, or as a (breaking) spec change making such code invalid?

The implementation of a vet check for this is pretty trivial, in case anyone wants to try it out on a code corpus to see how often it occurs. In fact, you can probably try this using some bigquery lookup with a simple regexp. (See e.g. https://medium.com/google-cloud/analyzing-go-code-with-bigquery-485c70c3b451)

cc @dominikh

[robpike]

@josharian I was thinking of just making such code invalid. It's nugatory. It would be easy to add a go fix module to rewrite them.

I have never seen one in the wild. I just did a grep and can't find any in my GOPATH, but I didn't do the bigquery search.

[josharian]

@robpike SGTM

cc @ianlancetaylor for Go2 discussion

[martisch]

In fact, you can probably try this using some bigquery lookup with a simple regexp.

Having a quick look with SQL and the Google Cloud BigQuery GitHub Activity Data - I was able to find some regexp matches (120 different lines, 1623 total matches), examples:

• https://github.com/flynn/flynn/blob/1033a42b27c1acee8d8bac742a0d65291ce31b2c/test/test_cli.go#L692
• https://github.com/moby/moby/blob/f8e29fdd683f8d1a79344df08342261c3991ad1b/image/rootfs.go#L41
• https://github.com/yanzay/autohome/blob/9b98aa56c498e21fd580dbbcd5459cdc0c15f0c6/modules/arduino/arduino.go#L95
• https://github.com/frezadev/colony-manager/blob/6ec588b081b9aab1d519cc27c3358ea26d942c92/controller/application.go#L988
• https://github.com/xingzhou/kubernetes/blob/master/pkg/kubelet/rkt/rkt_test.go
• https://github.com/akram/origin/blob/1b26a414ce1c2a4ca4f4491722a48a138aab6d3c/vendor/github.com/docker/docker/daemon/graphdriver/aufs/aufs_test.go#L768
• https://github.com/haxorof/terraform-provider-openstack/blob/b6e0cc9732f4379ac39a1506fbbd6f5cfea1438f/vendor/github.com/hashicorp/hcl2/hcl/merged.go#L174

[agnivade]

wow, even docker gets it wrong. From the code, it does look like it is giving the intended result. But why is it using append to achieve that is something I am not able to understand.

[beoran]

@agnivade

b := append(a) is not a no-op, b will be a new slice that refers to the to the same underlying array as a does. That could be useful for copy-on-write semantics as happens in docker in the Append() function above the code refer to. Arguably, this is an abuse of the function, since you could do a[0:len(a)], but I guess append(a) is shorter. See here for my playing around:

https://play.golang.org/p/5uwTF33EOw7

Since this is already used in real, working code, a go vet warning might still be the best approach.

[dominikh]

a[0:len(a)] is equivalent to a[:] which is equivalent to a. Similarly, append(a) is equivalent to a. There are no "copy on write" semantics here. Writing to, or appending to b will modify the memory pointed at by a in "surprising" ways, in particular in the way it interacts with capacity: https://play.golang.org/p/X4iOLsECduS

b := append(a) and b := a have equivalent behavior.