Expand const impls of PartialEq, Eq, PartialOrd and Ord #146097

npmccallum · 2025-09-01T17:04:59Z

Related to: #143800

rustbot · 2025-09-01T17:05:04Z

rustbot has assigned @Mark-Simulacrum.
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

rustbot · 2025-09-01T17:05:07Z

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

oli-obk · 2025-09-01T19:06:40Z

Oh no. What did I do with rustfmt. const pub trait Foo is funny

tgross35 · 2025-09-02T21:48:36Z

library/core/src/slice/cmp.rs

    default fn chaining_le(left: &[Self], right: &[Self]) -> ControlFlow<bool> {
-        chaining_impl(left, right, PartialOrd::__chaining_le, usize::__chaining_le)
+        let l = cmp::min(left.len(), right.len());
+
+        // Slice to the loop iteration range to enable bound check
+        // elimination in the compiler
+        let lhs = &left[..l];
+        let rhs = &right[..l];
+
+        let mut i = 0;
+        while i < l {
+            match PartialOrd::__chaining_le(&lhs[i], &rhs[i]) {
+                ControlFlow::Continue(()) => {}
+                ControlFlow::Break(b) => return ControlFlow::Break(b),
+            }
+            i += 1;
+        }
+
+        usize::__chaining_le(&left.len(), &right.len())
    }


Can't all of this wait for const iterators? I don't know that the motivation is strong enough to justify switching to less maintainable code.

I mean, main issue with const iterators is that a large portion of use cases require const closures, and that's probably going to take a pretty long while. It's entirely feasible that const traits get stabilised before const closures are viable.

Plus, there's just going to have to be a massive audit of const code once const iterators are usable anyway, so, it doesn't seem that bad to add more while loops for usefully-const code. But I think we definitely should be marking all of them for future updating.

I can buy the fact that we may want const trait impls before we have const closures, and on a case-by-case basis we occasionally do iterator->indexing conversions to make const fn impls work. And the changes in this specific PR aren't that bad. But the net effect of doing this across the codebase adds up, and that part doesn't thrill me if the plan is just to rewrite it in the not-so-distant future. (I realize we need a real policy here, I've started writing one up.)

It's totally fine to say that the motivation is stronger than for a one-off impl because it unblocks a lot. But isn't SliceChain only used for a specialization anyway?

The main issue with specialisation impls not being made const is they can implicitly mess with the non-const impls if we're not careful: some traits have const bounds instead of [const] bounds and I'm not sure every specialization impl has an associated codegen test. (This is a valid reason to put more scrutiny on these implementations! I just wanted to point out how subtle the changes can be.)

Besides, they exist to speed things up for a reason, and considering just how much slower miri can be, this could probably substantially affect compile times using this code if not done carefully.

Definitely looking forward to whatever you write up for a policy, though. I think most of the code should be detectable with clippy lints that currently don't fire in const code, but it is a fair bit concerning nonetheless.

@tgross35 I agree with you that it isn't that great. I solved this by removing all the inlining in favor of regular const functions. See the latest push.

npmccallum · 2025-09-07T01:19:25Z

@tgross35 @clarfonthey @Mark-Simulacrum I think this is ready for review now.

Mark-Simulacrum

@rust-lang/project-const-traits (is this the right group?) -- this is adding a bunch of const-trait related impls to core, could we get someone to at least skim through and confirm we're OK to do so or if there's any concerns we should flag for this or future PRs with this kind of activity?

View changes since this review

Mark-Simulacrum · 2025-09-07T16:49:38Z

tests/ui/consts/fn_trait_refs.stderr

@@ -4,19 +4,6 @@ error[E0635]: unknown feature `const_fn_trait_ref_impls`
 LL | #![feature(const_fn_trait_ref_impls)]
   |            ^^^^^^^^^^^^^^^^^^^^^^^^


Hm, this test may have been broken for a while then? This seems suspicious.

Should I remove it?

fee1-dead · 2025-09-07T18:05:00Z

More const impls seem fine to me

clarfonthey · 2025-09-08T04:36:55Z

I feel like, out of principle, I shouldn't say whether these impls are good, because not only did you ignore #144847 which contains part of the changes here, but both you and the author of that PR ignored the fact that I had mentioned working on this in the tracking issue and just wrote the code anyway.

I wanted to do some conversion impls in a separate PR first (now merged) before getting to these, so, in theory, there isn't really any remaining blockers to doing this. But I'll let you and @Randl fight over whose code gets merged first, which you both could have avoided by reading the tracking issues you linked before writing anything.

Once the two PRs don't overlap (either by one being merged first, or by more-voluntary consensus) I can provide additional input.

npmccallum · 2025-09-08T10:53:26Z

@clarfonthey Apologies. I'm only an occasional contributor and haven't been following the issue closely. It looks like the other PR got merged. So I'm happy to rebase and see what's left. If my changes aren't good, please let me know how to improve them!

Also constify the impls of basic types. One potentially controversial part of this change is making Eq, a marker trait, const. I chose to do this ease user adoption. Otherwise, code which already has an Eq bound and uses it to proxy a PartialEq bound would need to add a separate bound. This would cause a litering of bounds in downstream types.

rustbot · 2025-09-09T15:55:50Z

This PR was rebased onto a different master 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.

rustbot assigned Mark-Simulacrum Sep 1, 2025

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