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.

Sign up for GitHub

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

Jump to bottom

PartialEq, PartialOrd: update and synchronize handling of transitive chains #115386

Merged
merged 4 commits into from Feb 5, 2024
Merged

PartialEq, PartialOrd: update and synchronize handling of transitive chains #115386

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a63880c
PartialEq: handle longer transitive chains
RalfJung Nov 20, 2023
3e389ef
PartialOrd: transitivity and duality are required only if the corresp…
RalfJung Nov 20, 2023
baaf6d7
explain what crates should do when adding comparison with foreign types
RalfJung Dec 26, 2023
61d1ebe
fix typo
RalfJung Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 52 additions & 8 deletions library/core/src/cmp.rs
View file
Open in desktop
Expand Up @@ -61,11 +61,13 @@ use self::Ordering::*;
/// The equality relation `==` must satisfy the following conditions
/// (for all `a`, `b`, `c` of type `A`, `B`, `C`):
///
/// - **Symmetric**: if `A: PartialEq<B>` and `B: PartialEq<A>`, then **`a == b`
/// - **Symmetry**: if `A: PartialEq<B>` and `B: PartialEq<A>`, then **`a == b`
/// implies `b == a`**; and
///
/// - **Transitive**: if `A: PartialEq<B>` and `B: PartialEq<C>` and `A:
/// - **Transitivity**: if `A: PartialEq<B>` and `B: PartialEq<C>` and `A:
/// PartialEq<C>`, then **`a == b` and `b == c` implies `a == c`**.
/// This must also work for longer chains, such as when `A: PartialEq<B>`, `B: PartialEq<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: PartialEq<B>`, `B: PartialEq<C>`,
/// This should also work for longer chains, such as when `A: PartialEq<B>`, `B: PartialEq<C>`,

Attempting to account for the state of the ecosystem, 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.

Should the "must" above this enumeration also become a "should"? Or do you intentionally make a difference between the "basic" transitivity case and the cases involving longer chains (or symmetry, as per Mara's point)?

Copy link
Member

Choose a reason for hiding this comment

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

@RalfJung It's possible that the other "must"s here should also become "should"s, in practice, though longer chains are more likely to fail these properties. De facto, the ecosystem is going to continue to provide impls that don't satisfy all of these properties, and as a result, code can't have any definitive reliance on these properties. "should" acknowledges that these properties are more on the "try not to confuse the humans reading your code" side than the "allow computers to reason about your code" side.

Copy link
Member Author

Choose a reason for hiding this comment

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

as a result, code can't have any definitive reliance on these properties

We already document that "Violating these requirements is a logic error. The behavior resulting from a logic error is not specified, but users of the trait must ensure that such logic errors do not result in undefined behavior. This means that unsafe code must not rely on the correctness of these methods." In other words, we already expect a certain amount of resilience against these properties being violated.

I guess the open question is whether

  • we make this "must" and violating them is therefore declared a bug (albeit one that can, at worst, cause panics or logic misbehavior, not UB)
  • we make this "should" and... well I guess I am not sure what that would mean? Is now the code that relies (to the extent permitted by the docs) on the property the one that is buggy? Or is the conclusion that sometimes things just don't compose and we don't want to take a stance on where the bug lies in that case?

My personal preference would be to make this a "must" in both cases. But I could live with a "should" as well. Until we have explicit RFC-style policies for what "must" and "should" mean, this is all a bit fuzzy anyway.

Copy link
Member

Choose a reason for hiding this comment

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

In other words, we already expect a certain amount of resilience against these properties being violated.

💯

In my opinion "must", together with the existing documentation that violating is a logic error, captures the right intent better than changing to "should".

/// `C: PartialEq<D>`, and `A: PartialEq<D>` all exist.
///
/// Note that the `B: PartialEq<A>` (symmetric) and `A: PartialEq<C>`
/// (transitive) impls are not forced to exist, but these requirements apply
Expand All @@ -76,6 +78,25 @@ use self::Ordering::*;
/// undefined behavior. This means that `unsafe` code **must not** rely on the correctness of these
/// methods.
///
/// ## Cross-crate considerations
///
/// Upholding the requirements stated above can become tricky when one crate implements `PartialEq`
/// for a type of another crate (i.e., to allow comparing one of its own types with a type from the
/// standard library). The recommendation is to never implement this trait for a foreign type. In
/// other words, such a crate should do `impl PartialEq<ForeignType> for LocalType`, but it should
/// *not* do `impl PartialEq<LocalType> for ForeignType`.
///
/// This avoids the problem of transitive chains that criss-cross crate boundaries: for all local
/// types `T`, you may assume that no other crate will add `impl`s that allow comparing `T == U`. In
/// other words, if other crates add `impl`s that allow building longer transitive chains `U1 == ...
/// == T == V1 == ...`, then all the types that appear to the right of `T` must be types that the
/// crate defining `T` already knows about. This rules out transitive chains where downstream crates
/// can add new `impl`s that "stitch together" comparisons of foreign types in ways that violate
/// transitivity.
///
/// Not having such foreign `impl`s also avoids forward compatibility issues where one crate adding
/// more `PartialEq` implementations can cause build failures in downstream crates.
///
/// ## Derivable
///
/// This trait can be used with `#[derive]`. When `derive`d on structs, two
Expand Down Expand Up @@ -919,20 +940,43 @@ pub macro Ord($item:item) {
/// easy to accidentally make them disagree by deriving some of the traits and manually
/// implementing others.
///
/// The comparison must satisfy, for all `a`, `b` and `c`:
/// The comparison relations must satisfy the following conditions
/// (for all `a`, `b`, `c` of type `A`, `B`, `C`):
///
/// - 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`.
/// - **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>`,
/// `C: PartialOrd<D>`, and `A: PartialOrd<D>` all exist.
/// - **Duality**: if `A: PartialOrd<B>` and `B: PartialOrd<A>`, then `a < b` if and only if `b > a`.
///
/// Note that these requirements mean that the trait itself must be implemented symmetrically and
/// transitively: if `T: PartialOrd<U>` and `U: PartialOrd<V>` then `U: PartialOrd<T>` and `T:
/// PartialOrd<V>`.
/// Note that the `B: PartialOrd<A>` (dual) and `A: PartialOrd<C>`
/// (transitive) impls are not forced to exist, but these requirements apply
/// whenever they do exist.
///
/// Violating these requirements is a logic error. The behavior resulting from a logic error is not
/// specified, but users of the trait must ensure that such logic errors do *not* result in
/// undefined behavior. This means that `unsafe` code **must not** rely on the correctness of these
/// methods.
///
/// ## Cross-crate considerations
///
/// Upholding the requirements stated above can become tricky when one crate implements `PartialOrd`
/// for a type of another crate (i.e., to allow comparing one of its own types with a type from the
/// standard library). The recommendation is to never implement this trait for a foreign type. In
/// other words, such a crate should do `impl PartialOrd<ForeignType> for LocalType`, but it should
/// *not* do `impl PartialOrd<LocalType> for ForeignType`.
///
/// This avoids the problem of transitive chains that criss-cross crate boundaries: for all local
/// types `T`, you may assume that no other crate will add `impl`s that allow comparing `T < U`. In
/// other words, if other crates add `impl`s that allow building longer transitive chains `U1 < ...
/// < T < V1 < ...`, then all the types that appear to the right of `T` must be types that the crate
/// defining `T` already knows about. This rules out transitive chains where downstream crates can
/// add new `impl`s that "stitch together" comparisons of foreign types in ways that violate
/// transitivity.
///
/// Not having such foreign `impl`s also avoids forward compatibility issues where one crate adding
/// more `PartialOrd` implementations can cause build failures in downstream crates.
///
/// ## Corollaries
///
/// The following corollaries follow from the above requirements:
Expand Down