Fix parameter order for _by() variants of min / max/ minmax in std::cmp

[miried]

We saw a regression introduced in version 1.86 that seems to be coming from switching the order of v1 and v2 when calling comparison functions in min_by / max_by / minmax_by (cf. this PR: #136307)

When the compare function is not symmetric in the arguments, this leads to false results. Apparently, the test cases do not cover this scenario currently. While asymmetric comparison may be an edge case, but current behavior is unexpected nevertheless.

[rustbot]

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[miried]

@thomcc please let me know if you have any suggestions

[thomcc]

Can you add a test covering this?

[thomcc]

Hi, I've nominated this because it's a behavioral change. I gather it reverts an earlier behavioral change that we made unintentionally.

[miried]

doctest added & rebased on latest master

[Amanieu]

We discussed this in the libs meeting. We would like to make this a documented guarantee. Could you update the documentation to actually guarantee that the ordering is preserved when passed to the closure?

@rfcbot merge

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[scottmcm]

I just want to emphasize that violating duality like this is already explicitly called out as "a logic error" in the trait documentation, and thus anyone writing things like this should expect that their garbage in will result in garbage out in various situations.

It's ok if we want to give something stronger for these functions, since we know that they only need one check (to the extent that the _by ones use FnOnce), but this should not be considered a more general guarantee about the order in which comparisons will be done in the library.

---

Also, this was an intentional change, since historically lt has worked better than le, as seen by things like #106107.

Hopefully it's better now, as a bunch of work has happened in rust and LLVM to try to improve things, but...