From 4bbbc28d5de85c412ba7e80b0b7a70a7af278e69 Mon Sep 17 00:00:00 2001 From: elftausend <76885970+elftausend@users.noreply.github.com> Date: Fri, 14 Jul 2023 10:29:38 +0200 Subject: [PATCH 1/3] Add basic key exchange detection --- src/basic.rs | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++-- src/lib.rs | 60 ++++++++++++++++++++++++++++++-- 2 files changed, 153 insertions(+), 5 deletions(-) diff --git a/src/basic.rs b/src/basic.rs index 4811f57..64043bd 100644 --- a/src/basic.rs +++ b/src/basic.rs @@ -13,6 +13,9 @@ use core::marker::PhantomData; use core::mem::{ManuallyDrop, MaybeUninit}; use core::ops::{Index, IndexMut}; +#[cfg(debug_assertions)] +use core::panic::Location; + use crate::util::{Never, UnwrapUnchecked}; use crate::{DefaultKey, Key, KeyData}; @@ -132,6 +135,8 @@ pub struct SlotMap { free_head: u32, num_elems: u32, _k: PhantomData K>, + #[cfg(debug_assertions)] + unique_location: &'static Location<'static> } impl SlotMap { @@ -143,6 +148,7 @@ impl SlotMap { /// # use slotmap::*; /// let mut sm: SlotMap<_, i32> = SlotMap::new(); /// ``` + #[cfg_attr(debug_assertions, track_caller)] pub fn new() -> Self { Self::with_capacity_and_key(0) } @@ -158,6 +164,7 @@ impl SlotMap { /// # use slotmap::*; /// let mut sm: SlotMap<_, i32> = SlotMap::with_capacity(10); /// ``` + #[cfg_attr(debug_assertions, track_caller)] pub fn with_capacity(capacity: usize) -> Self { Self::with_capacity_and_key(capacity) } @@ -175,6 +182,7 @@ impl SlotMap { /// } /// let mut positions: SlotMap = SlotMap::with_key(); /// ``` + #[cfg_attr(debug_assertions, track_caller)] pub fn with_key() -> Self { Self::with_capacity_and_key(0) } @@ -197,6 +205,7 @@ impl SlotMap { /// let good_day = messages.insert("Good day"); /// let hello = messages.insert("Hello"); /// ``` + #[cfg_attr(debug_assertions, track_caller)] pub fn with_capacity_and_key(capacity: usize) -> Self { // Create slots with a sentinel at index 0. // We don't actually use the sentinel for anything currently, but @@ -213,6 +222,8 @@ impl SlotMap { free_head: 1, num_elems: 0, _k: PhantomData, + #[cfg(debug_assertions)] + unique_location: Location::caller() } } @@ -417,7 +428,7 @@ impl SlotMap { slot.version = occupied_version; } self.num_elems = new_num_elems; - return Ok(kd.into()); + return Ok(crate::new_key!(kd, self)); } let version = 1; @@ -426,14 +437,15 @@ impl SlotMap { // Create new slot before adjusting freelist in case f or the allocation panics or errors. self.slots.push(Slot { u: SlotUnion { - value: ManuallyDrop::new(f(kd.into())?), + value: ManuallyDrop::new(f(crate::new_key!(kd, self))?), }, version, }); self.free_head = kd.idx + 1; self.num_elems = new_num_elems; - Ok(kd.into()) + + Ok(crate::new_key!(kd, self)) } // Helper function to remove a value from a slot. Safe iff the slot is @@ -583,6 +595,9 @@ impl SlotMap { /// assert_eq!(sm.get(key), None); /// ``` pub fn get(&self, key: K) -> Option<&V> { + #[cfg(debug_assertions)] + debug_assert_eq!(key.location(), self.unique_location, "A key created by a specific SlotMap is used for another SlotMap."); + let kd = key.data(); self.slots .get(kd.idx as usize) @@ -1538,4 +1553,81 @@ mod tests { de.insert(2); assert_eq!(de.len(), 3); } + + #[cfg(debug_assertions)] + pub struct Map { + slot_map: SlotMap + } + + #[cfg(debug_assertions)] + impl Map { + fn new() -> Self { + Map { + slot_map: SlotMap::new() + } + } + + #[track_caller] + fn new_tracked() -> Self { + Map { + slot_map: SlotMap::new() + } + } + } + + #[cfg(debug_assertions)] + #[test] + fn wrapped_slotmap_type_untracked_key_exchange() { + let mut map1 = Map::new(); + let k1 = map1.slot_map.insert(4); + + let mut map2 = Map::new(); + + map2.slot_map.insert(4); + + map2.slot_map.get(k1).unwrap(); + } + + #[cfg(debug_assertions)] + #[should_panic] + #[test] + fn wrapped_slotmap_type_tracked_key_exchange() { + let mut map1 = Map::new_tracked(); + let k1 = map1.slot_map.insert(4); + + let mut map2 = Map::new_tracked(); + + map2.slot_map.insert(4); + + map2.slot_map.get(k1); + } + + #[cfg(debug_assertions)] + #[test] + fn slotmap_key_own_map() { + let mut sm1 = SlotMap::new(); + + let k1 = sm1.insert(5); + + + let mut sm2 = SlotMap::new(); + sm2.insert(3); + + sm1.get(k1).unwrap(); + } + + #[cfg(debug_assertions)] + #[should_panic] + #[test] + fn slotmap_key_exchange() { + let mut sm1 = SlotMap::new(); + + let k1 = sm1.insert(5); + + + let mut sm2 = SlotMap::new(); + sm2.insert(3); + + sm2.get(k1); + } } diff --git a/src/lib.rs b/src/lib.rs index 2eeb76c..9b05e89 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -408,8 +408,17 @@ pub unsafe trait Key: /// assert_eq!(dk.data(), mk.data()); /// ``` fn data(&self) -> KeyData; + + #[cfg(debug_assertions)] + /// Sets the location in the source code of a used slot map. + fn set_location(&mut self, location: &'static core::panic::Location<'static>); + + #[cfg(debug_assertions)] + /// Returns the location in the source code of a used slot map. + fn location(&self) -> &'static core::panic::Location<'static>; } + /// A helper macro to create new key types. If you use a new key type for each /// slot map you create you can entirely prevent using the wrong key on the /// wrong slot map. @@ -456,21 +465,48 @@ macro_rules! new_key_type { #[derive(Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] + + #[cfg(not(debug_assertions))] #[repr(transparent)] $vis struct $name($crate::KeyData); + #[derive(Copy, Clone, Default, + Eq, PartialEq, Ord, PartialOrd, + Hash, Debug)] + #[cfg(debug_assertions)] + $(#[$outer])* + $vis struct $name { + key_data: $crate::KeyData, + location: Option<&'static core::panic::Location<'static>> + } + impl $crate::__impl::From<$crate::KeyData> for $name { fn from(k: $crate::KeyData) -> Self { - $name(k) + $name { + key_data: k, + location: Some(core::panic::Location::caller()) + } } } unsafe impl $crate::Key for $name { fn data(&self) -> $crate::KeyData { - self.0 + self.key_data + } + + #[cfg(debug_assertions)] + #[inline] + fn set_location(&mut self, location: &'static core::panic::Location<'static>) { + self.location = Some(location); + } + #[cfg(debug_assertions)] + #[inline] + fn location(&self) -> &'static core::panic::Location<'static> { + self.location.expect("Should be set during key instantiation while in debug mode") } } + $crate::__serialize_key!($name); $crate::new_key_type!($($rest)*); @@ -518,6 +554,26 @@ new_key_type! { pub struct DefaultKey; } +#[macro_export] +/// TODO +macro_rules! new_key { + ($kd:ident, $self:ident) => { + { + let mut key: K; + #[cfg(debug_assertions)] + { + key = $kd.into(); + key.set_location($self.unique_location) + } + #[cfg(not(debug_assertions))] + { + key = kd.into() + } + key + } + }; +} + // Serialization with serde. #[cfg(feature = "serde")] mod serialize { From 3249e7400a464d374de8cdc3ff9f86f5752de22b Mon Sep 17 00:00:00 2001 From: elftausend <76885970+elftausend@users.noreply.github.com> Date: Fri, 14 Jul 2023 18:54:08 +0200 Subject: [PATCH 2/3] Fix failing tests by adapting the PartialEq and Hash impl --- src/basic.rs | 25 ++++++++++++++++++++++--- src/lib.rs | 21 ++++++++++++++++++--- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/basic.rs b/src/basic.rs index 64043bd..1385f41 100644 --- a/src/basic.rs +++ b/src/basic.rs @@ -17,7 +17,7 @@ use core::ops::{Index, IndexMut}; use core::panic::Location; use crate::util::{Never, UnwrapUnchecked}; -use crate::{DefaultKey, Key, KeyData}; +use crate::{DefaultKey, Key, KeyData, new_key}; // Storage inside a slot or metadata for the freelist when vacant. union SlotUnion { @@ -419,7 +419,7 @@ impl SlotMap { let kd = KeyData::new(self.free_head, occupied_version); // Get value first in case f panics or returns an error. - let value = f(kd.into())?; + let value = f(new_key!(kd, self))?; // Update. unsafe { @@ -704,7 +704,7 @@ impl SlotMap { let mut i = 0; while i < N { let kd = keys[i].data(); - if !self.contains_key(kd.into()) { + if !self.contains_key(new_key!(kd, self)) { break; } @@ -1630,4 +1630,23 @@ mod tests { sm2.get(k1); } + + #[cfg(debug_assertions)] + #[test] + fn slotmap_key_iter_mut() { + let mut sm1 = SlotMap::new(); + let k1 = sm1.insert(10); + let k2 = sm1.insert(20); + let k3 = sm1.insert(30); + + for (k, v) in sm1.iter_mut() { + println!("k: {k:?}"); + if k != k2 { + *v *= -1 + } + } + assert_eq!(sm1[k1], -10); + assert_eq!(sm1[k2], 20); + assert_eq!(sm1[k3], -30); + } } diff --git a/src/lib.rs b/src/lib.rs index 9b05e89..a68db15 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -471,20 +471,35 @@ macro_rules! new_key_type { $vis struct $name($crate::KeyData); #[derive(Copy, Clone, Default, - Eq, PartialEq, Ord, PartialOrd, - Hash, Debug)] + Eq, Ord, PartialOrd, Debug)] #[cfg(debug_assertions)] $(#[$outer])* $vis struct $name { key_data: $crate::KeyData, location: Option<&'static core::panic::Location<'static>> + } + + #[cfg(debug_assertions)] + impl PartialEq for $name { + #[inline] + fn eq(&self, other: &$name) -> bool { + self.key_data == other.key_data + } + } + + #[cfg(debug_assertions)] + impl core::hash::Hash for $name { + #[inline] + fn hash(&self, state: &mut H) { + self.key_data.hash(state); + } } impl $crate::__impl::From<$crate::KeyData> for $name { fn from(k: $crate::KeyData) -> Self { $name { key_data: k, - location: Some(core::panic::Location::caller()) + location: None } } } From 3de4b07117bd4291db8f849896a3c15eecd5d908 Mon Sep 17 00:00:00 2001 From: elftausend <76885970+elftausend@users.noreply.github.com> Date: Fri, 14 Jul 2023 21:32:16 +0200 Subject: [PATCH 3/3] Fix building in release mode --- src/lib.rs | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a68db15..e1a57fe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -497,16 +497,30 @@ macro_rules! new_key_type { impl $crate::__impl::From<$crate::KeyData> for $name { fn from(k: $crate::KeyData) -> Self { - $name { - key_data: k, - location: None + #[cfg(debug_assertions)] + { + $name { + key_data: k, + location: None + } + } + #[cfg(not(debug_assertions))] + { + $name(k) } } } unsafe impl $crate::Key for $name { fn data(&self) -> $crate::KeyData { - self.key_data + #[cfg(debug_assertions)] + { + self.key_data + } + #[cfg(not(debug_assertions))] + { + self.0 + } } #[cfg(debug_assertions)] @@ -574,17 +588,18 @@ new_key_type! { macro_rules! new_key { ($kd:ident, $self:ident) => { { - let mut key: K; + #[cfg(debug_assertions)] { - key = $kd.into(); - key.set_location($self.unique_location) + let mut key: K = $kd.into(); + key.set_location($self.unique_location); + key } #[cfg(not(debug_assertions))] { - key = kd.into() + Into::::into($kd) } - key + } }; }