PartialOrd: transitivity and duality are required only if the corresponding impls exist #118108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
RalfJung
added
T-libs-api
RalfJung
force-pushed
the
partial-ord
branch
from
801f4af
to
2c5dc64
Compare
cynecx
reviewed
Nov 20, 2023
…onding impls exist
RalfJung
force-pushed
the
partial-ord
branch
from
2c5dc64
to
0eecc9e
Compare
dtolnay
changed the title
|
r? libs-api |
joshtriplett
reviewed
Dec 5, 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>`, |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
The current state is that this is a must, no? "The comparison must satisfy, for all [...]".
There was a problem hiding this comment.
(Consolidating this to the similar discussion on PartialEq.)
|
I assume we want this to be consistent, despite the different starting points (one PR is relaxing the currently stable requirements/recommendations for I can also merge one PR into the other if you prefer. |
|
I've made this part of #115386. |
dtolnay
removed
the
I-libs-api-nominated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #87067. Currently, not even std itself upholds the requirements documented for
PartialOrd.This is basically doing for
PartialOrdwhat #81198 did forPartialEq. However, #81198 (likely accidentally) significantly weakened the transitivity requirement, which we are avoiding here: as of today, it is the case that ifA: PartialOrd<B>andB: PartialOrd<C>andC: PartialOrd<D>andA: PartialOrd<D>all hold, then ifa < b < c < d, we havea < 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
PartialOrdas well. I'm still nominating this for them again due to this change in how transitivity is handled.