unnecessary_sort_by false positive on shared reference keys

[tdyas]

The unnecessary_sort_by lint is a false positive when it suggests to use sort_by_key for keys that are shared references. The key cannot be returned in that case as it will not survive long enough.

See this PR to the Pants project for an example.

Clippy triggers on this code:

$ cargo clippy
    Checking fs v0.0.1 (XXX/pants/src/rust/engine/fs)
error: use Vec::sort_by_key here instead
   --> fs/src/glob_matching.rs:539:5
    |
539 |     path_stats.sort_by(|a, b| a.path().cmp(b.path()));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `path_stats.sort_by_key(|&a| a.path())`
    |
note: the lint level is defined here
   --> fs/src/lib.rs:7:3
    |
7   |   clippy::all,
    |   ^^^^^^^^^^^
    = note: `#[deny(clippy::unnecessary_sort_by)]` implied by `#[deny(clippy::all)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by

error: aborting due to previous error

error: could not compile `fs`.

To learn more, run the command again with --verbose.

Following Clippy's suggestion results in:

$ cargo clippy
    Checking fs v0.0.1 (XXX/pants/src/rust/engine/fs)
error[E0507]: cannot move out of a shared reference
   --> fs/src/glob_matching.rs:539:29
    |
539 |     path_stats.sort_by_key(|&a| a.path());
    |                             ^-
    |                             ||
    |                             |data moved here
    |                             |move occurs because `a` has type `PathStat`, which does not implement the `Copy` trait
    |                             help: consider removing the `&`: `a`

error[E0515]: cannot return value referencing local variable `a`
   --> fs/src/glob_matching.rs:539:33
    |
539 |     path_stats.sort_by_key(|&a| a.path());
    |                                 -^^^^^^^
    |                                 |
    |                                 returns a value referencing data owned by the current function
    |                                 `a` is borrowed here

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0507, E0515.
For more information about an error, try `rustc --explain E0507`.
error: could not compile `fs`.

To learn more, run the command again with --verbose.

Following the next suggestion to remove the & marker results in:

$ cargo clippy
    Checking fs v0.0.1 (XXX/pants/src/rust/engine/fs)
error: lifetime may not live long enough
   --> fs/src/glob_matching.rs:539:32
    |
539 |     path_stats.sort_by_key(|a| a.path());
    |                             -- ^^^^^^^^ returning this value requires that `'1` must outlive `'2`
    |                             ||
    |                             |return type of closure is &'2 std::path::Path
    |                             has type `&'1 PathStat`

error: aborting due to previous error

error: could not compile `fs`.

To learn more, run the command again with --verbose.

Meta

• cargo clippy -V: clippy 0.0.212 (04488afe3 2020-08-24)
• rustc -Vv:

rustc 1.46.0 (04488afe3 2020-08-24)
binary: rustc
commit-hash: 04488afe34512aa4c33566eb16d8c912a3ae04f9
commit-date: 2020-08-24
host: x86_64-apple-darwin
release: 1.46.0
LLVM version: 10.0 

[ebroto]

This was fixed in #5756 which will be available with Rust 1.47.

I put up a minimal reproduction with the types involved in your issue in the playground, which uses a more recent version of Clippy. You can check there that the lint does not trigger anymore in this case.

[ebroto]

To be fair, the aforementioned PR fixed the second problem you identified, which avoids triggering the lint in your case, but the first problem was still there and should be fixed now by #6006 😅