Skip to content

Conversation

conradludgate
Copy link
Contributor

@conradludgate conradludgate commented Jul 26, 2025

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
Copy link
Collaborator

rustbot commented Jul 26, 2025

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 26, 2025
@ibraheemdev
Copy link
Member

ibraheemdev commented Jul 28, 2025

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

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 28, 2025
@rustbot rustbot assigned Amanieu and unassigned ibraheemdev Jul 28, 2025
@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 26, 2025
@Amanieu
Copy link
Member

Amanieu commented Aug 28, 2025

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

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 28, 2025
@conradludgate
Copy link
Contributor Author

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

@rustbot

This comment has been minimized.

@conradludgate conradludgate changed the title introduce the Comparable trait for btree internals introduce the Comparable trait for BTree operations Aug 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

This PR was rebased onto a different master 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.

@rust-log-analyzer

This comment has been minimized.

@conradludgate
Copy link
Contributor Author

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)>

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu-llvm-19-1 failed! Check out the build log: (web) (plain enhanced) (plain)

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

---- [rustdoc] tests/rustdoc/internal.rs stdout ----
------python3 stdout------------------------------

------python3 stderr------------------------------
13: !has check failed
 `XPATH PATTERN` unexpectedly matched
 //@ !has internal/struct.S.html '//*[@class="stab unstable"]' ''

Encountered 1 errors

------------------------------------------

error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/aarch64-unknown-linux-gnu/test/rustdoc/internal" "/checkout/tests/rustdoc/internal.rs"
stdout: none
--- stderr -------------------------------
13: !has check failed
 `XPATH PATTERN` unexpectedly matched
 //@ !has internal/struct.S.html '//*[@class="stab unstable"]' ''

Encountered 1 errors
------------------------------------------

---- [rustdoc] tests/rustdoc/internal.rs stdout end ----

@cuviper
Copy link
Member

cuviper commented Aug 29, 2025

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
Copy link
Member

Amanieu commented Aug 29, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants