From a63880c4d3a7e7620f25283999eb4a27516f4816 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 20 Nov 2023 21:55:17 +0100 Subject: [PATCH 1/4] PartialEq: handle longer transitive chains --- library/core/src/cmp.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index bffd3b2af971a..ba7950a709771 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -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` and `B: PartialEq`, then **`a == b` +/// - **Symmetry**: if `A: PartialEq` and `B: PartialEq`, then **`a == b` /// implies `b == a`**; and /// -/// - **Transitive**: if `A: PartialEq` and `B: PartialEq` and `A: +/// - **Transitivity**: if `A: PartialEq` and `B: PartialEq` and `A: /// PartialEq`, then **`a == b` and `b == c` implies `a == c`**. +/// This must also work for longer chains, such as when `A: PartialEq`, `B: PartialEq`, +/// `C: PartialEq`, and `A: PartialEq` all exist. /// /// Note that the `B: PartialEq` (symmetric) and `A: PartialEq` /// (transitive) impls are not forced to exist, but these requirements apply From 3e389ef6d50c2cae6b561a04adb9224e666eb964 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 20 Nov 2023 21:45:51 +0100 Subject: [PATCH 2/4] PartialOrd: transitivity and duality are required only if the corresponding impls exist --- library/core/src/cmp.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index ba7950a709771..e95410ba648b5 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -921,14 +921,18 @@ 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` and `B: PartialOrd` and `A: +/// PartialOrd`, 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: PartialOrd`, +/// `C: PartialOrd`, and `A: PartialOrd` all exist. +/// - **Duality**: if `A: PartialOrd` and `B: PartialOrd`, 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` and `U: PartialOrd` then `U: PartialOrd` and `T: -/// PartialOrd`. +/// Note that the `B: PartialOrd` (dual) and `A: PartialOrd` +/// (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 From baaf6d706ac3cb8b306ebede165706b7a0038415 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 26 Dec 2023 19:52:20 +0100 Subject: [PATCH 3/4] explain what crates should do when adding comparison with foreign types --- library/core/src/cmp.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index e95410ba648b5..9c18d5573e627 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -78,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 for LocalType`, but it should +/// *not* do `impl PartialEq for ForeignType`. +/// +/// This avoids the problem of transitive chains that criss-cross crate boundaries: for all local +/// types `T`, you may assue 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 @@ -939,6 +958,25 @@ pub macro Ord($item:item) { /// 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 for LocalType`, but it should +/// *not* do `impl PartialOrd for ForeignType`. +/// +/// This avoids the problem of transitive chains that criss-cross crate boundaries: for all local +/// types `T`, you may assue 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: From 61d1ebe50be6343a3d5ee22db486163ff47e9ff1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 2 Feb 2024 20:12:37 +0100 Subject: [PATCH 4/4] fix typo Co-authored-by: teor --- library/core/src/cmp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/core/src/cmp.rs b/library/core/src/cmp.rs index 9c18d5573e627..fcb15b7ef65d8 100644 --- a/library/core/src/cmp.rs +++ b/library/core/src/cmp.rs @@ -87,7 +87,7 @@ use self::Ordering::*; /// *not* do `impl PartialEq for ForeignType`. /// /// This avoids the problem of transitive chains that criss-cross crate boundaries: for all local -/// types `T`, you may assue that no other crate will add `impl`s that allow comparing `T == U`. In +/// 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 @@ -967,7 +967,7 @@ pub macro Ord($item:item) { /// *not* do `impl PartialOrd for ForeignType`. /// /// This avoids the problem of transitive chains that criss-cross crate boundaries: for all local -/// types `T`, you may assue that no other crate will add `impl`s that allow comparing `T < U`. In +/// 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