From 9bd6318cb55ab628a6d5bfee93a8e7736bcc00c1 Mon Sep 17 00:00:00 2001
From: kirillDevPro <113171057+kirillDevPro@users.noreply.github.com>
Date: Wed, 4 Nov 2026 06:05:59 +0100
Subject: [PATCH] fix(collections): keep VecMap entry_ref pairs aligned when a
 key clone panics

Input: a VecMap whose key type has a Clone that panics; a vacant
entry_ref(&k).or_insert(v) is caught with catch_unwind, then a new key is
inserted through entry().

Wrong output: EntryRef::or_insert_with_key pushed the value before cloning
the key, so the panicking clone left one value with no key. Every key
inserted afterwards paired with its predecessor's value: iter() yielded
[(1, "one"), (2, "lost")].

Expected output: [(1, "one"), (2, "two")]. The type documents itself as a
drop-in replacement for std::collections::HashMap, whose entry API leaves
the map unchanged when caller code panics mid-insert. The fix builds the
value and the cloned key before pushing either; successful inserts are
unchanged (default still runs first, the key is still cloned once).

Test: vecmap_tests::test_entry_ref_panicking_key_clone_leaves_map_unchanged
fails on origin/master with the output above and passes now.
Re-run: cargo test -p moon-collections panicking

Reviewed by Codex (verdict: agree).
---
 crates/moon-collections/src/vecmap.rs       |  9 +++++--
 crates/moon-collections/src/vecmap_tests.rs | 29 +++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/crates/moon-collections/src/vecmap.rs b/crates/moon-collections/src/vecmap.rs
index bec6596..885fbff 100644
--- a/crates/moon-collections/src/vecmap.rs
+++ b/crates/moon-collections/src/vecmap.rs
@@ -157,8 +157,13 @@ where
         match self {
             EntryRef::Occupied(entry) => entry.value,
             EntryRef::Vacant(entry) => {
-                entry.map.values.push(default(entry.key));
-                entry.map.keys.push(entry.key.clone());
+                // Build both halves before pushing either: a panic in `default` or in the key's
+                // `Clone` must leave `keys` and `values` the same length, or every later key
+                // pairs with its predecessor's value.
+                let value = default(entry.key);
+                let key = entry.key.clone();
+                entry.map.values.push(value);
+                entry.map.keys.push(key);
                 match entry.map.values.last_mut() {
                     Some(value) => value,
                     None => unreachable!("vec empty after pushing to it"),
diff --git a/crates/moon-collections/src/vecmap_tests.rs b/crates/moon-collections/src/vecmap_tests.rs
index 1f698f8..c4f5e13 100644
--- a/crates/moon-collections/src/vecmap_tests.rs
+++ b/crates/moon-collections/src/vecmap_tests.rs
@@ -209,3 +209,32 @@ fn test_entry_ref_mut_ref_can_be_updated() {
     *value = 5;
     assert_eq!(map.iter().collect::<Vec<_>>(), vec![(&key, &5)]);
 }
+
+/// A key whose `Clone` panics, standing in for any fallible clone (an allocation, a refcount).
+#[derive(PartialEq, Eq, Debug)]
+struct PanickingCloneKey(u32);
+
+impl Clone for PanickingCloneKey {
+    fn clone(&self) -> Self {
+        panic!("clone of key {} failed", self.0);
+    }
+}
+
+/// Catches `EntryRef::or_insert_with_key` pushing the value before cloning the key: a clone that
+/// panics then leaves one value with no key, so every key inserted afterwards reads the value
+/// stored beside the one before it. Like `HashMap`, a panic in caller code must leave the map as
+/// it was: after the failed insert, key 2 maps to "two", exactly as if the insert never ran.
+#[test]
+fn test_entry_ref_panicking_key_clone_leaves_map_unchanged() {
+    let mut map: VecMap<PanickingCloneKey, &str> = VecMap::new();
+    map.entry(PanickingCloneKey(1)).or_insert("one");
+
+    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        map.entry_ref(&PanickingCloneKey(99)).or_insert("lost");
+    }));
+    assert!(result.is_err());
+
+    map.entry(PanickingCloneKey(2)).or_insert("two");
+    let pairs: Vec<_> = map.iter().map(|(key, value)| (key.0, *value)).collect();
+    assert_eq!(pairs, vec![(1, "one"), (2, "two")]);
+}