-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Constify Eq, Ord, PartialOrd #144847
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
base: master
Are you sure you want to change the base?
Constify Eq, Ord, PartialOrd #144847
Conversation
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Squash this into one commit please @rustbot author |
Reminder, once the PR becomes ready for a review, use |
@rustbot ready |
Not a review but just a question as I don't know the contexts very well 😅 |
This comment has been minimized.
This comment has been minimized.
Fair enough, I've added |
Sorry for taking a long time to review this. I'm actually somewhat uncomfortable by the scope of this PR. Could you please limit the number of types that this adds const impls for to just types that seem relevant for const programming? Specifically, things like:
Don't seem really relevant here. I think this constification could probably just stick to constifying some impls for built-in types and other important types, and not just try to constify everything that can be constified in the standard library. Also, since this adds new bounds to things, let's run a perf test. @bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Constify Eq, Ord, PartialOrd
@rustbot author |
pub trait Eq: PartialEq<Self> + PointeeSized { | ||
#[const_trait] | ||
#[rustc_const_unstable(feature = "const_cmp", issue = "143800")] | ||
pub trait Eq: [const] PartialEq<Self> + PointeeSized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, when I was working on this before I decided that I should do #144289 first, I figured that Eq
actually shouldn't be const even though it would make bounds easier because ultimately, for a majority of users, the difference between const Eq
and Eq
is negligible since Eq
itself doesn't contain any methods.
I guess it's a bit debatable, but I figured it wasn't worth distinguishing whether the Eq
impl was const or not, just the PartialEq
impl. It means that a lot of things will need Eq + [const] PartialEq
instead of [const] Eq
, but I figured that it would be better to make the former canonical than risk the former actually only being what's required despite the latter being easier to type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
risk the former actually only being what's required despite the latter
But the former implies the latter, right? There can't be Eq + [const] PartialEq
that is not practically [const] Eq
. So the difference is semantic and syntactic. From a semantic point of view, I see advantages of both approaches. x: [const] Eq
makes sense because it means that we want to use Eq
trait in a const context. On the other hand, if we interpret [const] trait
as "all the methods of trait are const callable," then any trait without methods shouldn't be const. From a syntactic point of view, I think [const] Eq
is clearly more convenient. We already have to sprinkle [const] Destruct
all over the place; having to be even more verbose is a disadvantage in my opinion. I guess that there should be some decision regarding the best practice for no-method traits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that you can't use Eq
in const contexts: Eq
is just a marker, and PartialEq
is what you're actually using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, at the risk of repeating myself, that depends on how you interpret [const] trait
. If [const] trait
means that "all methods of the trait are const callable" then you're 100% right. If it means "trait defines the functionality a particular type has that can be used in const context", then [const] Eq
makes sense: it defines functionality that can be used in const (via methods of PartialEq
, just like non-const version). This is purely semantic distinction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not involved much with const traits, Cc @fee1-dead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T: [const] Eq
is not so different from T: Eq + [const] PartialEq
if everyone writes impl const Eq for MyType
. On one hand, you can think of it as a silly additional requirement. There's no difference of impl const Eq
from impl Eq
if you know MyType
implements const PartialEq
.
On the other hand, it also seems silly to only allow derive_const
for PartialEq
and not Eq
. It isn't too much to ask people to implement [const] Eq
if they call a function that requires total equality in compile time.
I don't have a preference one way or the other.
Note that regressions on the PR you linked was ultimately triaged at the rollup. Those negative perf results are due to making built-in traits const, which then increases the times where we have to check const conditions of traits, find implied predicates (super traits), etc. They're pretty much unavoidable, there's nothing much we can do, and we'll have to stick with it if we want libstd constified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some leftover syntax, otherwise lgtm
This PR was rebased onto a different master commit! Check out the changes with our |
@bors r+ |
@bors p=4 (rollup interleaving) |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
Appears to be a legitimate codegen failure related to |
Ok. It is a legitimate codegen failure; I can reproduce it locally, including after rebase. I do not understand what is specifically wrong with the codegen (cc @scottmcm ), but the reason appears to be this change: I'm unsure why the difference between the while loop and the for loop results in incorrect codegen. I can revert this specific change for now (it doesn't give us slice rust/library/core/src/slice/cmp.rs Line 186 in f70c98f
) until we either figure out how to make the while loop work with codegen, or we can use for in const code (though if I understand correctly, this will require iterators, which by themselves will probably depend on this code).
|
☔ The latest upstream changes (presumably #145644) made this pull request unmergeable. Please resolve the merge conflicts. |
Adds
#[const_trait]
and impls forEq
,Ord
,PartialOrd
. Impl for some other traits (e.g., slices and arrays) are blocked mainly on const closures (#106003).For TypeId Ord we need const pointer comparison (#53020)
Tracking issue #143800