From fca3f6ad6cbe1ad11cfbc2c65ac9966e2c6e6d25 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Mon, 9 Mar 2015 11:07:14 +0700 Subject: [PATCH 01/14] Add unsafe-cmp RFC --- text/0000-unsafe-cmp.md | 103 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 text/0000-unsafe-cmp.md diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md new file mode 100644 index 00000000000..1cf16bf92b6 --- /dev/null +++ b/text/0000-unsafe-cmp.md @@ -0,0 +1,103 @@ +- Feature Name: unsafe_cmp +- Start Date: 2015-03-09 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary + +Make the four `cmp` traits (`PartialEq`, `Eq`, `PartialOrd`, and `Ord`) become +`unsafe` traits. + +# Motivation + +Some algorithms and data structures (such as the `SliceExt::sort()` algorithm +in the standard library) depend on comparisons to be sane in order to be +efficient. In those cases, ill-behaved comparison traits might cause undefined +behavior. However, since the `cmp` traits are currently normal traits (i.e. not +`unsafe`), they cannot be trusted to be well-behaved. As a result, such +optimizations are not possible. + +The proposed solution is to make the `PartialEq`, `Eq`, `PartialOrd`, and `Ord` +traits `unsafe`. This allows library to trust the trait implementations to +follow certain rules. + +Some might argue that these traits do not invoke `unsafe` behavior. However, +this usage of `unsafe` is intended by design, as described in RFC 19: + +> An *unsafe trait* is a trait that is unsafe to implement, because it +> represents some kind of trusted assertion. Note that unsafe traits are +> perfectly safe to *use*. `Send` and `Share` are examples of unsafe traits: +> implementing these traits is effectively an assertion that your type is safe +> for threading. + +In the case of comparison traits, the "trusted assertion" is that they behave +sanely, as described in the Detailed design section. + +The reason only the `cmp` traits are addressed here is because they have the +highest potential to be relied on by `unsafe` traits. (But see the Unresolved +questions section). + +Additionally, the properties required are made more strict and rigourous in +this RFC. + +# Detailed design + +`#[deriving]` is not affected by this change. It should work the same as they +always did. + +Mark the `PartialEq`, `Eq`, `PartialOrd`, and `Ord` traits as `unsafe` and +require implementations of these traits to satisfy the following properties: + +**Note**: +- `=>` stands for "if-then". A property of the form `X => Y` means that "if `X` + type-checks correctly, then `Y` must also do so. If `X` type-checks + correctly and evaluates to `true`, then `Y` must also do so". +- `<=>` stands for "if and only if". A property of the form `X <=> Y` means + that "`X` must type-check correctly if and only if `Y` does so. If they + type-check correctly, they must evaluate to the same boolean value". +- Properties of other forms must evaluate to `true` if they type-check + correctly. + +For `PartialEq`: +- `a.eq(b) <=> b.eq(a)` +- `a.eq(b) && b.eq(c) => a.eq(c)` +- `a.eq(b) <=> !(a.ne(b))` + +For `Eq`: +- `a.eq(a)` + +For `PartialOrd`: +- `a.partial_cmp(b) == Some(Less) <=> a.lt(b)` +- `a.partial_cmp(b) == Some(Greater) <=> a.gt(b)` +- `a.partial_cmp(b) == Some(Equal) <=> a.eq(b)` +- `a.lt(b) <=> b.gt(a)` +- `a.lt(b) && b.lt(c) => a.lt(c)` + +For `Ord`: +- `Some(a.cmp(b)) == a.partial_cmp(b)` + +# Drawbacks + +- Some types might want to implement these traits such that they do not satisfy + these properties. However, I consider this to be abuse of traits. +- Some people might just use `unsafe` without knowing the potential bad + consequences. +- Might cause too many `unsafe`s in otherwise safe code. +- This is a breaking change. + +# Alternatives + +- The status quo. +- Have separate traits for trusted behavior and untrusted behavior e.g. `Eq` as + a safe trait that is not trusted, and `EqStrict` that is an `unsafe` trait + that can be trusted by `unsafe` code. The problem is that there is no + obvious reason to implement `Eq` but not implement `EqStrict` (See the + Drawbacks section). + +# Unresolved questions + +- Are the properties required here complete? +- Is this worth the number of extra `unsafe`s? +- What about the `Iterator`, `ExactSizeIterator`, `DoubleEndedIterator`, and + `RandomAccessIterator` traits? +- Does this apply to other traits? From 4f75eb698a3dad04fc87e165feee2da2578785e6 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Mon, 9 Mar 2015 11:36:08 +0700 Subject: [PATCH 02/14] Add properties of .le() and .ge() --- text/0000-unsafe-cmp.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 1cf16bf92b6..c6e3f3a815e 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -70,6 +70,8 @@ For `PartialOrd`: - `a.partial_cmp(b) == Some(Less) <=> a.lt(b)` - `a.partial_cmp(b) == Some(Greater) <=> a.gt(b)` - `a.partial_cmp(b) == Some(Equal) <=> a.eq(b)` +- `a.le(b) <=> a.lt(b) || a.eq(b)` +- `a.ge(b) <=> a.gt(b) || a.eq(b)` - `a.lt(b) <=> b.gt(a)` - `a.lt(b) && b.lt(c) => a.lt(c)` From 261b3378c940c8d3ba902faf3a8aaf12eb17fa94 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Mon, 9 Mar 2015 18:33:13 +0700 Subject: [PATCH 03/14] Mention cross-crate transitivity issue as per @quantheory --- text/0000-unsafe-cmp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index c6e3f3a815e..92c1e06dae5 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -103,3 +103,4 @@ For `Ord`: - What about the `Iterator`, `ExactSizeIterator`, `DoubleEndedIterator`, and `RandomAccessIterator` traits? - Does this apply to other traits? +- Can the transitivity properties be enforced cross-crate? From 8c4ef29f05676d7db528890eb12b776f76eb3cea Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Tue, 10 Mar 2015 06:58:28 +0700 Subject: [PATCH 04/14] Add type parameter issue --- text/0000-unsafe-cmp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 92c1e06dae5..518e6e27677 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -104,3 +104,4 @@ For `Ord`: `RandomAccessIterator` traits? - Does this apply to other traits? - Can the transitivity properties be enforced cross-crate? +- Do we need the type parameters in `PartialEq` and `PartialOrd`? From 2cf814e8d91acf787fc0af574f5c4c7b091b0046 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Tue, 10 Mar 2015 07:10:19 +0700 Subject: [PATCH 05/14] Explicitly mention that not many `unsafe`s are required --- text/0000-unsafe-cmp.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 518e6e27677..a3c30782bf2 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -37,6 +37,10 @@ The reason only the `cmp` traits are addressed here is because they have the highest potential to be relied on by `unsafe` traits. (But see the Unresolved questions section). +I believe that in practice, only a few `unsafe`s will be required, since most +types will simply `#[derive]` the required traits, in which case the +correctness can be guaranteed. + Additionally, the properties required are made more strict and rigourous in this RFC. From d71228df29ba96044cc81e34e514054156fa82e4 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Tue, 10 Mar 2015 07:21:34 +0700 Subject: [PATCH 06/14] Mention #[derive] in the summary --- text/0000-unsafe-cmp.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index a3c30782bf2..4f2891cce5b 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -8,6 +8,8 @@ Make the four `cmp` traits (`PartialEq`, `Eq`, `PartialOrd`, and `Ord`) become `unsafe` traits. +`#[derive]` should work as usual, minimizing the amount of `unsafe`s required. + # Motivation Some algorithms and data structures (such as the `SliceExt::sort()` algorithm From 22b9794c592d903c5679449d0d08ca95816750f5 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Tue, 10 Mar 2015 08:23:43 +0700 Subject: [PATCH 07/14] Add `Relaxed` traits alternative --- text/0000-unsafe-cmp.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 4f2891cce5b..2affe7da08d 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -101,6 +101,10 @@ For `Ord`: that can be trusted by `unsafe` code. The problem is that there is no obvious reason to implement `Eq` but not implement `EqStrict` (See the Drawbacks section). +- Have separate `Relaxed` traits for untrusted behavior, and make the + non-`Relaxed` traits `unsafe`. Make the operators use the `Relaxed` + versions. This is similar to the previous alternative, but the `unsafe` + versions have the shorter names. # Unresolved questions From 768d6ccd9312db717147e56e6159960f4100306a Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Mon, 16 Mar 2015 12:21:11 +0700 Subject: [PATCH 08/14] Mention `sort_by()` --- text/0000-unsafe-cmp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 2affe7da08d..b823aaea108 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -115,3 +115,4 @@ For `Ord`: - Does this apply to other traits? - Can the transitivity properties be enforced cross-crate? - Do we need the type parameters in `PartialEq` and `PartialOrd`? +- What about functions that take custom comparison functions like `sort_by()`? From 7ca8fee848045355d08dbf007d11c323d821947f Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Wed, 18 Mar 2015 07:38:21 +0700 Subject: [PATCH 09/14] Add alternative as per @Gankro --- text/0000-unsafe-cmp.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index b823aaea108..900a82d6292 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -105,6 +105,8 @@ For `Ord`: non-`Relaxed` traits `unsafe`. Make the operators use the `Relaxed` versions. This is similar to the previous alternative, but the `unsafe` versions have the shorter names. +- Make `PartialEq` and `PartialOrd` normal traits without any guarantees, but + make `Eq` and `Ord` `unsafe` traits with guarantees. # Unresolved questions From 2b55259ed7da0a62572d5ac73940cf39194c6967 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 19 Mar 2015 12:28:50 +0700 Subject: [PATCH 10/14] Add drawback about accidentally incorrect code --- text/0000-unsafe-cmp.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 900a82d6292..9b229ab4516 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -91,6 +91,7 @@ For `Ord`: - Some people might just use `unsafe` without knowing the potential bad consequences. - Might cause too many `unsafe`s in otherwise safe code. +- Incorrect implementations might accidentally cause unsafe behavior. - This is a breaking change. # Alternatives From 589d19b7556bf4941a354230d2877a59659d4527 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Thu, 19 Mar 2015 12:39:55 +0700 Subject: [PATCH 11/14] Elaborate on alternatives --- text/0000-unsafe-cmp.md | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 9b229ab4516..9a0ea6c6fa3 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -97,15 +97,13 @@ For `Ord`: # Alternatives - The status quo. -- Have separate traits for trusted behavior and untrusted behavior e.g. `Eq` as - a safe trait that is not trusted, and `EqStrict` that is an `unsafe` trait - that can be trusted by `unsafe` code. The problem is that there is no - obvious reason to implement `Eq` but not implement `EqStrict` (See the - Drawbacks section). -- Have separate `Relaxed` traits for untrusted behavior, and make the - non-`Relaxed` traits `unsafe`. Make the operators use the `Relaxed` - versions. This is similar to the previous alternative, but the `unsafe` - versions have the shorter names. +- Have separate `RelaxedEq` and `RelaxedOrd` traits with untrusted behavior, + and make the original four traits `unsafe` and have trusted behavior. Make + the operator overloading use the `Relaxed` versions. Have separate + functions that depend on and do not depend on sane comparisons. One problem + about this is handling `#[derive]`s properly. +- Similar to previous alternative, but have `EqStrict` and other traits that + are `unsafe` instead. - Make `PartialEq` and `PartialOrd` normal traits without any guarantees, but make `Eq` and `Ord` `unsafe` traits with guarantees. From 3818ec2063842f58aaffbfbef37165a5c99a7fb6 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Sun, 22 Mar 2015 16:03:26 +0700 Subject: [PATCH 12/14] Mention `is_correct()` alternative as per @huonw --- text/0000-unsafe-cmp.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 9a0ea6c6fa3..e6df6e38ea0 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -97,6 +97,10 @@ For `Ord`: # Alternatives - The status quo. +- Have an `unsafe fn is_correct() -> bool` or a similar method that defaults to + returning `false`, but have `#derive` return `true` only if all of the + type's fields' `is_correct()` return `true`. It is still unclear about + where (i.e. in which trait) should this method be placed. - Have separate `RelaxedEq` and `RelaxedOrd` traits with untrusted behavior, and make the original four traits `unsafe` and have trusted behavior. Make the operator overloading use the `Relaxed` versions. Have separate From 52ec7679cc962b68132794ade2f3b63514310fc0 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Sun, 22 Mar 2015 16:13:12 +0700 Subject: [PATCH 13/14] Mention associated constants in alternative --- text/0000-unsafe-cmp.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index e6df6e38ea0..2febbddaa6c 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -100,7 +100,8 @@ For `Ord`: - Have an `unsafe fn is_correct() -> bool` or a similar method that defaults to returning `false`, but have `#derive` return `true` only if all of the type's fields' `is_correct()` return `true`. It is still unclear about - where (i.e. in which trait) should this method be placed. + where (i.e. in which trait) should this method be placed. It might also be + better to use associated constants for this purpose. - Have separate `RelaxedEq` and `RelaxedOrd` traits with untrusted behavior, and make the original four traits `unsafe` and have trusted behavior. Make the operator overloading use the `Relaxed` versions. Have separate From 58c893a46737bf89e54bf2ce3f4d8959a9388930 Mon Sep 17 00:00:00 2001 From: Theemathas Chirananthavat Date: Sun, 29 Mar 2015 18:38:26 +0700 Subject: [PATCH 14/14] Mention panics, as per @bluss --- text/0000-unsafe-cmp.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/text/0000-unsafe-cmp.md b/text/0000-unsafe-cmp.md index 2febbddaa6c..39a26937330 100644 --- a/text/0000-unsafe-cmp.md +++ b/text/0000-unsafe-cmp.md @@ -64,6 +64,9 @@ require implementations of these traits to satisfy the following properties: - Properties of other forms must evaluate to `true` if they type-check correctly. +For all comparison traits: +- The comparison methods must not panic. + For `PartialEq`: - `a.eq(b) <=> b.eq(a)` - `a.eq(b) && b.eq(c) => a.eq(c)`