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

New lint: Use Reverse with sort_by_key #5578

Closed
camsteffen opened this issue May 8, 2020 · 4 comments
Closed

New lint: Use Reverse with sort_by_key #5578

camsteffen opened this issue May 8, 2020 · 4 comments
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-complexity Lint: Belongs in the complexity lint group

Comments

@camsteffen
Copy link
Contributor

Suggesting a new lint to prefer using Reverse and sort_by_key.

Bad:

vec.sort_by(|a, b| b.foo().cmp(&a.foo()));

Good:

vec.sort_by_key(|e| Reverse(e.foo()));

It should work the same for sort_unstable_by / sort_unstable_by_key.

Lint name idea: sort_by_key_reverse

Maybe there are other cases where Reverse can simplify a sort?

@camsteffen camsteffen changed the title Use Reverse with sort_by_key New lint: Use Reverse with sort_by_key May 8, 2020
@flip1995 flip1995 added L-complexity Lint: Belongs in the complexity lint group E-medium Call for participation: Medium difficulty level problem and requires some initial experience. A-lint Area: New lints labels May 9, 2020
@JarredAllen
Copy link
Contributor

I can implement this.

@camsteffen
Copy link
Contributor Author

camsteffen commented May 18, 2020

I'm curious if this will work with a lambda that does any transformation to a and b? Does there need to be a complexity limit?

Another (contrived) example: vec.sort_by(|a, b| Ord::cmp(&d(e(b.f().g())), &d(e(a.f().g()))))

@JarredAllen
Copy link
Contributor

I think it should be possible to lint anything where b and a get the same transformation, so long as it looks completely identical (so things like b+b vs. a*2 would throw it off, even though they'd actually do the same thing), never mentions the other variable (so things like b+0*a would also throw it off), and never calls a function which could change one of it's arguments (by taking an &mut or some such).

bors added a commit that referenced this issue Jun 1, 2020
New lint: Use Reverse with sort_by_key

Implements  #5578
bors added a commit that referenced this issue Jun 1, 2020
New lint: Use Reverse with sort_by_key

Implements  #5578

changelog: New lint: [`unnecessary_sort_by`]
@phansch
Copy link
Member

phansch commented Jun 1, 2020

Closed with #5623

@phansch phansch closed this as completed Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-complexity Lint: Belongs in the complexity lint group
Projects
None yet
Development

No branches or pull requests

4 participants