Remove redundant `PartialOrd` chaining methods by maxdexh · Pull Request #164684 · rust-lang/rust

maxdexh · 2026-10-21T12:01:49Z

I'm currently trying to do some optimizations using the PartialOrd chaining methods, and I noticed that __chaining_le and __chaining_lt are actually exactly the same for all implementations (and likewise __chaining_ge/__chaining_gt).

In particular,

__chaining_le/__chaining_lt returns Continue(()) if self == other, else Break(self <= other)/Break(self < other)
We require that a <= b iff a < b or a == b. So it doesn't matter (for well-behaved impls) if we break with self <= other or self < other.
All explicit impls of the chaining methods are in std, and well-behaved, as this is an internal API (except tuples and slices may not be well-behaved if their items aren't. For them, equivalence follows from the equivalence for their items, inductively).

All other impls of the chaining methods are using the default impl, based on matching Option<Ordering> from partial_cmp:

rust/library/core/src/cmp.rs

Lines 1613 to 1617 in 1731fae

    
           match <T as PartialOrd<U>>::partial_cmp(lhs, rhs) { 
        
               Some(Equal) => ControlFlow::Continue(()), 
        
               Some(c) => ControlFlow::Break(p(c)), 
        
               None => ControlFlow::Break(false), 
        
           }

Using Ordering::is_le/Ordering::is_lt for p is the same here, as Some(Equal) gets matched early. Thus, even non-well-behaved PartialOrd impls (i.e. those with non-conforming le/lt/eq methods) have identically behaving __chaining_le/__chaining_lt.

We can conclude that merging __chaining_le/__chaining_lt doesn't change behavior. You can verify the above points by looking at the diff, as every such method pair has been touched.

r? scottmcm
Picking you as reviewer because you also reviewed my earlier PR that optimized chaining ops on tuples, and appear to also have done some work on these methods before. Feel free to reroll.

Remove redundant `PartialOrd` chaining methods

maxdexh · 2026-10-21T12:14:36Z

Oh I just realized, the regression test outputs for #163598 are overwriting each other for different panic strategies when I bless.
I should have put // EMIT_MIR_FOR_EACH_PANIC_STRATEGY when I added that test 😬
@bors try

Edit: Ah, seems that Jules already fixed it: #164638

Remove redundant `PartialOrd` chaining methods

rust-bors · 2026-10-21T13:14:37Z

☀️ Try build successful (CI)
Build commit: 8a96fbf (8a96fbf9ef84f6f7fe87d5d40e438324f922ef1f)
Base parent: 1731fae (1731faea0da5b4ebf881d8e53f98066c17a74711)

rust-timer · 2026-10-21T14:04:59Z

Finished benchmarking commit (8a96fbf): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (secondary 0.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[0.6%, 8.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.8%	[-4.9%, -4.6%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.4%, secondary -0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	1.4%	[1.3%, 1.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-2.3%, -0.4%]	6
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Binary size

Results (primary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.1%]	3

Bootstrap: 490.972s -> 489.047s (-0.39%)
Artifact size: 406.53 MiB -> 406.48 MiB (-0.01%)

rustbot · 2026-10-21T15:43:43Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

scottmcm · 2026-10-21T16:04:22Z

Thanks for pinging me. I added these methods in the first place, so happy to review here.

(I definitely wasn't brave enough to try to add these to the derive, since the derive doesn't include lt either -- it used to, but it was a huge perf improvement to not.)

maxdexh · 2026-10-21T16:12:46Z

Interesting, I didn't know about the lt part. I was kinda hoping to add that too 👀

I'll keep trying around, if it doesn't lead to anything that's fine.

scottmcm · 2026-10-21T16:15:08Z

    bb1: {
        StorageLive(_6);
-        _6 = Ge(copy _3, copy _4);
+        _6 = Gt(copy _3, copy _4);


(this comment is me musing, not a 👍 nor a 👎)

I'm always torn on this kind of change. There's a certain obviousness to "well of course we'll emit a Gt if you use >". But I also agree that it shouldn't matter. But also in LLVM x < 0 stays that way but x <= 0 changes to x < 1 so it might matter. But optimizations should be handling those canonicalizations properly later. But also equivalent things aren't always. …

View changes since the review

I flipped a coin to decide which one to remove

scottmcm · 2026-10-21T16:35:05Z

    /// Same as `__chaining_lt`, but for `>` instead of `<`.
    #[inline]
    #[unstable(feature = "partial_ord_chaining_methods", issue = "none")]
    #[doc(hidden)]
    fn __chaining_gt(&self, other: &Rhs) -> ControlFlow<bool> {
        default_chaining_impl(self, other, Ordering::is_gt)
    }


Having read through this, I think I quite like it. There's a note I wrote above that this might not be stabilization-track that was in part because needing 4 more just felt messy, and having it drop from 4 to 2 is a massive improvement to that. (Not saying that I now think it ought to get stabilized, necessarily, but at least it's less "this is too icky".)

That said, I think with the change we should lean into the two-vs-four more heavily in the naming and comments. How about changing the names to __chaining_less and __chaining_greater here or __chaining_[lg] or something? I would prefer not saying lt if it's the thing you should use for le as well; I think that makes it less clear.

Then similarly the description would update to not talk about </<=/>/>=, but instead about how the full ordering is represented in the ControlFlow<bool> (and of course continue to say why this is fruitful over Option<Ordering> in some situations).

View changes since the review

I like the less change. I feel like having < and > be implemented by different methods was a mistake ^^

wasn't entirely sure what you meant by "how the full ordering is represented", so i did it differently.

scottmcm · 2026-10-21T16:43:13Z

Interesting, I didn't know about the lt part

I think it's #81384 ?

maxdexh · 2026-10-21T16:47:14Z

Interesting. Reading that makes it sound like there were some bigger issues going on with those methods though. I'll take that as more motivation to experiment with derive!

maxdexh · 2026-10-21T16:57:36Z

@rustbot ready

scottmcm · 2026-10-21T17:11:16Z

On the polarity side: there are some interesting questions about whether one should expect the LHS to be the LHS or not in comparisons. If you have <T as PartialOrd<U>>::partial_cmp, it's not flippable.

Probably not this PR, but you could consider looking at some of the specific impls to see whether they need the asymmetry support, and delegate from one to the other in symmetric impls. (Are slices and tuples only symmetric? I don't actually remember. Though might also be worth checking with libs(-api) to see whether that's intentional or incidental before relying on it.)

maxdexh · 2026-10-21T17:14:55Z

Ah right, polarity only applies when you have both directions...
Yeah we need both directions then, assuming we ever want to support these methods for PartialEq<NotSelf>. Yeah slices and tuples are symmetric only IIRC, and it'd break inference to change that

maxdexh · 2026-10-21T17:23:16Z

Note that we could also get rid of __chaining_gt by using __chaining_lt with reversed argument order, but that would observably change behavior for non-well-behaved impls that don't have partial_cmp(b, a) == partial_cmp(a, b).map(Ordering::reverse) (violating of the requirement a < b iff b > a).
I'll hold off on this for now, although I'm open to doing that as well.

Removed the section from the pr description, don't want to gaslight anyone reading the git log.

But hey, at least we have a good reason for why we actually need both of these methods in case we ever want to stabilize them ^^

scottmcm · 2026-10-21T17:46:02Z

+    fn chaining_less(left: &[Self], right: &[Self]) -> ControlFlow<bool> {
        match SliceOrd::compare(left, right) {
            Ordering::Equal => ControlFlow::Continue(()),
            ne => ControlFlow::Break(ne.is_lt()),


not this PR: looking at this again, now that it doesn't need to be lt specifically, I wonder if changing this to

Suggested change

ne => ControlFlow::Break(ne.is_lt()),

Ordering::Less => ControlFlow::Break(true),

Ordering::Greater => ControlFlow::Break(false),

would be better or worse.

It's certainly clear, and avoids the lt mention, but I have no idea whether it'd be better.

View changes since the review

I'll add it to my list of things to try, but I suspect no difference

In codegen it's a 3-way switch vs two nested brs, so while ideally it wouldn't matter, it's the kind of thing where it's never as obvious as one would like.

scottmcm · 2026-10-21T17:55:24Z

-            fn __chaining_lt(&self, other: &Self) -> ControlFlow<bool> {
+            fn __chaining_less(&self, other: &Self) -> ControlFlow<bool> {
                let (lhs, rhs) = (*self, *other);
+                // We arbitrarily pick `<` here. Using `<=` gives the same result.


Started #t-compiler/llvm > `lt` vs `le` when you know it's not equal to ask if there's a non-arbitrary choice. Don't need to block on that, of course.

View changes since the review

scottmcm · 2026-10-21T18:11:49Z

observably change behavior for non-well-behaved impls

Note that that's ok; we're releasing a change like that in 3 weeks :) #161391

maxdexh · 2026-10-21T18:15:16Z

~~Oh wait, do I need a libs reviewer (for some reason I thought you were libs)?~~ Nvm I was quicker to ask than you were to r+ ^^

scottmcm · 2026-10-21T18:16:03Z

Thanks, looking good! I like the note about where the difference happens between < and <=.

@bors r+ rollup=maybe (I'm on the fence for this. it can affect codegen so I worry and would normally put iffy, but also the perf showed clear so probably better to roll it up and it's not supposed to really affect anything)

rust-bors · 2026-10-21T18:16:06Z

📌 Commit 9ff9ee8 has been approved by scottmcm

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.

Reason for tree closure: github outage

scottmcm · 2026-10-21T18:30:01Z

for some reason I thought you were libs

I used to be on the review rotation for libcore. I had to leave that a while ago, but I still take reviews from time to time :)

…uwer Rollup of 6 pull requests Successful merges: - #164383 (handle higher-ranked preds in imperfect derives diagnostic) - #164620 (Wrap expected_ty for cfi-{diag,recover} in with_no_trimmed_paths) - #164684 (Remove redundant `PartialOrd` chaining methods) - #158076 (Offload safe mutable args with `Region` and `PartitioningStrategy`) - #164680 (Add regression test for async fn declared with #[eii]) - #164696 (Add `#[default]` for `newtype_index!`)

Rollup merge of #164684 - maxdexh:partial-ord-chain-api-simplify, r=scottmcm Remove redundant `PartialOrd` chaining methods I'm currently trying to do some optimizations using the `PartialOrd` chaining methods, and I noticed that `__chaining_le` and `__chaining_lt` are actually exactly the same for all implementations (and likewise `__chaining_ge`/`__chaining_gt`). In particular, - `__chaining_le`/`__chaining_lt` returns `Continue(())` if `self == other`, else `Break(self <= other)`/`Break(self < other)` - We require that `a <= b` iff `a < b` or `a == b`. So it doesn't matter (for well-behaved impls) if we break with `self <= other` or `self < other`. - All explicit impls of the chaining methods are in std, and well-behaved, as this is an internal API (except tuples and slices may not be well-behaved if their items aren't. For them, equivalence follows from the equivalence for their items, inductively). - All other impls of the chaining methods are using the default impl, based on matching `Option<Ordering>` from `partial_cmp`: https://github.com/rust-lang/rust/blob/1731faea0da5b4ebf881d8e53f98066c17a74711/library/core/src/cmp.rs#L1613-L1617 Using `Ordering::is_le`/`Ordering::is_lt` for `p` is the same here, as `Some(Equal)` gets matched early. Thus, even non-well-behaved `PartialOrd` impls (i.e. those with non-conforming `le`/`lt`/`eq` methods) have identically behaving `__chaining_le`/`__chaining_lt`. We can conclude that merging `__chaining_le`/`__chaining_lt` doesn't change behavior. You can verify the above points by looking at the diff, as every such method pair has been touched. r? scottmcm Picking you as reviewer because you also reviewed my earlier PR that optimized chaining ops on tuples, and appear to also have done some work on these methods before. Feel free to reroll.

rustbot assigned scottmcm Oct 21, 2026

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 21, 2026

This comment was marked as outdated.

Sign in to view