impl PartialEq<Punct> for char; symmetry for #78636

[pthariensflame]

Also fixes the "since" version of the original.

Pinging @dtolnay and @petrochenkov.

[rust-highfive]

r? @petrochenkov

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

[pthariensflame]

This will also need relnotes like the previous one.

[petrochenkov]

cc #78636 (comment)
r? @dtolnay

[dtolnay]

@rust-lang/libs
@rfcbot fcp close

I don't think this impl is necessary, since it is only motivated by dogmatic reasons in #78636 (comment) rather than a use case. I would prefer instead to update the symmetry and transitivity description at the top of https://doc.rust-lang.org/1.49.0/std/cmp/trait.PartialEq.html. The condition should be that if b == a exists then it is symmetric with a == b, and if a == c exists then it is transitive with a == b and b == c, not that those impls must exist. I don't believe that we follow the previous stated requirements in the standard library, nor would it be reasonable to follow blindly. Those requirements would tend to force downstream code to add useless and potentially large numbers of impls in response to every PartialEq impl added by the standard library.

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @KodrAus
• @dtolnay
• @m-ou-se
• @sfackler
• @withoutboats

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.

[pthariensflame]

It does feel weird that one can write pct == '.' but not '.' == pct without this. My intuition says it also would prevent easy use of generic algorithms that operate over PartialEq if the "direction" happens to be wrong, but I can't immediately think of one.

[BurntSushi]

So just so I'm understanding things, we have punct == 'x' already, and this would make it possible to write 'x' == punct? Why wouldn't we want that? I'm not following.

[dtolnay]

@BurntSushi I don't object to the specific impl, but in this case it's being added only because of the rules dictated in the PartialEq documentation, and I think that those rules are wrong. Accepting the impl but changing the rules would be fine.

An example of how this goes wrong, if we don't change the rules, is that they insist every crate that defines impl PartialEq<char> for Something to also add impl PartialEq<Punct> for Something. For example regex has:

/// 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
    }
}

and clap has:

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,
        }
    }
}

where those types have nothing to do with proc macros, so asking them to include a PartialEq<proc_macro::Punct> would be silly.

[pthariensflame]

Those objections seem to only apply to transitivity, though. Completing symmetry only ever requires local changes of bounded size, whereas (as has been pointed out) completing transitivity might require instances that relate two types from crates that have no access to each other, and explosively grows in the size of the changes needed.

I think perhaps the requirement for transitivity should be clarified to a recommendation for transitive instances for types that are “already available for other reasons” (including those defined in the same crate) along with a “real” requirement that if a triangle of instances exists, it commutes.

I don’t see any reason for dropping the symmetry requirement, though; by doing so, we ensure that anyone who does want to write and/or use generic algorithms involving PartialEq/PartialOrd will have to deal with symmetry possibly breaking on them.

[pthariensflame]

For what it’s worth, I’d be willing to comb through std, alloc, and core to ensure that all standard instances are symmetric.

[pthariensflame]

I just thought of another reason to strongly prefer keeping symmetry a requirement: it allows instances that need to delegate to generic contents to be symmetric too. For example:

impl<T, U> PartialEq<Box<T>> for Box<U>
    where T: ?Sized, U: ?Sized + PartialEq<T>
{
    // …
}

If the underlying PartialEq<T> for U isn’t symmetric, there’s no way to make the Box instance symmetric, because it would violate coherence.

EDIT: Okay, yes, you could require T: PartialEq<U> as well, but not only would that restrict the Box instance from being used with non-symmetric underlying instances (thus de facto encouraging symmetry anyway) but it would break backwards compatibility for the existing instance, which the standard library cannot tolerate.

[pthariensflame]

(Defining non-symmetric Partial{Eq,Ord} instances is the kind of thing I’d expect to be a Clippy lint.)

[dtolnay]

Those objections seem to only apply to transitivity, though. [...] I don’t see any reason for dropping the symmetry requirement.

I object to both rules. For an example of why symmetry is bad, here is another example from the proc macro API: this is a great and super useful impl but the reverse impl does not typecheck. That's not a problem because if the_ident == "keyword" is almost always the natural order in which this comparison would come up for the programmer in real code.

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

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=be3aecb6e7947dc92110f65139a2edaf

[BurntSushi]

@dtolnay I agree that adding PartialEq impls in regex and clap for things like Punct would indeed be quite weird, and if the rules lead to that conclusion, then yes, they should be changed.

But I don't think we need to block this particular impl on that?

[pthariensflame]

I think @dtolnay's point is that, while this impl in and of itself is inoffensive, there's also no specific reason to have it, and merging it would thus set a precedent the core team doesn't want. (Personally I think symmetry where we can get it is reason enough, but I take it that's not @dtolnay's position.)

[BurntSushi]

there's also no specific reason to have it

See my comment above. My understanding is that this allows one to write 'x' == punct, instead of having to remember that punct has to go on the left side.

[pthariensflame]

Yes, that's correct. I also think that's enough of a justification to want this, but @dtolnay seems to disagree?

[dtolnay]

Marking as blocked on #81198.

@rustbot modify labels: +S-blocked

[dtolnay]

@rfcbot fcp cancel

[rfcbot]

@dtolnay proposal cancelled.

[pthariensflame]

Does this need its since bounds changed now?

[rfcbot]

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

[KodrAus]

@pthariensflame This will need to be updated to 1.52.0, but we can do that after the FCP finishes 🙂

[rfcbot]

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.

[pthariensflame]

Pushed an update to the since bounds as detailed above. No other changes relative to what was FCP'd.

[m-ou-se]

@bors r+ rollup

[bors]

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

[pthariensflame]

This also needs relnotes now, right?

[pthariensflame]

Bumping for the relnotes need.