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: Go 2: ban calling append with a single argument #30040

Open
kaedys opened this issue Jan 31, 2019 · 22 comments

Comments

@kaedys
Copy link

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Author

commented Jan 31, 2019

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

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Author

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

@robpike SGTM

cc @ianlancetaylor for Go2 discussion

@josharian josharian added the Go2 label Feb 1, 2019

@ianlancetaylor ianlancetaylor changed the title cmd/vet: check for append with no variadic args proposal: Go 2: ban calling append with a single argument Feb 1, 2019

@gopherbot gopherbot added this to the Proposal milestone Feb 1, 2019

@gopherbot gopherbot added the Proposal label Feb 1, 2019

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

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

This comment has been minimized.

Copy link

commented Feb 7, 2019

@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

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

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.

@dominikh

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Another example: https://play.golang.org/p/aPoMJZ4Po__8

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

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

Yes, I even checked the assembly. Both of them are optimized to no instructions; which I think means the compiler is tracking them in the exact same way. Somebody with more knowledge on the compiler can chime in .

I think the author was thinking that maybe append(a) creates a new slice (with a separate backing array).

I propose this should be a vet check for Go1 and a compiler error for Go2.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

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

Yes, I even checked the assembly. Both of them are optimized to no instructions; which I think means the compiler is tracking them in the exact same way. Somebody with more knowledge on the compiler can chime in .

Yep.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Code like a = append(b) works today, although it is the same as a = b, and people use it. At the moment I can't see a good reason to break that code.

But since it seems that it does happen in practice, I think a vet check would be reasonable.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

I fail to see why if you're going to block it by a vet check, you don't just block it up front with the compiler and avoid the overhead of looking for it on every test build. Either way it becomes illegal.

@agnivade

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

go test runs a subset of the vet checks. If we are going to add it to that subset, then yes it makes sense to block it up front. Otherwise, we can add it to the larger set of checks executed by go vet.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

Even if it's not in the set that blocks the build, many programmers work in environments where vet must be silent. Adding a vet check in effect disables the property in the wild.

If you're going to block this with vet, it's better to disable it in the language.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

I believe that there is plenty of code out there that people use without running go vet on it. For example, all vendored code. If we change the language, we break all that code immediately. If we add a vet check, we only break the code that people write themselves, not the code they depend on.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2019

Then I guess we can drop this, because this is not a problem worthy of a vet check. It's just a no-op, and we don't check for no-ops in vet, even ones that occur far more frequently than this one.

@kaedys

This comment has been minimized.

Copy link
Author

commented Feb 17, 2019

we don't check for no-ops in vet, even ones that occur far more frequently than this one.

We check for redundant boolean arguments, which falls into a similar category, since it's specifically checking for code that doesn't do anything under the expectation that the coder intended it to do something (or if not, can remove the extraneous code). I fail to see how no-op appends, which are almost always a mistake in coding (that are furthermore difficult to detect or debug) fail to pass muster when redundant booleans do.

I also object to the notion that adding vet checks violates the Go 1.0 guarantee, since we recently added vet unconditionally to go test, which "broke" builds that were running go test but not go vet and had vet issues (our code ran into this when we upgraded from Go 1.8 to 1.11). Yet this was not seen as a "breaking change", so I fail to see how adding a vet check that was not previously there, especially if it is not included in the default list of go test checks (though I fail to see why that should be the case), would be considered a breaking change.

Lastly, is there any check in at least the list of vet checks run during go test for which your logic of it should just be illegal in the base language does not apply? The point of vet is to find issues that are not illegal to compile but are almost certainly errors or mistakes in coding. Yet by your logic, vet checks have largely become the dreaded Compiler Warnings, since, as you point out, many (but not all) work in environments where vet must be silent for a build to succeed. If we're comfortable with changes to vet (like running it during go test) not breaking the compatibility guarantee, and we're comfortable with some checks being in vet even though your own logic says they should be compilation-illegal, then how does that logic sustain to resist the idea of putting an empty append check into go vet?

@tandr

This comment has been minimized.

Copy link

commented Mar 14, 2019

I have stepped on the same problem in our code twice - people forgot to add a second parameter, or it got "safely deleted" during the refactoring. So it does happen, and it was causing a mild error in both cases

@JAicewizard

This comment has been minimized.

Copy link

commented Jun 10, 2019

I have often heard that the go team wants to start implementing small spec changes, adding and removing things that are there just because of the go1 guarantee.
If this is true then I think this would be a great example of a small change that would have minimal impact on running code, and would likely fix bugs.
Maybe add a vet check for 1.13 if you are worried about breaking code and then change the spec in 1.14.
I think this proposal would be something that you can use to test how people react to changes and how to go about them in the future.

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