Add key_mut method to BTreeMap entries

[clarfonthey]

This implements part of the proposal in this comment to improve the BTreeMap cursor API.

I'm not a huge fan of including an entire Ord implementation in a doc example but this feels like the best way to show off how to use these methods. Definitely open to feedback on that.

For now, I've included the cursor API tracking issue as the tracking issue for these but a separate feature flag, since I feel these are technically disjoint from it. Would be open to separating it into a different tracking issue.

Also open to feedback on the safety guarantee for VacantEntry. Definitely open to changing that, but I figured being the most cautious upfront would be good, since we can always loosen the guarantees later.

ACP: rust-lang/libs-team#356

[rustbot]

r? @m-ou-se

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

[clarfonthey]

(side note, apparently ./x test library/std didn't include the doc tests for alloc 😞)

[ripytide]

Reposting my comment from #118208 (comment) here slightly adapted as I believe it is similarly relevant:

Would it not be a better idea to rather than creating this unsafe key_mut() method to have unlimited &mut key access, to instead expose some function fn try_replace_key(&mut self, new_key: K) -> Result<K, InvalidKeyError> on OccupiedEntry which would check if the new key held up to the Ord invariants with the surrounding keys when doing the replacement.

I suppose the disadvantage would be that performance wise you would always do up to two comparisons per key replacement. Perhaps then we could also add an unsafe fn replace_key_unchecked(&mut self, new_key: K) variant as well?

[clarfonthey]

I could add that, but the main reason why I prefer a key_mut API regardless is that most of these cases involve modifying data inside the key that isn't related to the Ord implementation, and having to do some kind of replace operation might be too costly since it could require reallocating the contents of the key.

I also am not sure how to perform those checks in the code at the moment, but I could probably figure it out.

[ripytide]

Hmm, that use-case sounds odd, why would you have data in the key not related to the Ord implementation that needs mutating, could you not move those things into the value type?

[clarfonthey]

Basically the primary case that was brought up when making this API was range maps, where your keys are ranges of values instead of values themselves. A common way of implementing this is to make your Ord implementation only sorts by the start of the range, then later ensure that the ranges in the map don't overlap by removing some keys and modifying others to extend them to a larger area.

So, having to re-check that the start of the range here is valid when that's an invariant that must be upheld here anyway doesn't really help. And since you're probably going to make breaking this invariant unsafe, there's not really a point to having a "safe" version of the re-insert code since you're already in unsafe territory anyway.

But I also am making a lot of assumptions here, so, your argument is probably also valid. It's just not what was on my mind when writing it!

Also, it's worth adding that you're right that these bits could be stored in the value instead. It's just easier to make the map go from Range<uN> to V instead of uN to (uN, V) where the two halves of the range are split.

[jeffparsons]

Also, it's worth adding that you're right that these bits could be stored in the value instead. It's just easier to make the map go from Range<uN> to V instead of uN to (uN, V) where the two halves of the range are split.

This is going back a bit, so my memory is hazy, but I think I tried both in the rangemap crate and settled on "store the range in the key" because it was much easier and enabled hacks like wrapper structs with different Ord implementations. So, yeah, basically what you said.

However, and I know this is a bit of a tangent to this work, I think all those concerns would be obviated by a "visitor" style search method that accepts a callback to decide where to go next ("found", "ascend", "descend right", etc.). Then the consumer of the API could use whatever information they want to decide where to go next. I don't know if that would be too low-level to expose in the public API but I've often found myself wishing I had something like that.

[clarfonthey]

It's also worth mentioning that any "coalescing" behaviour for map keys could also benefit from key_mut since, for example, let's imagine that we use the schema mentioned where A: (B, C) is equivalent to indicating (A..B): C in our map.

If we have B: (C, X) inside our map and want to insert (A..B): X into it, then it makes sense to replace the existing key B with A, rather than to insert a new key A and delete B. In this case, we'd prefer key mutation over the APIs proposed.

Granted, there are multiple cases to be made for reducing unsafe code and not ever doing key mutation, instead preferring to replace keys, but at least for the sake of flexibility and replacing the existing proposed key mutation methods for cursors, this feels like a reasonable compromise. We can litigate whether key mutation APIs should really exist before this is stabilised, since at least for now, they do exist and people are using them.

[ripytide]

However, and I know this is a bit of a tangent to this work, I think all those concerns would be obviated by a "visitor" style search method that accepts a callback to decide where to go next ("found", "ascend", "descend right", etc.). Then the consumer of the API could use whatever information they want to decide where to go next. I don't know if that would be too low-level to expose in the public API but I've often found myself wishing I had something like that.

I completely agree which is why I made https://github.com/ripytide/btree_monstrousity which essentially just removes the Ord bound from all methods on the BTreeMap and instead takes a closure that returns an Ordering to enable arbitrary search behavior just as you said.
It's what DiscreteRangeMap now uses instead of the Ord bodge which made the code much simpler and easier to reason about.

[joshtriplett]

Suggested change

	/// should preserve the ordering of the key with respect to all the others in the map.
	/// must preserve the ordering of the key with respect to all the others in the map.

[joshtriplett]

@clarfonthey Could you please change the name to something like key_mut_unchecked?

That would leave more room for some potential future checked version, which couldn't return &mut but could for instance take a closure that modifies the key, and then after the modification could assert that the key still sorts correctly relative to the previous and next key. (We don't need to add that checked version in the same PR.)

[clarfonthey]

So, I am fine with that change, but after thinking about it a bit more, it feels probably better to do something like BinaryHeap::peek_mut instead, where you return a DerefMut wrapper whose Drop handler will do the verification.

I think we might be able to do something that is both sound and allows an unchecked version, which I think would presumably bypass Drop entirely and consume the wrapper instead. Will ponder that and try to draft up a version.

Since this feels a bit more complicated than the existing method, I'll also draft up an ACP for it.

[clarfonthey]

@rustbot waiting-on-ACP

rust-lang/libs-team#356

[clarfonthey]

@rustbot ACP

[clarfonthey]

Gonna just close this since it doesn't match the current ACP. If the ACP gets accepted I'll open a new PR with the implementation.