I intend to implement this.

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

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

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

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

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?

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?

Adding vet experts to discuss the proposed check and semantics.

@robpike @mvdan @josharian

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.

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

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.

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.

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.

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.

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