slice: allow [T]::contains to accept any Q where T: Borrow<Q>

[sunshowers]

This is most useful to allow matching string literals against a
[String], or bytestring literals against a [Vec<u8>].

Since Vec<T> derefs to [T], this also applies to Vec<String> and
Vec<Vec<u8>>.

This implements https://internals.rust-lang.org/t/vec-contains-should-accept-anything-that-t-implements-borrow-for/5455 -- I'm really not sure about the stability guarantees required here, so some guidance would be really appreciated. Thanks :)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[sunshowers]

BTW, something similar can be done for binary_search too.

[sunshowers]

Hmm, the error in https://travis-ci.org/rust-lang/rust/jobs/249466135 is interesting -- looks like maybe we should implement Borrow<&&T> for &T?

[sunshowers]

One option here would be to have something like contains_borrow that takes in Borrow, and keep the existing contains the same.

The other option might be to implement contains both when T: Borrow<Q> and when Q: Borrow<T> (is this even correct, and can this be done through specialization?)

Thoughts?

[golddranks]

I just bumped into this today. I've got a Vec<String>, and I'd like to use .contains("static string"). So a big thumbs up for this!

[Mark-Simulacrum]

This feels like an unrelated change? Unless inference is failing, as I suspect it is, in which case I'm not sure we'd be able to land this -- too much breakage.

[sunshowers]

Yeah, inference was failing in this case.

[vitalyd]

@golddranks , a workaround would be to use a Vec<Cow<'static, str>>; just an FYI if you've not thought of that already.

[golddranks]

@vitalyd Unfortunately that's not an acceptable workaround if a Vec<String> is all you've got. A working workaround in this situation was to use .iter().any(|s| s == "static string").

[vitalyd]

@golddranks , you mean if you don't have control over the Vec? Yeah, indeed the workaround isn't possible.

Your workaround using any over an iterator brings up the question of whether providing contains(Q) where T: Borrow<Q> is worth its weight. Given this is an arbitrary slice, a linear search is unavoidable in either case. So this is more about ergonomics, I guess. For containers that perform sub-linear lookups, like HashSet, this is more critical as there's a performance implication of course.

[aturon]

Note, this PR is closely related to:

• SliceExt::binary_search has a different signature than slice::binary_search #41561
• Fix slice binary search signature mismatch #41590

The latter is a PR doing something similar for binary_search, which resulted in too much inference breakage for us to merge. I suspect that would also be the case here, though we could try a crater run if you like -- let me know!

[sunshowers]

Yeah the inference breaking is unfortunate :( seems like this could have been fixed before 1.0, but it's too late now.

@aturon Do you think it's worth adding _borrow methods (or maybe contains2, binary_search2 etc, or maybe some other set of names) and deprecating the existing methods?

[golddranks]

Maybe we should do that cargobomb run first before thinking the plan B?

[sunshowers]

I'm fine with that. Note that inference wasn't the only thing failing -- https://travis-ci.org/rust-lang/rust/jobs/249466135 has an example of code that was accepted earlier (extra &) now failing.

[aturon]

@brson Flagging for crater run!

[jethrogb]

Why Borrow and not PartialEq?

[carols10cents]

ping @alexcrichton, @brson, @tomprince, and @frewsxcv for a crater run!

[brson]

Crater does not work any more. @tomprince has cargobomb running against PRs. Maybe he can do it. I will add this to my list.

[Mark-Simulacrum]

Looking at the PR's travis log, this failed cargotest -- clap had inference failures. So I worry that we shouldn't do this. However, in order to run cargobomb, we'll need to get the artifacts; this may not work due to the cargotest failure but I guess we can try:

@bors try

[bors]

🔒 Merge conflict

[brson]

Looks like the merge conflict needs to be resolved, then the try run, then cargobomb.

[briansmith]

I agree with @jethrogb that it seems to make more sense to use PartialEq instead of Borrow here, and just make the constrait looser. I think contains()'s semantics is by definition self.iter().any(|elem| *elem == *x) and the expression self.iter().any(|elem| *elem == *x) requires x: U where T: PartialEq<U>:

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

impl<T> Contains<T> for [T] {
    fn contains<U>(&self, x: &U) -> bool where T: PartialEq<U> {
        self.iter().any(|e| e == x)
    }
}

[brson]

@bors try

[aidanhs]

@Sid0 could you take a look at rebasing?

In order to get artifacts, I believe we'll need to do alter the CI for this job to not build rustbook - I don't see how we'll get to the deploy stage otherwise. I don't know how to achieve that off the top of my head, so I'll look into it.

[alexcrichton]

I'm going to close this out of inactivity, but please feel free to resubmit!