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

proposal: cmd/vet: add check for multiple appends to the same slice that are assigning return values to multiple variables #21202

Closed
medzin opened this issue Jul 28, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@medzin
Copy link
Contributor

commented Jul 28, 2017

Let's consider the following code (Go Playground link):

package main

import "fmt"

func main() {
	x := []int{0, 1}
	x = append(x, 2)
	y := append(x, 3)
	z := append(x, 4) // y will be modified here also
	fmt.Println(y, z) // a lot of people expect here [0 1 2 3] [0 1 2 4] but they will get [0 1 2 4] [0 1 2 4]
}

Go vet could report that kind of code as potential problem with an append modifying other slices that are pointing to the underlying array.

@gopherbot gopherbot added this to the Proposal milestone Jul 28, 2017

@gopherbot gopherbot added the Proposal label Jul 28, 2017

@medzin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2017

Here is the code that shows how such use of append can be problematic:

https://github.com/allegro/marathon-consul/blob/59157ee11d52e1e3ca51f45a947bb5a2f1a3a6b9/apps/app.go#L105-L132

Tags field (line 128) of RegistrationIntent structure, that is created in every loop iteration, can be overwritten in subsequent ones.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

See https://golang.org/src/cmd/vet/README for criteria for adding a vet check. I understand that this is a possible error but I don't see much evidence for "frequency", namely that it's a very common error. (It may be a common new Go developer error, but it seems like one that's not very common after the initial learning of what append does.)

Do you have any data about frequency?

There's also an issue with precision, namely flagging only bad code. Sometimes this kind of append might be correct, and we'd need to make sure that vet doesn't flag bad code or has a clear way to write that code to not look bad.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2017

@medzin, any thoughts on questions in previous comment?

@medzin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

I do not have any statistics on the frequency of this error - only the experience from my previous projects where it happened several times - and yes, errors were introduced by a new Go developers, but experienced developers were unable to catch it on code review also. The frequency criteria is not quite satisfied.

Do you have any examples that this kind of append might be correct?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

This doesn't meet the frequency criterion for vet changes. It would also be hard to implement (vet can not in general know how large the underlying array is, so the code might actually be working as intended).

@golang golang locked and limited conversation to collaborators Mar 19, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.