Generalise slice::contains over PartialEq

[varkor]

This allows contains to be used on slices to search for elements that
satisfy a partial equality with the slice item type, rather than an
identity. This closes #42671. Note that string literals need to be
referenced in this implementation due to str not implementing
Sized. It’s likely this will have to go through a Crater run to test
for breakage (see #43020).

[rust-highfive]

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

[varkor]

Investigating the issue with clap now.

[varkor]

The issue here is that some crates define PartialEq implementations of the form A: PartialEq<B> without declaring B: PartialEq<A> and vice versa (which is "not allowed" but also not prevented), so declaring contains in terms of either of them causes breakage.

I'm not sure of the best way to address this problem: I think we certainly want to warn/error against non-symmetric partial equalities (or implement them automatically), but that would take time.

Can you think of any ways to get around this that wouldn't involve updating the incorrect crates?

EDIT: On the other hand, if a crate has previously used contains(), they must have been passing an element of some type T such that T: PartialEq, so this at least shouldn't cause breakage, unless the problem is with the type inference, with seems plausible.

[CAD97]

I think I may have found a way to make this work...

A reduction of what this implementation ran into: (playground link)

trait Contains<T>
where
    T: ?Sized,
{
    fn contains<U>(&self, x: &U) -> bool
    where
        T: PartialEq<U>,
        U: ?Sized,
    {
        true
    }
}

impl<'a, T> Contains<T> for &'a [T] {}

#[test]
fn test() {
    let a: &str = "string";
    let v: &[&str] = &[a];
    Contains::contains(&v, a);
}

   Compiling playground v0.0.1 (file:///playground)
error[E0277]: the trait bound `&str: std::cmp::PartialEq<str>` is not satisfied
  --> src/main.rs:20:5
   |
20 |     Contains::contains(&v, a);
   |     ^^^^^^^^^^^^^^^^^^ can't compare `&str` with `str`
   |
   = help: the trait `std::cmp::PartialEq<str>` is not implemented for `&str`
   = note: required by `Contains::contains`

In these types, we have contains(&[&str], &str). This means that the compiler enforces &str: PartialEq<str>, which does not exist. This may be an unrelated issue, but we can work around this by being specific on what bound we want:

fn contains<'a, U>(&self, x: &'a U) -> bool
where T: PartialEq<&'a U>

The other choice is to go the other way and make the reference part of the <U> type:

fn contains<U>&self, x: U) -> bool
where T: PartialEq<U>

I think this is the solution that we want to use here, assuming it works, but one of the two definitely should. @varkor ?

---

On &str: PartialEq<str>, from nightly docs:

impl PartialEq<str> for str

impl PartialEq<str> for String
impl PartialEq<String> for str

impl PartialEq<&str> for String
impl PartialEq<String> for &str

impl PartialEq<str> for Cow<str>
impl PartialEq<Cow<str>> for str

impl PartialEq<&str> for Cow<str>
impl PartialEq<Cow<str>> for &str

impl PartialEq<String> for Cow<str>
impl PartialEq<Cow<str>> for String

The only impl that doesn't involve non-str types here is PartialEq<str> for str. As an &str impl is provided for String and Cow, is it just missing here for str: PartialEq<&str> and &str: PartialEq<str>, or is this supposed to be provided by Deref cooersions, which are failing?

[varkor]

@CAD97: In order to conserve the behaviour of type signature of contains at the moment, it has to take a reference to the element we're searching for, and the comparison PartialEq is defined over the base types, not the reference types. Maybe I'm not understanding, but your suggestion signatures seem to fail for most of the existing contains operations — whenever you actually do want to compare a U directly to a T.

Regarding the question about the nonexistence of str: PartialEq<&str> (and vice versa), that seems related to this question: rust-lang/rfcs#1332 (in the generic case). I'm not sure why it hasn't been implemented in the specific case of str (although it couldn't be implemented in the same location as the other examples, as str doesn't belong to the same crate as the other string types, so it's a little trickier than simply adding another case).

[CAD97]

OK, I missed some of the complexity here, due to only working with my minimized example. Here's an updated small example: playground.

[varkor]

After implementing str: PartialEq<&str> and &str: PartialEq<str>, the tests seem to be passing (the ones that were broken before, anyway). But this could point to problems in other crates. It'd be good to do a Crater run to check.

[kennytm]

@bors try

[bors]

⌛ Trying commit 6ea22ad with merge 196146b...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[kennytm]

@aidanhs @Mark-Simulacrum This needs a crater run (new=196146baff1626dd3a19458e9cecf545120f3ddf vs old=3fd27b2718438ceec6243869cde3edde7e5a904e)

[aidanhs]

Crater run started.

[varkor]

#46713 has broken this implementation. I'll figure out what to do if the Crater run has a positive outcome, because a fix is non-trivial.

[aidanhs]

Crater run failed and started again (workaround for the issue has been put in place).

[Mark-Simulacrum]

http://cargobomb-reports.s3.amazonaws.com/pr-46934/index.html

[varkor]

Okay, it's quite clear that this causes a ton of type inference issues. Breakage is probably not worth the benefit. I'll close and mark #42671 as unlikely to be fixed if there are no other reservations?

[bluss]

Just to showcase why naked unqalified .into() is a bad idea, it is fragile:

Jan 09 21:58:29.356 INFO kablam! error[E0283]: type annotations required: cannot resolve `std::string::String: std::cmp::PartialEq<_>`
Jan 09 21:58:29.356 INFO kablam!   --> src/migrations/migration.rs:60:15
Jan 09 21:58:29.356 INFO kablam!    |
Jan 09 21:58:29.356 INFO kablam! 60 |         files.contains(&"down.sql".into()) && files.contains(&"up.sql".into())
Jan 09 21:58:29.357 INFO kablam!    |               ^^^^^^^^
Jan 09 21:58:29.357 INFO kablam! 

This code would want this PR, to use files.contains("down.sql") and not create a String.

A whole large part of the crater issues are exactly like that, and it's a shame they are blocking themselves from improving. There are other less simple cases though.

[leoyvens]

I suppose default type parameter fallback would allow this by setting T as a default U = T.

[aidanhs]

I'm going to go ahead and close this PR, looks like another approach is being discussed over at the original issue though.