-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Std implementations of PartialOrd are violating the conditions regarding transitivity and symmetry. And the transitivity requirements are i̶m̶p̶o̶s̶s̶i̶b̶l̶e̶ hard to ensure in a multi-crate ecosystem anyways. #87067
Comments
Those requirements are for values that are actually comparable. |
@sfackler
and that final paragraph is
|
Ah right - we already changed that language for PartialEq: #81198. We should make the same change here IMO. |
The solution that // crate `a`
pub struct A(pub u8); and a crate // crate `b`, depends on `a`
#[derive(PartialEq)]
pub struct B; then – AFAICT – this kind of // still in crate `b`
use a::A;
impl PartialEq<A> for B {
pub fn eq(&self, _other: &A) -> bool {
true
}
}
impl PartialEq<B> for A {
pub fn eq(&self, _other: &B) -> bool {
true
}
} However, now the crate // in crate `a`, minor version update adding:
impl PartialEq for A {
pub fn eq(&self, other: &Self) -> bool {
self.0 == other.0
}
}
// or equivalently, adding `#[derive(PartialEq)]` to `A` AFAIK, minor version updates are allowed to add trait impls such as the A(0) == B && B == A(1) but A(0) != A(1) |
In what context are those |
This struct A(u8);
struct B(u8);
struct C(u8);
struct D(u8);
impl PartialEq<B> for A {
fn eq(&self, other: &B) -> bool {
self.0 == other.0
}
}
impl PartialEq<A> for B {
fn eq(&self, other: &A) -> bool {
self.0 == other.0
}
}
impl PartialEq<C> for B {
fn eq(&self, other: &C) -> bool {
self.0 == other.0
}
}
impl PartialEq<B> for C {
fn eq(&self, other: &B) -> bool {
self.0 == other.0
}
}
impl PartialEq<D> for C {
fn eq(&self, other: &D) -> bool {
self.0 == other.0
}
}
impl PartialEq<C> for D {
fn eq(&self, other: &C) -> bool {
self.0 == other.0
}
}
impl PartialEq<A> for D {
fn eq(&self, other: &A) -> bool {
true
}
}
impl PartialEq<D> for A {
fn eq(&self, other: &D) -> bool {
true
}
} since it doesn’t disallow a situation where b == c && c == d && d == a
// but
b != a // using
let (a, b, c, d) = (A(1), B(0), C(0), D(0)); Similarly, this wouldn’t be disallowed either struct A(u8);
struct B(u8);
struct C(u8);
impl PartialEq<B> for A {
fn eq(&self, other: &B) -> bool {
self.0 == other.0
}
}
impl PartialEq<C> for B {
fn eq(&self, other: &C) -> bool {
self.0 == other.0
}
}
impl PartialEq<A> for C {
fn eq(&self, other: &A) -> bool {
true
}
} where we’ll have b == c && c == a
// but
a != b // using
let (a, b, c) = (A(1), B(0), C(0)); These kinds of examples can also be used for new examples like in my previous comment demonstrating that semver makes it unclear which crate needs to ensure which kinds of properties: imagine |
The documentation states useful properties that code working with partial equality can assume. I don't think it is necessary or even particularly useful to guard against every hypothetically possible malicious or bizarre combination of impls. |
In any case, I agree that the conditions as in |
That sounds reasonable.
So the point is that if
This still requires |
Yes
That’s true…
I agree, intuitively it’s clear
My problem is perhaps more the problem that I’m not even sure if there exists a way to make it watertight without contradictions at all. And even if there is a way there might be multiple ways to make this mathematically accurate and watertight. And then one crate creator could intuitively have one understanding of the rules while another one could have a different, incompatible intuition (either intuition might be justified by one of the multiple ways to make the situation formally rigid), with the incompatibility of the approaches resulting in violation of even the minimal set of rules we’d like to have. TL;DR: What I already said, “semver makes it unclear which crate needs to ensure which kinds of properties”. We have no story of who’s responsibility for what in order to enforce the rules we want to have. The problem seems highly non-trivial because you’d need to consider semver as well as orphan rules. Maybe there’d be a need to establish additional orphan-rule-like restrictions, e.g. the example of a type |
Yeah I agree it's not entirely trivial. Basically, instead of reasoning about the impls that currently exist, you have to reason about the impls that might exist in the future -- "For all legal supersets of the current set of implemented traits, if This can be made precise with a "multiple worlds" kinds of model -- coherence already talks about "compatible words" and I recall blog posts along those lines but can't find them right now. The statement in the |
This was briefly discussed in the library api meeting last week. Our conclusion was that since we already accepted #81198 for |
The problem with how See this IRLO thread. Example code from the thread: enum A {
A1,
A2,
}
enum B {
B1,
B2,
}
impl PartialEq<B> for A {
fn eq(&self, other: &B) -> bool {
match (self, other) {
(A::A1, B::B1) => true,
(A::A1, B::B2) => true,
(A::A2, B::B1) => true,
(A::A2, B::B2) => false,
}
}
} This satisfies the Therefore any generic code bound by I think the solution is to only require these conditions for An even more drastic potential solution would be to only have these symmetry and transitivity requirements for |
The notion of a PER is defined on binary relations over some set X. I am not aware of it even being a well-defined question to ask whether a relation on We should probably clarify the docs to say that "if |
Agreed! But what is the value of having such rules for The rules seem totally useless if you can't even deduce basic equivalence properties between objects, such as "when a = x, a = y, b = x, then b = y".
Agreed. That's one reason why I suggested weakening the conditions to only apply to
I am taking the notion that it should be a PER directly from the first line of documentation of You could in theory have PER between different types: it's just same thing as Wikipedia says, as a relation on In another world, one would more reasonably define three traits like this:
Frankly, I don't really see the practical value of even having a trait for "partial equivalence relations". Is there some code that specifically wants partial equivalence relations? I doubt it. Hence my other possible proposal: given that Having a trait require certain things for I suspect the only reason the trait was conceived with that name in the first place is that somebody noticed that IEEE-754 floating point equality happens to satisfy this strange concept of partial equivalence relations, not because anything required such a trait specifically. |
Your previous post presupposes that there is some definition of "PER on The stdlib docs also presuppose this, but your example doesn't demonstrate a problem with the stdlib docs, it just repeats the same mistake that the stdlib docs made by using a term outside of the domain it is defined on.
That doesn't seem like a sensible definition to me. Symmetry is impossible to satisfy for elements in the symmetric difference of
When precedence for such a concept exists in mathematics, I think it would be foolish to deviate from that without a good justification. It's also not at all a strange concept; partial orders are pretty common and partial equivalence relations are the natural name for a partial order that is also symmetric. But anyway, we seem to agree that the docs should emphasize PERs less. It still seems like a useful fact to mention for the common case where |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
It's not "my ad-hoc extension" -- before #81198 it was actually required by the docs. |
No it was not. The docs never said to consider a relation on |
I feel like you're just trying to trap me on some word games. They didn't say so in those words, but what they did say mathematically implied it. The docs implied that if In other words, the rules implied that it must be defined on The requirements also stated that The docs also did say explicitly that this constitutes a "partial equivalence relation". And they were correct. After the change, it's no longer correct except for T = U. |
No they did not. They used (and use) the term PER in an ill-defined way. That doesn't imply anything, except that the docs need to be fixed. |
#115386 intends to fix this for PartialEq. If that lands, we can then do the same for PartialOrd. |
PartialEq, PartialOrd: update and synchronize handling of transitive chains It was brought up in https://internals.rust-lang.org/t/total-equality-relations-as-std-eq-rhs/19232 that we currently have a gap in our `PartialEq` rules, which this PR aims to close: > For example, with PartialEq's conditions you may have a = b = c = d ≠ a (where a and c are of type A, b and d are of type B). The second commit fixes rust-lang#87067 by updating PartialOrd to handle the requirements the same way PartialEq does.
PartialEq, PartialOrd: update and synchronize handling of transitive chains It was brought up in https://internals.rust-lang.org/t/total-equality-relations-as-std-eq-rhs/19232 that we currently have a gap in our `PartialEq` rules, which this PR aims to close: > For example, with PartialEq's conditions you may have a = b = c = d ≠ a (where a and c are of type A, b and d are of type B). The second commit fixes rust-lang#87067 by updating PartialOrd to handle the requirements the same way PartialEq does.
Rollup merge of rust-lang#115386 - RalfJung:partial-eq-chain, r=dtolnay PartialEq, PartialOrd: update and synchronize handling of transitive chains It was brought up in https://internals.rust-lang.org/t/total-equality-relations-as-std-eq-rhs/19232 that we currently have a gap in our `PartialEq` rules, which this PR aims to close: > For example, with PartialEq's conditions you may have a = b = c = d ≠ a (where a and c are of type A, b and d are of type B). The second commit fixes rust-lang#87067 by updating PartialOrd to handle the requirements the same way PartialEq does.
Std implementations of PartialOrd are violating the conditions regarding transitivity and symmetry.
From the standard library docs:
(emphasis mine)
focus on the final paragraph in the quote above and look at the following example
(in the playground)
So either the library documentation is off or the implementations are flawed.
And the transitivity requirements are
impossiblehard to ensure in a multi-crate ecosystem anyways.Note that, technically, it’s impossible to enforce transitive existence of impls for the trait unless using operands of fully “external” types is completely prohibited:
If I’m writing a crate
foo
providing a typestruct Foo(…);
and animpl PartialOrd<i32> for Foo
as well as animpl PartialOrd<Foo> for i32
, it seems like I’m following the rules set byPartialOrd
’s documentation.If I’m writing a crate
bar
providing a typestruct Bar(…);
and animpl PartialOrd<i32> for Bar
as well as animpl PartialOrd<Bar> for i32
, it seems like I’m following the rules set byPartialOrd
’s documentation.Now, if I’m writing a third crate that imports both
foo
andbar
, then I’ll haveimpl PartialOrd<Foo> for i32
as well asimpl PartialOrd<i32> for Bar
, but obviouslyimpl PartialOrd<Foo> for Bar
is missing.In this example, the crates
foo
andbar
each provided animpl
where one of the operands,i32
, was a type that’s fully external to the crate itself (in the sense that neither the type nor any of its generic arguments are part offoo
, or part of a different crate that has the same owners asfoo
[i32
has no generic arguments to begin with]).@rustbot label C-bug, T-libs
The text was updated successfully, but these errors were encountered: