-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
PartialEq, PartialOrd: update and synchronize handling of transitive chains #115386
Conversation
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
There was another comment posed here that seemingly got removed again, starting with
Note that when considering semver-compatible evolution of crates, this is already the case today. The alpha/beta example in the docs doesn't need the new "chain" part of transitivity, it already works on today's published docs. (Generally please don't remove comments, that kind of rewriting of history makes discussions very confused to follow later, or to follow the email notification chain. Just edit or post a 2nd reply to explain why you changed your mind.) |
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.
We discussed this PR in today's T-libs-api meeting and were skeptical that the new content (neither the clarification on transitivity nor the new paragraph regarding cross-crate impls) would have a practical consequence on people writing more correct PartialEq impls.
My personal view: I can see that the existing definition is not bulletproof, but if I imagine a hypothetical bulletproof definition or something in that direction (such as this PR), it is not obvious to me that a PartialEq impl which does not conform to the new definition is more likely to mean the impl is wrong or the new definition is bad. This significantly reduces my interest in pinning down a more precise definition of what makes a PartialEq impl correct. I think the intended use for PartialEq conveyed by the current docs, and the remaining wiggle room for impls, is pretty good.
So, the docs are ambiguous but there is no intent to fix them? That's the worst possible outcome. :( It runs a high risk of people interpreting the docs in different ways without even realizing that they are deliberately ambiguous. Note that these questions about transitivity come up for real, see e.g. here and here. The current documentation is not answering the questions people have. |
8aebbc1
to
f2719e9
Compare
I have removed the new paragraph about dealing with the multi-crate situation. Now all this PR does is un-do a likely accidental side-effect of #81198: before that PR, it was the case that if It is true that this transitivity requirement cannot be upheld on a per-crate level. However, that is already the case for the transitivity requirement that we have documented right now. In other words, the docs are already somewhat aspirational when it comes to multi-crate situations. Therefore I think we really should document that, ideally, this property holds for longer chains as well. #118108 does the same thing for Re-nominating due to the changed PR contents and the new information (this un-does a likely accidental change, bringing us closer to how things were documented before 2021, rather than introducing something completely new). |
What about this situation?
In json, there is no distinction between integers and floats. Having both a In cbor, there is a distinction between integers and floats. Having both a But that allows for:
Which would break the proposed chaining rule. Does that mean there's a problem with one of the PartialEq implementations in this example, or with the proposed rule? |
Similar problems already arise with the rule as documented today: assuming both JsonValue and CborValue can be compared with both i32 and i64 (and assuming CborValue are inequal whenever the type differs). Now imagine we wanted to add a Arguably libraries shouldn't "connect" two outside-the-crate types by equality that are not already connected. The moment they do, they are defining a notion of equality on a pair of types they do not control, and different libraries could define conflicting notions of equality. So, I don't have a solution to this issue, but it's not an issue introduced by this PR. The issue was introduced by #81198. |
@RalfJung Some additional thoughts on that issue: If The rule is about Does that mean that 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>`, |
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.
/// 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.
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.
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)?
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.
@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.
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.
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.
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.
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".
@m-ou-se Hm, that is a good question. Before #81198, if we had So it would only be consistent to allow the transitive-chain rule to also use allow using comparisons "backwards", i.e. to require that if |
I agree, but that would mean that it'd be wrong to have both I don't think that is something we can go forward with. It is very common for an |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@dtolnay can I take your starting the FCP process as "r=me once FCP finishes"? |
@bors r+ |
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
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.
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113833 (`std::error::Error` -> Trait Implementations: lifetimes consistency improvement) - rust-lang#115386 (PartialEq, PartialOrd: update and synchronize handling of transitive chains) - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern) - rust-lang#118960 (Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.) - rust-lang#120060 (Use the same mir-opt bless targets on all platforms) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120384 (Use `<T, U>` for array/slice equality `impl`s) r? `@ghost` `@rustbot` modify labels: rollup
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.
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#113833 (`std::error::Error` -> Trait Implementations: lifetimes consistency improvement) - rust-lang#115386 (PartialEq, PartialOrd: update and synchronize handling of transitive chains) - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern) - rust-lang#118960 (Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.) - rust-lang#120384 (Use `<T, U>` for array/slice equality `impl`s) - rust-lang#120518 (riscv only supports split_debuginfo=off for now) - rust-lang#120619 (Assert that params with the same *index* have the same *name*) - rust-lang#120657 (Remove unused struct) - rust-lang#120661 (target: default to the medium code model on LoongArch targets) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113833 (`std::error::Error` -> Trait Implementations: lifetimes consistency improvement) - rust-lang#115386 (PartialEq, PartialOrd: update and synchronize handling of transitive chains) - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern) - rust-lang#118960 (Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.) - rust-lang#120384 (Use `<T, U>` for array/slice equality `impl`s) - rust-lang#120518 (riscv only supports split_debuginfo=off for now) - rust-lang#120657 (Remove unused struct) - rust-lang#120661 (target: default to the medium code model on LoongArch targets) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113833 (`std::error::Error` -> Trait Implementations: lifetimes consistency improvement) - rust-lang#115386 (PartialEq, PartialOrd: update and synchronize handling of transitive chains) - rust-lang#116284 (make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern) - rust-lang#118960 (Add LocalWaker and ContextBuilder types to core, and LocalWake trait to alloc.) - rust-lang#120384 (Use `<T, U>` for array/slice equality `impl`s) - rust-lang#120518 (riscv only supports split_debuginfo=off for now) - rust-lang#120657 (Remove unused struct) - rust-lang#120661 (target: default to the medium code model on LoongArch targets) r? `@ghost` `@rustbot` modify labels: rollup
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.
Pkgsrc changes: * Adapt checksums and patches, some have beene intregrated upstream. Upstream chnages: Version 1.78.0 (2024-05-02) =========================== Language -------- - [Stabilize `#[cfg(target_abi = ...)]`] (rust-lang/rust#119590) - [Stabilize the `#[diagnostic]` namespace and `#[diagnostic::on_unimplemented]` attribute] (rust-lang/rust#119888) - [Make async-fn-in-trait implementable with concrete signatures] (rust-lang/rust#120103) - [Make matching on NaN a hard error, and remove the rest of `illegal_floating_point_literal_pattern`] (rust-lang/rust#116284) - [static mut: allow mutable reference to arbitrary types, not just slices and arrays] (rust-lang/rust#117614) - [Extend `invalid_reference_casting` to include references casting to bigger memory layout] (rust-lang/rust#118983) - [Add `non_contiguous_range_endpoints` lint for singleton gaps after exclusive ranges] (rust-lang/rust#118879) - [Add `wasm_c_abi` lint for use of older wasm-bindgen versions] (rust-lang/rust#117918) This lint currently only works when using Cargo. - [Update `indirect_structural_match` and `pointer_structural_match` lints to match RFC] (rust-lang/rust#120423) - [Make non-`PartialEq`-typed consts as patterns a hard error] (rust-lang/rust#120805) - [Split `refining_impl_trait` lint into `_reachable`, `_internal` variants] (rust-lang/rust#121720) - [Remove unnecessary type inference when using associated types inside of higher ranked `where`-bounds] (rust-lang/rust#119849) - [Weaken eager detection of cyclic types during type inference] (rust-lang/rust#119989) - [`trait Trait: Auto {}`: allow upcasting from `dyn Trait` to `dyn Auto`] (rust-lang/rust#119338) Compiler -------- - [Made `INVALID_DOC_ATTRIBUTES` lint deny by default] (rust-lang/rust#111505) - [Increase accuracy of redundant `use` checking] (rust-lang/rust#117772) - [Suggest moving definition if non-found macro_rules! is defined later] (rust-lang/rust#121130) - [Lower transmutes from int to pointer type as gep on null] (rust-lang/rust#121282) Target changes: - [Windows tier 1 targets now require at least Windows 10] (rust-lang/rust#115141) - [Enable CMPXCHG16B, SSE3, SAHF/LAHF and 128-bit Atomics in tier 1 Windows] (rust-lang/rust#120820) - [Add `wasm32-wasip1` tier 2 (without host tools) target] (rust-lang/rust#120468) - [Add `wasm32-wasip2` tier 3 target] (rust-lang/rust#119616) - [Rename `wasm32-wasi-preview1-threads` to `wasm32-wasip1-threads`] (rust-lang/rust#122170) - [Add `arm64ec-pc-windows-msvc` tier 3 target] (rust-lang/rust#119199) - [Add `armv8r-none-eabihf` tier 3 target for the Cortex-R52] (rust-lang/rust#110482) - [Add `loongarch64-unknown-linux-musl` tier 3 target] (rust-lang/rust#121832) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Bump Unicode to version 15.1.0, regenerate tables] (rust-lang/rust#120777) - [Make align_offset, align_to well-behaved in all cases] (rust-lang/rust#121201) - [PartialEq, PartialOrd: document expectations for transitive chains] (rust-lang/rust#115386) - [Optimize away poison guards when std is built with panic=abort] (rust-lang/rust#100603) - [Replace pthread `RwLock` with custom implementation] (rust-lang/rust#110211) - [Implement unwind safety for Condvar on all platforms] (rust-lang/rust#121768) - [Add ASCII fast-path for `char::is_grapheme_extended`] (rust-lang/rust#121138) Stabilized APIs --------------- - [`impl Read for &Stdin`] (https://doc.rust-lang.org/stable/std/io/struct.Stdin.html#impl-Read-for-%26Stdin) - [Accept non `'static` lifetimes for several `std::error::Error` related implementations] (rust-lang/rust#113833) - [Make `impl<Fd: AsFd>` impl take `?Sized`] (rust-lang/rust#114655) - [`impl From<TryReserveError> for io::Error`] (https://doc.rust-lang.org/stable/std/io/struct.Error.html#impl-From%3CTryReserveError%3E-for-Error) These APIs are now stable in const contexts: - [`Barrier::new()`] (https://doc.rust-lang.org/stable/std/sync/struct.Barrier.html#method.new) Cargo ----- - [Stabilize lockfile v4](rust-lang/cargo#12852) - [Respect `rust-version` when generating lockfile] (rust-lang/cargo#12861) - [Control `--charset` via auto-detecting config value] (rust-lang/cargo#13337) - [Support `target.<triple>.rustdocflags` officially] (rust-lang/cargo#13197) - [Stabilize global cache data tracking] (rust-lang/cargo#13492) Misc ---- - [rustdoc: add `--test-builder-wrapper` arg to support wrappers such as RUSTC_WRAPPER when building doctests] (rust-lang/rust#114651) Compatibility Notes ------------------- - [Many unsafe precondition checks now run for user code with debug assertions enabled] (rust-lang/rust#120594) This change helps users catch undefined behavior in their code, though the details of how much is checked are generally not stable. - [riscv only supports split_debuginfo=off for now] (rust-lang/rust#120518) - [Consistently check bounds on hidden types of `impl Trait`] (rust-lang/rust#121679) - [Change equality of higher ranked types to not rely on subtyping] (rust-lang/rust#118247) - [When called, additionally check bounds on normalized function return type] (rust-lang/rust#118882) - [Expand coverage for `arithmetic_overflow` lint] (rust-lang/rust#119432) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Update to LLVM 18](rust-lang/rust#120055) - [Build `rustc` with 1CGU on `x86_64-pc-windows-msvc`] (rust-lang/rust#112267) - [Build `rustc` with 1CGU on `x86_64-apple-darwin`] (rust-lang/rust#112268) - [Introduce `run-make` V2 infrastructure, a `run_make_support` library and port over 2 tests as example] (rust-lang/rust#113026) - [Windows: Implement condvar, mutex and rwlock using futex] (rust-lang/rust#121956)
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:The second commit fixes #87067 by updating PartialOrd to handle the requirements the same way PartialEq does.