Skip to content

Conversation

ebroto
Copy link
Contributor

@ebroto ebroto commented Sep 23, 2020

This change tries to simplify the solution for problematic cases but is less restrictive than #6006.

  • We can't dereference shared references to non-Copy types, so the new suggestion does not do that. Note that this implies that the suggested closure parameter will be a reference.
  • We can't take a reference to the closure parameter in the returned key, so we don't lint in those cases. This can happen either because the key borrows from the parameter (e.g. |a| a.borrows()), or because we suggest |a| Reverse(a). If we did we would hit this error:
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

Note that Clippy does not currently have the (MIR-based) machinery necessary to check that what is borrowed is actually the closure parameter.

changelog: [unnecessary_sort_by]: avoid dereferencing the suggested closure parameter

Fixes #6001

@rust-highfive
Copy link

r? @phansch

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 23, 2020
@phansch
Copy link
Contributor

phansch commented Oct 6, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2020

📌 Commit 9365660 has been approved by phansch

@bors
Copy link
Contributor

bors commented Oct 6, 2020

⌛ Testing commit 9365660 with merge c9fdeef...

@bors
Copy link
Contributor

bors commented Oct 6, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing c9fdeef to master...

@bors bors merged commit c9fdeef into rust-lang:master Oct 6, 2020
@ebroto ebroto deleted the unnecessary_sort_by_take_2 branch October 6, 2020 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect suggestion in "unnecessary-sort-by"
4 participants