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

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

Closed
Deleplace opened this issue Sep 19, 2023 · 27 comments
Closed

Comments

@Deleplace
Copy link
Contributor

Deleplace commented Sep 19, 2023

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.

@gopherbot gopherbot added this to the Proposal milestone Sep 19, 2023
@ianlancetaylor ianlancetaylor changed the title proposal: slices: vet warns when ignoring the return value of Delete* proposal: cmd/vet: warn when ignoring the return value of slices.Delete* Sep 19, 2023
@ianlancetaylor
Copy link
Contributor

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

@bcmills
Copy link
Contributor

bcmills commented Sep 19, 2023

(Compare #20803 (comment).)

@timothy-king
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

The search also proves the mistake does sometimes happen:

image image

This was referenced Sep 20, 2023
@seankhliao
Copy link
Member

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
Copy link
Contributor

bcmills commented Sep 20, 2023

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
Copy link
Contributor Author

Deleplace commented Sep 20, 2023

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

@Deleplace
Copy link
Contributor Author

Some more results when searching for DeleteFunc and for Compact.

Nothing for CompactFunc.

@Deleplace
Copy link
Contributor Author

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
Copy link
Contributor

nemith commented Sep 20, 2023

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
Copy link
Contributor

bcmills commented Sep 20, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

image

@earthboundkid
Copy link
Contributor

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
Copy link
Contributor

I am persuaded.

@gopherbot
Copy link

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

@Deleplace
Copy link
Contributor Author

image

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
Copy link
Contributor Author

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

@znkr
Copy link
Contributor

znkr commented Dec 11, 2023

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
Copy link
Contributor Author

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
Copy link
Member

adonovan commented Jan 5, 2024

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
Copy link
Contributor Author

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

@adonovan
Copy link
Member

adonovan commented Jan 5, 2024

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 rsc changed the title proposal: cmd/vet: warn when ignoring the return value of slices.Delete* proposal: cmd/vet: warn when ignoring the return value of slices.Delete and friends Jan 10, 2024
@rsc
Copy link
Contributor

rsc commented Jan 10, 2024

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
Copy link
Contributor

rsc commented Jan 19, 2024

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.

@rsc rsc changed the title proposal: cmd/vet: warn when ignoring the return value of slices.Delete and friends cmd/vet: warn when ignoring the return value of slices.Delete and friends Jan 19, 2024
@rsc rsc modified the milestones: Proposal, Backlog Jan 19, 2024
@adonovan
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests