Go: proposal: cmd/vet: append with a single argument should be a go vet warning

[natefinch]

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?

a := []int{1,2,3}
b := []int{4}
a = append(b)  // <------
fmt.Println(a)
// prints [4]

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

[natefinch]

Originally posted here: https://groups.google.com/forum/#!topic/golang-nuts/YXKJeuI8t_k

[cespare]

The frequency criterion is probably the questionable one here. We probably need some analysis of how often this occurs.

[robpike]

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.

[rwcarlsen]

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.

[dominikh]

3 matches in the Go corpus.

[bradleyfalzon]

For frequency (there are a few more):

https://github.com/kubernetes/kubernetes/blob/6430b1002c2900a1761cbd07cb77a1e4cf82b64e/pkg/kubelet/rkt/rkt_test.go#L941

Appears to be intentional:

https://github.com/kubernetes/heapster/blob/67f593913a56d49cf020ad6008217d37b507b7c8/integration/heapster_api_test.go#L575-L579

Possible edge case:

https://github.com/moby/moby/blob/db73f3daee21a28edbf6991d24a4653adc85f1a2/daemon/graphdriver/aufs/aufs_test.go#L763

[robpike]

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.

[robpike]

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.

[martisch]

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:
"The variadic function append appends zero or more values x to s of type S"
+
"A function with such a parameter is called variadic and may be invoked with zero or more arguments for that parameter"

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.

[dominikh]

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.

[natefinch]

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.

[robpike]

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.

[DrGo]

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.
Perhaps in Go 2.0, append() will get revamped, so reassigning to the same slice is no longer needed and this and other complaints about append() will go away.

[rsc]

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 go test, the essentially 100% accuracy might suggest doing it, to help people more directly (instead of having to debug a test failure). But it sure doesn't seem to happen much at all.

Note that x = append(x) is a no-op and maybe reportable but x = append(y) is not a no-op and probably not worth reporting. Yes it's the same as x = y but so is x = y+0 and we don't report that.

go-corpus-0.01/src/go.uber.org/zap/zbark/debark_test.go has

for _, l := range append(levels) {

which looks weird but then elsewhere in the file it says:

for _, l := range append(levels, zap.PanicLevel, zap.FatalLevel) {

so it looks like maybe a placeholder.

go-corpus-0.01/src/github.com/openshift/origin/pkg/auth/ldaputil/query_test.go has:

testSearchRequest := &ldap.SearchRequest{
    BaseDN:       "dc=example,dc=com",
    Scope:        ldap.ScopeWholeSubtree,
    DerefAliases: int(DefaultDerefAliases),
    SizeLimit:    DefaultSizeLimit,
    TimeLimit:    DefaultTimeLimit,
    TypesOnly:    DefaultTypesOnly,
    Filter:       "(objectClass=*)",
    Attributes:   append(DefaultAttributes),
    Controls:     DefaultControls,
}

and

expectedRequest: &ldap.SearchRequest{
    BaseDN:       DefaultBaseDN,
    Scope:        int(DefaultScope),
    DerefAliases: int(DefaultDerefAliases),
    SizeLimit:    DefaultSizeLimit,
    TimeLimit:    DefaultTimeLimit,
    TypesOnly:    DefaultTypesOnly,
    Filter:       fmt.Sprintf("(&(%s)(%s=%s))", DefaultFilter, DefaultQueryAttribute, "bar"),
    Attributes:   append(DefaultAttributes, []string{"email", "phone"}...),
    Controls:     DefaultControls,
},

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 x = append(y) and x = append(y, y1), so this is not the right place to complain.)

[rsc]

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.