-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Unsafe comparison traits (PartialEq, Eq, PartialOrd, Ord) #956
Closed
Closed
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
fca3f6a
Add unsafe-cmp RFC
theemathas 4f75eb6
Add properties of .le() and .ge()
theemathas 261b337
Mention cross-crate transitivity issue as per @quantheory
theemathas 8c4ef29
Add type parameter issue
theemathas 2cf814e
Explicitly mention that not many `unsafe`s are required
theemathas d71228d
Mention #[derive] in the summary
theemathas 22b9794
Add `Relaxed` traits alternative
theemathas 768d6cc
Mention `sort_by()`
theemathas 7ca8fee
Add alternative as per @Gankro
theemathas 2b55259
Add drawback about accidentally incorrect code
theemathas 589d19b
Elaborate on alternatives
theemathas 3818ec2
Mention `is_correct()` alternative as per @huonw
theemathas 52ec767
Mention associated constants in alternative
theemathas 58c893a
Mention panics, as per @bluss
theemathas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
- 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.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)` | ||
|
||
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? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably add this form of antisymmetry:
a.lt(b) => !(b.lt(a))
This is in the library documentation already. Of course, the implication must be one-way, because for IEEE NaNs
a.lt(b)
andb.lt(a)
will both be false.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quantheory It is implied by the other rules.
Proof:
Suppose that
a.lt(b)
andb.lt(a)
From
Rule 6 of PartialOrd
(witha
andb
swapped) andb.lt(a)
, we geta.gt(b)
From
Rule 1 of PartialOrd
anda.lt(b)
, we geta.partial_cmp(b) == Some(Less)
From
Rule 2 of PartialOrd
anda.gt(b)
, we geta.partial_cmp(b) == Some(Greater)
Therefore,
a.partial_cmp(b) == Some(Less)
anda.partial_cmp(b) == Some(Greater)
, which is absurd.Thus, the assumption is false, and that one of
a.lt(b)
andb.lt(a)
is false.Therefore,
a.lt(b) => !(b.lt(a))
Correct me if this proof is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I misread the
partial_cmp
bits as=>
. (Well, there is a tiny problem with your proof, which is thatb.lt(a)
doesn't have to return at all, but that's not what I meant and not relevant here, probably.)On a different note, I'm pretty sure that the transitive property is no good here (and as written in the current docs). The reason is that you can define two types in different crates that can compare to the same type in some third crate, but not to each other. Then if you use all three together there's suddenly a problem. It's a modularity hazard.
I think that transitivity should only be required if all of the comparisons are defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's impossible to guarantee this unless adding an implementation of
PartialOrd
is a breaking change.Consider two crates
A
andB
.B
defines a typeY
.A
defines typeX
and implements the<=
operator betweenX
andY
. NowB
defines a new typeZ
and implements the<=
operator forY
andZ
. ThenX <= Y
andY <= Z
are defined butX <= Z
is not defined.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see that @quantheory already mentioned this.