Fix slice binary search signature mismatch

[circuitfox]

#37761 paramaterized binary_search over Borrow in core::SliceExt. This PR does the same for std::slice, so that the std and core slice APIs match.

Fixes #41561

[rust-highfive]

r? @aturon

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

[shepmaster]

Thanks for the PR! We'll make sure that @aturon or another reviewer takes a look soon.

[mzji]

The error message "the trait bound &std::string::String: std::borrow::Borrow<&str> is not satisfied" looks like legitimate. The simplest solution that makes the code compiles is changing the definition of function binary_search_by_key() (for both the std one and the libcore one) from

    pub fn binary_search_by_key<'a, B, F, Q>(&'a self, b: &Q, f: F) -> Result<usize, usize>
        where F: FnMut(&'a T) -> B,
              B: Borrow<Q>,
              Q: Ord + ?Sized

to

    pub fn binary_search_by_key<'a, B, F, Q>(&'a self, b: &Q, f: F) -> Result<usize, usize>
        where F: FnMut(&'a T) -> &'a B,    // Note the type of the return value
              B: Borrow<Q>,
              Q: Ord + ?Sized

, and change the line 27 of test run-pass/slice_binary_search.rs from

 let r = xs.binary_search_by_key(&key, |e| &e.topic);

to

 let r = xs.binary_search_by_key(key, |e| &e.topic);

. However, this is a breaking-change.

[aturon]

It might be possible to add a Borrow impl for &String, but I worry that this change will cause breakage more broadly. Still, if you want to try that, we could then try running the change through Crater, to get a sense for how much it breaks across the ecosystem.

cc @rust-lang/libs, does anyone recall how we even got to a place where core and std were discrepant?

[alexcrichton]

Added in #37761 the orginal intention was to generalize a bunch, we then backed it out because of breakage. I suspect it was just forgotten to back out the libcore changes, and the reviewer (me) wasn't paying enough attention to backing out the changes.

I would personally desire a crater run for this PR before we land it, as the last one turned up lots of breakage so we may not be able to land.

[aturon]

@alexcrichton Hm, that's not quite my read of #37761. It looks to me like the PR originally included similar generalizations to other functions, but never included a change to binary_search in std. See #32822 (comment)

In any case, seems like a crater run is in order.

[circuitfox]

This commit implements the changes to binary_search_by_key @mzji suggested. If this breaks too much on a Crater run then it could be removed like the changes to contains were in #37761.

[alexcrichton]

@aturon the original commit (I think?) did indeed only change libcore wrt binary_search, I must be misremembering.

In any case, seems like a crater run is in order.

Agreed.

[mzji]

Ok, seems that we should add a type annotation for the [] in line 357 & 358 in libcollections/tests/slice.rs. How about change these [] to [0usize; 0]?

[circuitfox]

So the tests now fail with binary_search_by_key's doc tests, because it wants a reference that (afaik) it can't actually get (because it won't live long enough.) This may be a dead end for fixing that original error via binary_search_by_key's signature.

[mzji]

@circuitfox Changing all 4 appearance of |&(a,b)| b to |item: &(_, _)| &item.1 at line 1118 to 1121 of libcollections/slice.rs will just work I think?

[circuitfox]

@mzji You're right. Changing |&(a,b)| to |&(a, ref b)| also works (I forgot about ref!), and it keeps the tuples destructured.

[mzji]

@circuitfox And that's shorter. 😄

[Mark-Simulacrum]

@brson @eddyb @alexcrichton Could someone do a crater run?

[carols10cents]

ping @brson @eddyb @alexcrichton reminder that this is waiting on a crater run!

[brson]

I've started building toolchains for crater

• crater Fix slice binary search signature mismatch #41590
  • https://github.com/circuitfox/rust
  • before 94e884b
  • https://tools.taskcluster.net/task-inspector/#sfbZosx3QuCxeq22S7Kl2g
  • after 8016849
  • https://tools.taskcluster.net/task-inspector/#YuRL6SOUShWt0gKhuPFmTw

[brson]

I've started the crate build for crater.

[brson]

https://gist.github.com/anonymous/3018e679203920f6b6dbaf9763c1ca37

[Mark-Simulacrum]

Actual root regressions below.

• cargo 0.18.0: error[E0277]: the trait bound &core::package_id::PackageId: std::borrow::Borrow<&&core::package_id::PackageId> is not satisfied
• roaring 0.5.2: expected &_, got u16
• filecheck 0.0.1: mismatched types, &_ got usize
• noise_search 0.5.0: mismatched types &_ got usize
• consistent_hash 0.1.4: mismatched types
• ructe 0.3.0: error[E0277]: the trait bound str: std::borrow::Borrow<&str> is not satisfied
• smoltcp 0.3.0: mismatched types
• unicode-casefold 0.2.0: mismatched types
• vertree 0.1.2: error[E0277]: the trait bound std::string::String: std::borrow::Borrow<&str> is not satisfied
• addr2line 0.3.0: mismatched types

[carols10cents]

So what are the next steps here @aturon @alexcrichton ?

[alexcrichton]

The next step is for the @rust-lang/libs team to decide whether this is acceptable breakage and whether or not to merge this PR. The S-waiting-on-team label and lack of needs-crater should be sufficient for coming up during triage.

[alexcrichton]

The libs team discussed this PR during triage and the conclusion was to close, and I've posted more comments on the tracking issue