PartialOrd: transitivity and duality are required only if the corresponding impls exist#118108
Closed
RalfJung wants to merge 1 commit intorust-lang:masterfrom
Closed
PartialOrd: transitivity and duality are required only if the corresponding impls exist#118108RalfJung wants to merge 1 commit intorust-lang:masterfrom
RalfJung wants to merge 1 commit intorust-lang:masterfrom
Conversation
Collaborator
|
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
Member
|
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>`, |
Member
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.)
Member
Author
There was a problem hiding this comment.
The current state is that this is a must, no? "The comparison must satisfy, for all [...]".
Member
There was a problem hiding this comment.
(Consolidating this to the similar discussion on PartialEq.)
Member
Member
Author
|
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. |
Member
Author
|
I've made this part of #115386. |
dtolnay
removed
the
I-libs-api-nominated
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.