Skip to content
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.

Sign up for GitHub

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

Make Compare traits have ~const super traits. #107820

Closed
wants to merge 1 commit into from
Closed

Make Compare traits have ~const super traits. #107820

wants to merge 1 commit into from

Conversation

onestacked
Copy link
Contributor

This changes the trait definitions for Eq, PartialOrd and Ord to have ~const super trait bounds.

This simplifies const code that uses (for example) both the Ord and PartialEq Traits since no additional trait bounds will have to be added for those (only the existing ones changed to ~const).

cc @fee1-dead

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Feb 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@onestacked
Copy link
Contributor Author

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 9, 2023
@cuviper
Copy link
Member

cuviper commented Feb 9, 2023

r? libs-api

@rustbot rustbot assigned joshtriplett and unassigned cuviper Feb 9, 2023
onestacked
onestacked commented Feb 9, 2023
View reviewed changes
tests/ui/consts/issue-25826.stderr
= help: the trait `~const PartialOrd` is not implemented for `*const ()`
note: the trait `PartialOrd` is implemented for `*const ()`, but that implementation is not `const`
= help: the trait `~const PartialOrd<_>` is not implemented for `*const ()`
note: the trait `PartialOrd<_>` is implemented for `*const ()`, but that implementation is not `const`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those diagnostics changes are kind of bad, but I'm not really sure what causes them / how to fix them

onestacked
onestacked commented Feb 10, 2023
View reviewed changes
library/core/src/cmp.rs
@@ -1032,7 +1047,7 @@ pub macro Ord($item:item) {
)]
#[const_trait]
#[rustc_diagnostic_item = "PartialOrd"]
pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
pub trait PartialOrd<Rhs: ?Sized = Self>: ~const PartialEq<Rhs> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change causes the diagnostic regression, but I don't understand why, maybe caused by something in the compiler?
@oli-obk Do you know what causes this / how to fix it?

@onestacked
Copy link
Contributor Author

I'm also not certain if we really want Eq to be #[const_trait](and Ord: ~const Eq).
Since Eq doesn't have any functions it self, so anything you do with a ~const Eq you should also be able to do with a Eq + ~const PartialEq, but that is kind a long and weird trait bound.
Also having to make all the Eq impl's const seems not so great.
Maybe we could add a clippy lint in the future that warns if implementing const PartialEq and non-const Eq?

@bors
Copy link
Contributor

bors commented Apr 19, 2023

☔ The latest upstream changes (presumably #110393) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 19, 2023

For the near future we're avoiding any kind of ~const in libcore/libstd. We just removed all of it and will implement it from scratch just within ui tests.

I'm closing this PR as it's not actionable right now.

@fee1-dead fee1-dead closed this Apr 20, 2023
@fee1-dead
Copy link
Member

see #110395

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@onestacked @rustbot @cuviper @bors @oli-obk @fee1-dead @joshtriplett