From 33b3b93d10a95bbcc475a500c2132b6d8a9e16fd Mon Sep 17 00:00:00 2001 From: Wilco Kusee Date: Sun, 23 Mar 2025 08:32:36 +0100 Subject: [PATCH 1/2] Document most safety blocks, allow others with todo --- components/salsa-macros/src/db.rs | 2 +- src/database_impl.rs | 5 +---- src/function.rs | 12 ++++++++---- src/function/delete.rs | 3 +++ src/function/fetch.rs | 8 ++++---- src/function/memo.rs | 2 ++ src/interned.rs | 2 ++ src/lib.rs | 1 + src/revision.rs | 4 ++-- src/storage.rs | 1 + src/table.rs | 3 ++- src/table/memo.rs | 6 +++--- src/tracked_struct.rs | 2 ++ src/update.rs | 7 ++++++- src/views.rs | 1 + src/zalsa.rs | 3 +++ 16 files changed, 42 insertions(+), 20 deletions(-) diff --git a/components/salsa-macros/src/db.rs b/components/salsa-macros/src/db.rs index 6de2d3b79..d83982758 100644 --- a/components/salsa-macros/src/db.rs +++ b/components/salsa-macros/src/db.rs @@ -143,7 +143,7 @@ impl DbMacro { #[inline(always)] unsafe fn downcast(db: &dyn salsa::plumbing::Database) -> &dyn #TraitPath where Self: Sized { debug_assert_eq!(db.type_id(), ::core::any::TypeId::of::()); - // SAFETY: Same as the safety of the `downcast` method. + // SAFETY: The input database must be of type `Self`. unsafe { &*salsa::plumbing::transmute_data_ptr::(db) } } }); diff --git a/src/database_impl.rs b/src/database_impl.rs index dfea83f5a..71f8cc17c 100644 --- a/src/database_impl.rs +++ b/src/database_impl.rs @@ -25,10 +25,7 @@ impl Database for DatabaseImpl { } } -// # Safety -// -// The `storage` and `storage_mut` fields return a reference to the same -// storage field owned by `self`. +// SAFETY: The `storage` and `storage_mut` fields return a reference to the same storage field owned by `self`. unsafe impl HasStorage for DatabaseImpl { fn storage(&self) -> &Storage { &self.storage diff --git a/src/function.rs b/src/function.rs index 38338820e..e8d548b8b 100644 --- a/src/function.rs +++ b/src/function.rs @@ -179,6 +179,7 @@ where &'this self, memo: &memo::Memo>, ) -> &'this memo::Memo> { + // SAFETY: the caller must guarantee that the memo will not be released before `&self` unsafe { std::mem::transmute(memo) } } @@ -191,16 +192,19 @@ where ) -> &'db memo::Memo> { // We convert to a `NonNull` here as soon as possible because we are going to alias // into the `Box`, which is a `noalias` type. + // SAFETY: memo is not null let memo = unsafe { NonNull::new_unchecked(Box::into_raw(Box::new(memo))) }; - // Unsafety conditions: memo must be in the map (it's not yet, but it will be by the time this + // SAFETY: memo must be in the map (it's not yet, but it will be by the time this // value is returned) and anything removed from map is added to deleted entries (ensured elsewhere). let db_memo = unsafe { self.extend_memo_lifetime(memo.as_ref()) }; - // Safety: We delay the drop of `old_value` until a new revision starts which ensures no - // references will exist for the memo contents. if let Some(old_value) = - unsafe { self.insert_memo_into_table_for(zalsa, id, memo, memo_ingredient_index) } + // SAFETY: We delay the drop of `old_value` until a new revision starts which ensures no + // references will exist for the memo contents. + unsafe { + self.insert_memo_into_table_for(zalsa, id, memo, memo_ingredient_index) + } { // In case there is a reference to the old memo out there, we have to store it // in the deleted entries. This will get cleared when a new revision starts. diff --git a/src/function/delete.rs b/src/function/delete.rs index 8206dd2f0..cca1b884e 100644 --- a/src/function/delete.rs +++ b/src/function/delete.rs @@ -10,7 +10,9 @@ pub(super) struct DeletedEntries { memos: boxcar::Vec>>>, } +#[allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety unsafe impl Send for SharedBox {} +#[allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety unsafe impl Sync for SharedBox {} impl Default for DeletedEntries { @@ -26,6 +28,7 @@ impl DeletedEntries { /// /// The memo must be valid and safe to free when the `DeletedEntries` list is cleared or dropped. pub(super) unsafe fn push(&self, memo: NonNull>>) { + // Safety: The memo must be valid and safe to free when the `DeletedEntries` list is cleared or dropped. let memo = unsafe { std::mem::transmute::>>, NonNull>>>( memo, diff --git a/src/function/fetch.rs b/src/function/fetch.rs index ac5cd8b35..c00d86640 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -18,13 +18,13 @@ where zalsa.unwind_if_revision_cancelled(db); let memo = self.refresh_memo(db, id); - // SAFETY: We just refreshed the memo so it is guaranteed to contain a value now. let StampedValue { value, durability, changed_at, } = memo .revisions + // SAFETY: We just refreshed the memo so it is guaranteed to contain a value now. .stamped_value(unsafe { memo.value.as_ref().unwrap_unchecked() }); self.lru.record_use(id); @@ -89,7 +89,7 @@ where && self.validate_may_be_provisional(db, zalsa, database_key_index, memo) && self.shallow_verify_memo(db, zalsa, database_key_index, memo) { - // Unsafety invariant: memo is present in memo_map and we have verified that it is + // SAFETY: memo is present in memo_map and we have verified that it is // still valid for the current revision. return unsafe { Some(self.extend_memo_lifetime(memo)) }; } @@ -124,7 +124,7 @@ where && memo.revisions.cycle_heads.contains(&database_key_index) && self.shallow_verify_memo(db, zalsa, database_key_index, memo) { - // Unsafety invariant: memo is present in memo_map. + // SAFETY: memo is present in memo_map. return unsafe { Some(self.extend_memo_lifetime(memo)) }; } } @@ -171,7 +171,7 @@ where self.deep_verify_memo(db, zalsa, old_memo, &active_query) { if cycle_heads.is_empty() { - // Unsafety invariant: memo is present in memo_map and we have verified that it is + // SAFETY: memo is present in memo_map and we have verified that it is // still valid for the current revision. return unsafe { Some(self.extend_memo_lifetime(old_memo)) }; } diff --git a/src/function/memo.rs b/src/function/memo.rs index 0b16fffec..194c8721a 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -1,3 +1,5 @@ +#![allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety + use std::any::Any; use std::fmt::Debug; use std::fmt::Formatter; diff --git a/src/interned.rs b/src/interned.rs index a18a62ee2..8cc8d4814 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1,3 +1,5 @@ +#![allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety + use dashmap::SharedValue; use crate::durability::Durability; diff --git a/src/lib.rs b/src/lib.rs index 88d6fbd78..ecc01ac92 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,4 @@ +#![deny(clippy::undocumented_unsafe_blocks)] #![forbid(unsafe_op_in_unsafe_fn)] mod accumulator; diff --git a/src/revision.rs b/src/revision.rs index f71fc29ca..a13ed9c99 100644 --- a/src/revision.rs +++ b/src/revision.rs @@ -61,9 +61,9 @@ impl From for AtomicRevision { impl AtomicRevision { pub(crate) fn load(&self) -> Revision { - // Safety: We know that the value is non-zero because we only ever store `START` which 1, or a - // Revision which is guaranteed to be non-zero. Revision { + // SAFETY: We know that the value is non-zero because we only ever store `START` which 1, or a + // Revision which is guaranteed to be non-zero. generation: unsafe { NonZeroUsize::new_unchecked(self.data.load(Ordering::Acquire)) }, } } diff --git a/src/storage.rs b/src/storage.rs index 29831beb1..f335b6a8b 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -146,6 +146,7 @@ impl Storage { // ANCHOR_END: cancel_other_workers } +#[allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety unsafe impl ZalsaDatabase for T { fn zalsa(&self) -> &Zalsa { &self.storage().handle.zalsa_impl diff --git a/src/table.rs b/src/table.rs index dfb9fddd5..95c8426f1 100644 --- a/src/table.rs +++ b/src/table.rs @@ -81,8 +81,9 @@ impl SlotVTable { const fn of() -> &'static Self { const { &Self { + drop_impl: |data, initialized| // SAFETY: The caller is required to supply a correct data pointer and initialized length - drop_impl: |data, initialized| unsafe { + unsafe { let data = Box::from_raw(data.cast::>()); for i in 0..initialized { ptr::drop_in_place(data[i].get().cast::()); diff --git a/src/table/memo.rs b/src/table/memo.rs index a56c10cd5..c8e2c5e8e 100644 --- a/src/table/memo.rs +++ b/src/table/memo.rs @@ -73,6 +73,7 @@ impl MemoTable { fn to_dyn_fn() -> fn(NonNull) -> NonNull { let f: fn(NonNull) -> NonNull = |x| x; + #[allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety unsafe { std::mem::transmute::< fn(NonNull) -> NonNull, @@ -145,10 +146,9 @@ impl MemoTable { type_id: _, to_dyn_fn: _, atomic_memo, - }| unsafe { + }| // SAFETY: The `atomic_memo` field is never null. - Self::from_dummy(NonNull::new_unchecked(atomic_memo.into_inner())) - }, + unsafe { Self::from_dummy(NonNull::new_unchecked(atomic_memo.into_inner())) }, ) } diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index b4343c927..323b9e3e5 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -1,3 +1,5 @@ +#![allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety + use std::{any::TypeId, fmt, hash::Hash, marker::PhantomData, ops::DerefMut}; use crossbeam_queue::SegQueue; diff --git a/src/update.rs b/src/update.rs index 77169142f..9162d28af 100644 --- a/src/update.rs +++ b/src/update.rs @@ -1,3 +1,5 @@ +#![allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety + use std::{ collections::{BTreeMap, BTreeSet, HashMap, HashSet}, hash::{BuildHasher, Hash}, @@ -52,6 +54,7 @@ pub mod helper { /// /// See the `maybe_update` method in the [`Update`][] trait. pub unsafe fn maybe_update(old_pointer: *mut D, new_value: D) -> bool { + // SAFETY: Same safety conditions as `Update::maybe_update` unsafe { D::maybe_update(old_pointer, new_value) } } } @@ -66,8 +69,10 @@ pub mod helper { unsafe fn maybe_update(old_pointer: *mut T, new_value: T) -> bool; } + // SAFETY: Same safety conditions as `Update::maybe_update` unsafe impl Fallback for Dispatch { unsafe fn maybe_update(old_pointer: *mut T, new_value: T) -> bool { + // SAFETY: Same safety conditions as `Update::maybe_update` unsafe { update_fallback(old_pointer, new_value) } } } @@ -87,7 +92,7 @@ pub unsafe fn update_fallback(old_pointer: *mut T, new_value: T) -> bool where T: 'static + PartialEq, { - // Because everything is owned, this ref is simply a valid `&mut` + // SAFETY: Because everything is owned, this ref is simply a valid `&mut` let old_ref: &mut T = unsafe { &mut *old_pointer }; if *old_ref != new_value { diff --git a/src/views.rs b/src/views.rs index 230cfbffe..a14852898 100644 --- a/src/views.rs +++ b/src/views.rs @@ -62,6 +62,7 @@ impl DatabaseDownCaster { /// /// The caller must ensure that `db` is of the correct type. pub unsafe fn downcast_unchecked<'db>(&self, db: &'db dyn Database) -> &'db DbView { + // SAFETY: The caller must ensure that `db` is of the correct type. unsafe { (self.1)(db) } } } diff --git a/src/zalsa.rs b/src/zalsa.rs index f6a29e7cc..e656cf883 100644 --- a/src/zalsa.rs +++ b/src/zalsa.rs @@ -393,6 +393,7 @@ where phantom: PhantomData I>, } +#[allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety unsafe impl Sync for IngredientCache where I: Ingredient + Sync {} impl Default for IngredientCache @@ -469,6 +470,7 @@ where pub unsafe fn transmute_data_ptr(t: &T) -> &U { let t: *const T = t; let u: *const U = t as *const U; + // SAFETY: the caller must guarantee that `T` is a wide pointer for `U` unsafe { &*u } } @@ -480,5 +482,6 @@ pub unsafe fn transmute_data_ptr(t: &T) -> &U { pub(crate) unsafe fn transmute_data_mut_ptr(t: &mut T) -> &mut U { let t: *mut T = t; let u: *mut U = t as *mut U; + // SAFETY: the caller must guarantee that `T` is a wide pointer for `U` unsafe { &mut *u } } From 87a3ae03af474262f64b24b47660f1d9633f8cd5 Mon Sep 17 00:00:00 2001 From: Wilco Kusee Date: Sun, 23 Mar 2025 12:07:59 +0100 Subject: [PATCH 2/2] Remove unused impl --- src/function/fetch.rs | 7 +++---- src/zalsa.rs | 3 --- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index c00d86640..88f46f702 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -18,14 +18,13 @@ where zalsa.unwind_if_revision_cancelled(db); let memo = self.refresh_memo(db, id); + // SAFETY: We just refreshed the memo so it is guaranteed to contain a value now. + let memo_value = unsafe { memo.value.as_ref().unwrap_unchecked() }; let StampedValue { value, durability, changed_at, - } = memo - .revisions - // SAFETY: We just refreshed the memo so it is guaranteed to contain a value now. - .stamped_value(unsafe { memo.value.as_ref().unwrap_unchecked() }); + } = memo.revisions.stamped_value(memo_value); self.lru.record_use(id); diff --git a/src/zalsa.rs b/src/zalsa.rs index e656cf883..872821176 100644 --- a/src/zalsa.rs +++ b/src/zalsa.rs @@ -393,9 +393,6 @@ where phantom: PhantomData I>, } -#[allow(clippy::undocumented_unsafe_blocks)] // TODO(#697) document safety -unsafe impl Sync for IngredientCache where I: Ingredient + Sync {} - impl Default for IngredientCache where I: Ingredient,