BTree: remove Ord bound where it is absent elsewhere

[ssomers]

Some btree methods don't really need an Ord bound and don't have one, while some methods that more obviously don't need it, do have one.

An example of the former is iter, even though it explicitly exposes the work of the Ord implementation ("sorted by key" - but I'm not suggesting it should have the Ord bound). An example of the latter is new, which doesn't involve any keys whatsoever.

[jyn514]

I think this should have a reviewer from T-libs.

r? @dtolnay , maybe?

[ssomers]

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[dtolnay]

@rust-lang/libs:
@rfcbot fcp merge

This affects the following individual stable APIs. There are four kinds:

• Functions in which no comparison can ever take place because no elements exist to compare. new(), default()
• Functions that have no need to do comparisons. map.clear(), entry.key()
• Functions where all comparison takes place not in terms of K, but some other type Q for which K: Borrow<Q>. We only ever compare by borrowing from K and comparing using Q: Ord.
• Functions that just construct an iterator without doing work, but there remains an Ord bound on the Iterator impl. symmetric_difference, union

 impl<K, V> BTreeMap<K, V> {
pub const fn new() -> BTreeMap<K, V>
- where- K: Ord;
pub fn clear(&mut self)
- where- K: Ord;
pub fn get<Q>(&self, key: &Q) -> Option<&V>
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
pub fn get_key_value<Q>(&self, k: &Q) -> Option<(&K, &V)>
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
pub fn contains_key<Q>(&self, key: &Q) -> bool
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
pub fn get_mut<Q>(&mut self, key: &Q) -> Option<&mut V>
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
pub fn remove<Q>(&mut self, key: &Q) -> Option<V>
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
pub fn remove_entry<Q>(&mut self, key: &Q) -> Option<(K, V)>
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
pub fn range<T, R>(&self, range: R) -> Range<'_, K, V>
where
- K: Ord + Borrow<T>,+ K: Borrow<T>,
T: ?Sized + Ord,
R: RangeBounds<T>;
pub fn range_mut<T, R>(&mut self, range: R) -> RangeMut<'_, K, V>
where
- K: Ord + Borrow<T>,+ K: Borrow<T>,
T: ?Sized + Ord,
R: RangeBounds<T>;
pub fn split_off<Q>(&mut self, key: &Q) -> Self
where
- K: Ord + Borrow<Q>,+ K: Borrow<Q>,
Q: ?Sized + Ord;
}
impl<K, V> Default for BTreeMap<K, V>
- where- K: Ord;
impl<K, V> Debug for Entry<'_, K, V>
where
- K: Ord + Debug,+ K: Debug,
V: Debug;
impl<K, V> Debug for VacantEntry<'_, K, V>
where
- K: Ord + Debug,+ K: Debug;
impl<K, V> Debug for OccupiedEntry<'_, K, V>
where
- K: Ord + Debug,+ K: Debug,
V: Debug;
impl<'a, K, V> Entry<'a, K, V> {
pub fn or_insert(self, default: V) -> &'a mut V
- where- K: Ord;
pub fn or_insert_with<F>(self, default: F) -> &'a mut V
where
- K: Ord,
F: FnOnce() -> V;
pub fn key(&self) -> &K
- where- K: Ord;
pub fn and_modify<F>(self, f: F) -> Self
where
- K: Ord,
F: FnOnce(&mut V);
pub fn or_default(self) -> &'a mut V
where
- K: Ord,
V: Default;
}
impl<'a, K, V> VacantEntry<'a, K, V> {
pub fn key(&self) -> &K
- where- K: Ord;
pub fn into_key(self) -> K
- where- K: Ord;
pub fn insert(self, value: V) -> &'a mut V
- where- K: Ord;
}
impl<'a, K, V> OccupiedEntry<'a, K, V> {
pub fn key(&self) -> &K
- where- K: Ord;
pub fn remove_entry(self) -> (K, V)
- where- K: Ord;
pub fn get(&self) -> &V
- where- K: Ord;
pub fn get_mut(&mut self) -> &mut V
- where- K: Ord;
pub fn into_mut(self) -> &'a mut V
- where- K: Ord;
pub fn insert(&mut self, value: V) -> V
- where- K: Ord;
pub fn remove(self) -> V
- where- K: Ord;
}
impl<T> BTreeSet<T> {
pub const fn new() -> BTreeSet<T>
- where- T: Ord;
pub fn range<K, R>(&self, range: R) -> Range<'_, T>
where
- T: Ord + Borrow<K>,+ T: Borrow<K>,
K: ?Sized + Ord,
R: RangeBounds<K>;
pub fn symmetric_difference<'a>(&'a self, other: &'a BTreeSet<T>) -> SymmetricDifference<'a, T>
- where- T: Ord;
pub fn union<'a>(&'a self, other: &'a BTreeSet<T>) -> Union<'a, T>
- where- T: Ord;
pub fn clear(&mut self)
- where- T: Ord;
pub fn contains<Q>(&self, value: &Q) -> bool
where
+ T: Ord + Borrow<Q>,- T: Borrow<Q>,
Q: ?Sized + Ord;
pub fn remove<Q>(&mut self, value: &Q) -> bool
where
- T: Ord + Borrow<Q>,+ T: Borrow<Q>,
Q: ?Sized + Ord;
pub fn split_off<Q>(&mut self, key: &Q) -> Self
where
- T: Ord + Borrow<Q>,+ T: Borrow<Q>,
Q: ?Sized + Ord;
}
impl<T> Default for BTreeSet<T>
- where- T: Ord;

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @KodrAus
• @dtolnay
• @m-ou-se
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[dtolnay]

I would request to keep the Ord bound on all Entry methods that mutate the collection, and on symmetric_difference/union.

[ssomers]

I would request to keep the Ord bound on all Entry methods that mutate the collection

Does that include the methods that do not actually mutate but allow mutation, i.e., OccupiedEntry::get_mut and into_mut? If so, I can't imagine why plain get is allowed to give up its Ord bound.

[ssomers]

I would request to keep the Ord bound […] on symmetric_difference/union.

Pointing out that inconsistency in the initial form of this PR, answers my implicit question: why impose unused bounds anywhere? It allows tuning the implementation later.

Another inconsistency is that BTreeMap::get loses the bound, but BTreeSet::get doesn't, and can't as long as it reuses an implementation of Recover. So I'm motivated to turn away from touching those.

That leaves version 2 with this set of changes:

 impl<K, V> BTreeMap<K, V> {
pub const fn new() -> BTreeMap<K, V>
- where- K: Ord;
pub fn clear(&mut self)
- where- K: Ord;
}
impl<K, V> Default for BTreeMap<K, V>
- where- K: Ord;
}
impl<K, V> Debug for Entry<'_, K, V>
where
- K: Ord + Debug,+ K: Debug,
V: Debug;
}
impl<K, V> Debug for VacantEntry<'_, K, V>
where
- K: Ord + Debug,+ K: Debug;
}
impl<K, V> Debug for OccupiedEntry<'_, K, V>
where
- K: Ord + Debug,+ K: Debug,
V: Debug;
}
impl<'a, K, V> Entry<'a, K, V> {
pub fn key(&self) -> &K
- where- K: Ord;
}
impl<'a, K, V> VacantEntry<'a, K, V> {
pub fn key(&self) -> &K
- where- K: Ord;
pub fn into_key(self) -> K
- where- K: Ord;
}
impl<'a, K, V> OccupiedEntry<'a, K, V> {
pub fn key(&self) -> &K
- where- K: Ord;
pub fn get(&self) -> &V
- where- K: Ord;
pub fn get_mut(&mut self) -> &mut V
- where- K: Ord;
pub fn into_mut(self) -> &'a mut V
- where- K: Ord;
}
impl<T> BTreeSet<T> {
pub const fn new() -> BTreeSet<T>
- where- T: Ord;
pub fn clear(&mut self)
- where- T: Ord;
}
impl<T> Default for BTreeSet<T>
- where- T: Ord;

[BurntSushi]

Can someone say why we would want to do this, other than minor simplifications in the type signature? Is there a concrete benefit to this? Conversely, if we do this and accidentally remove Ord from an API that wants it in the future, we're stuck. (The APIs proposed look fine to me, so maybe that's a weak argument, but it still seems like a risk to me.)

[ssomers]

Is there a concrete benefit to this?

• Consistency in the current API: I've read posts, and long ago experienced myself, stumbling upon the bound on new while making code generic over types of containers (e.g. when comparing BTreeSet vs. HashSet). Though I can't find any concrete traces of that.
• Consistency in the future API. Methods like drain_filter and first_entry impose an Ord bound just because it seems popular somehow, but their behaviour is close to iter which doesn't. The recent into_keys and into_values impose an Ord bound, while into_iter doesn't. I'm sure explicitly listing the Ord bound everywhere would help realizing that, but the second version doesn't do that anymore.
• In implementations in btree, the inability to call new or default requires us to rewrite it sometimes, though I'm surprised to only find one occurrence that made it to master.

[BurntSushi]

Thanks for explaining! I'm a little skeptical that those things are worth the risk here personally. The last bullet point (writing generic code) seems the most compelling to me I think, but I'm having a hard time of thinking of how that actually happens in practice.

[ssomers]

I've added two things:

• A 1st commit that makes the existing bounds explicit where they were sneakily declared far away.
• In the 2nd commit, apart from clear and Debug impls, also removed the bound from the unstable into_keys and into_values.

[KodrAus]

To cater for the unknown.

This is probably our most compelling reason to keep bounds right now. But the changes you've made here seem pretty defensible and are a subset of the ones originally proposed in the FCP that already completed. Until we have some concrete recommendations on when bounds are needed in our collection types we're probably just going to have to tread carefully like you're already doing.

@dtolnay are you satisfied with the state of things here so we can merge?

[dtolnay]

Could someone make an updated list of every API change, like #79245 (comment)? This PR is difficult to review otherwise because all the API changes are taking place in lines of code that are not touched.

[dtolnay]

For review it might even be better to do this as 2 PRs, one moving Ord bounds from impl blocks to functions (no API change) and then an API changing PR that only removes bounds.

[ssomers]

I've isolated the first commit as #81610 and will rebase here if and when it gets merged.
@rustbot modify labels: +S-waiting-on-author -S-waiting-on-review

[ssomers]

I backed out on all changes in Entry because the "everything Ord" rule applies there.
@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

[dtolnay]

This is a subset of the changes from the prior completed FCP.

 impl<K, V> BTreeMap<K, V> {
pub fn clear(&mut self)
- where- K: Ord;
}
impl<T> BTreeSet<T> {
pub fn clear(&mut self)
- where- T: Ord;
}

and a few unstable ones (tracking issue #75294):

 impl<K, V> BTreeMap<K, V> {
pub fn into_keys(self) -> IntoKeys<K, V>
- where- K: Ord;-
pub fn into_values(self) -> IntoValues<K, V>
- where- K: Ord;
}

[dtolnay]

@bors r+

[bors]

📌 Commit 9066c73 has been approved by dtolnay

[bors]

⌛ Testing commit 9066c73 with merge 0b96f60...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 0b96f60 to master...

[pthariensflame]

relnotes needed here?

[ssomers]

I wouldn't know the answer even if I understood the question, but as a summary: the only change to the public stable API is that BTreeMap::clear and BTreeSet::clear no longer have the Ord bound.