introduce the Comparable trait for BTree operations

[conradludgate]

View all comments

Tracking issue: #145986

This introduces the Equivalent and Comparable traits into core, under the comparable_trait feature flag. This exposes Comparable for btree key lookups, with a blanket impl for Borrow keys to remain backwards compatible.

The one annoying thing is that we still need Q: Ord in order to provide the panic messages for bad usage in range queries. One could likely address this but it would move the branch inside the search loop which might impact performance. Another approach is to move the condition out, but that won't help if we want the same helpful panic condition in any potential exposed API. Note that this check is not needed for memory safety (as we already have to deal with invalid Ord impls) so maybe such a check is not really that necessary. The alternative is we accept this Q: Ord bound as not a significant limitation.

[rustbot]

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ibraheemdev]

r? libs-api because this is related to a future API change, I'm sure if this is the best way to proceed.

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting and would be interested in having these traits in the standard library. An idea also came up to also provide blanket implementations of these traits for tuples of types implementing Borrow so that (String, String) could be indexed by (&str, &str).

We are however not willing to accept this PR as it is: we want these traits to be public and tracked by a tracking issue so that people can start experimenting with them on nightly.

cc @cuviper

[conradludgate]

Ok. I can create a tracking issue and update the PR to expose these traits in core with some blanket impls

[conradludgate]

An idea also came up to also provide blanket implementations of these traits for tuples of types implementing Borrow so that (String, String) could be indexed by (&str, &str).

Doesn't seem to be possible. (K1, K2) might be (&str, &str) which implements Borrow<(&str, &str)> which causes a conflict for Equivalent<(&Q1, &Q2)>

[cuviper]

An idea also came up to also provide blanket implementations of these traits for tuples of types implementing Borrow so that (String, String) could be indexed by (&str, &str).

Doesn't seem to be possible. (K1, K2) might be (&str, &str) which implements Borrow<(&str, &str)> which causes a conflict for Equivalent<(&Q1, &Q2)>

Right, impl<T> Borrow<T> for T gets in the way.

It would probably make a good example though, e.g. some kind of struct Lookup(&str, &str) newtype.

[Amanieu]

Could we make this work by moving the reference to Q into the trait? Playground

[bors]

☔ The latest upstream changes (presumably #146160) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Amanieu]

This will most likely need a crater run to see if changing the parameters of stable functions breaks anything.

[conradludgate]

I believe we also have an insta-stable hazard for the (&K1, &K2) impl

[rust-log-analyzer]

The job x86_64-gnu-gcc failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] git2 test:false 16.596
error[E0277]: the trait bound `str: std::clone::Clone` is not satisfied
    --> src/tools/cargo/src/cargo/ops/cargo_new.rs:391:46
     |
 391 |             if let Some(x) = BTreeMap::get::<str>(&duplicates_checker, &name) {
     |                                              ^^^ the trait `std::clone::Clone` is not implemented for `str`
     |
help: the trait `std::clone::Clone` is implemented for `std::string::String`
    --> library/alloc/src/string.rs:2350:1
     |
2350 | impl Clone for String {
     | ^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `BTreeMap::<K, V, A>::get`
    --> library/alloc/src/collections/btree/map.rs:717:19
     |
 717 |     pub fn get<Q: Clone>(&self, key: Q) -> Option<&V>
     |                   ^^^^^ required by this bound in `BTreeMap::<K, V, A>::get`

error[E0277]: the trait bound `&str: Comparable<str>` is not satisfied
    --> src/tools/cargo/src/cargo/ops/cargo_new.rs:391:51
     |
 391 |             if let Some(x) = BTreeMap::get::<str>(&duplicates_checker, &name) {
     |                              -------------------- ^^^^^^^^^^^^^^^^^^^ the nightly-only, unstable trait `Comparable<str>` is not implemented for `&str`
     |                              |
     |                              required by a bound introduced by this call
     |
help: the trait `Comparable<(&'a Q1, &'a Q2)>` is implemented for `(K1, K2)`
    --> library/core/src/cmp.rs:1601:1
     |
1601 | / impl<'a, Q1: ?Sized, K1, Q2: ?Sized, K2> Comparable<(&'a Q1, &'a Q2)> for (K1, K2)
1602 | | where
1603 | |     Q1: Ord,
1604 | |     K1: crate::borrow::Borrow<Q1>,
1605 | |     Q2: Ord,
1606 | |     K2: crate::borrow::Borrow<Q2>,
     | |__________________________________^
note: required by a bound in `BTreeMap::<K, V, A>::get`
    --> library/alloc/src/collections/btree/map.rs:719:12
     |
 717 |     pub fn get<Q: Clone>(&self, key: Q) -> Option<&V>
     |            --- required by a bound in this associated function
 718 |     where
 719 |         K: Comparable<Q> + Ord,
     |            ^^^^^^^^^^^^^ required by this bound in `BTreeMap::<K, V, A>::get`

error[E0308]: mismatched types
   --> src/tools/cargo/src/cargo/ops/cargo_new.rs:391:72
    |
391 |             if let Some(x) = BTreeMap::get::<str>(&duplicates_checker, &name) {
    |                              --------------------                      ^^^^^ expected `str`, found `&&str`
    |                              |
    |                              arguments to this function are incorrect
    |
note: method defined here
   --> library/alloc/src/collections/btree/map.rs:717:12
    |
717 |     pub fn get<Q: Clone>(&self, key: Q) -> Option<&V>
    |            ^^^

error[E0277]: the size for values of type `str` cannot be known at compilation time
   --> src/tools/cargo/src/cargo/ops/cargo_new.rs:391:72
    |
391 |             if let Some(x) = BTreeMap::get::<str>(&duplicates_checker, &name) {
    |                                                                        ^^^^^ doesn't have a size known at compile-time
    |
    = help: the trait `Sized` is not implemented for `str`
    = note: all function arguments must have a statically known size
    = help: unsized fn params are gated as an unstable feature

For more information how to resolve CI failures of this job, visit this link.

[conradludgate]

Well, I don't think we need a crater run 😅

[cuviper]

With all your Clone additions, it sure would be nice if we could force Comparable<Q: Clone> here instead, but I don't think that would get elaborated elsewhere. This would work better the other way with Q: Comparable<K>, because then we could add a supertrait Comparable: Clone.

Also, I think Copy would be better, because it would be a perf footgun if some user type with non-trivial Clone was use for lookup.

[cuviper]

 391 |             if let Some(x) = BTreeMap::get::<str>(&duplicates_checker, &name) {
     |                                              ^^^ the trait `std::clone::Clone` is not implemented for `str`

Yeah, this is damning against changing the type parameters. Too bad.

[cuviper]

BTW, the tracking issue declares Comparable: Equivalent, but that relationship is missing here -- are you purposely leaving that out?

[conradludgate]

I was implementing as it was given in rust-lang/libs-team#699 (comment). No supertrait relationship there, but I did consider that it was potentially an oversight

[Amanieu]

Yeah, this is damning against changing the type parameters. Too bad.

I think we can still do this over an edition by making use of edition-dependent name resolution. But yea, this is effectively dead until we have support for this in the compiler.

[cuviper]

View changes since the review

I think this should allow the inner members to use the new trait too:

Suggested change

	impl<'a, Q1: ?Sized, K1, Q2: ?Sized, K2> Equivalent<(&'a Q1, &'a Q2)> for (K1, K2)
	where
	Q1: Eq,
	K1: crate::borrow::Borrow<Q1>,
	Q2: Eq,
	K2: crate::borrow::Borrow<Q2>,
	{
	#[inline]
	fn equivalent(&self, key: (&'a Q1, &'a Q2)) -> bool {
	PartialEq::eq(self.0.borrow(), key.0) && PartialEq::eq(self.1.borrow(), key.1)
	}
	}
	impl<Q1, K1, Q2, K2> Equivalent<(Q1, Q2)> for (K1, K2)
	where
	K1: Equivalent<Q1>,
	K2: Equivalent<Q2>,
	{
	#[inline]
	fn equivalent(&self, key: (Q1, Q2)) -> bool {
	K1::equivalent(&self.0, key.0) && K2::equivalent(&self.1, key.1)
	}
	}

(assuming we solve the API evolution problem to enable this at all)