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.

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

PartialOrd: transitivity and duality are required only if the corresponding impls exist #118108

Closed
wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Member

Fixes #87067. Currently, not even std itself upholds the requirements documented for PartialOrd.

This is basically doing for PartialOrd what #81198 did for PartialEq. However, #81198 (likely accidentally) significantly weakened the transitivity requirement, which we are avoiding here: as of today, it is the case that if A: PartialOrd<B> and B: PartialOrd<C> and C: PartialOrd<D> and A: PartialOrd<D> all hold, then if a < b < c < d, we have a < d. If we just did the same thing as #81198, we would lose that property. Therefore, we explicitly require transitivity for longer chains as well.

Libs-api decided here that they are fine with applying #81198 to PartialOrd as well. I'm still nominating this for them again due to this change in how transitivity is handled.

@rustbot
Copy link
Collaborator

rustbot commented Nov 20, 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 Nov 20, 2023
@RalfJung RalfJung added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 20, 2023
library/core/src/cmp.rs Outdated Show resolved Hide resolved
@dtolnay dtolnay changed the title PartialOrd: transitivty and duality are required only if the corresponding impls exist PartialOrd: transitivity and duality are required only if the corresponding impls exist Nov 21, 2023
@cuviper
Copy link
Member

cuviper commented Nov 30, 2023

r? libs-api

@rustbot rustbot assigned m-ou-se and unassigned cuviper Nov 30, 2023
/// - duality: `a < b` if and only if `b > a`.
/// - **Transitivity**: if `A: PartialOrd<B>` and `B: PartialOrd<C>` and `A:
/// PartialOrd<C>`, then `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
/// This must also work for longer chains, such as when `A: PartialOrd<B>`, `B: PartialOrd<C>`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This must also work for longer chains, such as when `A: PartialOrd<B>`, `B: PartialOrd<C>`,
/// This should also work for longer chains, such as when `A: PartialOrd<B>`, `B: PartialOrd<C>`,

(Acknowledging the current state of things here.)

Copy link
Member Author

Choose a reason for hiding this comment

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

The current state is that this is a must, no? "The comparison must satisfy, for all [...]".

Copy link
Member

@joshtriplett joshtriplett Dec 5, 2023

Choose a reason for hiding this comment

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

(Consolidating this to the similar discussion on PartialEq.)

@m-ou-se
Copy link
Member

m-ou-se commented Dec 5, 2023

@RalfJung Do you prefer we run an FCP on this now, or do you prefer to wait until we've resolved #115386 and can apply that resolution here as well?

@RalfJung
Copy link
Member Author

RalfJung commented Dec 8, 2023

I assume we want this to be consistent, despite the different starting points (one PR is relaxing the currently stable requirements/recommendations for PartialOrd, the other is strengthening them for PartialEq). So it probably makes sense to treat them together.

I can also merge one PR into the other if you prefer.

@RalfJung
Copy link
Member Author

I've made this part of #115386.

@RalfJung RalfJung closed this Dec 26, 2023
@RalfJung RalfJung deleted the partial-ord branch December 26, 2023 18:40
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
7 participants