Skip to content
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

Remove requirement that forces symmetric and transitive PartialEq impls to exist #81198

Merged
merged 1 commit into from
Jan 31, 2021

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 19, 2021

Counterexample of symmetry:

If you have an impl like:

impl<T> PartialEq<T> for Ident
where
    T: ?Sized + AsRef<str>

then Rust will not even allow the symmetric impl to exist.

error[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Ident`)
 --> src/main.rs:9:6
  |
9 | impl<T> PartialEq<Ident> for T where T: ?Sized + AsRef<str> {
  |      ^ type parameter `T` must be covered by another type when it appears before the first local type (`Ident`)
  |
  = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
  = note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

Counterexample of transitivity:

Consider these two existing impls from regex and clap:

// regex

/// An inline representation of `Option<char>`.
pub struct Char(u32);

impl PartialEq<char> for Char {
    fn eq(&self, other: &char) -> bool {
        self.0 == *other as u32
    }
}
// clap

pub(crate) enum KeyType {
    Short(char),
    Long(OsString),
    Position(u64),
}

impl PartialEq<char> for KeyType {
    fn eq(&self, rhs: &char) -> bool {
        match self {
            KeyType::Short(c) => c == rhs,
            _ => false,
        }
    }
}

It's nice to be able to add PartialEq<proc_macro::Punct> for char in libproc_macro (#80595), but it makes no sense to force an impl PartialEq<Punct> for Char and impl PartialEq<Punct> for KeyType in regex and clap in code that otherwise has nothing to do with proc macros.


@rust-lang/libs

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 19, 2021
@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 19, 2021
@dtolnay
Copy link
Member Author

dtolnay commented Jan 19, 2021

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jan 19, 2021

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 19, 2021
@pthariensflame
Copy link
Contributor

I think we should at least have something that says symmetry and transitivity are encouraged if they can be done without undue difficulty.

@c410-f3r

This comment has been minimized.

@sfackler
Copy link
Member

Agreed that it would be good to recommend the symmetric impl when possible.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 20, 2021
@rfcbot
Copy link

rfcbot commented Jan 20, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 20, 2021
@pthariensflame
Copy link
Contributor

@sfackler Can you register a Concern, then? (Can I?)

@KodrAus
Copy link
Contributor

KodrAus commented Jan 21, 2021

This seems reasonable to me. I could imagine we could find ways to help users around symmetry and transitivity issues through error messages or explicitly codified symmetric or transitive functions that polyfill missing impls.

error[E0369]: binary operation `==` cannot be applied to type `B`
  --> src/main.rs:70:15
   |
70 |     let _ = b == a;
   |             - ^^ - A
   |             |
   |             B
   |
   = note: an implementation of `std::cmp::PartialEq` is available for `A`, try `let _ = a == b;`

and for impl PartialEq bounds in functions:

error[E0277]: can't compare `B` with `A`
  --> src/main.rs:71:33
   |
17 | fn needs_partial_eq(a: A, lhs: impl PartialEq<A>) -> bool {
   |                                     ------------ required by this bound in `needs_partial_eq`
...
71 |     let _ = needs_partial_eq(A, B);
   |                                 ^ no implementation for `B == A`
   |
   = help: the trait `PartialEq<A>` is not implemented for `B`, but `PartialEq<B>` is implemented for `A`
   = help: try `let _ = needs_partial_eq(A, std::cmp::symmetric(B));`

Playground

@dtolnay
Copy link
Member Author

dtolnay commented Jan 21, 2021

I think we should at least have something that says symmetry and transitivity are encouraged if they can be done without undue difficulty.

I don't necessarily agree with this. Skimming the clap PartialEq code that's mentioned in the transitivity counterexample above (https://github.com/clap-rs/clap/blob/a0269a41d4abaf4b0a9ec4f9a059fe62ea0ba3a7/src/mkeymap.rs#L34-L68), it's hard to say it would be "unduly difficult" to write the full set of reverse impls, but at the same time I really see no benefit of clap doing that. I'd rather they just write whatever impls are needed, and not write whatever impls are not needed.

If your recommendation was intended to apply to public APIs only, that seems more suited to API design guidelines than any official requirement of the trait.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 22, 2021

On the topic of public APIs specifically I opened up a discussion in the API Guidelines for it: rust-lang/api-guidelines#235

@dylni
Copy link
Contributor

dylni commented Jan 23, 2021

Aside from when it's not possible, is there ever a reason to not make an impl symmetric? If they're not, comparisons need to have a specific order, which should only be a style decision. This can make assert_eq inconsistent for tests.

That's why more PartialEq impls were added later to compare Vec and slice: #71660

@dtolnay
Copy link
Member Author

dtolnay commented Jan 23, 2021

Aside from when it's not possible, is there ever a reason to not make an impl symmetric?

Yes, there is. If I'm only going to perform the comparisons in one direction in my codebase, I'm not going to write 2 impls, one of which is dead code. I pointed this out in #81198 (comment).

@dylni
Copy link
Contributor

dylni commented Jan 23, 2021

Sorry for not reading the comments well enough. I was looking closer through #80595.

If I'm only going to perform the comparisons in one direction in my codebase, I'm not going to write 2 impls, one of which is dead code.

I can understand this, even though I don't think I would do it myself. Requiring both to exist does seem unnecessary, but shouldn't a recommendation stay on this documentation page? If it's in the API guidelines, it might not be seen by those that want to learn more about the trait.

@varkor
Copy link
Member

varkor commented Jan 23, 2021

Perhaps it would be worth being able to derive the symmetry after having implemented one direction. I agree that it's better to have both if possible (and the inconsistency in the past has caused problems with generalising some of the existing library methods).

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 30, 2021
@rfcbot
Copy link

rfcbot commented Jan 30, 2021

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.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jan 30, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jan 30, 2021

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 30, 2021

📌 Commit 8758083 has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2021
…as-schievink

Rollup of 18 pull requests

Successful merges:

 - rust-lang#78044 (Implement io::Seek for io::Empty)
 - rust-lang#79285 (Stabilize Arc::{increment,decrement}_strong_count)
 - rust-lang#80053 (stabilise `cargo test -- --include-ignored`)
 - rust-lang#80279 (Implement missing `AsMut<str>` for `str`)
 - rust-lang#80470 (Stabilize by-value `[T; N]` iterator `core::array::IntoIter`)
 - rust-lang#80945 (Add Box::downcast() for dyn Any + Send + Sync)
 - rust-lang#81048 (Stabilize `core::slice::fill_with`)
 - rust-lang#81198 (Remove requirement that forces symmetric and transitive PartialEq impls to exist)
 - rust-lang#81422 (Account for existing `_` field pattern when suggesting `..`)
 - rust-lang#81472 (Clone entire `TokenCursor` when collecting tokens)
 - rust-lang#81484 (Optimize decimal formatting of 128-bit integers)
 - rust-lang#81491 (Balance sidebar `Deref` cycle check with main content)
 - rust-lang#81509 (Add a regression test for ICE of bad_placeholder_type)
 - rust-lang#81547 (Edit rustc_typeck top-level docs)
 - rust-lang#81550 (Replace predecessor with range in collections documentation)
 - rust-lang#81558 (Fix ascii art text wrapping in mobile)
 - rust-lang#81562 (Clarify that InPlaceIterable guarantees extend to all advancing iterator methods.)
 - rust-lang#81563 (Improve docblock readability on small screen)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 13b3294 into rust-lang:master Jan 31, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 31, 2021
/// - **Symmetric**: if `A: PartialEq<B>` and `B: PartialEq<A>`, then **`a == b`
/// implies `b == a`**; and
///
/// - **Transitive**: if `A: PartialEq<B>` and `B: PartialEq<C>` and `A:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this part is updated for styling then Eq part should also be updated. Let me send a pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.