document PartialEq, PartialOrd, Ord requirements more explicitly by RalfJung · Pull Request #85637 · rust-lang/rust

RalfJung · 2021-05-24T16:19:56Z

This is the result of discussion in #50230, in particular this summary comment.

rust-highfive · 2021-05-24T16:20:30Z

(rust-highfive has picked a reviewer for you, use r? to override)

RalfJung · 2021-05-24T16:21:46Z

 ///
-/// - asymmetry: if `a < b` then `!(a > b)`, as well as `a > b` implying `!(a < b)`; and
 /// - transitivity: `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
+/// - duality: `a < b` if and only if `b > a`.


This is probably the most questionable part of this PR -- it could be considered a new requirement. On the other hand, given that we demand symmetry for ==, it would seem really strange to not also demand duality here.

RalfJung · 2021-05-24T16:24:44Z

 ///
-/// - asymmetry: if `a < b` then `!(a > b)`, as well as `a > b` implying `!(a < b)`; and
 /// - transitivity: `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
+/// - duality: `a < b` if and only if `b > a`.


This is probably the most questionable part of this PR -- it could be considered a new requirement. On the other hand, given that we demand symmetry for ==, it would seem really strange to not also demand duality here.

RalfJung · 2021-05-24T16:29:03Z

+///
 /// The comparison must satisfy, for all `a`, `b` and `c`:
 ///
-/// - asymmetry: if `a < b` then `!(a > b)`, as well as `a > b` implying `!(a < b)`; and


I removed this line because:
It is redundant: it already follows from a < b being defined as partial_cmp(a, b) == Some(Less), which implies !(a > b) (defined as partial_cmp(a, b) != Some(Greater)).
"asymmetry" is the wrong term, an "asymmetric" relation is a relation that satisfies "if a < b then !(b < a)".
asymmtery (in the correct sense of the word) is a consequence of duality, so we could state it in the corollary section if you wish. antisymmetry is more closely related to what the docs are currently stating, but it is defined for <=-style relations: R is antisymmetric if R(a, b) && R(b, a) implies a == b.

RalfJung · 2021-05-24T16:30:48Z

 ///
-/// - asymmetry: if `a < b` then `!(a > b)`, as well as `a > b` implying `!(a < b)`; and
 /// - transitivity: `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
+/// - duality: `a < b` if and only if `b > a`.


There is another property here that we might want to add: compatibility of partial_cmp with ==:
when a1 == a2 and b1 == b2, then partial_cmp(a1, b1) == partial_cmp(a2, b2).

steveklabnik · 2021-05-24T19:36:23Z

cc @rust-lang/libs

BurntSushi · 2021-05-24T22:14:40Z

+/// If [`PartialOrd`] or [`Ord`] are also implemented for `Self` and `Rhs`, their methods must also
+/// be consistent with `PartialEq` (see the documentation of those traits for the exact
+/// requirememts). It's easy to accidentally make them disagree by deriving some of the traits and
+/// manually implementing others.


I think this sentence is begging for a code example. I was saying to myself, "Oh wow, I should watch out for that, I'm hoping an example comes next" while reading it.

Note that I did not write this sentence, I just moved it around. I don't know what the original author had in their mind when writing this.

So what I am saying is, this issue is pre-existing, and I hope my PR doesn't have to fix it. :) It seems largely orthogonal to what this PR is about.

BurntSushi · 2021-05-24T22:15:42Z

+/// If [`Ord`] is also implemented for `Self` and `Rhs`, it must also be consistent with
+/// `partial_cmp` (see the documentation of that trait for the exact requirements). It's
+/// easy to accidentally make them disagree by deriving some of the traits and manually
+/// implementing others.


Same thought as above on an example.

…rties are ensured by default impls

Co-authored-by: Jane Lusby <jlusby42@gmail.com>

RalfJung · 2021-06-15T08:20:43Z

@rust-lang/libs this has been sitting without a review for quite some time now

m-ou-se · 2021-06-20T21:18:07Z

Since unsafe code can't count on these requirements anyway, we don't have to be too careful with adding clarifications like this about what these traits mean. None of your changes are surprising new requirements. Just clarifications of what was already implied/known. Thanks!

@bors r+

We might want to clarify that 'must' in this documentation doesn't mean you get UB otherwise, and that you may not rely on these guarantees in unsafe code.

The HashSet documentation contains:

The behavior resulting from such a logic error is not specified, but will not result in undefined behavior. This could include panics, incorrect results, aborts, memory leaks, and non-termination.

We could add something like that here too.

bors · 2021-06-20T21:18:09Z

📌 Commit 45675f3 has been approved by m-ou-se

RalfJung · 2021-06-20T22:14:46Z

We might want to clarify that 'must' in this documentation doesn't mean you get UB otherwise, and that you may not rely on these guarantees in unsafe code.

That would be #73682. Could you post your suggestion there?

document PartialEq, PartialOrd, Ord requirements more explicitly This is the result of discussion in rust-lang#50230, in particular [this summary comment](rust-lang#50230 (comment)). Fixesrust-lang#50230.

Rollup of 8 pull requests Successful merges: - rust-lang#83739 (Account for bad placeholder errors on consts/statics with trait objects) - rust-lang#85637 (document PartialEq, PartialOrd, Ord requirements more explicitly) - rust-lang#86152 (Lazify is_really_default condition in the RustdocGUI bootstrap step) - rust-lang#86156 (Fix a bug in the linkchecker) - rust-lang#86427 (Updated release note) - rust-lang#86452 (fix panic-safety in specialized Zip::next_back) - rust-lang#86484 (Do not set depth to 0 in fully_expand_fragment) - rust-lang#86491 (expand: Move some more derive logic to rustc_builtin_macros) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

document PartialEq, PartialOrd, Ord requirements more explicitly This is the result of discussion in rust-lang#50230, in particular [this summary comment](rust-lang#50230 (comment)). Fixesrust-lang#50230.

Rollup of 8 pull requests Successful merges: - rust-lang#83739 (Account for bad placeholder errors on consts/statics with trait objects) - rust-lang#85637 (document PartialEq, PartialOrd, Ord requirements more explicitly) - rust-lang#86152 (Lazify is_really_default condition in the RustdocGUI bootstrap step) - rust-lang#86156 (Fix a bug in the linkchecker) - rust-lang#86427 (Updated release note) - rust-lang#86452 (fix panic-safety in specialized Zip::next_back) - rust-lang#86484 (Do not set depth to 0 in fully_expand_fragment) - rust-lang#86491 (expand: Move some more derive logic to rustc_builtin_macros) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

document PartialEq, PartialOrd, Ord requirements more explicitly
1cfc187

RalfJung mentioned this pull request May 24, 2021
Document "latent requirements" of PartialOrd #50230
Closed

rust-highfive assigned joshtriplett May 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2021

RalfJung commented May 24, 2021
View reviewed changes

BurntSushi reviewed May 24, 2021
View reviewed changes

RalfJung commented May 25, 2021
View reviewed changes

Comment threadlibrary/core/src/cmp.rs Outdated

RalfJung commented May 25, 2021
View reviewed changes

Comment threadlibrary/core/src/cmp.rs Outdated

make Ord doc style consistent with the other two; explain which prope…
a7abd13
…rties are ensured by default impls

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021

yaahc reviewed Jun 14, 2021
View reviewed changes

Comment threadlibrary/core/src/cmp.rs Outdated

wording
45675f3
Co-authored-by: Jane Lusby <jlusby42@gmail.com>

m-ou-se added the T-libs-api [DEPRECATED; DO NOT USE] label Jun 20, 2021

m-ou-se assigned m-ou-se and unassigned joshtriplett Jun 20, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2021

This was referenced Jun 21, 2021
Rollup of 9 pull requests #86500
Closed
Rollup of 8 pull requests #86502
Merged

bors merged commit e6732e0 into rust-lang:masterJun 21, 2021

rustbot added this to the 1.55.0 milestone Jun 21, 2021

m-ou-se mentioned this pull request Jun 21, 2021
Clarify documentation of traits with behavioral contracts (PartialOrd etc.) #73682
Closed

RalfJung deleted the partial-ord branch June 28, 2021 07:13

memoryruins mentioned this pull request Aug 4, 2021
PartialOrd documentation mixes functions and operators #87485
Closed

dfoxfranke mentioned this pull request Jun 4, 2024
Documentation should clarify what it means for PartialEq and Ord/PartialOrd to "agree" #67938
Closed

DDtKey mentioned this pull request Jul 8, 2024
Why OrderedFloat is not IEEE conformant? reem/rust-ordered-float#36
Open

Uh oh!

Uh oh!

Conversation

RalfJung commented May 24, 2021

Uh oh!

rust-highfive commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveklabnik commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Jun 15, 2021

Uh oh!

m-ou-se commented Jun 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jun 20, 2021

Uh oh!

RalfJung commented Jun 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

m-ou-se commented Jun 20, 2021•
edited
Loading