Avoid rechecking for duplicates in IdOrdMap::from_iter_unique

crates/iddqd-soteria-runner/tests/runner.rs

@@ -74,7 +74,7 @@ lib_proof!(item_set_remove_then_insert_reuses_freed_slot);
 test_module!(id_hash_map {
     lawful_roundtrip,
     lawless_operation_sequence,
-    overwrite_fail_fast_is_sound,
+    lawless_overwrite_is_sound,
 });
 
 test_module!(id_ord_map {
@@ -86,11 +86,11 @@ test_module!(id_ord_map {
 test_module!(bi_hash_map {
     lawful_roundtrip,
     lawless_operation_sequence,
-    overwrite_fail_fast_is_sound,
+    lawless_overwrite_is_sound,
 });
 
 test_module!(tri_hash_map {
     lawful_roundtrip,
     lawless_operation_sequence,
-    overwrite_fail_fast_is_sound,
+    lawless_overwrite_is_sound,
 });

crates/iddqd/src/id_hash_map/entry.rs

@@ -133,6 +133,23 @@ impl<'a, T: IdHashItem, S: Clone + BuildHasher, A: Allocator>
         map.get_by_index_mut(index).expect("index is known to be valid")
     }
 
+    /// Sets the entry to a new value without checking for duplicates or
+    /// recomputing its key hash.
+    ///
+    /// Only call this on a vacant entry obtained from `value.key()`. Unlike
+    /// `insert`, this method does not validate the hash or recheck uniqueness.
+    #[inline]
+    pub(super) fn insert_known_unique(self, value: T) {
+        // SAFETY: The safety assumption behind `Self::new` guarantees that the
+        // original reference to the map is not used at this point.
+        let map = unsafe { self.map.awaken() };
+        map.tables.key_to_item.reserve(1);
+        let next_index = map.items.assert_can_grow().insert(value);
+        map.tables
+            .key_to_item
+            .insert_prehashed_unchecked(self.hash, next_index);
+    }
+
     /// Sets the value of the entry, and returns an `OccupiedEntry`.
     #[inline]
     pub fn insert_entry(mut self, value: T) -> OccupiedEntry<'a, T, S, A> {

crates/iddqd/src/id_hash_map/imp.rs

@@ -983,13 +983,15 @@ impl<T: IdHashItem, S: Clone + BuildHasher, A: Allocator> IdHashMap<T, S, A> {
         // mutation. A panic in user code therefore leaves the map in its
         // pre-call state.
         //
-        // We use `vacant.insert_entry` rather than `vacant.insert` to avoid
-        // creating a `RefMut`, which would (unnecessarily) re-hash the key
-        // after the mutation when that `RefMut` is created.
+        // In the vacant case, the Entry lookup has already established that the
+        // key is unique. Calling `vacant.insert_entry` would route back through
+        // `insert_unique_impl` and check for duplicates again, while
+        // `vacant.insert` would also create a `RefMut` and re-hash the key. We
+        // use `vacant.insert_known_unique` instead, which avoids both.
         match self.entry(value.key()) {
             Entry::Occupied(mut occupied) => Some(occupied.insert(value)),
             Entry::Vacant(vacant) => {
-                vacant.insert_entry(value);
+                vacant.insert_known_unique(value);
                 None
             }
         }

crates/iddqd/src/id_ord_map/imp.rs

@@ -112,7 +112,7 @@ impl<T: IdOrdItem> IdOrdMap<T> {
 
     /// Creates a new `IdOrdMap` with the given capacity.
     ///
-    /// The capacity will be used to initialize the underlying hash table.
+    /// The capacity will be used to initialize the underlying item set.
     ///
     /// # Examples
     ///
@@ -223,7 +223,8 @@ impl<T: IdOrdItem> IdOrdMap<T> {
     pub fn from_iter_unique<I: IntoIterator<Item = T>>(
         iter: I,
     ) -> Result<Self, DuplicateItem<T>> {
-        let mut map = IdOrdMap::new();
+        let iter = iter.into_iter();
+        let mut map = IdOrdMap::with_capacity(iter.size_hint().0);
         for value in iter {
             // It would be nice to use insert_unique here, but that would return
             // a `DuplicateItem<T, &T>`, which can only be converted into an
@@ -237,8 +238,8 @@ impl<T: IdOrdItem> IdOrdMap<T> {
                         vec![duplicate],
                     ));
                 }
-                Entry::Vacant(entry) => {
-                    entry.insert_ref(value);
+                Entry::Vacant(_) => {
+                    map.insert_known_unique_impl(value);
                 }
             }
         }
@@ -756,13 +757,15 @@ impl<T: IdOrdItem> IdOrdMap<T> {
         // mutation. A panic in user code therefore leaves the map in its
         // pre-call state.
         //
-        // We use `vacant.insert_entry` rather than `vacant.insert` to avoid
-        // creating a `RefMut`, which would (unnecessarily) re-hash the key
-        // after the mutation when that `RefMut` is created.
+        // In the vacant case, the Entry lookup has already established that the
+        // key is unique. Calling `vacant.insert_entry` would route back through
+        // `insert_unique_impl` and check for duplicates again, while
+        // `vacant.insert` would also create a `RefMut` and hash the key. We use
+        // `insert_known_unique_impl` instead, which avoids both.
         match self.entry(value.key()) {
             Entry::Occupied(mut occupied) => Some(occupied.insert(value)),
-            Entry::Vacant(vacant) => {
-                vacant.insert_entry(value);
+            Entry::Vacant(_) => {
+                self.insert_known_unique_impl(value);
                 None
             }
         }
@@ -1491,10 +1494,20 @@ impl<T: IdOrdItem> IdOrdMap<T> {
             }
         }
 
-        // Take the `GrowHandle` after the read-only duplicate check but before
-        // the B-tree mutation. With this approach, a panic from
-        // `assert_can_grow` (which means that the map is full) cannot leave the
-        // B-tree referencing an index that was never assigned to an item.
+        Ok(self.insert_known_unique_impl(value))
+    }
+
+    /// Inserts `value` without checking for duplicates.
+    ///
+    /// Only call this after verifying that `value` does not conflict with any
+    /// existing item. Callers that haven't determined uniqueness should use
+    /// `insert_unique_impl` instead.
+    fn insert_known_unique_impl(&mut self, value: T) -> ItemIndex {
+        // Take the `GrowHandle` now, after the caller has checked that `value`
+        // does not conflict with any existing item, but before the B-tree
+        // mutation. With this approach, a panic from `assert_can_grow` (which
+        // means that the map is full) cannot leave the B-tree referencing an
+        // index that was never assigned to an item.
         //
         // The handle holds `&mut self.items` and is consumed by
         // `GrowHandle::insert`, so the type system enforces that we cannot
@@ -1528,7 +1541,7 @@ impl<T: IdOrdItem> IdOrdMap<T> {
         grow_handle.insert(value);
         insert.insert();
 
-        Ok(next_index)
+        next_index
     }
 
     pub(super) fn remove_by_index(

crates/iddqd/tests/integration/pathological.rs

@@ -1015,19 +1015,6 @@ fn tri_hash_silent_tertiary_key_change_insert_overwrite() {
         .expect("map remains valid after silent-mutation insert_overwrite");
 }
 
-fn assert_panic_message(f: impl FnOnce(), expected: &str) {
-    let payload = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f))
-        .expect_err("the armed flip should trigger a fail-fast panic");
-    let message: &str = if let Some(s) = payload.downcast_ref::<&str>() {
-        s
-    } else if let Some(s) = payload.downcast_ref::<String>() {
-        s.as_str()
-    } else {
-        panic!("fail-fast panics carry a string payload");
-    };
-    assert_eq!(message, expected);
-}
-
 #[derive(Debug)]
 struct FlipItem {
     id: u32,
@@ -1073,7 +1060,7 @@ impl IdOrdItem for FlipItem {
 }
 
 #[test]
-fn id_hash_flip_key_insert_overwrite_panics() {
+fn id_hash_flip_key_insert_overwrite_inserts_under_stale_hash() {
     let mut map: IdHashMap<FlipItem, _> = IdHashMap::with_capacity_and_hasher(
         8,
         foldhash::fast::FixedState::with_seed(0),
@@ -1082,50 +1069,65 @@ fn id_hash_flip_key_insert_overwrite_panics() {
         map.insert_unique(FlipItem::plain(id)).unwrap();
     }
 
-    assert_panic_message(
-        || {
-            map.insert_overwrite(FlipItem::flips_after_first_key_call(99, 42));
-        },
-        "key hashes do not match",
-    );
+    let displaced =
+        map.insert_overwrite(FlipItem::flips_after_first_key_call(99, 42));
+    assert!(displaced.is_none(), "the flipped key displaced nothing");
 
-    assert_eq!(map.len(), 8);
+    assert_eq!(map.len(), 9);
     assert!(map.get(&99u32).is_none());
     assert!(map.get(&42u32).is_none());
-    map.validate(ValidateCompact::NonCompact)
-        .expect("map remains valid after a flip-key insert_overwrite panic");
+    assert!(
+        map.validate(ValidateCompact::NonCompact).is_err(),
+        "the stale-hash item is unfindable by its current key"
+    );
+    map.validate_structural(ValidateCompact::NonCompact).expect(
+        "map remains structurally sound after a flip-key insert_overwrite",
+    );
 }
 
 #[test]
-fn id_ord_flip_key_insert_overwrite_panics() {
+fn id_ord_flip_key_insert_overwrite_inserts_logical_duplicate() {
     let mut map = IdOrdMap::<FlipItem>::new();
     for id in 0..8u32 {
         map.insert_unique(FlipItem::plain(id)).unwrap();
     }
 
-    assert_panic_message(
-        || {
-            map.insert_overwrite(FlipItem::flips_after_first_key_call(99, 3));
-        },
-        "key already present in map",
-    );
+    let displaced =
+        map.insert_overwrite(FlipItem::flips_after_first_key_call(99, 3));
+    assert!(displaced.is_none(), "the flipped key displaced nothing");
 
-    assert_eq!(map.len(), 8);
+    assert_eq!(map.len(), 9);
     assert!(map.get(&99u32).is_none());
-    assert_eq!(map.get(&3u32).expect("original id 3 remains").id, 3);
-    map.validate(ValidateCompact::NonCompact, ValidateChaos::No)
-        .expect("map remains valid after a flip-key insert_overwrite panic");
+    assert_eq!(map.get(&3u32).expect("key 3 still resolves").id, 3);
+    assert!(
+        map.validate(ValidateCompact::NonCompact, ValidateChaos::No).is_err(),
+        "one of the two key-3 items is unfindable"
+    );
+    map.validate_structural(ValidateCompact::NonCompact).expect(
+        "map remains structurally sound after a flip-key insert_overwrite",
+    );
 }
 
 #[test]
-#[should_panic = "key already present in map"]
-fn id_ord_flip_key_from_iter_unique_existing_key_panics() {
-    let _ = IdOrdMap::<FlipItem>::from_iter_unique([
+fn id_ord_flip_key_from_iter_unique_inserts_logical_duplicate() {
+    let map = IdOrdMap::<FlipItem>::from_iter_unique([
         FlipItem::plain(0),
         FlipItem::plain(1),
         FlipItem::plain(2),
         FlipItem::flips_after_first_key_call(99, 1),
-    ]);
+    ])
+    .expect("the flip evades the duplicate check");
+
+    assert_eq!(map.len(), 4);
+    assert!(map.get(&99u32).is_none());
+    assert_eq!(map.get(&1u32).expect("key 1 still resolves").id, 1);
+    assert!(
+        map.validate(ValidateCompact::NonCompact, ValidateChaos::No).is_err(),
+        "one of the two key-1 items is unfindable"
+    );
+    map.validate_structural(ValidateCompact::NonCompact).expect(
+        "map remains structurally sound after a flip-key from_iter_unique",
+    );
 }
 
 #[test]

crates/iddqd/tests/soteria/bi_hash_map.rs

@@ -67,10 +67,8 @@ fn lawful_roundtrip() {
 /// We only exercise `remove1` -- `remove2` is symmetric, and we want to
 /// keep the proofs fast enough to run in CI.
 ///
-/// We don't cover `insert_overwrite` here since that will panic under
-/// adversarial input. We could catch the panic here but that slows down
-/// proof execution tremendously. So instead, we have a separate proof
-/// for `insert_overwrite` below.
+/// We don't cover `insert_overwrite` here to avoid bloating up the runtime too
+/// much. We have a separate proof for `insert_overwrite` below.
 ///
 /// We only call `validate_structural`, not full `validate`, since under
 /// an adversarial hash we can end up not finding items by their key. Only
@@ -112,19 +110,17 @@ fn lawless_operation_sequence() {
 }
 
 #[test]
-fn overwrite_fail_fast_is_sound() {
+fn lawless_overwrite_is_sound() {
     let mut map: BiHashMap<BiItem, LawlessHasher> =
         BiHashMap::with_hasher(LawlessHasher);
     let k1 = nondet_u8_below(SEQ_KEYS);
     let k2 = nondet_u8_below(SEQ_KEYS);
 
     let _ = map.insert_unique(BiItem { key1: k1, key2: k2, value: 0 });
-    let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
-        let _ = map.insert_overwrite(BiItem { key1: k1, key2: k2, value: 1 });
-    }));
+    let _ = map.insert_overwrite(BiItem { key1: k1, key2: k2, value: 1 });
 
     map.validate_structural(ValidateCompact::NonCompact).expect(
-        "sound whether insert_overwrite completed or fail-fast panicked",
+        "sound whether the overwrite displaced the old item or duplicated it",
     );
     std::mem::forget(map);
 }

crates/iddqd/tests/soteria/id_hash_map.rs

@@ -59,10 +59,8 @@ fn lawful_roundtrip() {
 ///
 /// # Notes
 ///
-/// We don't cover `insert_overwrite` here since that will panic under
-/// adversarial input. We could catch the panic here but that slows down
-/// proof execution tremendously. So instead, we have a separate proof
-/// for `insert_overwrite` below.
+/// We don't cover `insert_overwrite` here to avoid bloating up the runtime too
+/// much. We have a separate proof for `insert_overwrite` below.
 ///
 /// We only call `validate_structural`, not full `validate`, since under
 /// an adversarial hash we can end up not finding items by their key. Only
@@ -100,26 +98,24 @@ fn lawless_operation_sequence() {
     std::mem::forget(map);
 }
 
-/// `insert_overwrite` is structurally sound under a lawless hash, whether it
-/// completes or trips iddqd's own `is_same_hash` fail-fast guard (which the
-/// lawless hash induces by disagreeing on the recomputed key hash).
+/// Prove that `insert_overwrite` is structurally sound under a lawless hash,
+/// whichever arm the lawless lookup steers it into:
 ///
-/// Note that this isn't proving panic safety in general, only that an
-/// `insert_overwrite` panic leaves the map in a valid state. For panic safety,
-/// see the corresponding model-based tests.
+/// * If the existing item is found, an in-place replace.
+/// * If the item is not found, an unchecked insert that reuses the vacant entry's
+///   stored hash.
+///
+/// This proof shows that in either case, the map is still structurally valid.
 #[test]
-fn overwrite_fail_fast_is_sound() {
+fn lawless_overwrite_is_sound() {
     let mut map: IdHashMap<Item, LawlessHasher> =
         IdHashMap::with_hasher(LawlessHasher);
     let k = nondet_u8_below(2);
 
     let _ = map.insert_unique(Item { key: k, value: 0 });
-    let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
-        let _ = map.insert_overwrite(Item { key: k, value: 1 });
-    }));
+    let _ = map.insert_overwrite(Item { key: k, value: 1 });
 
-    map.validate_structural(ValidateCompact::NonCompact).expect(
-        "sound whether insert_overwrite completed or fail-fast panicked",
-    );
+    map.validate_structural(ValidateCompact::NonCompact)
+        .expect("sound whether the overwrite replaced or duplicated");
     std::mem::forget(map);
 }