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

The std::cmp::Ord docs should be specific about how implementations "must agree" with PartialEq/Eq/PartialOrd #57157

Closed
pm215 opened this issue Dec 28, 2018 · 2 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@pm215
Copy link

pm215 commented Dec 28, 2018

https://doc.rust-lang.org/std/cmp/trait.Ord.html says "Implementations of PartialEq, PartialOrd, and Ord must agree with each other. It's easy to accidentally make them disagree by deriving some of the traits and manually implementing others." -- but it doesn't define in what ways specifically these trait implementations "must agree with each other". I think it would be useful if it did. (Is there also a requirement for consistency with Eq ?)

This is particularly significant because std::collections::BinaryHeap does not provide a method for "create heap which sorts by this comparison function", so people who aren't trying to implement arithmetic types are nonetheless quite likely to find they need to implement Ord for a type, and worse still Ord with an oddball ordering like "reverse order by struct field X and don't care about fields Y or Z" for a type that's probably using the derived implementations of Eq and PartialEq. Documenting the requirements between these traits would help in being confident that the set of impls you're writing fulfils them.

(Ideally we'd also improve BinaryHeap so fewer people ever needed to implement Ord; #38886 is a suggestion to do that, which was closed as "should be an RFC" but has no link to any corresponding RFC. I thought I'd seen an RFC at some point too but googling can't find me it again. Possibly some of the theoretical discussions in #50230 and its closed-due-to-inactivity pullreq touch on consistency requirements between Ord and other traits, but that conversation seems to have stalled and is trying to cover rather wider ground in any case.)

@sfackler
Copy link
Member

The agreement is pretty straightforward: a.cmp(b) == Ordering::Equal iff a == b.

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 28, 2018
@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Dec 28, 2018

(Is there also a requirement for consistency with Eq ?)

Eq doesn't have methods, so there is no way of accidentally implementing Eq wrongly if PartialEq, PartialOrd, and Ord were implemented correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants