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 user uses non-slice parameter to sort.Slice #33817

Closed
johanbrandhorst opened this issue Aug 24, 2019 · 15 comments

Comments

@johanbrandhorst
Copy link
Member

commented Aug 24, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.7 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

I used sort.Slice with a non-slice first parameter. It paniced at runtime.

https://play.golang.org/p/SqdG2hd5yE8

What did you expect to see?

A go vet warning

What did you see instead?

No go vet warning

I have already implemented an analyzer for this (thanks @matloob and @empijei).

@johanbrandhorst

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2019

I intend to implement this.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 24, 2019

Change https://golang.org/cl/191778 mentions this issue: cmd/vet: Add sortslice to vet analyzers

@gopherbot

This comment has been minimized.

Copy link

commented Aug 24, 2019

Change https://golang.org/cl/191598 mentions this issue: go/analysis: add sortslice pass

@martisch

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Will this new vet check complain when sort.Slice is used with an interface type as first argument, which can be a valid use case when the comparison function is abstracted with an interface and the concrete underlying type may still be slice? Simplified example: https://play.golang.org/p/KEFLcgiO_xU

@johanbrandhorst

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2019

Great question! It probably will, as implemented. I will fix it in the proposed implementation.

@johanbrandhorst

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2019

Oh shoot, is that going to be really hard to do? I had a quick look at it and I guess I can't get the concrete implementation from the parameter? Any ideas?

@martisch

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

We cant always statically prove what the concrete type is that was put into an interface typed variable because that information might be far away (different function) and only known at runtime (due to different control flow). Solving that for all programs is not possible as it would mean solving the halting problem. I think to avoid false alarms the vet check would need to only warn when a non interface type that is not a slice is passed to sort.Slice.

How often does this mistake happen and wont be discovered by simple tests resulting in a runtime error/panic?

@martisch martisch added the Proposal label Aug 25, 2019

@martisch

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Adding vet experts to discuss the proposed check and semantics.

@robpike @mvdan @josharian

@johanbrandhorst

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2019

Thanks for the input, I can certainly remove the false positive regarding an interface parameter being used. I underestimated the overall complexity of this vetting so I understand if this means we don't think it's useful enough for its own analyzer.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

First you must satisfy the three conditions in the vet README: correctness, frequency, and precision.

@johanbrandhorst

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2019

First you must satisfy the three conditions in the vet README: correctness, frequency, and precision.

To review:

Correctness:
Vet's checks are about correctness, not style. A vet check must identify real or potential bugs that could cause incorrect compilation or execution. A check that only identifies stylistic points or alternative correct approaches to a situation is not acceptable.

I think this check passes this test

Frequency:
Vet is run every day by many programmers, often as part of every compilation or submission. The cost in execution time is considerable, especially in aggregate, so checks must be likely enough to find real problems that they are worth the overhead of the added check. A new check that finds only a handful of problems across all existing programs, even if the problem is significant, is not worth adding to the suite everyone runs daily.

I am unsure it passes this test, but then I am also unsure where the threshold lies. I got the impression that the new analyzer implementation might make the cost of new checks very small, but I am unsure enough as to not argue that it passes this test.

Precision:
Most of vet's checks are heuristic and can generate both false positives (flagging correct programs) and false negatives (not flagging incorrect ones). The rate of both these failures must be very small. A check that is too noisy will be ignored by the programmer overwhelmed by the output; a check that misses too many of the cases it's looking for will give a false sense of security. Neither is acceptable. A vet check must be accurate enough that everything it reports is worth examining, and complete enough to encourage real confidence.

As explained by Martin in #33817 (comment), this test will necessarily suffer from a small set of false negatives in the case of interface types being used incorrectly. I again am not confident enough to say it passes this test.

Any more thoughts on my analysis? @matloob and @empijei were part of the discussion with me to add this check in the first place and may want to weigh in on either side.

@matloob

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

Correctness: I agree that this check clearly passes the Correctness test.
Frequency: I believe sort.Slice is used frequently enough but I'm also not sure where the bar is.
Precision: There aren't any false positives, and I believe false negatives will be rare enough that this shouldn't be an issue. I think it's uncommon that a user tries to call sort.Slice with an interface that's not a slice, but it can happen.

For now I think it should be uncontroversial to check this analysis into x/tools/go/analysis/passes and not hook it up into vet until a decision is made. We'll still be able to expose it through gopls.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

The frequency criterion is about how often the problem arises, not how often the function appears in source. Personally, I have never seen it happen, so I am skeptical.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

This bug will be noticed when the program crashes the first time the code path is taken. It's not subtle. As @martisch said, any tests that exercise such code would expose the bug.

@johanbrandhorst

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

Sounds to me like this doesn't quite pass the threshold to be considered for vet, thanks for all your thoughts.

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