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

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

Jump to bottom

Open

npmccallum wants to merge 7 commits into rust-lang:master

from npmccallum:const_cmp

Open

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

npmccallum wants to merge 7 commits into rust-lang:master from npmccallum:const_cmp

+360 −188

Conversation

npmccallum

Copy link

Contributor

@npmccallum npmccallum commented Sep 1, 2025 •

edited

Loading

Related to: #143800
Requires (includes): #146088
Requires (not included): #146089

@rustbot rustbot assigned Mark-Simulacrum

Sep 1, 2025

@rustbot

Copy link

Collaborator

rustbot commented Sep 1, 2025

r? @Mark-Simulacrum

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 rustbot added S-waiting-on-review T-libs labels

Sep 1, 2025

@rustbot

Copy link

Collaborator

rustbot commented Sep 1, 2025

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

@rust-log-analyzer

This comment has been minimized.

@oli-obk

Copy link

Contributor

oli-obk commented Sep 1, 2025

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

tgross35

tgross35 reviewed

Sep 2, 2025

View reviewed changes

library/core/src/slice/cmp.rs Outdated

Comment on lines 220 to 211

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())

}

Copy link

Contributor

@tgross35 tgross35 Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor

@clarfonthey clarfonthey Sep 2, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor

@tgross35 tgross35 Sep 2, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Contributor

@clarfonthey clarfonthey Sep 2, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@npmccallum npmccallum Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 npmccallum force-pushed the const_cmp branch from 1641931 to 30432d8 Compare

September 3, 2025 01:05

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

npmccallum added 7 commits

September 3, 2025 18:44

@npmccallum


 constify the Eq, PartialOrd and Ord traits

1308d51

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.

@npmccallum


 constify the min() and max() functions

2a1939b

@npmccallum


 constify comparison traits on many simple types

39aa5e4

@npmccallum


 constify comparison traits on slices

dd31c51

@npmccallum


 constify comparison traits on multiple sliced types

f7717a2

@npmccallum


 constify comparison traits on arrays

c966ab8

@npmccallum


 constify comparison traits on network addresses

8cb1c63

@npmccallum npmccallum force-pushed the const_cmp branch from 30432d8 to 8cb1c63 Compare

September 3, 2025 22:44

@rustbot

Copy link

Collaborator

rustbot commented Sep 3, 2025

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.

@rust-log-analyzer

Copy link

Collaborator

rust-log-analyzer commented Sep 3, 2025

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[TIMING:end] tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu } -- 0.000
fmt check
Diff in /checkout/library/core/src/cmp.rs:336:
 #[stable(feature = "rust1", since = "1.0.0")]
 #[rustc_diagnostic_item = "Eq"]
 #[rustc_const_unstable(feature = "const_cmp", issue = "143800")]
-pub const trait Eq: [const] PartialEq<Self> + PointeeSized {
+const pub trait Eq: [const] PartialEq<Self> + PointeeSized {
 // this method is used solely by `impl Eq or #[derive(Eq)]` to assert that every component of a
 // type implements `Eq` itself. The current deriving infrastructure means doing this assertion
 // without using a method on this trait is nearly impossible.
Diff in /checkout/library/core/src/cmp.rs:963:
 #[stable(feature = "rust1", since = "1.0.0")]
 #[rustc_diagnostic_item = "Ord"]
 #[rustc_const_unstable(feature = "const_cmp", issue = "143800")]
-pub const trait Ord: [const] Eq + [const] PartialOrd<Self> + PointeeSized {
+const pub trait Ord: [const] Eq + [const] PartialOrd<Self> + PointeeSized {
 /// This method returns an [`Ordering`] between `self` and `other`.
 ///
 /// By convention, `self.cmp(&other)` returns the ordering matching the expression
Diff in /checkout/library/core/src/cmp.rs:1348:
 #[rustc_diagnostic_item = "PartialOrd"]
 #[allow(multiple_supertrait_upcastable)] // FIXME(sized_hierarchy): remove this
 #[rustc_const_unstable(feature = "const_cmp", issue = "143800")]
-pub const trait PartialOrd<Rhs: PointeeSized = Self>:
+const pub trait PartialOrd<Rhs: PointeeSized = Self>:
 [const] PartialEq<Rhs> + PointeeSized
 {
 /// This method returns an ordering between `self` and `other` values if one exists.
fmt: checked 6354 files
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Build completed unsuccessfully in 0:00:48

Labels

S-waiting-on-review T-libs

7 participants

@npmccallum @rustbot @rust-log-analyzer @oli-obk @tgross35 @clarfonthey @Mark-Simulacrum

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

Are you sure you want to change the base?

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

Conversation

@npmccallum npmccallum commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 1, 2025

Uh oh!

rustbot commented Sep 1, 2025

Uh oh!

This comment has been minimized.

oli-obk commented Sep 1, 2025

Uh oh!

@tgross35 tgross35 Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

@clarfonthey clarfonthey Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@tgross35 tgross35 Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@clarfonthey clarfonthey Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@npmccallum npmccallum Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

rustbot commented Sep 3, 2025

Uh oh!

rust-log-analyzer commented Sep 3, 2025

Uh oh!

Uh oh!

@npmccallum npmccallum commented Sep 1, 2025 •

edited

Loading

@clarfonthey clarfonthey Sep 2, 2025 •

edited

Loading

@tgross35 tgross35 Sep 2, 2025 •

edited

Loading

@clarfonthey clarfonthey Sep 2, 2025 •

edited

Loading