From 213f780b727ad5ebbecee9e464c8cc0268a491ab Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 12:08:03 +0200 Subject: [PATCH 01/10] Prepare TriHashMap hash helpers for entry API --- crates/iddqd/src/tri_hash_map/imp.rs | 14 +++++++------- crates/iddqd/src/tri_hash_map/iter.rs | 2 +- crates/iddqd/src/tri_hash_map/tables.rs | 9 ++++++--- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index 3454a562..75b9f220 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -136,7 +136,7 @@ pub struct TriHashMap { pub(super) items: ItemSet, // Invariant: the values (ItemIndex) in these tables are valid indexes into // `items`, and are a 1:1 mapping. - tables: TriHashMapTables, + pub(super) tables: TriHashMapTables, } impl Default @@ -1760,7 +1760,7 @@ impl TriHashMap { let awakened_map = unsafe { dormant_map.awaken() }; let item = &mut awakened_map.items[index]; let state = awakened_map.tables.state.clone(); - let hashes = awakened_map.tables.make_hashes(&item); + let hashes = awakened_map.tables.make_hashes_for_item(&item); Some(RefMut::new(state, hashes, item)) } @@ -2016,7 +2016,7 @@ impl TriHashMap { let awakened_map = unsafe { dormant_map.awaken() }; let item = &mut awakened_map.items[index]; let state = awakened_map.tables.state.clone(); - let hashes = awakened_map.tables.make_hashes(&item); + let hashes = awakened_map.tables.make_hashes_for_item(&item); Some(RefMut::new(state, hashes, item)) } @@ -2253,7 +2253,7 @@ impl TriHashMap { let awakened_map = unsafe { dormant_map.awaken() }; let item = &mut awakened_map.items[index]; let state = awakened_map.tables.state.clone(); - let hashes = awakened_map.tables.make_hashes(&item); + let hashes = awakened_map.tables.make_hashes_for_item(&item); Some(RefMut::new(state, hashes, item)) } @@ -2490,7 +2490,7 @@ impl TriHashMap { let awakened_map = unsafe { dormant_map.awaken() }; let item = &mut awakened_map.items[index]; let state = awakened_map.tables.state.clone(); - let hashes = awakened_map.tables.make_hashes(&item); + let hashes = awakened_map.tables.make_hashes_for_item(&item); Some(RefMut::new(state, hashes, item)) } @@ -2801,7 +2801,7 @@ impl TriHashMap { let index1 = self.find1_index(&key1); let index2 = self.find2_index(&key2); let index3 = self.find3_index(&key3); - let hashes = self.tables.make_hashes_for_keys::(&key1, &key2, &key3); + let hashes = self.tables.make_hashes::(&key1, &key2, &key3); let duplicates = PreparedDuplicate::from_indexes( [index1, index2, index3], @@ -2813,7 +2813,7 @@ impl TriHashMap { fn prepare_duplicate(&self, index: ItemIndex) -> PreparedDuplicate { let item = &self.items[index]; - let hashes = self.tables.make_hashes::(item); + let hashes = self.tables.make_hashes_for_item::(item); PreparedDuplicate { index, hashes } } diff --git a/crates/iddqd/src/tri_hash_map/iter.rs b/crates/iddqd/src/tri_hash_map/iter.rs index da2cbe4e..9460dc68 100644 --- a/crates/iddqd/src/tri_hash_map/iter.rs +++ b/crates/iddqd/src/tri_hash_map/iter.rs @@ -87,7 +87,7 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> Iterator #[inline] fn next(&mut self) -> Option { let next = self.inner.next()?; - let hashes = self.tables.make_hashes(next); + let hashes = self.tables.make_hashes_for_item(next); Some(RefMut::new(self.tables.state.clone(), hashes, next)) } } diff --git a/crates/iddqd/src/tri_hash_map/tables.rs b/crates/iddqd/src/tri_hash_map/tables.rs index 8081dd2e..af51da5f 100644 --- a/crates/iddqd/src/tri_hash_map/tables.rs +++ b/crates/iddqd/src/tri_hash_map/tables.rs @@ -68,15 +68,18 @@ impl TriHashMapTables { Ok(()) } - pub(super) fn make_hashes(&self, item: &T) -> [MapHash; 3] { + pub(super) fn make_hashes_for_item( + &self, + item: &T, + ) -> [MapHash; 3] { let k1 = item.key1(); let k2 = item.key2(); let k3 = item.key3(); - self.make_hashes_for_keys::(&k1, &k2, &k3) + self.make_hashes::(&k1, &k2, &k3) } - pub(super) fn make_hashes_for_keys( + pub(super) fn make_hashes( &self, key1: &T::K1<'_>, key2: &T::K2<'_>, From 60714200190e5a003fb20ed1146d4b40f3dc1982 Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 13:00:34 +0200 Subject: [PATCH 02/10] Add internal entry lookup support primitive --- crates/iddqd/src/support/entry.rs | 297 ++++++++++++++++++++++++++++++ crates/iddqd/src/support/mod.rs | 1 + 2 files changed, 298 insertions(+) create mode 100644 crates/iddqd/src/support/entry.rs diff --git a/crates/iddqd/src/support/entry.rs b/crates/iddqd/src/support/entry.rs new file mode 100644 index 00000000..069bab40 --- /dev/null +++ b/crates/iddqd/src/support/entry.rs @@ -0,0 +1,297 @@ +#![expect( + dead_code, + reason = "temporary until the next PR wires the support primitive into BiHashMap" +)] + +//! Crate-internal support for classifying multi-key entry lookups. +//! +//! This module is intentionally independent of the map implementations. It only +//! understands fixed arrays of optional item indexes, preserving enough state for +//! entry APIs to reason about vacant, unique, and non-unique lookup results. + +use crate::support::ItemIndex; + +/// Classification of a multi-key entry lookup. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum EntryLookup { + /// No key matched an existing item. + Vacant, + /// Every key matched the same existing item. + Unique(ItemIndex), + /// At least one key matched, but the lookup was not unique. + NonUnique(NonUniqueIndexes), +} + +/// Per-key lookup indexes for an entry operation. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct EntryIndexes { + indexes: [Option; N], +} + +/// Non-unique per-key lookup indexes. +/// +/// Invariant: at least one index is `Some`, and the indexes are not all the +/// same `Some` value. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct NonUniqueIndexes { + indexes: [Option; N], +} + +/// Distinct indexes referenced by a non-vacant lookup. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct DistinctIndexes { + indexes: [Option; N], + len: usize, + key_to_slot: [Option; N], +} + +impl EntryIndexes { + #[inline] + pub(crate) const fn new(indexes: [Option; N]) -> Self { + Self { indexes } + } + + #[inline] + pub(crate) const fn indexes(&self) -> &[Option; N] { + &self.indexes + } + + #[inline] + pub(crate) fn classify(self) -> EntryLookup { + let mut first = None; + let mut saw_none = false; + let mut all_some_same = true; + + for index in self.indexes { + match (first, index) { + (None, Some(index)) => first = Some(index), + (Some(first_index), Some(index)) if first_index != index => { + all_some_same = false; + } + (_, None) => saw_none = true, + _ => {} + } + } + + match (first, saw_none, all_some_same) { + (None, _, _) => EntryLookup::Vacant, + (Some(index), false, true) => EntryLookup::Unique(index), + (Some(_), _, _) => EntryLookup::NonUnique(NonUniqueIndexes { + indexes: self.indexes, + }), + } + } +} + +impl NonUniqueIndexes { + #[inline] + pub(crate) const fn indexes(&self) -> &[Option; N] { + &self.indexes + } + + #[inline] + pub(crate) fn distinct(self) -> DistinctIndexes { + DistinctIndexes::from_indexes(self.indexes) + } +} + +impl DistinctIndexes { + fn from_indexes(source: [Option; N]) -> Self { + let mut indexes = [None; N]; + let mut key_to_slot = [None; N]; + let mut len = 0; + + for (key, source_index) in source.into_iter().enumerate() { + if let Some(source_index) = source_index { + let mut slot = None; + + // Distinct indexes are stored densely in first-key-hit order. + // Only the initialized prefix `..len` is inspected here. + for (candidate_slot, candidate) in + indexes[..len].iter().enumerate() + { + if *candidate == Some(source_index) { + slot = Some(candidate_slot); + break; + } + } + + let slot = match slot { + Some(slot) => slot, + None => { + let slot = len; + indexes[slot] = Some(source_index); + len += 1; + slot + } + }; + key_to_slot[key] = Some(slot); + } + } + + Self { indexes, len, key_to_slot } + } + + #[inline] + pub(crate) const fn indexes(&self) -> &[Option; N] { + &self.indexes + } + + #[inline] + pub(crate) const fn len(&self) -> usize { + self.len + } + + #[inline] + pub(crate) const fn key_to_slot(&self) -> &[Option; N] { + &self.key_to_slot + } +} + +#[cfg(test)] +mod tests { + use super::{EntryIndexes, EntryLookup}; + use crate::support::ItemIndex; + + fn ix(value: u32) -> ItemIndex { + ItemIndex::new(value) + } + + fn classify( + indexes: [Option; N], + ) -> EntryLookup { + EntryIndexes::new(indexes).classify() + } + + fn non_unique_distinct( + indexes: [Option; N], + ) -> (usize, [Option; N], [Option; N]) { + let EntryLookup::NonUnique(indexes) = classify(indexes) else { + panic!("expected non-unique indexes") + }; + let distinct = indexes.distinct(); + (distinct.len(), *distinct.indexes(), *distinct.key_to_slot()) + } + + #[test] + fn arity_2_vacant_classification() { + assert_eq!(classify([None, None]), EntryLookup::Vacant); + } + + #[test] + fn arity_2_unique_classification() { + assert_eq!( + classify([Some(ix(1)), Some(ix(1))]), + EntryLookup::Unique(ix(1)) + ); + } + + #[test] + fn arity_2_partial_classification() { + assert!(matches!( + classify([Some(ix(1)), None]), + EntryLookup::NonUnique(_) + )); + assert!(matches!( + classify([None, Some(ix(1))]), + EntryLookup::NonUnique(_) + )); + } + + #[test] + fn arity_2_mixed_classification() { + assert!(matches!( + classify([Some(ix(1)), Some(ix(2))]), + EntryLookup::NonUnique(_) + )); + } + + #[test] + fn arity_3_vacant_classification() { + assert_eq!(classify([None, None, None]), EntryLookup::Vacant); + } + + #[test] + fn arity_3_unique_classification() { + assert_eq!( + classify([Some(ix(1)), Some(ix(1)), Some(ix(1))]), + EntryLookup::Unique(ix(1)) + ); + } + + #[test] + fn arity_3_partial_duplicate_classification() { + assert!(matches!( + classify([Some(ix(1)), Some(ix(1)), None]), + EntryLookup::NonUnique(_) + )); + assert!(matches!( + classify([None, Some(ix(1)), Some(ix(1))]), + EntryLookup::NonUnique(_) + )); + } + + #[test] + fn arity_3_separated_duplicate_classification() { + assert!(matches!( + classify([Some(ix(1)), None, Some(ix(1))]), + EntryLookup::NonUnique(_) + )); + } + + #[test] + fn arity_3_mixed_duplicate_classification() { + assert!(matches!( + classify([Some(ix(1)), Some(ix(1)), Some(ix(2))]), + EntryLookup::NonUnique(_) + )); + assert!(matches!( + classify([Some(ix(1)), Some(ix(2)), Some(ix(1))]), + EntryLookup::NonUnique(_) + )); + } + + #[test] + fn arity_3_all_distinct_classification() { + assert!(matches!( + classify([Some(ix(1)), Some(ix(2)), Some(ix(3))]), + EntryLookup::NonUnique(_) + )); + } + + #[test] + fn deterministic_first_key_hit_distinct_ordering() { + assert_eq!( + non_unique_distinct([Some(ix(1)), Some(ix(1)), Some(ix(2))]), + (2, [Some(ix(1)), Some(ix(2)), None], [Some(0), Some(0), Some(1)]) + ); + assert_eq!( + non_unique_distinct([Some(ix(1)), Some(ix(2)), Some(ix(1))]), + (2, [Some(ix(1)), Some(ix(2)), None], [Some(0), Some(1), Some(0)]) + ); + assert_eq!( + non_unique_distinct([None, Some(ix(2)), Some(ix(1))]), + (2, [Some(ix(2)), Some(ix(1)), None], [None, Some(0), Some(1)]) + ); + } + + #[test] + fn key_to_slot_mapping_for_repeated_indexes() { + assert_eq!( + non_unique_distinct([Some(ix(1)), None, Some(ix(1))]), + (1, [Some(ix(1)), None, None], [Some(0), None, Some(0)]) + ); + } + + #[test] + fn no_duplicate_distinct_indexes() { + assert_eq!( + non_unique_distinct([Some(ix(1)), Some(ix(2)), Some(ix(3))]), + ( + 3, + [Some(ix(1)), Some(ix(2)), Some(ix(3))], + [Some(0), Some(1), Some(2)] + ) + ); + } +} diff --git a/crates/iddqd/src/support/mod.rs b/crates/iddqd/src/support/mod.rs index 1da16df5..a7829402 100644 --- a/crates/iddqd/src/support/mod.rs +++ b/crates/iddqd/src/support/mod.rs @@ -4,6 +4,7 @@ pub(crate) mod borrow; pub(crate) mod btree_table; #[cfg(feature = "daft")] pub(crate) mod daft_utils; +pub(crate) mod entry; pub(crate) mod fmt_utils; pub(crate) mod hash_builder; pub(crate) mod hash_table; From 88e150ef2caac1045a84e9ca50936cd660311da0 Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 14:16:41 +0200 Subject: [PATCH 03/10] Migrate BiHashMap entry classification to support primitive --- crates/iddqd/src/bi_hash_map/entry_indexes.rs | 25 +++++++++++-- crates/iddqd/src/bi_hash_map/imp.rs | 13 ++++--- crates/iddqd/src/support/entry.rs | 37 ++++++++++++++++--- 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/crates/iddqd/src/bi_hash_map/entry_indexes.rs b/crates/iddqd/src/bi_hash_map/entry_indexes.rs index 2ac61aca..e015b50f 100644 --- a/crates/iddqd/src/bi_hash_map/entry_indexes.rs +++ b/crates/iddqd/src/bi_hash_map/entry_indexes.rs @@ -1,17 +1,36 @@ -use crate::support::ItemIndex; +use crate::support::{ + ItemIndex, + entry::{ + EntryIndexes as SupportEntryIndexes, EntryLookup, NonUniqueIndexes, + }, +}; #[derive(Clone, Copy, Debug)] pub(super) enum EntryIndexes { Unique(ItemIndex), NonUnique { - // Invariant: at least one index is Some, and indexes are different from - // each other. + // Invariant: at least one index is Some, and indexes are not all the + // same Some value. index1: Option, index2: Option, }, } impl EntryIndexes { + #[inline] + pub(super) fn classify( + index1: Option, + index2: Option, + ) -> EntryLookup<2> { + SupportEntryIndexes::new([index1, index2]).classify() + } + + #[inline] + pub(super) fn from_non_unique(indexes: NonUniqueIndexes<2>) -> Self { + let [index1, index2] = *indexes.indexes(); + EntryIndexes::NonUnique { index1, index2 } + } + #[inline] pub(super) fn is_unique(&self) -> bool { matches!(self, EntryIndexes::Unique(_)) diff --git a/crates/iddqd/src/bi_hash_map/imp.rs b/crates/iddqd/src/bi_hash_map/imp.rs index ab010741..2b976993 100644 --- a/crates/iddqd/src/bi_hash_map/imp.rs +++ b/crates/iddqd/src/bi_hash_map/imp.rs @@ -13,6 +13,7 @@ use crate::{ ItemIndex, alloc::{Allocator, Global, global_alloc}, borrow::DormantMutRef, + entry::EntryLookup, fmt_utils::StrDisplayAsDebug, hash_table, item_set::ItemSet, @@ -1918,8 +1919,8 @@ impl BiHashMap { (index1, index2) }; - match (index1, index2) { - (Some(index1), Some(index2)) if index1 == index2 => { + match EntryIndexes::classify(index1, index2) { + EntryLookup::Unique(index) => { // The item is already in the map. drop(key1); Entry::Occupied( @@ -1927,24 +1928,24 @@ impl BiHashMap { unsafe { OccupiedEntry::new( dormant_map, - EntryIndexes::Unique(index1), + EntryIndexes::Unique(index), ) }, ) } - (None, None) => { + EntryLookup::Vacant => { let hashes = map.tables.make_hashes::(&key1, &key2); Entry::Vacant( // SAFETY: `map` is not used after this point. unsafe { VacantEntry::new(dormant_map, hashes) }, ) } - (index1, index2) => Entry::Occupied( + EntryLookup::NonUnique(indexes) => Entry::Occupied( // SAFETY: `map` is not used after this point. unsafe { OccupiedEntry::new( dormant_map, - EntryIndexes::NonUnique { index1, index2 }, + EntryIndexes::from_non_unique(indexes), ) }, ), diff --git a/crates/iddqd/src/support/entry.rs b/crates/iddqd/src/support/entry.rs index 069bab40..600fd87d 100644 --- a/crates/iddqd/src/support/entry.rs +++ b/crates/iddqd/src/support/entry.rs @@ -1,8 +1,3 @@ -#![expect( - dead_code, - reason = "temporary until the next PR wires the support primitive into BiHashMap" -)] - //! Crate-internal support for classifying multi-key entry lookups. //! //! This module is intentionally independent of the map implementations. It only @@ -52,6 +47,10 @@ impl EntryIndexes { } #[inline] + #[expect( + dead_code, + reason = "reserved for upcoming TriHashMap entry wiring" + )] pub(crate) const fn indexes(&self) -> &[Option; N] { &self.indexes } @@ -90,6 +89,13 @@ impl NonUniqueIndexes { } #[inline] + #[cfg_attr( + not(test), + expect( + dead_code, + reason = "reserved for upcoming TriHashMap entry deduplication" + ) + )] pub(crate) fn distinct(self) -> DistinctIndexes { DistinctIndexes::from_indexes(self.indexes) } @@ -133,16 +139,37 @@ impl DistinctIndexes { } #[inline] + #[cfg_attr( + not(test), + expect( + dead_code, + reason = "reserved for upcoming TriHashMap entry deduplication" + ) + )] pub(crate) const fn indexes(&self) -> &[Option; N] { &self.indexes } #[inline] + #[cfg_attr( + not(test), + expect( + dead_code, + reason = "reserved for upcoming TriHashMap entry deduplication" + ) + )] pub(crate) const fn len(&self) -> usize { self.len } #[inline] + #[cfg_attr( + not(test), + expect( + dead_code, + reason = "reserved for upcoming TriHashMap entry deduplication" + ) + )] pub(crate) const fn key_to_slot(&self) -> &[Option; N] { &self.key_to_slot } From b9a25362676893feeba6c9fbc2d4a0b9ebbcc052 Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 15:04:08 +0200 Subject: [PATCH 04/10] Add initial TriHashMap entry API --- crates/iddqd/src/support/entry.rs | 30 +- crates/iddqd/src/tri_hash_map/entry.rs | 306 ++++++++++++++++++ .../iddqd/src/tri_hash_map/entry_indexes.rs | 42 +++ crates/iddqd/src/tri_hash_map/imp.rs | 199 +++++++++--- crates/iddqd/src/tri_hash_map/mod.rs | 5 + .../iddqd/tests/integration/tri_hash_map.rs | 148 +++++++++ 6 files changed, 654 insertions(+), 76 deletions(-) create mode 100644 crates/iddqd/src/tri_hash_map/entry.rs create mode 100644 crates/iddqd/src/tri_hash_map/entry_indexes.rs diff --git a/crates/iddqd/src/support/entry.rs b/crates/iddqd/src/support/entry.rs index 600fd87d..4aee9091 100644 --- a/crates/iddqd/src/support/entry.rs +++ b/crates/iddqd/src/support/entry.rs @@ -49,7 +49,7 @@ impl EntryIndexes { #[inline] #[expect( dead_code, - reason = "reserved for upcoming TriHashMap entry wiring" + reason = "reserved for upcoming TriHashMap occupied entry replacement validation" )] pub(crate) const fn indexes(&self) -> &[Option; N] { &self.indexes @@ -89,13 +89,6 @@ impl NonUniqueIndexes { } #[inline] - #[cfg_attr( - not(test), - expect( - dead_code, - reason = "reserved for upcoming TriHashMap entry deduplication" - ) - )] pub(crate) fn distinct(self) -> DistinctIndexes { DistinctIndexes::from_indexes(self.indexes) } @@ -139,37 +132,16 @@ impl DistinctIndexes { } #[inline] - #[cfg_attr( - not(test), - expect( - dead_code, - reason = "reserved for upcoming TriHashMap entry deduplication" - ) - )] pub(crate) const fn indexes(&self) -> &[Option; N] { &self.indexes } #[inline] - #[cfg_attr( - not(test), - expect( - dead_code, - reason = "reserved for upcoming TriHashMap entry deduplication" - ) - )] pub(crate) const fn len(&self) -> usize { self.len } #[inline] - #[cfg_attr( - not(test), - expect( - dead_code, - reason = "reserved for upcoming TriHashMap entry deduplication" - ) - )] pub(crate) const fn key_to_slot(&self) -> &[Option; N] { &self.key_to_slot } diff --git a/crates/iddqd/src/tri_hash_map/entry.rs b/crates/iddqd/src/tri_hash_map/entry.rs new file mode 100644 index 00000000..954f1914 --- /dev/null +++ b/crates/iddqd/src/tri_hash_map/entry.rs @@ -0,0 +1,306 @@ +use super::{ + TriHashItem, TriHashMap, entry_indexes::EntryIndexes, ref_mut::RefMut, +}; +use crate::{ + DefaultHashBuilder, + support::{ + alloc::{Allocator, Global}, + borrow::DormantMutRef, + map_hash::MapHash, + }, +}; +use core::{fmt, hash::BuildHasher}; + +/// An implementation of the Entry API for [`TriHashMap`]. +/// +/// A vacant entry means none of the three provided keys are present. An +/// occupied entry is unique only when all three keys point to the same item; +/// partial matches and mixed matches are occupied non-unique entries. +pub enum Entry< + 'a, + T: TriHashItem, + S = DefaultHashBuilder, + A: Allocator = Global, +> { + /// A vacant entry: none of the provided keys are present. + Vacant(VacantEntry<'a, T, S, A>), + /// An occupied entry where at least one of the keys is present in the map. + Occupied(OccupiedEntry<'a, T, S, A>), +} + +impl<'a, T: TriHashItem, S, A: Allocator> fmt::Debug for Entry<'a, T, S, A> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Entry::Vacant(entry) => { + f.debug_tuple("Vacant").field(entry).finish() + } + Entry::Occupied(entry) => { + f.debug_tuple("Occupied").field(entry).finish() + } + } + } +} + +/// A vacant entry. +pub struct VacantEntry< + 'a, + T: TriHashItem, + S = DefaultHashBuilder, + A: Allocator = Global, +> { + map: DormantMutRef<'a, TriHashMap>, + hashes: [MapHash; 3], +} + +impl<'a, T: TriHashItem, S, A: Allocator> fmt::Debug + for VacantEntry<'a, T, S, A> +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("VacantEntry") + .field("hashes", &self.hashes) + .finish_non_exhaustive() + } +} + +impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> + VacantEntry<'a, T, S, A> +{ + pub(super) unsafe fn new( + map: DormantMutRef<'a, TriHashMap>, + hashes: [MapHash; 3], + ) -> Self { + VacantEntry { map, hashes } + } + + /// Sets the entry to a new value, returning a mutable reference to it. + /// + /// # Panics + /// + /// Panics before mutation if any value key hashes differently from the + /// corresponding key passed to [`TriHashMap::entry`]. + pub fn insert(self, value: T) -> RefMut<'a, T, S> { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point. + let map = unsafe { self.map.awaken() }; + validate_hashes(map, self.hashes, &value); + let Ok(index) = map.insert_unique_impl(value) else { + panic!("key already present in map"); + }; + map.get_by_index_mut(index).expect("index is known to be valid") + } + + /// Sets the entry to a new value, and returns an `OccupiedEntry`. + #[inline] + pub fn insert_entry(mut self, value: T) -> OccupiedEntry<'a, T, S, A> { + let index = { + // SAFETY: The safety assumption behind `Self::new` guarantees that + // the original reference to the map is no longer used at this + // point. + let map = unsafe { self.map.reborrow() }; + validate_hashes(map, self.hashes, &value); + let Ok(index) = map.insert_unique_impl(value) else { + panic!("key already present in map"); + }; + index + }; + + // SAFETY: `map`, as well as anything borrowed from it, is dropped + // above, so the temporary reborrow has ended before awakening again. + unsafe { OccupiedEntry::new(self.map, EntryIndexes::Unique(index)) } + } +} + +fn validate_hashes( + map: &TriHashMap, + hashes: [MapHash; 3], + value: &T, +) { + let state = &map.tables.state; + if !hashes[0].is_same_hash(state, value.key1()) { + panic!("key1 hashes do not match"); + } + if !hashes[1].is_same_hash(state, value.key2()) { + panic!("key2 hashes do not match"); + } + if !hashes[2].is_same_hash(state, value.key3()) { + panic!("key3 hashes do not match"); + } +} + +/// A view into an occupied entry in a [`TriHashMap`]. +pub struct OccupiedEntry< + 'a, + T: TriHashItem, + S = DefaultHashBuilder, + A: Allocator = Global, +> { + map: DormantMutRef<'a, TriHashMap>, + indexes: EntryIndexes, +} + +impl<'a, T: TriHashItem, S, A: Allocator> fmt::Debug + for OccupiedEntry<'a, T, S, A> +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("OccupiedEntry") + .field("indexes", &self.indexes) + .finish_non_exhaustive() + } +} + +impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> + OccupiedEntry<'a, T, S, A> +{ + /// # Safety + /// + /// After self is created, the original reference created by + /// `DormantMutRef::new` must not be used. + pub(super) unsafe fn new( + map: DormantMutRef<'a, TriHashMap>, + indexes: EntryIndexes, + ) -> Self { + OccupiedEntry { map, indexes } + } + + /// Returns true if all three keys point to exactly one item. + #[inline] + pub fn is_unique(&self) -> bool { + self.indexes.is_unique() + } + + /// Returns true if more than one item matched, or if some keys are absent. + #[inline] + pub fn is_non_unique(&self) -> bool { + !self.is_unique() + } + + /// Returns shared references to values that match the provided keys. + pub fn get(&self) -> OccupiedEntryRef<'_, T> { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point, and + // there is no active temporary reborrow. + let map = unsafe { self.map.reborrow_shared() }; + map.get_by_entry_index(self.indexes) + } + + /// Converts self into shared references to items that match the provided keys. + pub fn into_ref(self) -> OccupiedEntryRef<'a, T> { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point, and + // there is no active temporary reborrow. + let map = unsafe { self.map.awaken() }; + map.get_by_entry_index(self.indexes) + } +} + +/// Shared references to values matched by a [`TriHashMap`] occupied entry. +#[derive(Debug)] +pub enum OccupiedEntryRef<'a, T: TriHashItem> { + /// All keys point to the same entry. + Unique(&'a T), + /// The keys point to different entries, or some keys are not present. + NonUnique(NonUniqueEntryRef<'a, T>), +} + +/// Accessor-backed shared non-unique entry references. +#[derive(Debug)] +pub struct NonUniqueEntryRef<'a, T: TriHashItem> { + values: [Option<&'a T>; 3], + len: usize, + key_to_slot: [Option; 3], +} + +impl<'a, T: TriHashItem> OccupiedEntryRef<'a, T> { + /// Returns true if all three keys point to exactly one item. + #[inline] + pub fn is_unique(&self) -> bool { + matches!(self, Self::Unique(_)) + } + /// Returns true if more than one item matched, or if some keys are absent. + #[inline] + pub fn is_non_unique(&self) -> bool { + matches!(self, Self::NonUnique(_)) + } + /// Returns a reference to the value if the entry is unique. + #[inline] + pub fn as_unique(&self) -> Option<&'a T> { + match self { + Self::Unique(v) => Some(v), + Self::NonUnique(_) => None, + } + } + /// Returns a reference to the value fetched by the first key. + #[inline] + pub fn by_key1(&self) -> Option<&'a T> { + self.by_key(0) + } + /// Returns a reference to the value fetched by the second key. + #[inline] + pub fn by_key2(&self) -> Option<&'a T> { + self.by_key(1) + } + /// Returns a reference to the value fetched by the third key. + #[inline] + pub fn by_key3(&self) -> Option<&'a T> { + self.by_key(2) + } + fn by_key(&self, key: usize) -> Option<&'a T> { + match self { + Self::Unique(v) => Some(v), + Self::NonUnique(non_unique) => non_unique.by_key(key), + } + } + /// Calls `f` once for each distinct matched value in first-key-hit order. + pub fn for_each(&self, mut f: F) + where + F: FnMut(&'a T), + { + match self { + Self::Unique(v) => f(v), + Self::NonUnique(non_unique) => non_unique.for_each(f), + } + } +} + +impl<'a, T: TriHashItem> NonUniqueEntryRef<'a, T> { + pub(super) fn new( + values: [Option<&'a T>; 3], + len: usize, + key_to_slot: [Option; 3], + ) -> Self { + Self { values, len, key_to_slot } + } + + /// Returns a reference to the value fetched by the first key. + #[inline] + pub fn by_key1(&self) -> Option<&'a T> { + self.by_key(0) + } + + /// Returns a reference to the value fetched by the second key. + #[inline] + pub fn by_key2(&self) -> Option<&'a T> { + self.by_key(1) + } + + /// Returns a reference to the value fetched by the third key. + #[inline] + pub fn by_key3(&self) -> Option<&'a T> { + self.by_key(2) + } + + #[inline] + fn by_key(&self, key: usize) -> Option<&'a T> { + self.key_to_slot[key].and_then(|slot| self.values[slot]) + } + + /// Calls `f` once for each distinct matched value in first-key-hit order. + pub fn for_each(&self, mut f: F) + where + F: FnMut(&'a T), + { + for value in self.values[..self.len].iter().flatten() { + f(value); + } + } +} diff --git a/crates/iddqd/src/tri_hash_map/entry_indexes.rs b/crates/iddqd/src/tri_hash_map/entry_indexes.rs new file mode 100644 index 00000000..fe54b0ab --- /dev/null +++ b/crates/iddqd/src/tri_hash_map/entry_indexes.rs @@ -0,0 +1,42 @@ +use crate::support::{ + ItemIndex, + entry::{ + DistinctIndexes, EntryIndexes as SupportEntryIndexes, EntryLookup, + NonUniqueIndexes, + }, +}; + +#[derive(Clone, Copy, Debug)] +pub(super) enum EntryIndexes { + Unique(ItemIndex), + NonUnique(NonUniqueIndexes<3>), +} + +impl EntryIndexes { + #[inline] + pub(super) fn classify( + index1: Option, + index2: Option, + index3: Option, + ) -> EntryLookup<3> { + SupportEntryIndexes::new([index1, index2, index3]).classify() + } + + #[inline] + pub(super) fn from_non_unique(indexes: NonUniqueIndexes<3>) -> Self { + EntryIndexes::NonUnique(indexes) + } + + #[inline] + pub(super) fn is_unique(&self) -> bool { + matches!(self, EntryIndexes::Unique(_)) + } + + #[inline] + pub(super) fn distinct(&self) -> Option> { + match self { + EntryIndexes::Unique(_) => None, + EntryIndexes::NonUnique(indexes) => Some(indexes.distinct()), + } + } +} diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index 75b9f220..b1b24087 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -1,4 +1,8 @@ -use super::{IntoIter, Iter, IterMut, RefMut, tables::TriHashMapTables}; +use super::{ + Entry, IntoIter, Iter, IterMut, OccupiedEntry, OccupiedEntryRef, RefMut, + VacantEntry, entry::NonUniqueEntryRef, entry_indexes::EntryIndexes, + tables::TriHashMapTables, +}; use crate::{ DefaultHashBuilder, TriHashItem, errors::{DuplicateItem, TryReserveError}, @@ -7,6 +11,7 @@ use crate::{ ItemIndex, alloc::{Allocator, Global, global_alloc}, borrow::DormantMutRef, + entry::EntryLookup, fmt_utils::StrDisplayAsDebug, hash_table, item_set::ItemSet, @@ -1302,6 +1307,89 @@ impl TriHashMap { Ok(()) } + pub(super) fn get_by_entry_index( + &self, + indexes: EntryIndexes, + ) -> OccupiedEntryRef<'_, T> { + match indexes { + EntryIndexes::Unique(index) => OccupiedEntryRef::Unique( + self.items.get(index).expect("index is valid"), + ), + EntryIndexes::NonUnique(_) => { + let distinct = indexes.distinct().expect("non-unique indexes"); + let mut values = [None; 3]; + for (slot, index) in distinct.indexes()[..distinct.len()] + .iter() + .copied() + .flatten() + .enumerate() + { + values[slot] = Some( + self.items.get(index).expect("entry index is valid"), + ); + } + OccupiedEntryRef::NonUnique(NonUniqueEntryRef::new( + values, + distinct.len(), + *distinct.key_to_slot(), + )) + } + } + } + + pub(super) fn get_by_index_mut( + &mut self, + index: ItemIndex, + ) -> Option> { + let item = self.items.get_mut(index)?; + let state = self.tables.state.clone(); + let hashes = self.tables.make_hashes_for_item(&item); + Some(RefMut::new(state, hashes, item)) + } + + pub(super) fn insert_unique_impl( + &mut self, + value: T, + ) -> Result> { + let mut duplicates = BTreeSet::new(); + let state = &self.tables.state; + let (e1, e2, e3) = { + let k1 = value.key1(); + let k2 = value.key2(); + let k3 = value.key3(); + let e1 = detect_dup_or_insert( + self.tables + .k1_to_item + .entry(state, k1, |index| self.items[index].key1()), + &mut duplicates, + ); + let e2 = detect_dup_or_insert( + self.tables + .k2_to_item + .entry(state, k2, |index| self.items[index].key2()), + &mut duplicates, + ); + let e3 = detect_dup_or_insert( + self.tables + .k3_to_item + .entry(state, k3, |index| self.items[index].key3()), + &mut duplicates, + ); + (e1, e2, e3) + }; + if !duplicates.is_empty() { + return Err(DuplicateItem::__internal_new( + value, + duplicates.iter().map(|ix| &self.items[*ix]).collect(), + )); + } + let next_index = self.items.assert_can_grow().insert(value); + e1.unwrap().insert(next_index); + e2.unwrap().insert(next_index); + e3.unwrap().insert(next_index); + Ok(next_index) + } + /// Checks the structural invariants of the map: /// /// * The item set is well-formed. @@ -1489,52 +1577,7 @@ impl TriHashMap { &mut self, value: T, ) -> Result<(), DuplicateItem> { - let mut duplicates = BTreeSet::new(); - - // Check for duplicates *before* inserting the new item, because we - // don't want to partially insert the new item and then have to roll - // back. - let state = &self.tables.state; - let (e1, e2, e3) = { - let k1 = value.key1(); - let k2 = value.key2(); - let k3 = value.key3(); - - let e1 = detect_dup_or_insert( - self.tables - .k1_to_item - .entry(state, k1, |index| self.items[index].key1()), - &mut duplicates, - ); - let e2 = detect_dup_or_insert( - self.tables - .k2_to_item - .entry(state, k2, |index| self.items[index].key2()), - &mut duplicates, - ); - let e3 = detect_dup_or_insert( - self.tables - .k3_to_item - .entry(state, k3, |index| self.items[index].key3()), - &mut duplicates, - ); - (e1, e2, e3) - }; - - if !duplicates.is_empty() { - return Err(DuplicateItem::__internal_new( - value, - duplicates.iter().map(|ix| &self.items[*ix]).collect(), - )); - } - - let next_index = self.items.assert_can_grow().insert(value); - // e1, e2 and e3 are all Some because if they were None, duplicates - // would be non-empty, and we'd have bailed out earlier. - e1.unwrap().insert(next_index); - e2.unwrap().insert(next_index); - e3.unwrap().insert(next_index); - + let _ = self.insert_unique_impl(value)?; Ok(()) } @@ -2558,6 +2601,68 @@ impl TriHashMap { awakened_map.remove_by_index(remove_index) } + /// Gets the entry corresponding to the three provided keys. + /// + /// A vacant entry is returned only when none of the keys are present. If all + /// three keys point to the same item, the occupied entry is unique. Partial + /// matches and mixed matches are occupied non-unique entries. + pub fn entry<'a>( + &'a mut self, + key1: T::K1<'_>, + key2: T::K2<'_>, + key3: T::K3<'_>, + ) -> Entry<'a, T, S, A> { + // As with BiHashMap::entry, owned keys avoid borrowing caller-owned + // query keys for the entire entry lifetime. + let (map, dormant_map) = DormantMutRef::new(self); + let key1 = T::upcast_key1(key1); + let key2 = T::upcast_key2(key2); + let key3 = T::upcast_key3(key3); + let (index1, index2, index3) = { + let index1: Option = map.tables.k1_to_item.find_index( + &map.tables.state, + &key1, + |index| map.items[index].key1(), + ); + let index2: Option = map.tables.k2_to_item.find_index( + &map.tables.state, + &key2, + |index| map.items[index].key2(), + ); + let index3: Option = map.tables.k3_to_item.find_index( + &map.tables.state, + &key3, + |index| map.items[index].key3(), + ); + (index1, index2, index3) + }; + + match EntryIndexes::classify(index1, index2, index3) { + EntryLookup::Unique(index) => Entry::Occupied( + // SAFETY: `map` is not used after this point. + unsafe { + OccupiedEntry::new(dormant_map, EntryIndexes::Unique(index)) + }, + ), + EntryLookup::Vacant => { + let hashes = map.tables.make_hashes::(&key1, &key2, &key3); + Entry::Vacant( + // SAFETY: `map` is not used after this point. + unsafe { VacantEntry::new(dormant_map, hashes) }, + ) + } + EntryLookup::NonUnique(indexes) => Entry::Occupied( + // SAFETY: `map` is not used after this point. + unsafe { + OccupiedEntry::new( + dormant_map, + EntryIndexes::from_non_unique(indexes), + ) + }, + ), + } + } + /// Retains only the elements specified by the predicate. /// /// In other words, remove all items `T` for which `f(RefMut)` returns diff --git a/crates/iddqd/src/tri_hash_map/mod.rs b/crates/iddqd/src/tri_hash_map/mod.rs index dff922ff..68d9a77b 100644 --- a/crates/iddqd/src/tri_hash_map/mod.rs +++ b/crates/iddqd/src/tri_hash_map/mod.rs @@ -4,6 +4,8 @@ #[cfg(feature = "daft")] mod daft_impls; +mod entry; +mod entry_indexes; pub(crate) mod imp; mod iter; #[cfg(feature = "proptest")] @@ -18,6 +20,9 @@ pub(crate) mod trait_defs; #[cfg(feature = "daft")] pub use daft_impls::{ByK1, ByK2, ByK3, Diff, MapLeaf}; +pub use entry::{ + Entry, NonUniqueEntryRef, OccupiedEntry, OccupiedEntryRef, VacantEntry, +}; pub use imp::TriHashMap; pub use iter::{IntoIter, Iter, IterMut}; #[cfg(all(feature = "proptest", feature = "default-hasher"))] diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index bc19a592..098adfe8 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -1343,3 +1343,151 @@ mod proptest_panic_safety { } } } + +#[test] +fn entry_vacant_insert_and_insert_entry() { + let mut map = TriHashMap::::make_new(); + + match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => { + let inserted = + entry.insert(SimpleItem { key1: 1, key2: 'a', key3: 1 }); + assert_eq!(inserted.key1, 1); + } + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + } + assert_eq!(map.len(), 1); + + match map.entry(2, 'b', 2) { + tri_hash_map::Entry::Vacant(entry) => { + let occupied = + entry.insert_entry(SimpleItem { key1: 2, key2: 'b', key3: 2 }); + assert!(occupied.is_unique()); + assert_eq!(occupied.into_ref().as_unique().unwrap().key1, 2); + } + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + } +} + +#[test] +fn entry_classifies_unique_partial_and_mixed_lookups() { + let mut map = TriHashMap::::make_new(); + map.insert_unique(SimpleItem { key1: 1, key2: 'a', key3: 1 }).unwrap(); + map.insert_unique(SimpleItem { key1: 2, key2: 'b', key3: 2 }).unwrap(); + map.insert_unique(SimpleItem { key1: 3, key2: 'c', key3: 3 }).unwrap(); + + match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Occupied(entry) => assert!(entry.is_unique()), + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + for (key1, key2, key3) in [ + (1, 'a', 9), + (1, 'z', 1), + (9, 'a', 1), + (1, 'a', 2), + (1, 'b', 1), + (1, 'b', 3), + ] { + match map.entry(key1, key2, key3) { + tri_hash_map::Entry::Occupied(entry) => { + assert!(entry.is_non_unique()) + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + } +} + +#[test] +fn entry_shared_access_preserves_mapping_and_distinct_order() { + let mut map = TriHashMap::::make_new(); + map.insert_unique(SimpleItem { key1: 1, key2: 'a', key3: 1 }).unwrap(); + map.insert_unique(SimpleItem { key1: 2, key2: 'b', key3: 2 }).unwrap(); + + match map.entry(1, 'a', 2) { + tri_hash_map::Entry::Occupied(entry) => { + let entry_ref = entry.get(); + assert_eq!(entry_ref.by_key1().unwrap().key1, 1); + assert_eq!(entry_ref.by_key2().unwrap().key1, 1); + assert_eq!(entry_ref.by_key3().unwrap().key1, 2); + let tri_hash_map::OccupiedEntryRef::NonUnique(non_unique) = + entry_ref + else { + panic!("expected non-unique entry ref"); + }; + assert_eq!(non_unique.by_key1().unwrap().key1, 1); + assert_eq!(non_unique.by_key2().unwrap().key1, 1); + assert_eq!(non_unique.by_key3().unwrap().key1, 2); + let mut seen = Vec::new(); + non_unique.for_each(|item| seen.push(item.key1)); + assert_eq!(seen, vec![1, 2]); + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + + match map.entry(1, 'z', 1) { + tri_hash_map::Entry::Occupied(entry) => { + let entry_ref = entry.get(); + assert_eq!(entry_ref.by_key1().unwrap().key1, 1); + assert!(entry_ref.by_key2().is_none()); + assert_eq!(entry_ref.by_key3().unwrap().key1, 1); + let tri_hash_map::OccupiedEntryRef::NonUnique(non_unique) = + entry_ref + else { + panic!("expected non-unique entry ref"); + }; + assert_eq!(non_unique.by_key1().unwrap().key1, 1); + assert!(non_unique.by_key2().is_none()); + assert_eq!(non_unique.by_key3().unwrap().key1, 1); + let mut seen = Vec::new(); + non_unique.for_each(|item| seen.push(item.key1)); + assert_eq!(seen, vec![1]); + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + + match map.entry(9, 'b', 1) { + tri_hash_map::Entry::Occupied(entry) => { + let entry_ref = entry.get(); + assert!(entry_ref.by_key1().is_none()); + assert_eq!(entry_ref.by_key2().unwrap().key1, 2); + assert_eq!(entry_ref.by_key3().unwrap().key1, 1); + let mut seen = Vec::new(); + entry_ref.for_each(|item| seen.push(item.key1)); + assert_eq!(seen, vec![2, 1]); + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } +} + +#[test] +#[should_panic(expected = "key1 hashes do not match")] +fn entry_vacant_insert_panics_on_mismatched_key1() { + let mut map = TriHashMap::::make_new(); + let entry = match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => entry, + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + }; + entry.insert(SimpleItem { key1: 2, key2: 'a', key3: 1 }); +} + +#[test] +#[should_panic(expected = "key2 hashes do not match")] +fn entry_vacant_insert_panics_on_mismatched_key2() { + let mut map = TriHashMap::::make_new(); + let entry = match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => entry, + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + }; + entry.insert(SimpleItem { key1: 1, key2: 'b', key3: 1 }); +} + +#[test] +#[should_panic(expected = "key3 hashes do not match")] +fn entry_vacant_insert_panics_on_mismatched_key3() { + let mut map = TriHashMap::::make_new(); + let entry = match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => entry, + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + }; + entry.insert(SimpleItem { key1: 1, key2: 'a', key3: 2 }); +} From d8e06a1f08f889562c90249c23d6faae4b9428d3 Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:04:09 +0200 Subject: [PATCH 05/10] Add TriHashMap mutable occupied entries --- crates/iddqd/src/tri_hash_map/entry.rs | 181 ++++++++++++++++++ crates/iddqd/src/tri_hash_map/imp.rs | 42 +++- crates/iddqd/src/tri_hash_map/mod.rs | 3 +- .../iddqd/tests/integration/tri_hash_map.rs | 135 +++++++++++++ .../tri_hash_occupied_entry_mut_alias.rs | 43 +++++ .../tri_hash_occupied_entry_mut_alias.stderr | 9 + 6 files changed, 410 insertions(+), 3 deletions(-) create mode 100644 crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.rs create mode 100644 crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.stderr diff --git a/crates/iddqd/src/tri_hash_map/entry.rs b/crates/iddqd/src/tri_hash_map/entry.rs index 954f1914..181c078e 100644 --- a/crates/iddqd/src/tri_hash_map/entry.rs +++ b/crates/iddqd/src/tri_hash_map/entry.rs @@ -41,6 +41,28 @@ impl<'a, T: TriHashItem, S, A: Allocator> fmt::Debug for Entry<'a, T, S, A> { } } +impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> + Entry<'a, T, S, A> +{ + /// Provides in-place mutable access to occupied entries before any + /// potential inserts into the map. + /// + /// `F` is called once for each distinct entry that matches the provided + /// keys, in first-key-hit order. Vacant entries are left unchanged. + #[inline] + pub fn and_modify(self, f: F) -> Self + where + F: FnMut(RefMut<'_, T, S>), + { + match self { + Entry::Occupied(mut entry) => { + entry.get_mut().for_each(f); + Entry::Occupied(entry) + } + Entry::Vacant(entry) => Entry::Vacant(entry), + } + } +} /// A vacant entry. pub struct VacantEntry< 'a, @@ -183,6 +205,15 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> map.get_by_entry_index(self.indexes) } + /// Returns mutable references to values that match the provided keys. + pub fn get_mut(&mut self) -> OccupiedEntryMut<'_, T, S> { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point, and + // there is no active temporary reborrow. + let map = unsafe { self.map.reborrow() }; + map.get_by_entry_index_mut(self.indexes) + } + /// Converts self into shared references to items that match the provided keys. pub fn into_ref(self) -> OccupiedEntryRef<'a, T> { // SAFETY: The safety assumption behind `Self::new` guarantees that the @@ -191,6 +222,15 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> let map = unsafe { self.map.awaken() }; map.get_by_entry_index(self.indexes) } + + /// Converts self into mutable references to items that match the provided keys. + pub fn into_mut(self) -> OccupiedEntryMut<'a, T, S> { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point, and + // there is no active temporary reborrow. + let map = unsafe { self.map.awaken() }; + map.get_by_entry_index_mut(self.indexes) + } } /// Shared references to values matched by a [`TriHashMap`] occupied entry. @@ -304,3 +344,144 @@ impl<'a, T: TriHashItem> NonUniqueEntryRef<'a, T> { } } } + +/// Mutable references to values matched by a [`TriHashMap`] occupied entry. +pub enum OccupiedEntryMut< + 'a, + T: TriHashItem, + S: Clone + BuildHasher = DefaultHashBuilder, +> { + /// All keys point to the same entry. + Unique(RefMut<'a, T, S>), + /// The keys point to different entries, or some keys are not present. + NonUnique(NonUniqueEntryMut<'a, T, S>), +} + +impl<'a, T: TriHashItem + fmt::Debug, S: Clone + BuildHasher> fmt::Debug + for OccupiedEntryMut<'a, T, S> +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Unique(ref_mut) => { + f.debug_tuple("Unique").field(ref_mut).finish() + } + Self::NonUnique(non_unique) => { + f.debug_tuple("NonUnique").field(non_unique).finish() + } + } + } +} + +/// Accessor-backed mutable non-unique entry references. +pub struct NonUniqueEntryMut< + 'a, + T: TriHashItem, + S: Clone + BuildHasher = DefaultHashBuilder, +> { + refs: [Option>; 3], + len: usize, + key_to_slot: [Option; 3], +} + +impl<'a, T: TriHashItem + fmt::Debug, S: Clone + BuildHasher> fmt::Debug + for NonUniqueEntryMut<'a, T, S> +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("NonUniqueEntryMut") + .field("refs", &self.refs) + .field("len", &self.len) + .field("key_to_slot", &self.key_to_slot) + .finish() + } +} + +impl<'a, T: TriHashItem, S: Clone + BuildHasher> OccupiedEntryMut<'a, T, S> { + /// Returns true if all three keys point to exactly one item. + #[inline] + pub fn is_unique(&self) -> bool { + matches!(self, Self::Unique(_)) + } + /// Returns true if more than one item matched, or if some keys are absent. + #[inline] + pub fn is_non_unique(&self) -> bool { + matches!(self, Self::NonUnique(_)) + } + /// Returns a mutable reference to the value if the entry is unique. + #[inline] + pub fn as_unique(&mut self) -> Option> { + match self { + Self::Unique(v) => Some(v.reborrow()), + Self::NonUnique(_) => None, + } + } + /// Returns a mutable reference to the value fetched by the first key. + #[inline] + pub fn by_key1(&mut self) -> Option> { + self.by_key(0) + } + /// Returns a mutable reference to the value fetched by the second key. + #[inline] + pub fn by_key2(&mut self) -> Option> { + self.by_key(1) + } + /// Returns a mutable reference to the value fetched by the third key. + #[inline] + pub fn by_key3(&mut self) -> Option> { + self.by_key(2) + } + fn by_key(&mut self, key: usize) -> Option> { + match self { + Self::Unique(v) => Some(v.reborrow()), + Self::NonUnique(n) => n.by_key(key), + } + } + /// Calls `f` once for each distinct matched value in first-key-hit order. + pub fn for_each(&mut self, mut f: F) + where + F: FnMut(RefMut<'_, T, S>), + { + match self { + Self::Unique(v) => f(v.reborrow()), + Self::NonUnique(n) => n.for_each(f), + } + } +} + +impl<'a, T: TriHashItem, S: Clone + BuildHasher> NonUniqueEntryMut<'a, T, S> { + pub(super) fn new( + refs: [Option>; 3], + len: usize, + key_to_slot: [Option; 3], + ) -> Self { + Self { refs, len, key_to_slot } + } + /// Returns a mutable reference to the value fetched by the first key. + #[inline] + pub fn by_key1(&mut self) -> Option> { + self.by_key(0) + } + /// Returns a mutable reference to the value fetched by the second key. + #[inline] + pub fn by_key2(&mut self) -> Option> { + self.by_key(1) + } + /// Returns a mutable reference to the value fetched by the third key. + #[inline] + pub fn by_key3(&mut self) -> Option> { + self.by_key(2) + } + #[inline] + fn by_key(&mut self, key: usize) -> Option> { + self.key_to_slot[key] + .and_then(|slot| self.refs[slot].as_mut().map(RefMut::reborrow)) + } + /// Calls `f` once for each distinct matched value in first-key-hit order. + pub fn for_each(&mut self, mut f: F) + where + F: FnMut(RefMut<'_, T, S>), + { + for value in self.refs[..self.len].iter_mut().flatten() { + f(value.reborrow()); + } + } +} diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index b1b24087..b350ca1e 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -1,6 +1,8 @@ use super::{ - Entry, IntoIter, Iter, IterMut, OccupiedEntry, OccupiedEntryRef, RefMut, - VacantEntry, entry::NonUniqueEntryRef, entry_indexes::EntryIndexes, + Entry, IntoIter, Iter, IterMut, OccupiedEntry, OccupiedEntryMut, + OccupiedEntryRef, RefMut, VacantEntry, + entry::{NonUniqueEntryMut, NonUniqueEntryRef}, + entry_indexes::EntryIndexes, tables::TriHashMapTables, }; use crate::{ @@ -1337,6 +1339,42 @@ impl TriHashMap { } } + pub(super) fn get_by_entry_index_mut( + &mut self, + indexes: EntryIndexes, + ) -> OccupiedEntryMut<'_, T, S> { + match indexes { + EntryIndexes::Unique(index) => { + let item = self.items.get_mut(index).expect("index is valid"); + let state = self.tables.state.clone(); + let hashes = self.tables.make_hashes_for_item(&item); + OccupiedEntryMut::Unique(RefMut::new(state, hashes, item)) + } + EntryIndexes::NonUnique(_) => { + let distinct = indexes.distinct().expect("non-unique indexes"); + let state = self.tables.state.clone(); + let indexes = distinct.indexes(); + let mut items = self.items.get_disjoint_mut([ + indexes[0].as_ref().expect("distinct slot is present"), + indexes[1].as_ref().unwrap_or(&ItemIndex::SENTINEL), + indexes[2].as_ref().unwrap_or(&ItemIndex::SENTINEL), + ]); + let mut refs = core::array::from_fn(|_| None); + for slot in 0..distinct.len() { + let item = + items[slot].take().expect("entry index is valid"); + let hashes = self.tables.make_hashes_for_item(&item); + refs[slot] = Some(RefMut::new(state.clone(), hashes, item)); + } + OccupiedEntryMut::NonUnique(NonUniqueEntryMut::new( + refs, + distinct.len(), + *distinct.key_to_slot(), + )) + } + } + } + pub(super) fn get_by_index_mut( &mut self, index: ItemIndex, diff --git a/crates/iddqd/src/tri_hash_map/mod.rs b/crates/iddqd/src/tri_hash_map/mod.rs index 68d9a77b..7ba34ba8 100644 --- a/crates/iddqd/src/tri_hash_map/mod.rs +++ b/crates/iddqd/src/tri_hash_map/mod.rs @@ -21,7 +21,8 @@ pub(crate) mod trait_defs; #[cfg(feature = "daft")] pub use daft_impls::{ByK1, ByK2, ByK3, Diff, MapLeaf}; pub use entry::{ - Entry, NonUniqueEntryRef, OccupiedEntry, OccupiedEntryRef, VacantEntry, + Entry, NonUniqueEntryMut, NonUniqueEntryRef, OccupiedEntry, + OccupiedEntryMut, OccupiedEntryRef, VacantEntry, }; pub use imp::TriHashMap; pub use iter::{IntoIter, Iter, IterMut}; diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index 098adfe8..ca92398e 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -1491,3 +1491,138 @@ fn entry_vacant_insert_panics_on_mismatched_key3() { }; entry.insert(SimpleItem { key1: 1, key2: 'a', key3: 2 }); } + +#[derive(Clone, Debug)] +struct EntryMutItem { + key1: u32, + key2: char, + key3: u8, + value: &'static str, +} + +impl TriHashItem for EntryMutItem { + type K1<'a> = u32; + type K2<'a> = char; + type K3<'a> = u8; + + fn key1(&self) -> Self::K1<'_> { + self.key1 + } + fn key2(&self) -> Self::K2<'_> { + self.key2 + } + fn key3(&self) -> Self::K3<'_> { + self.key3 + } + tri_upcast!(); +} + +fn entry_mut_map() -> TriHashMap { + let mut map = TriHashMap::::make_new(); + map.insert_unique(EntryMutItem { key1: 1, key2: 'a', key3: 1, value: "A" }) + .unwrap(); + map.insert_unique(EntryMutItem { key1: 2, key2: 'b', key3: 2, value: "B" }) + .unwrap(); + map.insert_unique(EntryMutItem { key1: 3, key2: 'c', key3: 3, value: "C" }) + .unwrap(); + map +} + +#[test] +fn entry_mut_unique_and_non_unique_accessors_preserve_mapping() { + let mut map = entry_mut_map(); + match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Occupied(mut entry) => { + let mut view = entry.get_mut(); + assert!(view.is_unique()); + view.as_unique().unwrap().value = "unique"; + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + assert_eq!(map.get1(&1).unwrap().value, "unique"); + + for (keys, expected) in [ + ((1, 'a', 9), [Some("A"), Some("A"), None]), + ((1, 'z', 1), [Some("A"), None, Some("A")]), + ((9, 'a', 1), [None, Some("A"), Some("A")]), + ((1, 'a', 2), [Some("A"), Some("A"), Some("B")]), + ((1, 'b', 1), [Some("A"), Some("B"), Some("A")]), + ((1, 'b', 3), [Some("A"), Some("B"), Some("C")]), + ] { + let mut map = entry_mut_map(); + match map.entry(keys.0, keys.1, keys.2) { + tri_hash_map::Entry::Occupied(mut entry) => { + let mut view = entry.get_mut(); + assert!(view.is_non_unique()); + assert_eq!(view.by_key1().map(|item| item.value), expected[0]); + assert_eq!(view.by_key2().map(|item| item.value), expected[1]); + assert_eq!(view.by_key3().map(|item| item.value), expected[2]); + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + } +} + +#[test] +fn entry_mut_sequential_overlapping_access_and_for_each_order() { + let mut map = entry_mut_map(); + match map.entry(1, 'a', 9) { + tri_hash_map::Entry::Occupied(mut entry) => { + let mut view = entry.get_mut(); + view.by_key1().unwrap().value = "first"; + view.by_key2().unwrap().value = "second"; + assert!(view.by_key3().is_none()); + let mut seen = Vec::new(); + view.for_each(|mut item| { + seen.push(item.value); + item.value = "done"; + }); + assert_eq!(seen, vec!["second"]); + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + assert_eq!(map.get1(&1).unwrap().value, "done"); + + for (keys, expected) in [ + ((1, 'z', 1), vec!["A"]), + ((9, 'a', 1), vec!["A"]), + ((1, 'a', 2), vec!["A", "B"]), + ((1, 'b', 1), vec!["A", "B"]), + ((1, 'b', 3), vec!["A", "B", "C"]), + ] { + let mut map = entry_mut_map(); + match map.entry(keys.0, keys.1, keys.2) { + tri_hash_map::Entry::Occupied(mut entry) => { + let mut seen = Vec::new(); + entry.get_mut().for_each(|item| seen.push(item.value)); + assert_eq!(seen, expected); + } + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + } + } +} + +#[test] +fn entry_and_modify_visits_distinct_occupied_items_only() { + let mut map = entry_mut_map(); + map.entry(1, 'a', 9).and_modify(|mut item| item.value = "AA"); + assert_eq!(map.get1(&1).unwrap().value, "AA"); + + map.entry(1, 'a', 2).and_modify(|mut item| { + item.value = match item.value { + "AA" => "A2", + "B" => "B2", + other => other, + }; + }); + assert_eq!(map.get1(&1).unwrap().value, "A2"); + assert_eq!(map.get1(&2).unwrap().value, "B2"); + + let mut seen = Vec::new(); + map.entry(1, 'b', 3).and_modify(|item| seen.push(item.value)); + assert_eq!(seen, vec!["A2", "B2", "C"]); + + map.entry(9, 'z', 9).and_modify(|mut item| item.value = "vacant"); + assert_eq!(map.len(), 3); + assert!(map.get1(&9).is_none()); +} diff --git a/crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.rs b/crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.rs new file mode 100644 index 00000000..e2cf9be1 --- /dev/null +++ b/crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.rs @@ -0,0 +1,43 @@ +use iddqd::{TriHashItem, tri_hash_map, tri_upcast}; + +#[derive(Debug)] +struct Item { + key1: u32, + key2: u32, + key3: u32, +} + +impl TriHashItem for Item { + type K1<'a> = u32; + type K2<'a> = u32; + type K3<'a> = u32; + + fn key1(&self) -> Self::K1<'_> { + self.key1 + } + + fn key2(&self) -> Self::K2<'_> { + self.key2 + } + + fn key3(&self) -> Self::K3<'_> { + self.key3 + } + + tri_upcast!(); +} + +fn main() { + let mut map = tri_hash_map! { + Item { key1: 0, key2: 10, key3: 20 }, + }; + + let mut entry = match map.entry(0, 10, 99) { + iddqd::tri_hash_map::Entry::Occupied(entry) => entry, + iddqd::tri_hash_map::Entry::Vacant(_) => unreachable!(), + }; + + let mut refs = entry.get_mut(); + let _by_key1 = refs.by_key1().unwrap(); + let _by_key2 = refs.by_key2().unwrap(); +} diff --git a/crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.stderr b/crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.stderr new file mode 100644 index 00000000..d9cee313 --- /dev/null +++ b/crates/iddqd/tests/ui/invalid/tri_hash_occupied_entry_mut_alias.stderr @@ -0,0 +1,9 @@ +error[E0499]: cannot borrow `refs` as mutable more than once at a time + --> tests/ui/invalid/tri_hash_occupied_entry_mut_alias.rs:42:20 + | +41 | let _by_key1 = refs.by_key1().unwrap(); + | ---- first mutable borrow occurs here +42 | let _by_key2 = refs.by_key2().unwrap(); + | ^^^^ second mutable borrow occurs here +43 | } + | - first borrow might be used here, when `_by_key1` is dropped and runs the `Drop` code for type `iddqd::tri_hash_map::RefMut` From 06c6b73ac8516eb1143b8cca811ca9b1bc696eac Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:20:41 +0200 Subject: [PATCH 06/10] Implement TriHashMap occupied entry mutation --- crates/iddqd/src/tri_hash_map/entry.rs | 110 ++++++++++++++++- crates/iddqd/src/tri_hash_map/imp.rs | 66 ++++++++--- .../iddqd/tests/integration/tri_hash_map.rs | 111 ++++++++++++++++++ 3 files changed, 265 insertions(+), 22 deletions(-) diff --git a/crates/iddqd/src/tri_hash_map/entry.rs b/crates/iddqd/src/tri_hash_map/entry.rs index 181c078e..9aea66f6 100644 --- a/crates/iddqd/src/tri_hash_map/entry.rs +++ b/crates/iddqd/src/tri_hash_map/entry.rs @@ -4,11 +4,13 @@ use super::{ use crate::{ DefaultHashBuilder, support::{ + ItemIndex, alloc::{Allocator, Global}, borrow::DormantMutRef, map_hash::MapHash, }, }; +use alloc::vec::Vec; use core::{fmt, hash::BuildHasher}; /// An implementation of the Entry API for [`TriHashMap`]. @@ -62,6 +64,33 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> Entry::Vacant(entry) => Entry::Vacant(entry), } } + + /// Ensures a value is in the entry by inserting `value` if vacant, and + /// returns mutable occupied access to the entry. + #[inline] + pub fn or_insert(self, value: T) -> OccupiedEntryMut<'a, T, S> { + match self { + Entry::Occupied(entry) => entry.into_mut(), + Entry::Vacant(entry) => { + OccupiedEntryMut::Unique(entry.insert(value)) + } + } + } + + /// Ensures a value is in the entry by inserting the result of `default` if + /// vacant, and returns mutable occupied access to the entry. + #[inline] + pub fn or_insert_with(self, default: F) -> OccupiedEntryMut<'a, T, S> + where + F: FnOnce() -> T, + { + match self { + Entry::Occupied(entry) => entry.into_mut(), + Entry::Vacant(entry) => { + OccupiedEntryMut::Unique(entry.insert(default())) + } + } + } } /// A vacant entry. pub struct VacantEntry< @@ -104,7 +133,7 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point. let map = unsafe { self.map.awaken() }; - validate_hashes(map, self.hashes, &value); + validate_hashes(map, self.hashes.clone(), &value); let Ok(index) = map.insert_unique_impl(value) else { panic!("key already present in map"); }; @@ -119,7 +148,7 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> // the original reference to the map is no longer used at this // point. let map = unsafe { self.map.reborrow() }; - validate_hashes(map, self.hashes, &value); + validate_hashes(map, self.hashes.clone(), &value); let Ok(index) = map.insert_unique_impl(value) else { panic!("key already present in map"); }; @@ -128,7 +157,13 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> // SAFETY: `map`, as well as anything borrowed from it, is dropped // above, so the temporary reborrow has ended before awakening again. - unsafe { OccupiedEntry::new(self.map, EntryIndexes::Unique(index)) } + unsafe { + OccupiedEntry::new( + self.map, + EntryIndexes::Unique(index), + self.hashes, + ) + } } } @@ -158,6 +193,7 @@ pub struct OccupiedEntry< > { map: DormantMutRef<'a, TriHashMap>, indexes: EntryIndexes, + hashes: [MapHash; 3], } impl<'a, T: TriHashItem, S, A: Allocator> fmt::Debug @@ -180,8 +216,9 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> pub(super) unsafe fn new( map: DormantMutRef<'a, TriHashMap>, indexes: EntryIndexes, + hashes: [MapHash; 3], ) -> Self { - OccupiedEntry { map, indexes } + OccupiedEntry { map, indexes, hashes } } /// Returns true if all three keys point to exactly one item. @@ -231,6 +268,71 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> let map = unsafe { self.map.awaken() }; map.get_by_entry_index_mut(self.indexes) } + + /// Removes all distinct values matched by this entry. + pub fn remove(self) -> Vec { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point. + let map = unsafe { self.map.awaken() }; + let duplicates = prepare_entry_removal(map, self.indexes); + let mut removed = Vec::with_capacity(duplicates.len()); + map.remove_prepared_duplicates(duplicates, &mut removed); + removed + } + + /// Replaces all distinct values matched by this entry with `value`. + /// + /// # Panics + /// + /// Panics before mutation if `value` does not match the entry key hashes + /// or if its duplicate state is incompatible with this entry. + pub fn insert(&mut self, value: T) -> Vec { + // SAFETY: The safety assumption behind `Self::new` guarantees that the + // original reference to the map is no longer used at this point, and + // there is no active temporary reborrow. + let map = unsafe { self.map.reborrow() }; + validate_hashes(map, self.hashes.clone(), &value); + let prepared = map.prepare_insert_overwrite(&value); + validate_prepared_indexes(self.indexes, prepared.indexes); + + let mut removed = Vec::with_capacity(prepared.duplicate_count()); + map.try_reserve_insert_overwrite_commit(prepared.needs_new_item_slot()) + .expect("reserved capacity for entry replacement commit"); + let next_index = + map.commit_insert_overwrite(value, prepared, &mut removed); + self.indexes = EntryIndexes::Unique(next_index); + removed + } +} + +fn validate_prepared_indexes( + indexes: EntryIndexes, + prepared: [Option; 3], +) { + let expected = match indexes { + EntryIndexes::Unique(index) => [Some(index), Some(index), Some(index)], + EntryIndexes::NonUnique(indexes) => *indexes.indexes(), + }; + if prepared != expected { + panic!("replacement duplicate state does not match entry lookup"); + } +} + +fn prepare_entry_removal< + T: TriHashItem, + S: Clone + BuildHasher, + A: Allocator, +>( + map: &TriHashMap, + indexes: EntryIndexes, +) -> Vec { + let distinct = match indexes { + EntryIndexes::Unique(index) => [Some(index), None, None], + EntryIndexes::NonUnique(indexes) => *indexes.distinct().indexes(), + }; + super::imp::PreparedDuplicate::from_indexes(distinct, |index| { + map.prepare_duplicate(index) + }) } /// Shared references to values matched by a [`TriHashMap`] occupied entry. diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index b350ca1e..7c174ab9 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -29,13 +29,13 @@ use equivalent::Equivalent; #[derive(Debug)] #[must_use] -struct PreparedDuplicate { - index: ItemIndex, - hashes: [MapHash; 3], +pub(super) struct PreparedDuplicate { + pub(super) index: ItemIndex, + pub(super) hashes: [MapHash; 3], } impl PreparedDuplicate { - fn from_indexes( + pub(super) fn from_indexes( indexes: [Option; N], mut prepare: impl FnMut(ItemIndex) -> Self, ) -> Vec { @@ -58,19 +58,20 @@ impl PreparedDuplicate { #[derive(Debug)] #[must_use] -struct PreparedInsertOverwrite { - duplicates: Vec, - hashes: [MapHash; 3], +pub(super) struct PreparedInsertOverwrite { + pub(super) indexes: [Option; 3], + pub(super) duplicates: Vec, + pub(super) hashes: [MapHash; 3], } impl PreparedInsertOverwrite { #[inline] - fn duplicate_count(&self) -> usize { + pub(super) fn duplicate_count(&self) -> usize { self.duplicates.len() } #[inline] - fn needs_new_item_slot(&self) -> bool { + pub(super) fn needs_new_item_slot(&self) -> bool { self.duplicates.is_empty() } } @@ -1514,7 +1515,7 @@ impl TriHashMap { self.try_reserve_insert_overwrite_commit( prepared.needs_new_item_slot(), ) - .expect("reserved space successfully"); + .expect("reserved capacity for entry replacement commit"); self.commit_insert_overwrite(value, prepared, &mut duplicates); @@ -2675,15 +2676,20 @@ impl TriHashMap { (index1, index2, index3) }; + let hashes = map.tables.make_hashes::(&key1, &key2, &key3); + match EntryIndexes::classify(index1, index2, index3) { EntryLookup::Unique(index) => Entry::Occupied( // SAFETY: `map` is not used after this point. unsafe { - OccupiedEntry::new(dormant_map, EntryIndexes::Unique(index)) + OccupiedEntry::new( + dormant_map, + EntryIndexes::Unique(index), + hashes, + ) }, ), EntryLookup::Vacant => { - let hashes = map.tables.make_hashes::(&key1, &key2, &key3); Entry::Vacant( // SAFETY: `map` is not used after this point. unsafe { VacantEntry::new(dormant_map, hashes) }, @@ -2695,6 +2701,7 @@ impl TriHashMap { OccupiedEntry::new( dormant_map, EntryIndexes::from_non_unique(indexes), + hashes, ) }, ), @@ -2936,7 +2943,10 @@ impl TriHashMap { .find_index(&self.tables.state, k, |index| self.items[index].key3()) } - fn prepare_insert_overwrite(&self, value: &T) -> PreparedInsertOverwrite { + pub(super) fn prepare_insert_overwrite( + &self, + value: &T, + ) -> PreparedInsertOverwrite { let key1 = value.key1(); let key2 = value.key2(); let key3 = value.key3(); @@ -2951,17 +2961,24 @@ impl TriHashMap { |index| self.prepare_duplicate(index), ); - PreparedInsertOverwrite { duplicates, hashes } + PreparedInsertOverwrite { + indexes: [index1, index2, index3], + duplicates, + hashes, + } } - fn prepare_duplicate(&self, index: ItemIndex) -> PreparedDuplicate { + pub(super) fn prepare_duplicate( + &self, + index: ItemIndex, + ) -> PreparedDuplicate { let item = &self.items[index]; let hashes = self.tables.make_hashes_for_item::(item); PreparedDuplicate { index, hashes } } - fn try_reserve_insert_overwrite_commit( + pub(super) fn try_reserve_insert_overwrite_commit( &mut self, needs_new_item_slot: bool, ) -> Result<(), TryReserveError> { @@ -2987,7 +3004,7 @@ impl TriHashMap { Ok(()) } - fn commit_insert_overwrite( + pub(super) fn commit_insert_overwrite( &mut self, value: T, prepared: PreparedInsertOverwrite, @@ -2998,7 +3015,7 @@ impl TriHashMap { for duplicate in prepared.duplicates { duplicates.push( self.remove_duplicate(duplicate) - .expect("duplicate index was prepared"), + .expect("prepared duplicate index was present"), ); } @@ -3095,6 +3112,19 @@ impl TriHashMap { /// prehashed lookup misses, this falls back to removing by `ItemIndex`, /// which performs a linear scan over cached indexes and does not re-enter /// user code. + pub(super) fn remove_prepared_duplicates( + &mut self, + duplicates: Vec, + removed: &mut Vec, + ) { + for duplicate in duplicates { + removed.push( + self.remove_duplicate(duplicate) + .expect("prepared duplicate index was present"), + ); + } + } + fn remove_duplicate(&mut self, duplicate: PreparedDuplicate) -> Option { let _ = self.items.get(duplicate.index)?; diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index ca92398e..be4edb15 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -1626,3 +1626,114 @@ fn entry_and_modify_visits_distinct_occupied_items_only() { assert_eq!(map.len(), 3); assert!(map.get1(&9).is_none()); } + +fn removed_values(items: Vec) -> Vec<&'static str> { + items.into_iter().map(|item| item.value).collect() +} + +#[test] +fn entry_remove_returns_first_key_hit_order_and_removes_once() { + for (keys, expected) in [ + ((1, 'a', 9), vec!["A"]), + ((1, 'z', 1), vec!["A"]), + ((9, 'a', 1), vec!["A"]), + ((1, 'a', 2), vec!["A", "B"]), + ((1, 'b', 1), vec!["A", "B"]), + ((1, 'b', 3), vec!["A", "B", "C"]), + ((9, 'b', 1), vec!["B", "A"]), + ] { + let mut map = entry_mut_map(); + let removed = match map.entry(keys.0, keys.1, keys.2) { + tri_hash_map::Entry::Occupied(entry) => entry.remove(), + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + }; + assert_eq!(removed_values(removed), expected); + assert!(map.validate(ValidateCompact::NonCompact).is_ok()); + } +} + +#[test] +fn entry_insert_replaces_first_key_hit_order_and_becomes_unique() { + for (keys, expected) in [ + ((1, 'a', 9), vec!["A"]), + ((1, 'z', 1), vec!["A"]), + ((9, 'a', 1), vec!["A"]), + ((1, 'a', 2), vec!["A", "B"]), + ((1, 'b', 1), vec!["A", "B"]), + ((1, 'b', 3), vec!["A", "B", "C"]), + ((9, 'b', 1), vec!["B", "A"]), + ] { + let mut map = entry_mut_map(); + let mut entry = match map.entry(keys.0, keys.1, keys.2) { + tri_hash_map::Entry::Occupied(entry) => entry, + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + }; + let removed = entry.insert(EntryMutItem { + key1: keys.0, + key2: keys.1, + key3: keys.2, + value: "R", + }); + assert_eq!(removed_values(removed), expected); + assert!(entry.is_unique()); + assert_eq!(entry.get().as_unique().unwrap().value, "R"); + drop(entry); + assert_eq!(map.get1(&keys.0).unwrap().value, "R"); + assert_eq!(map.get2(&keys.1).unwrap().value, "R"); + assert_eq!(map.get3(&keys.2).unwrap().value, "R"); + assert!(map.validate(ValidateCompact::NonCompact).is_ok()); + } +} + +#[test] +fn entry_or_insert_only_inserts_vacant_and_or_insert_with_is_lazy() { + let mut map = entry_mut_map(); + let mut inserted = map.entry(4, 'd', 4).or_insert(EntryMutItem { + key1: 4, + key2: 'd', + key3: 4, + value: "D", + }); + assert_eq!(inserted.as_unique().unwrap().value, "D"); + drop(inserted); + assert_eq!(map.len(), 4); + + for keys in + [(1, 'a', 9), (1, 'z', 1), (9, 'a', 1), (1, 'a', 2), (1, 'b', 3)] + { + let before = map.len(); + let view = map.entry(keys.0, keys.1, keys.2).or_insert_with(|| { + panic!("or_insert_with called for occupied entry") + }); + assert!(view.is_non_unique()); + drop(view); + assert_eq!(map.len(), before); + } +} + +#[test] +fn entry_insert_panics_before_mutation_before_changing_map() { + for bad in [ + EntryMutItem { key1: 9, key2: 'a', key3: 2, value: "bad-k1" }, + EntryMutItem { key1: 1, key2: 'z', key3: 2, value: "bad-k2" }, + EntryMutItem { key1: 1, key2: 'a', key3: 9, value: "bad-k3" }, + EntryMutItem { key1: 1, key2: 'a', key3: 1, value: "bad-dup" }, + ] { + let mut map = entry_mut_map(); + let result = + std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let mut entry = match map.entry(1, 'a', 2) { + tri_hash_map::Entry::Occupied(entry) => entry, + tri_hash_map::Entry::Vacant(_) => { + panic!("expected occupied") + } + }; + entry.insert(bad); + })); + assert!(result.is_err()); + assert_eq!(map.len(), 3); + assert_eq!(map.get1(&1).unwrap().value, "A"); + assert_eq!(map.get1(&2).unwrap().value, "B"); + assert!(map.validate(ValidateCompact::NonCompact).is_ok()); + } +} From 467474b2c1ae79dd805379a716ee862175ec3926 Mon Sep 17 00:00:00 2001 From: Sebastian <78623477+SG-devel@users.noreply.github.com> Date: Fri, 19 Jun 2026 20:10:56 +0200 Subject: [PATCH 07/10] Document TriHashMap entry API --- crates/iddqd-test-utils/src/panic_safety.rs | 8 +- crates/iddqd/src/tri_hash_map/entry.rs | 232 +++++++++++++++++- crates/iddqd/src/tri_hash_map/imp.rs | 12 +- .../iddqd/tests/integration/pathological.rs | 2 +- .../iddqd/tests/integration/tri_hash_map.rs | 29 +-- 5 files changed, 247 insertions(+), 36 deletions(-) diff --git a/crates/iddqd-test-utils/src/panic_safety.rs b/crates/iddqd-test-utils/src/panic_safety.rs index 690a484a..ccd22f4c 100644 --- a/crates/iddqd-test-utils/src/panic_safety.rs +++ b/crates/iddqd-test-utils/src/panic_safety.rs @@ -407,12 +407,10 @@ pub fn record_observation( #[derive(Clone, Copy, Debug, Default)] pub struct PanickyAlloc(pub A); -// SAFETY: -// -// * On the non-panic path, forwards to the wrapped allocator. -// * On the armed path, panics before any inner allocation, so no pointer is -// observable to the caller. #[cfg(all(feature = "default-hasher", feature = "allocator-api2"))] +// SAFETY: On the non-panic path, this forwards to the wrapped allocator. On +// the armed path, it panics before any inner allocation, so no pointer is +// observable to the caller. unsafe impl Allocator for PanickyAlloc { fn allocate(&self, layout: Layout) -> Result, AllocError> { observe_panicky_call("alloc"); diff --git a/crates/iddqd/src/tri_hash_map/entry.rs b/crates/iddqd/src/tri_hash_map/entry.rs index 9aea66f6..6a508b69 100644 --- a/crates/iddqd/src/tri_hash_map/entry.rs +++ b/crates/iddqd/src/tri_hash_map/entry.rs @@ -15,9 +15,153 @@ use core::{fmt, hash::BuildHasher}; /// An implementation of the Entry API for [`TriHashMap`]. /// -/// A vacant entry means none of the three provided keys are present. An -/// occupied entry is unique only when all three keys point to the same item; -/// partial matches and mixed matches are occupied non-unique entries. +/// # Differences from single-key entries +/// +/// This entry API does not behave exactly like +/// [`std::collections::HashMap::entry`], because three independent key lookups +/// can match zero, one, or several existing items. +/// +/// [`VacantEntry`] is returned only when none of `key1`, `key2`, or `key3` +/// passed to [`TriHashMap::entry`] matches an item. [`VacantEntry::insert`] and +/// [`VacantEntry::insert_entry`] insert the item for those three keys. +/// +/// [`OccupiedEntry`] is returned whenever at least one key matches. It is +/// unique only when all three key positions hit the same item (`A / A / A`). +/// Partial hits (`A / A / None`, `A / None / A`, `None / A / A`) and mixed hits +/// (`A / A / B`, `A / B / A`, `A / B / C`) are occupied non-unique entries, so +/// [`Entry::or_insert`] and [`Entry::or_insert_with`] do not insert for them. +/// +/// Non-unique access preserves the per-key mapping. For example, `A / A / B` +/// means key 1 and key 2 both map to `A`, while key 3 maps to `B`. Shared and +/// mutable accessors may therefore return the same item for more than one key +/// position. Mutable non-unique access is accessor-backed: it stores one +/// mutable reference per distinct item and maps key positions to those slots, +/// so cases such as `A / A / B` do not expose aliased mutable references. +/// Mutable accessors take `&mut self` and reborrow sequentially. +/// +/// Methods that visit, remove, or replace multiple matched items use +/// deterministic first-key-hit order and deduplicate repeated indexes: +/// +/// * `A / A / None`, `A / None / A`, and `None / A / A` visit `[A]`. +/// * `A / A / B` and `A / B / A` visit `[A, B]`. +/// * `A / B / C` visits `[A, B, C]`. +/// * `None / B / A` visits `[B, A]`. +/// +/// [`Entry::and_modify`] visits each distinct occupied item once. +/// [`OccupiedEntry::remove`] removes each distinct matched item once. +/// [`OccupiedEntry::insert`] replaces each distinct matched item once, returns +/// the removed items in first-key-hit order, and leaves the occupied entry +/// unique for the replacement item after a successful replacement. +/// +/// # Examples +/// +/// ``` +/// # #[cfg(feature = "default-hasher")] { +/// use iddqd::{TriHashItem, TriHashMap, tri_hash_map, tri_upcast}; +/// +/// #[derive(Debug, PartialEq, Eq)] +/// struct Item { +/// id: u32, +/// name: String, +/// tag: char, +/// value: i32, +/// } +/// +/// impl TriHashItem for Item { +/// type K1<'a> = u32; +/// type K2<'a> = &'a str; +/// type K3<'a> = char; +/// +/// fn key1(&self) -> Self::K1<'_> { +/// self.id +/// } +/// fn key2(&self) -> Self::K2<'_> { +/// &self.name +/// } +/// fn key3(&self) -> Self::K3<'_> { +/// self.tag +/// } +/// tri_upcast!(); +/// } +/// +/// let mut map = TriHashMap::new(); +/// map.insert_unique(Item { +/// id: 1, +/// name: "foo".to_string(), +/// tag: 'x', +/// value: 10, +/// }) +/// .unwrap(); +/// map.insert_unique(Item { +/// id: 2, +/// name: "bar".to_string(), +/// tag: 'y', +/// value: 20, +/// }) +/// .unwrap(); +/// +/// // A / A / A: all three keys point to the same item, so the entry is unique. +/// match map.entry(1, "foo", 'x') { +/// tri_hash_map::Entry::Occupied(entry) => { +/// assert!(entry.is_unique()); +/// assert_eq!(entry.get().as_unique().unwrap().value, 10); +/// } +/// tri_hash_map::Entry::Vacant(_) => panic!("should be occupied"), +/// } +/// +/// // None / None / None: no key is present, so the entry is vacant. +/// map.entry(3, "baz", 'z').or_insert(Item { +/// id: 3, +/// name: "baz".to_string(), +/// tag: 'z', +/// value: 30, +/// }); +/// assert_eq!(map.len(), 3); +/// +/// // A / A / None: partial hits are occupied non-unique entries. +/// let entry_ref = match map.entry(1, "foo", 'q') { +/// tri_hash_map::Entry::Occupied(entry) => { +/// assert!(entry.is_non_unique()); +/// entry.into_ref() +/// } +/// tri_hash_map::Entry::Vacant(_) => panic!("should be occupied"), +/// }; +/// assert_eq!(entry_ref.by_key1().unwrap().id, 1); +/// assert_eq!(entry_ref.by_key2().unwrap().id, 1); +/// assert_eq!(entry_ref.by_key3(), None); +/// +/// // A / A / B: mixed hits preserve per-key mapping and are not inserted into +/// // by or_insert. +/// let before = map.len(); +/// let mut entry_mut = map.entry(1, "foo", 'y').or_insert_with(|| { +/// panic!("occupied non-unique entries do not call the default") +/// }); +/// assert_eq!(entry_mut.by_key1().unwrap().id, 1); +/// assert_eq!(entry_mut.by_key2().unwrap().id, 1); +/// assert_eq!(entry_mut.by_key3().unwrap().id, 2); +/// drop(entry_mut); +/// assert_eq!(map.len(), before); +/// +/// // Replacement removes each distinct matched item once in first-key-hit +/// // order, then the entry becomes unique for the replacement. +/// match map.entry(1, "foo", 'y') { +/// tri_hash_map::Entry::Occupied(mut entry) => { +/// let removed = entry.insert(Item { +/// id: 1, +/// name: "foo".to_string(), +/// tag: 'y', +/// value: 99, +/// }); +/// assert_eq!( +/// removed.iter().map(|item| item.id).collect::>(), +/// vec![1, 2] +/// ); +/// assert!(entry.is_unique()); +/// } +/// tri_hash_map::Entry::Vacant(_) => panic!("should be occupied"), +/// } +/// # } +/// ``` pub enum Entry< 'a, T: TriHashItem, @@ -46,8 +190,8 @@ impl<'a, T: TriHashItem, S, A: Allocator> fmt::Debug for Entry<'a, T, S, A> { impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> Entry<'a, T, S, A> { - /// Provides in-place mutable access to occupied entries before any - /// potential inserts into the map. + /// Provides in-place mutable access to occupied entries before returning + /// the entry for further chaining. /// /// `F` is called once for each distinct entry that matches the provided /// keys, in first-key-hit order. Vacant entries are left unchanged. @@ -65,8 +209,16 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> } } - /// Ensures a value is in the entry by inserting `value` if vacant, and - /// returns mutable occupied access to the entry. + /// Ensures a value is in the entry by inserting `value` only if vacant, + /// and returns mutable occupied access to the entry. + /// + /// Partial and mixed occupied entries are not vacant, so this method does + /// not insert for states such as `A / A / None` or `A / A / B`. + /// + /// # Panics + /// + /// Panics before mutation if `value`'s key hashes differ from the hashes + /// of the keys passed to [`TriHashMap::entry`]. #[inline] pub fn or_insert(self, value: T) -> OccupiedEntryMut<'a, T, S> { match self { @@ -77,8 +229,15 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> } } - /// Ensures a value is in the entry by inserting the result of `default` if - /// vacant, and returns mutable occupied access to the entry. + /// Ensures a value is in the entry by inserting the result of `default` + /// only if vacant, and returns mutable occupied access to the entry. + /// + /// `default` is not called for unique, partial, or mixed occupied entries. + /// + /// # Panics + /// + /// Panics before mutation if the produced value's key hashes differ from + /// the hashes of the keys passed to [`TriHashMap::entry`]. #[inline] pub fn or_insert_with(self, default: F) -> OccupiedEntryMut<'a, T, S> where @@ -93,6 +252,9 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> } } /// A vacant entry. +/// +/// This is produced by [`TriHashMap::entry`] only when none of the three +/// provided keys match an existing item. pub struct VacantEntry< 'a, T: TriHashItem, @@ -125,6 +287,8 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> /// Sets the entry to a new value, returning a mutable reference to it. /// + /// Validation is performed before mutation. + /// /// # Panics /// /// Panics before mutation if any value key hashes differently from the @@ -140,7 +304,14 @@ impl<'a, T: TriHashItem, 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, and returns an `OccupiedEntry`. + /// Sets the entry to a new value, and returns a unique [`OccupiedEntry`]. + /// + /// Validation is performed before mutation. + /// + /// # Panics + /// + /// Panics before mutation if any value key hashes differently from the + /// corresponding key passed to [`TriHashMap::entry`]. #[inline] pub fn insert_entry(mut self, value: T) -> OccupiedEntry<'a, T, S, A> { let index = { @@ -185,6 +356,10 @@ fn validate_hashes( } /// A view into an occupied entry in a [`TriHashMap`]. +/// +/// An occupied entry exists whenever at least one of the three key lookups +/// matches. It is unique only when all three keys match the same item; partial +/// and mixed hits are non-unique. pub struct OccupiedEntry< 'a, T: TriHashItem, @@ -234,6 +409,9 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> } /// Returns shared references to values that match the provided keys. + /// + /// Non-unique shared access preserves per-key mapping. Multiple key + /// positions may return the same item. pub fn get(&self) -> OccupiedEntryRef<'_, T> { // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point, and @@ -243,6 +421,9 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> } /// Returns mutable references to values that match the provided keys. + /// + /// Non-unique mutable access is accessor-backed and reborrows each + /// distinct item sequentially, preventing aliased mutable references. pub fn get_mut(&mut self) -> OccupiedEntryMut<'_, T, S> { // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point, and @@ -251,7 +432,8 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> map.get_by_entry_index_mut(self.indexes) } - /// Converts self into shared references to items that match the provided keys. + /// Converts self into shared references to items that match the provided + /// keys. pub fn into_ref(self) -> OccupiedEntryRef<'a, T> { // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point, and @@ -260,7 +442,8 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> map.get_by_entry_index(self.indexes) } - /// Converts self into mutable references to items that match the provided keys. + /// Converts self into mutable references to items that match the provided + /// keys. pub fn into_mut(self) -> OccupiedEntryMut<'a, T, S> { // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point, and @@ -270,6 +453,10 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> } /// Removes all distinct values matched by this entry. + /// + /// Each distinct matched item is removed once. Returned items are ordered + /// by first key hit: for example `A / A / B` returns `[A, B]`, while + /// `None / B / A` returns `[B, A]`. pub fn remove(self) -> Vec { // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point. @@ -282,10 +469,14 @@ impl<'a, T: TriHashItem, S: Clone + BuildHasher, A: Allocator> /// Replaces all distinct values matched by this entry with `value`. /// + /// Each distinct matched item is replaced once. Removed items are returned + /// in first-key-hit order, and after success this entry is unique for the + /// replacement item. + /// /// # Panics /// /// Panics before mutation if `value` does not match the entry key hashes - /// or if its duplicate state is incompatible with this entry. + /// or if its duplicate/index state is incompatible with this entry. pub fn insert(&mut self, value: T) -> Vec { // SAFETY: The safety assumption behind `Self::new` guarantees that the // original reference to the map is no longer used at this point, and @@ -336,6 +527,9 @@ fn prepare_entry_removal< } /// Shared references to values matched by a [`TriHashMap`] occupied entry. +/// +/// The unique variant means all three keys matched one item. The non-unique +/// variant preserves per-key mapping for partial and mixed matches. #[derive(Debug)] pub enum OccupiedEntryRef<'a, T: TriHashItem> { /// All keys point to the same entry. @@ -345,6 +539,10 @@ pub enum OccupiedEntryRef<'a, T: TriHashItem> { } /// Accessor-backed shared non-unique entry references. +/// +/// This type stores each distinct matched item once, records which slot each +/// key position matched, and exposes the mapping through accessor methods. +/// `for_each` visits distinct items once in first-key-hit order. #[derive(Debug)] pub struct NonUniqueEntryRef<'a, T: TriHashItem> { values: [Option<&'a T>; 3], @@ -448,6 +646,9 @@ impl<'a, T: TriHashItem> NonUniqueEntryRef<'a, T> { } /// Mutable references to values matched by a [`TriHashMap`] occupied entry. +/// +/// Mutable non-unique access is accessor-backed. Accessors take `&mut self` and +/// reborrow one distinct item at a time. pub enum OccupiedEntryMut< 'a, T: TriHashItem, @@ -475,6 +676,11 @@ impl<'a, T: TriHashItem + fmt::Debug, S: Clone + BuildHasher> fmt::Debug } /// Accessor-backed mutable non-unique entry references. +/// +/// This type stores one mutable reference per distinct matched item and maps +/// key positions to those slots. It does not expose public `by_key1`, +/// `by_key2`, or `by_key3` fields, which prevents aliased mutable references +/// in states such as `A / A / B`. pub struct NonUniqueEntryMut< 'a, T: TriHashItem, diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index 7c174ab9..c985beed 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -2642,9 +2642,15 @@ impl TriHashMap { /// Gets the entry corresponding to the three provided keys. /// - /// A vacant entry is returned only when none of the keys are present. If all - /// three keys point to the same item, the occupied entry is unique. Partial - /// matches and mixed matches are occupied non-unique entries. + /// A vacant entry is returned only when none of `key1`, `key2`, or `key3` + /// is present. If all three keys point to the same item (`A / A / A`), the + /// occupied entry is unique. Partial hits (`A / A / None`, + /// `A / None / A`, `None / A / A`) and mixed hits (`A / A / B`, + /// `A / B / A`, `A / B / C`) are occupied non-unique entries. + /// + /// Non-unique entries preserve per-key mapping and visit distinct matched + /// items in first-key-hit order. See [`Entry`] for examples and the full + /// mutation semantics. pub fn entry<'a>( &'a mut self, key1: T::K1<'_>, diff --git a/crates/iddqd/tests/integration/pathological.rs b/crates/iddqd/tests/integration/pathological.rs index fc3a1325..d5b95749 100644 --- a/crates/iddqd/tests/integration/pathological.rs +++ b/crates/iddqd/tests/integration/pathological.rs @@ -384,7 +384,7 @@ struct LyingEqItem { id: u32, } -#[expect(clippy::derived_hash_with_manual_eq)] +#[allow(clippy::derived_hash_with_manual_eq)] #[derive(Hash)] struct LyingEqKey { id: u32, diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index be4edb15..66d2e86f 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -1664,20 +1664,21 @@ fn entry_insert_replaces_first_key_hit_order_and_becomes_unique() { ((9, 'b', 1), vec!["B", "A"]), ] { let mut map = entry_mut_map(); - let mut entry = match map.entry(keys.0, keys.1, keys.2) { - tri_hash_map::Entry::Occupied(entry) => entry, - tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), - }; - let removed = entry.insert(EntryMutItem { - key1: keys.0, - key2: keys.1, - key3: keys.2, - value: "R", - }); - assert_eq!(removed_values(removed), expected); - assert!(entry.is_unique()); - assert_eq!(entry.get().as_unique().unwrap().value, "R"); - drop(entry); + { + let mut entry = match map.entry(keys.0, keys.1, keys.2) { + tri_hash_map::Entry::Occupied(entry) => entry, + tri_hash_map::Entry::Vacant(_) => panic!("expected occupied"), + }; + let removed = entry.insert(EntryMutItem { + key1: keys.0, + key2: keys.1, + key3: keys.2, + value: "R", + }); + assert_eq!(removed_values(removed), expected); + assert!(entry.is_unique()); + assert_eq!(entry.get().as_unique().unwrap().value, "R"); + } assert_eq!(map.get1(&keys.0).unwrap().value, "R"); assert_eq!(map.get2(&keys.1).unwrap().value, "R"); assert_eq!(map.get3(&keys.2).unwrap().value, "R"); From 309ca104e2b0d3ff7bdc8a2ea2b347d1fd59dcc7 Mon Sep 17 00:00:00 2001 From: SG-devel <78623477+SG-devel@users.noreply.github.com> Date: Sat, 20 Jun 2026 13:00:42 +0200 Subject: [PATCH 08/10] Clean up TriHashMap entry debug output Remove a stale dead-code helper from support::entry and make TriHashMap non-unique entry Debug output use the public per-key view instead of internal slot-mapping fields. --- crates/iddqd/src/support/entry.rs | 9 -------- crates/iddqd/src/tri_hash_map/entry.rs | 30 ++++++++++++++++++++------ 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/crates/iddqd/src/support/entry.rs b/crates/iddqd/src/support/entry.rs index 4aee9091..4380ed3b 100644 --- a/crates/iddqd/src/support/entry.rs +++ b/crates/iddqd/src/support/entry.rs @@ -46,15 +46,6 @@ impl EntryIndexes { Self { indexes } } - #[inline] - #[expect( - dead_code, - reason = "reserved for upcoming TriHashMap occupied entry replacement validation" - )] - pub(crate) const fn indexes(&self) -> &[Option; N] { - &self.indexes - } - #[inline] pub(crate) fn classify(self) -> EntryLookup { let mut first = None; diff --git a/crates/iddqd/src/tri_hash_map/entry.rs b/crates/iddqd/src/tri_hash_map/entry.rs index 6a508b69..66c6b203 100644 --- a/crates/iddqd/src/tri_hash_map/entry.rs +++ b/crates/iddqd/src/tri_hash_map/entry.rs @@ -543,13 +543,22 @@ pub enum OccupiedEntryRef<'a, T: TriHashItem> { /// This type stores each distinct matched item once, records which slot each /// key position matched, and exposes the mapping through accessor methods. /// `for_each` visits distinct items once in first-key-hit order. -#[derive(Debug)] pub struct NonUniqueEntryRef<'a, T: TriHashItem> { values: [Option<&'a T>; 3], len: usize, key_to_slot: [Option; 3], } +impl<'a, T: TriHashItem + fmt::Debug> fmt::Debug for NonUniqueEntryRef<'a, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("NonUniqueEntryRef") + .field("by_key1", &self.by_key1()) + .field("by_key2", &self.by_key2()) + .field("by_key3", &self.by_key3()) + .finish() + } +} + impl<'a, T: TriHashItem> OccupiedEntryRef<'a, T> { /// Returns true if all three keys point to exactly one item. #[inline] @@ -691,14 +700,23 @@ pub struct NonUniqueEntryMut< key_to_slot: [Option; 3], } -impl<'a, T: TriHashItem + fmt::Debug, S: Clone + BuildHasher> fmt::Debug - for NonUniqueEntryMut<'a, T, S> +impl<'a, T: TriHashItem, S: Clone + BuildHasher> NonUniqueEntryMut<'a, T, S> { + #[inline] + fn fmt_by_key(&self, key: usize) -> Option<&RefMut<'a, T, S>> { + self.key_to_slot[key].and_then(|slot| self.refs[slot].as_ref()) + } +} + +impl<'a, T, S> fmt::Debug for NonUniqueEntryMut<'a, T, S> +where + T: TriHashItem + fmt::Debug, + S: Clone + BuildHasher, { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("NonUniqueEntryMut") - .field("refs", &self.refs) - .field("len", &self.len) - .field("key_to_slot", &self.key_to_slot) + .field("by_key1", &self.fmt_by_key(0)) + .field("by_key2", &self.fmt_by_key(1)) + .field("by_key3", &self.fmt_by_key(2)) .finish() } } From dc011a374f3cd74e40dcd6dedf9886cc55577503 Mon Sep 17 00:00:00 2001 From: SG-devel <78623477+SG-devel@users.noreply.github.com> Date: Sat, 20 Jun 2026 13:21:04 +0200 Subject: [PATCH 09/10] Test TriHashMap entry repeated non-unique mapping --- .../iddqd/tests/integration/tri_hash_map.rs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index 66d2e86f..f2273ec1 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -204,6 +204,38 @@ fn test_insert_unique() { assert_eq!(map.remove_unique(&v5.key1(), &v5.key2(), &v5.key3()), Some(v5)); } +#[test] +fn entry_non_unique_preserves_repeated_key_mapping() { + let mut map = TriHashMap::::make_new(); + + let a = TestItem::new(1, 'a', "x", "a"); + let b = TestItem::new(2, 'b', "y", "b"); + + map.insert_unique(a.clone()).unwrap(); + map.insert_unique(b.clone()).unwrap(); + + // A / B / A: key1 and key3 both match `a`, while key2 matches `b`. + let entry_ref = match map.entry( + TestKey1::new(&a.key1), + TestKey2::new(b.key2), + TestKey3::new(&a.key3), + ) { + tri_hash_map::Entry::Occupied(entry) => { + assert!(entry.is_non_unique()); + entry.into_ref() + } + tri_hash_map::Entry::Vacant(_) => panic!("entry should be occupied"), + }; + + assert_eq!(entry_ref.by_key1(), Some(&a)); + assert_eq!(entry_ref.by_key2(), Some(&b)); + assert_eq!(entry_ref.by_key3(), Some(&a)); + + let mut visited = Vec::new(); + entry_ref.for_each(|item| visited.push(item.value.as_str())); + assert_eq!(visited, vec!["a", "b"]); +} + // Test that the unsafe block within RefMut doesn't trip up miri. #[test] fn test_ref_mut_aliasing() { From 56359a0c4f7eef47b363b8f410736f4ffdd2320c Mon Sep 17 00:00:00 2001 From: SG-devel <78623477+SG-devel@users.noreply.github.com> Date: Sun, 21 Jun 2026 01:17:16 +0200 Subject: [PATCH 10/10] Add TRI entry API property coverage Add model-based property tests for TriHashMap entry insertion and removal, matching the BiHashMap entry API coverage while preserving TRI's first-key-hit removal order. Extend the naive test oracle with TRI entry insert/remove behavior, including partial and mixed occupied entries. Also cover vacant insert_entry hash mismatch panics, exact unique occupied entry insert/remove cases, and or_insert_with laziness for unique and repeated mixed entries. Tighten TRI entry documentation around hash validation and improve entry-related panic messages. --- crates/iddqd-test-utils/src/naive_map.rs | 78 ++++++++++++ crates/iddqd/src/tri_hash_map/entry.rs | 5 +- crates/iddqd/src/tri_hash_map/imp.rs | 2 +- .../iddqd/tests/integration/tri_hash_map.rs | 119 +++++++++++++++++- 4 files changed, 197 insertions(+), 7 deletions(-) diff --git a/crates/iddqd-test-utils/src/naive_map.rs b/crates/iddqd-test-utils/src/naive_map.rs index 4d695879..48c69bf7 100644 --- a/crates/iddqd-test-utils/src/naive_map.rs +++ b/crates/iddqd-test-utils/src/naive_map.rs @@ -190,6 +190,84 @@ impl NaiveMap { Some(self.items.remove(index)) } + /// Removes and returns every item covered by a `TriHashMap` entry keyed on + /// `(key1, key2, key3)`, i.e. every distinct item matching `key1`, `key2`, + /// or `key3`. + /// + /// Mirrors `tri_hash_map::OccupiedEntry::remove`. The returned items are in + /// first-key-hit order. + pub fn entry_remove123( + &mut self, + key1: u8, + key2: char, + key3: &str, + ) -> Vec { + fn push_unique(indexes: &mut Vec, index: Option) { + if let Some(index) = index { + if !indexes.contains(&index) { + indexes.push(index); + } + } + } + + let mut key_order = Vec::new(); + + push_unique( + &mut key_order, + self.items.iter().position(|e| e.key1 == key1), + ); + push_unique( + &mut key_order, + self.items.iter().position(|e| e.key2 == key2), + ); + push_unique( + &mut key_order, + self.items.iter().position(|e| e.key3 == key3), + ); + + let mut remove_order = key_order.clone(); + remove_order.sort_unstable_by(|a, b| b.cmp(a)); + + let mut removed = Vec::with_capacity(remove_order.len()); + for index in remove_order { + removed.push((index, self.items.remove(index))); + } + + key_order + .into_iter() + .map(|index| { + let removed_index = removed + .iter() + .position(|(removed_index, _)| *removed_index == index) + .expect("index was removed"); + removed.swap_remove(removed_index).1 + }) + .collect() + } + + /// Mirrors the test harness behavior for + /// `TriHashMap::entry(...).Occupied(...).insert(...)`. + /// + /// Returns `None` for a vacant entry and does not insert. Returns + /// `Some(removed)` for an occupied entry, where `removed` is in + /// first-key-hit order. + pub fn entry_insert_overwrite123( + &mut self, + item: TestItem, + ) -> Option> { + let occupied = self.get1(item.key1).is_some() + || self.get2(item.key2).is_some() + || self.get3(&item.key3).is_some(); + + if !occupied { + return None; + } + + let removed = self.entry_remove123(item.key1, item.key2, &item.key3); + self.items.push(item); + Some(removed) + } + pub fn iter(&self) -> impl Iterator { self.items.iter() } diff --git a/crates/iddqd/src/tri_hash_map/entry.rs b/crates/iddqd/src/tri_hash_map/entry.rs index 66c6b203..dda6bd1c 100644 --- a/crates/iddqd/src/tri_hash_map/entry.rs +++ b/crates/iddqd/src/tri_hash_map/entry.rs @@ -23,7 +23,8 @@ use core::{fmt, hash::BuildHasher}; /// /// [`VacantEntry`] is returned only when none of `key1`, `key2`, or `key3` /// passed to [`TriHashMap::entry`] matches an item. [`VacantEntry::insert`] and -/// [`VacantEntry::insert_entry`] insert the item for those three keys. +/// [`VacantEntry::insert_entry`] insert only after checking that the inserted +/// value's key hashes match the hashes of those three entry keys. /// /// [`OccupiedEntry`] is returned whenever at least one key matches. It is /// unique only when all three key positions hit the same item (`A / A / A`). @@ -505,7 +506,7 @@ fn validate_prepared_indexes( EntryIndexes::NonUnique(indexes) => *indexes.indexes(), }; if prepared != expected { - panic!("replacement duplicate state does not match entry lookup"); + panic!("replacement item keys do not match this occupied entry"); } } diff --git a/crates/iddqd/src/tri_hash_map/imp.rs b/crates/iddqd/src/tri_hash_map/imp.rs index c985beed..821a6863 100644 --- a/crates/iddqd/src/tri_hash_map/imp.rs +++ b/crates/iddqd/src/tri_hash_map/imp.rs @@ -1515,7 +1515,7 @@ impl TriHashMap { self.try_reserve_insert_overwrite_commit( prepared.needs_new_item_slot(), ) - .expect("reserved capacity for entry replacement commit"); + .expect("reserved capacity for insert_overwrite commit"); self.commit_insert_overwrite(value, prepared, &mut duplicates); diff --git a/crates/iddqd/tests/integration/tri_hash_map.rs b/crates/iddqd/tests/integration/tri_hash_map.rs index f2273ec1..4eec8a92 100644 --- a/crates/iddqd/tests/integration/tri_hash_map.rs +++ b/crates/iddqd/tests/integration/tri_hash_map.rs @@ -345,6 +345,25 @@ impl UniqueKeysOp { } } +/// A replacement operation for `TriHashMap::entry(...).insert(...)`. +/// +/// The entry keys are resolved the same way as `UniqueKeysOp`, which makes +/// partial and mixed occupied entries common. The replacement value is then +/// constructed from those exact entry keys so `OccupiedEntry::insert` is testing +/// compatible replacement, not panic behavior. +#[derive(Clone, Debug, Arbitrary)] +struct EntryInsertOverwriteOp { + keys: UniqueKeysOp, + value: String, +} + +impl EntryInsertOverwriteOp { + fn resolve(&self, naive_map: &NaiveMap) -> TestItem { + let (key1, key2, key3) = self.keys.resolve(naive_map); + TestItem::new(key1, key2, key3, self.value.clone()) + } +} + #[derive(Debug, Arbitrary)] enum Operation { // Make inserts a bit more common to try and fill up the map. @@ -353,6 +372,10 @@ enum Operation { #[weight(3)] InsertOverwrite(TestItem), #[weight(2)] + EntryInsertOverwrite(EntryInsertOverwriteOp), + #[weight(2)] + EntryRemove(UniqueKeysOp), + #[weight(2)] Get1(u8), #[weight(2)] Get2(char), @@ -403,6 +426,8 @@ impl Operation { // The act of removing items, including calls to insert_overwrite, // can make the map non-compact. Operation::InsertOverwrite(_) + | Operation::EntryInsertOverwrite(_) + | Operation::EntryRemove(_) | Operation::Remove1(_) | Operation::Remove2(_) | Operation::Remove3(_) @@ -471,6 +496,48 @@ fn proptest_ops( ); map.validate(compactness).expect("map should be valid"); } + Operation::EntryInsertOverwrite(op) => { + let item = op.resolve(&naive_map); + + let map_res = match map.entry( + TestKey1::new(&item.key1), + TestKey2::new(item.key2), + TestKey3::new(&item.key3), + ) { + tri_hash_map::Entry::Occupied(mut entry) => { + Some(entry.insert(item.clone())) + } + tri_hash_map::Entry::Vacant(_) => None, + }; + + let naive_res = naive_map.entry_insert_overwrite123(item); + + assert_eq!( + map_res, naive_res, + "map and naive map should agree on Entry::insert removed items" + ); + map.validate(compactness).expect("map should be valid"); + } + Operation::EntryRemove(keys) => { + let (key1, key2, key3) = keys.resolve(&naive_map); + + let map_res = match map.entry( + TestKey1::new(&key1), + TestKey2::new(key2), + TestKey3::new(&key3), + ) { + tri_hash_map::Entry::Occupied(entry) => entry.remove(), + tri_hash_map::Entry::Vacant(_) => Vec::new(), + }; + + let naive_res = naive_map.entry_remove123(key1, key2, &key3); + + assert_eq!( + map_res, naive_res, + "map and naive map should agree on Entry::remove items" + ); + map.validate(compactness).expect("map should be valid"); + } Operation::Get1(key1) => { let map_res = map.get1(&TestKey1::new(&key1)); let naive_res = naive_map.get1(key1); @@ -1502,6 +1569,17 @@ fn entry_vacant_insert_panics_on_mismatched_key1() { entry.insert(SimpleItem { key1: 2, key2: 'a', key3: 1 }); } +#[test] +#[should_panic(expected = "key1 hashes do not match")] +fn entry_vacant_insert_entry_panics_on_mismatched_key1() { + let mut map = TriHashMap::::make_new(); + let entry = match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => entry, + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + }; + entry.insert_entry(SimpleItem { key1: 2, key2: 'a', key3: 1 }); +} + #[test] #[should_panic(expected = "key2 hashes do not match")] fn entry_vacant_insert_panics_on_mismatched_key2() { @@ -1513,6 +1591,17 @@ fn entry_vacant_insert_panics_on_mismatched_key2() { entry.insert(SimpleItem { key1: 1, key2: 'b', key3: 1 }); } +#[test] +#[should_panic(expected = "key2 hashes do not match")] +fn entry_vacant_insert_entry_panics_on_mismatched_key2() { + let mut map = TriHashMap::::make_new(); + let entry = match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => entry, + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + }; + entry.insert_entry(SimpleItem { key1: 1, key2: 'b', key3: 1 }); +} + #[test] #[should_panic(expected = "key3 hashes do not match")] fn entry_vacant_insert_panics_on_mismatched_key3() { @@ -1524,6 +1613,17 @@ fn entry_vacant_insert_panics_on_mismatched_key3() { entry.insert(SimpleItem { key1: 1, key2: 'a', key3: 2 }); } +#[test] +#[should_panic(expected = "key3 hashes do not match")] +fn entry_vacant_insert_entry_panics_on_mismatched_key3() { + let mut map = TriHashMap::::make_new(); + let entry = match map.entry(1, 'a', 1) { + tri_hash_map::Entry::Vacant(entry) => entry, + tri_hash_map::Entry::Occupied(_) => panic!("expected vacant"), + }; + entry.insert_entry(SimpleItem { key1: 1, key2: 'a', key3: 2 }); +} + #[derive(Clone, Debug)] struct EntryMutItem { key1: u32, @@ -1666,6 +1766,7 @@ fn removed_values(items: Vec) -> Vec<&'static str> { #[test] fn entry_remove_returns_first_key_hit_order_and_removes_once() { for (keys, expected) in [ + ((1, 'a', 1), vec!["A"]), ((1, 'a', 9), vec!["A"]), ((1, 'z', 1), vec!["A"]), ((9, 'a', 1), vec!["A"]), @@ -1687,6 +1788,7 @@ fn entry_remove_returns_first_key_hit_order_and_removes_once() { #[test] fn entry_insert_replaces_first_key_hit_order_and_becomes_unique() { for (keys, expected) in [ + ((1, 'a', 1), vec!["A"]), ((1, 'a', 9), vec!["A"]), ((1, 'z', 1), vec!["A"]), ((9, 'a', 1), vec!["A"]), @@ -1731,14 +1833,23 @@ fn entry_or_insert_only_inserts_vacant_and_or_insert_with_is_lazy() { drop(inserted); assert_eq!(map.len(), 4); - for keys in - [(1, 'a', 9), (1, 'z', 1), (9, 'a', 1), (1, 'a', 2), (1, 'b', 3)] - { + for (keys, is_unique) in [ + ((1, 'a', 1), true), + ((1, 'a', 9), false), + ((1, 'z', 1), false), + ((9, 'a', 1), false), + ((1, 'a', 2), false), + ((1, 'b', 1), false), + ((1, 'b', 3), false), + ] { let before = map.len(); let view = map.entry(keys.0, keys.1, keys.2).or_insert_with(|| { panic!("or_insert_with called for occupied entry") }); - assert!(view.is_non_unique()); + + assert_eq!(view.is_unique(), is_unique); + assert_eq!(view.is_non_unique(), !is_unique); + drop(view); assert_eq!(map.len(), before); }