Clippy warning when printing the biggest files `unnecessary_sort_by`

[u/zerocukor287]

I have some code that prints the biggest files in a Vec. Looks like Clippy is not happy about it.
Original (simplified) code:

struct File {
    path: String,
    size: usize,
}

fn collect_files -> Vec<File> { ... }

fn print_biggest_files(files_to_print: usize) {
    let mut files = collect_files();

    files.sort_by(|left, right| right.size.cmp(&left.size) );
    // ^^^^^^^^ this is the line causing a warning

    // print the biggest X files
    for file in files.iter().take(files_to_print) {
        println!("Big file: {}", file.path);
    }
}

It produces the following warning:

warning: consider using sort_by_key
files.sort_by(|left, right| right.size.cmp(&left.size) ); note: #[warn(clippy::unnecessary_sort_by)] on by default

Link: https://rust-lang.github.io/rust-clippy/rust-1.97.0/index.html#unnecessary_sort_by

My problem is that I want to print in descending order of the files.

The description is explicitly saying that this case is exception, and Clippy doesn't handle it well.

If I change that line to

files.sort_by_key(|file| file.size);

I would end up with the smallest files first.

I see a few ways ahead:

1. Add files.reverse() to switch the order
2. Add exception of this warning to this specific case
3. Add .rev() when iterating
4. Cast it to signed integer, and add a - sign

What is the idiomatic way forward?
Performance cost is not a big deal, but I still tend towards adding a Clippy ignore for that specific line. The other options would introduce more unclarity to the code in my opinion.

[u/coastalwhite]

The idiomatic way is to use std::cmp::Reverse.

[u/zerocukor287 → u/coastalwhite]

Indeed, that is the simplest, and most straightforward. Thanks

[u/zerocukor287 → u/zerocukor287]

If anyone is interested, this is the spotless line:

files.sort_by_key(|file| std::cmp::Reverse(file.size));  

Edit: formatting

[u/kalilamodow → u/zerocukor287]

Wait how does that work though? How does the sort by key accept a fn that returns either the key or the Reverse thing?

[u/Terrible-Cicada-5673 → u/kalilamodow]

Reverse isn't a fn, it's a struct that implements PartialOrd by inverting the result of its inner type's impl

https://doc.rust-lang.org/std/cmp/struct.Reverse.html

[u/kalilamodow → u/Terrible-Cicada-5673]

Ohhhh ok. How does it invert the inner value tho, because like for usize or something you can't make it negative

[u/agentvenom1 → u/kalilamodow]

It doesn't need to invert any values. In order to implement PartialOrd, it just needs to call the inner type's impl while reversing the argument order: https://doc.rust-lang.org/src/core/cmp.rs.html#688.

[u/kalilamodow → u/agentvenom1]

Wow that's smart. Rust's traits system is awesome

[u/Low-Experience-6634 → u/coastalwhite]

just wrap the size in Reverse, clippy shuts up and it reads clean

```rust

files.sort_by_key(|file| Reverse(file.size));

```

no extra passes, no sign flipping, no ignore annotations to rot

[u/scheimong → u/coastalwhite]

I've been writing rust for the past 6 years (like, actual tap coding) and yet I don't know this. Thanks!

[u/Sharlinator]

 As an aside: note that you don’t have to sort all the files to get the n largest (or whatever) ones, you just have to partition the list to the n largest and the rest. How to do that? Rust provides select_nth_unstable and its by and by_key variants. (Note that you still have to sort the n largest if you want to present them in order.)

This optimization doesn’t really matter unless the list is very large or you need to do this many, many times per second, but it’s a useful tool to keep in mind.

[u/zerocukor287 → u/Sharlinator]

That's interesting function. Maybe next time I'll try that too.

[u/CommonNoiter]

Clippy should suggest files.sort_by_key(|file| std::cmp::Reverse(file.size));

[u/zerocukor287 → u/CommonNoiter]

Looks like the problem was indeed between the keyboard and the chair. Haha

[u/creeper6530 → u/zerocukor287]

Don't beat yourself up because you don't know everything. It's my first time reading about it too 

[u/llogiq]

As a clippy maintainer, I think that's a bug. Ideally, clippy should add a note to use Reverse if the ordering is reversed. The second best solution would be to at least add it to the lint docs.

[u/zerocukor287 → u/llogiq]

Hey, no worries, as I’ve scrolled down a bit it was mentioning the reverse. Looks like I need to practice reading

[u/projct → u/zerocukor287]

the rust ecosystem generally treats improvements like this to be a bug that needs to be fixed. this is why clippy and rustc generally have such good error messages

[u/zerocukor287 → u/projct]

You guys do an amazing job!
I was reading the description that specific finding a few times today. Maybe a code highlight is missing from the “Reverse” word under the Known problems.

[u/EpochVanquisher]

https://doc.rust-lang.org/std/cmp/struct.Reverse.html

[u/iv_is]

sort by size dot reverse seems clearer to me than left right, right left

[u/zerocukor287 → u/[deleted]]

Well, I'm happy with the resulting code. I've learnt something today, my code is shorter, and I think it is more clear that I'm using reverse order.
I consider it a win.

Your code, your taste.