Use Borrow for binary_search and contains methods in the standard library

[christophebiocca]

Fixes all standard library methods in #32822 that can be fixed without backwards compatibility issues.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

[bluss]

There's another choice isn't there, to use fn contains<Q: ?Sized>(&self, x: &Q) -> bool where T: PartialEq<Q> instead? I think that is more flexible than using Borrow.

[christophebiocca]

Yes, that seems like it would work better for all the contains implementations, given that anyone who implements Borrow<Q> usually implements PartialEq<Q>. Bit of tunnel vision on my part given the issue description.

The binary searches still do need to use Borrow (unless Ord<RHS> were to become a thing).

[bluss]

The test failure looks legit unfortunately — so that is a hint that it can cause type inference-related regressions. Should probably get a crater run when it's ready.

[alexcrichton]

Thanks for the PR! I've started a crater run to evaluate the impact across the ecosystem here.

[alexcrichton]

Unfortunately crater turned up with 28 regressions, so we may not be able to make this change :(

[bluss]

@alexcrichton Note other comments, about revising the PR to use PartialEq for contains.

I guess with crates having code such as .contains(&"down.sql".into()) there's not much hope for that one either..

[christophebiocca]

The type-inference regression will still occur, so I suspect this new code will cause essentially the same regressions.

[jethrogb]

PartialEq has a default type parameter though?

Edit: just tested this (playpen), not sure why that's not sufficient.

[bluss]

There are no regressions due to .binary_search in the crater root regressions. Then, a minority is due to type inference failure or coercion failure when using .contains().

The absolute majority of the root regressions are due to an unrelated error:

curl: (56) GnuTLS recv error (-54): Error in the pull function.

[taskcluster 2016-11-15 00:23:23.175Z] === Task Finished ===

[taskcluster 2016-11-15 00:23:23.176Z] Unsuccessful task run with exit code: 56 completed in 2.348 seconds

[alexcrichton]

Crater report for current PR reports 10 regressions

[BurntSushi]

If we remove the changes to contains, can we still make the changes to binary_search? Of the 10 root regressions in the most recent crater report, 5 look spurious and the other 5 only appear on contains.

[letheed]

True. The fixes are trivial though and usually lead to code simplification:

• afterparty and clapx2: remove leading &
• Inflector: remove .as_ref()
• diesel: remove .into()

[BurntSushi]

@letheed I think we should be mindful/conservative about making this change even if crater said things were all clear. If it's going to break popular crates, then I don't think we should do it, no matter how trivial the fix is.

[bluss]

Crater is only an indication of the issues, it's far from all Rust code in existence.

[letheed]

@BurntSushi And I enjoy that things don't break at every new release of Rust. Still this proposed implementation for contains has merits. It should have been the one provided from the start.
Should a dedicated issue be created and tagged rust-2-breakage-wishlist?

[bluss]

Can this be solved by type parameter fallback? If so, it doesn't need Rust 2.

[christophebiocca]

@bluss, as best as I can tell type parameter fallback defaults do not work for functions, and it isn't 100% clear to me that they ever could work for functions.

[bluss]

@christophebiocca Did you try the feature in #27336?

[christophebiocca]

#27336 (comment) seems to indicate that default parameters do not work for fn, and there isn't any PR implementing it. But I might be wrong, it seems like that feature was talked about a lot over the last 2 years.

[bluss]

Ok, some experiments. It is implemented for functions, because fn get<T: Default = u32>() -> T { T::default() } works as expected and falls back to the given T type.

But the example in https://is.gd/cKIj8U says that the default fallback can not be used to cover up for type inference breaks.

[christophebiocca]

Rebased the branch to only have the binary_search changes, since those can land on their own.

I'm not sure what we want to do for the contains changes.

[alexcrichton]

cc @rust-lang/libs

This is kinda unfortunate where in theory we'd want to extend multiple APIs, but backwards compatibility forces us to only extend a few. That has the risk of leading to a surprisingly inconsistent experience across the standard library. Not doing this, however, also leads to a surprise where it's something that "should work" but doesn't. I'm not entirely sure what the best resolution is here.

[aturon]

I think I'm in favor. My biggest worry isn't about the inconsistency (which already exists, since we have this functionality for maps but not other structures). Rather, I worry about degrading the docs experience elsewhere. Still, given that we've committed to it for maps, which are one of the most common data structures, we may as well double down (and work on improving docs across the board).

[alexcrichton]

@bors: r+

Discussed during triage today and decision was to move forward, thanks for being patient @christophebiocca!

[bors]

📌 Commit c470d4a has been approved by alexcrichton

[bors]

⌛️ Testing commit c470d4a with merge 7bad2c3...

[bors]

💔 Test failed - auto-mac-32-opt

[alexcrichton]

@bors: retry

…

On Mon, Dec 19, 2016 at 6:03 PM, bors ***@***.***> wrote: 💔 Test failed - auto-mac-32-opt <https://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/11392> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37761 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95AplOW_8e5ncjk9l8V4Qfp5kU5OPks5rJzddgaJpZM4Kw8uE> .