cmd/vet: warn when ignoring the return value of slices.Delete and friends

[Deleplace]

Calling slices.Delete while ignoring the returned slice value is almost certainly a bug, doing some element shifting but losing track of which elements are kept/discarded.

See sample: https://go.dev/play/p/2jV918bTDAX

Suggestion

vet warning:

./prog.go:11:14: ignoring the return value of slices.Delete

Same for DeleteFunc, Compact and CompactFunc.

[ianlancetaylor]

One of the criteria for vet checks is frequency. How often do people make this mistake?

[bcmills]

(Compare #20803 (comment).)

[timothy-king]

@ianlancetaylor There is already an x/tools/go/analysis/passes/unusedresult checker. These functions would fit into that I believe. The additional [computational] cost of adding these there would be low. This partially addresses the vet frequency requirement.

[Deleplace]

Frequency: looking at the results of this GitHub search, the rate of incorrect usage seems low. Almost all of the matched lines are using either return, or =, or :=.

[Deleplace]

The search also proves the mistake does sometimes happen:

[seankhliao]

doesn't appear to be too common https://github.com/search?type=code&q=language%3AGo+%2F%5E%5Cs%2Bslices.Delete%5C%28%2F&p=1

[bcmills]

Given how new slices.Delete is, I would not expect to have much of a sample to work with one way or the other.

But this is also one of those kinds of bugs where the bug is likely to be detected during testing: the vet check could make the test failures much easier to diagnose, but in its absence the bugs are likely to be under-represented in committed code.

[Deleplace]

The GitHub search results include usage of golang.org/x/exp/slices, which has been around for a while.

[Deleplace]

Some more results when searching for DeleteFunc and for Compact.

Nothing for CompactFunc.

[Deleplace]

We often see this pattern:

slices.Sort(s)
slices.Compact(s)

It's not obvious at first glance for everyone that the first line is correct, and the second is not.

[nemith]

I literally ran into this issue yesterday with DeleteFunc. It was obvious what the problem was, but it caught me off guard.

It took a little bit to fix, but a vet would have told me before even getting to the unittests. I think i made the mistake as I was updating a map at the same time with delete and it became very obvious once I took a longer look at it. I can imagine someone newer to the language without knowledge of slice semantics (something that I have had to teach to many people new to Go) could easily run into this and not understand why.

I am not sure how to capture hard number data on issues that are not checked into code.

[bcmills]

I am not sure how to capture hard number data on issues that are not checked into code.

In theory, given the new telemetry mechanism we could add the proposed check to cmd/vet, but have it increment a telemetry counter instead of actually emitting the warning.

That said, that approach seems strictly more complicated than just adding this case to the existing unusedresult checker. 😅

[Deleplace]

That said, that approach seems strictly more complicated than just adding this case to the existing unusedresult checker.

agree!

A profileration of telemetry metrics comes with subtle security concerns. A proliferation of vet sanity checks only comes with a (modest and measurable) performance burden.

[Deleplace]

Projects are responding very positively when the bug is brought to their attention. The messenger did not get shot!

[earthboundkid]

In #57433, @rsc said an unused result check would be added for the slices functions that need to be reassigned to work. That resolved my concern about using pointers to slices to prevent misuse. I still think we should do it.

[ianlancetaylor]

I am persuaded.

[gopherbot]

Change https://go.dev/cl/530116 mentions this issue: go/analysis/passes/unusedresult: Add functions from slices

[Deleplace]

FWIW, a whopping 84% of people are guessing wrong in an informal poll.

This implies a misconception about how slices are supposed to work (you'd either want to not change the length like with slices.Sort, or to pass a pointer to the slice, or to take the return value like we do with append), but my point is that such innocent misuses will often pass code reviews.

[Deleplace]

I now realize that slices.Replace deserves the same warning.

[znkr]

Inside Google there's been a few instances where people were confused about this API, used it incorrectly, and then spend time to understand what is happening. This analyzer would have saved them a lot of time.

[Deleplace]

I now realize that slices.Insert, slices.Clip and slices.Grow deserve the same warning.

slices.Clone too, even though Clone is not subtle or ambiguous.

[adonovan]

I was blissfully unaware of this proposal and independently implemented the change today in https://go.dev/cl/554317. (How much faster things move when you completely bypass the proper process!) Sorry about that.

The new list includes Clip, Compact{,Func}, Delete{,Func}, Grow, Insert, Replace, but not Clone for (by chance) the same reason given in the previous comment.

Happy to revert the change if necessary, but it seems like there's a clear consensus in favor of accepting this proposal; I'll add this to the agenda for next week's proposal review.

[Deleplace]

The pending CL https://go.dev/cl/530116 has tests! So we'll want to merge them somehow.

[adonovan]

The pending CL https://go.dev/cl/530116 has tests! So we'll want to merge them somehow.

I'm not convinced it's necessary. The tests exercise the mechanism; they needn't exercise every data point in the input configuration.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Details: Warn about ignored return values for the new package slices functions: Clip, Compact, CompactFunc, Delete, DeleteFunc, Grow, Insert, Replace.

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Details: Warn about ignored return values for the new package slices functions: Clip, Compact, CompactFunc, Delete, DeleteFunc, Grow, Insert, Replace.

[adonovan]

Phew. ;-) This was implemented (prematurely) in #62729 (comment).