implement BTreeMap::merge by asder8215 · Pull Request #151981 · rust-lang/rust

asder8215 · 2026-02-02T04:37:18Z

This PR implements BTreeMap::append_with() as mentioned in #147700 (and discussed by the forum post linked in the issue). I'm not sure if the issue made is an approval for this feature, but regardless I'm going to make an ACP soon. I decided to made a small change to the API for append_with than the one proposed by @nagisa:

pub fn append_with(&mut self, other: &mut BTreeMap<K, V, A>, conflict: impl FnMut(&K, V, V) -> V);

I think having the conflict function own self's value and returning an owned value might give a lot more flexibility to the user. It also feels a bit more intuitive to me as the user that a value of type V is being put into self when there's conflicting keys between self and other.

For example, if my values are String types and I want to combine my values when keys are conflicting, then they can either mutate selfs value with .push_str() passing in other's value and return self's value, or they can use format!() to return a concatenation of self's and other's value.

Now, a consequence for this approach is that it's very well possible that the user decides to return a value V that has nothing to do with self or other when it comes to a conflicting key. However, should we decide to do conflict: impl FnMut(&K, &mut V, V), I could have mutated self's value easily to produce a whole different value that has nothing to do with the original value of self or other.

rustbot · 2026-02-02T04:37:22Z

r? @jhpratt

rustbot has assigned @jhpratt.
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

… block in append_with()

jhpratt · 2026-02-03T01:37:18Z

Marked as waiting on ACP.

…rified doc comments

asder8215 · 2026-02-05T06:04:28Z

@rustbot label -S-waiting-on-review

Implemented the ACP changes with renaming function, changing signature, and changed the doc comments for merge a bit (it's similar to BTreeMap::append, but adds clarity to the callback function). Let me know if there's anything that I should modify or clarify in code/documentation!

asder8215 · 2026-02-05T06:05:46Z

Oops, I think it's:
@rustbot ready

asder8215 · 2026-02-05T06:09:10Z

I don't think the CI issue is coming from my end since it's saying spurious network error?

jhpratt · 2026-02-07T05:23:09Z

Stepping off rotation for a bit.

@rustbot reroll

asder8215 · 2026-02-09T06:47:01Z

@rustbot ready

asder8215 · 2026-02-10T02:27:39Z

Note: I have a different implementation of BTreeMap::merge that uses CursorMut on self BTreeMap and IntoIter of other BTreeMap, which should result in it being more efficient than this current implementation because we don't need to take an IntoIter of self BTreeMap and reconstruct the merged and sorted BTreeMap from bottom up. It doesn't change the signature of the function, but it uses a lot of unsafe code as a result using CursorMut's methods and needing ownership of the value part from self CursorMut for the callback conflict function (the newer implementation also needs a lot more polish as well).

I think I'll put the optimized version in a separate PR because I know that this PR's code is safe (or safer) due to the implementation being a mirror of BTreeMap::append just with a callback conflict function. Also, if the unsafe optimized code is good for the implementation of BTreeMap::merge, then I might also make a PR on optimizing BTreeMap::append with CursorMut and IntoIter.

Let me know if this sounds good to you.

Mark-Simulacrum · 2026-02-14T19:35:20Z

library/alloc/src/collections/btree/map/tests.rs

+            let mut b = BTreeMap::new();
+            for i in 5..$len {
+                b.insert(i, 2 * i);
+            }


Can we add some comments on why certain constants were chosen here or what the intent of this code is? E.g., 5..$len is not immediately apparent when reading this to me.

In the new PR, I mentioned a comment above macro_rules! create_merge_test {...} saying:

// On merging keys 5..8, the values are // added together (i + 2 * i)

Mark-Simulacrum · 2026-02-14T19:41:15Z

library/alloc/src/collections/btree/map/tests.rs

+    lhs.insert(IdBased { id: 0, name: "lhs_k".to_string() }, "1".to_string());
+    rhs.insert(IdBased { id: 0, name: "rhs_k".to_string() }, "2".to_string());
+
+    lhs.merge(rhs, |_, mut lhs_val, rhs_val| {


Can we add a test / extend this one to verify that lhs_val and rhs_val are from left/right respectively?

I hope I'm understanding correctly, but do you want me to add assertion cases in this merge callback function that verifies that lhs_val == "1", and rhs_val == "2"?

I could do that.

Mark-Simulacrum · 2026-02-14T19:41:57Z

library/alloc/src/collections/btree/append.rs

@@ -33,6 +33,36 @@ impl<K, V> Root<K, V> {
        self.bulk_push(iter, length, alloc)
    }

+    /// Merges all key-value pairs from the union of two ascending iterators,
+    /// incrementing a `length` variable along the way. The latter makes it
+    /// easier for the caller to avoid a leak when a drop handler panicks.


Suggested change

/// easier for the caller to avoid a leak when a drop handler panicks.

/// easier for the caller to avoid a leak when a drop handler panics.

In the new PR, I use CursorMut to help me merge the keys from other BTreeMap into self BTreeMap, so I don't have to consume the self BTreeMap reconstruct the merged BTreeMap from scratch.

Mark-Simulacrum · 2026-02-14T19:43:54Z

library/alloc/src/collections/btree/append.rs

+    /// If you want the tree to end up in a strictly ascending order, like for
+    /// a `BTreeMap`, both iterators should produce keys in strictly ascending
+    /// order, each greater than all keys in the tree, including any keys
+    /// already in the tree upon entry.


When we say "including any keys already in the tree", what keys are not in that set but relevant here?

I copied the comment from the merge_from_sorted_iters function above, only making changes to the second paragraph I believe. However, looking at this comment, it's kinda already implied that a valid BTreeMap will be in strictly ascending/descending order if both BTreeMap follow the rule that their keys are in ascending/descending order. Not sure what the comment was getting at it here "including any keys already in the tree upon entry"

Mark-Simulacrum · 2026-02-14T19:47:30Z

library/alloc/src/collections/btree/map.rs

+
+        let self_iter = mem::replace(self, Self::new_in((*self.alloc).clone())).into_iter();
+        let other_iter = mem::replace(&mut other, Self::new_in((*self.alloc).clone())).into_iter();
+        let root = self.root.get_or_insert_with(|| Root::new((*self.alloc).clone()));


Why is this get_or_insert_with? Isn't the root guaranteed to be absent after the replace with a new tree above?

To be honest with you, I copied the code verbatim from BTreeMap::append(), which does this as well. The root does look to be absent though since we mem::replace() self with Self::new_in((*self.alloc).clone()).

The new PR doesn't use this approach though and preserves the content of self BTreeMap and inserting the other BTreeMap items in the right spots using CursorMut

rustbot · 2026-02-14T19:49:18Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

asder8215 · 2026-02-14T22:54:20Z

@Mark-Simulacrum this PR will be closed in favor of #152418. I appreciate your time looking into this PR though!

feat: implement append_with() for BTreeMap

6a2f234

rustbot assigned jhpratt Feb 2, 2026

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 Feb 2, 2026