Rust-clippy: Incorrect suggestion in "unnecessary-sort-by"

[RalfJung]

This code triggers the following suggestion:

error: use Vec::sort_by_key here instead
   --> src/report/mod.rs:223:5
    |
223 |     crates.sort_unstable_by(|a, b| a.id().cmp(&b.id()));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `crates.sort_unstable_by_key(|&a| a.id())`
    |
    = note: `-D clippy::unnecessary-sort-by` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by

However, the suggested code does not work: &a is rejected as a pattern, it says I "cannot move out of a shared reference". using just a for the closure argument works.

[ebroto]

I will work on this one

[RalfJung]

Restrict unnecessary_sort_by to non-reference, Copy types

Hm, that's not really a fix, is it? The lint was very useful here and helped improve the code. Just the auto-suggestions was wrong.

[ebroto]

The problem is that the lint suggests using Reverse where applicable

pub struct Reverse<T>(pub T);

For example in this case:

    // Reverse examples
    vec.sort_by(|a, b| b.cmp(a));

The suggestion ends up being:

    vec.sort_by_key(|b| Reverse(b));

If we don't destructure the referenced type, we hit https://github.com/rust-lang/rust/issues/34162

error: lifetime may not live long enough
  --> /home/ebroto/src/ebroto-clippy/tests/ui/unnecessary_sort_by.fixed:19:25
   |
19 |     vec.sort_by_key(|b| Reverse(b));
   |                      -- ^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
   |                      ||
   |                      |return type of closure is Reverse<&'2 isize>
   |                      has type `&'1 isize`

error: aborting due to previous error

But after giving this a bit more thought I agree and this is too restrictive, we should be able to identify when the argument to Reverse is a reference.

I will give this another shot.

[RalfJung]

Or maybe if the situation is too complicated (references / non-Copy types) it could not to the Reverse thing, but still detect cmp like before to suggest the by_key? That seems like it is unrelated in principle, though I know little about how clippy detects these things.

[wpbrown]

I ran in to failure with the suggestion applied to snapshots.sort_unstable_by(|a, b| b.datetime().cmp(&a.datetime())) where a and b are borrowed. I didn't know about Reverse :)

If we don't destructure the referenced type, we hit rust-lang/rust#34162

In my case I'm just taking the reference and giving Reverse an owned Copy type... DateTime, so it works fine snapshots.sort_unstable_by_key(|b| Reverse(b.datetime())).

Thank you for working on this. This is a great clippy.