From 6d23f3bf069a301517f6be7f1f3a7558a0cce869 Mon Sep 17 00:00:00 2001 From: David Barsky Date: Thu, 5 Dec 2024 12:36:33 +0000 Subject: [PATCH] feature: allow calling ingredient on some interned structs --- .../setup_interned_struct_sans_lifetime.rs | 45 +++++++++++-------- .../src/interned_sans_lifetime.rs | 2 + components/salsa-macros/src/lib.rs | 44 ++++++++++++++++++ src/table/memo.rs | 2 +- src/table/sync.rs | 2 +- tests/interned-sans-lifetime.rs | 37 +++++++++++++-- 6 files changed, 108 insertions(+), 24 deletions(-) diff --git a/components/salsa-macro-rules/src/setup_interned_struct_sans_lifetime.rs b/components/salsa-macro-rules/src/setup_interned_struct_sans_lifetime.rs index bcb254d03..0d0e4ba76 100644 --- a/components/salsa-macro-rules/src/setup_interned_struct_sans_lifetime.rs +++ b/components/salsa-macro-rules/src/setup_interned_struct_sans_lifetime.rs @@ -11,6 +11,10 @@ macro_rules! setup_interned_struct_sans_lifetime { // Name of the struct Struct: $Struct:ident, + // Name of the struct data. This is a parameter because `std::concat_idents` + // is unstable and taking an addition dependency is unnecessary. + StructData: $StructDataIdent:ident, + // Name of the `'db` lifetime that the user gave db_lt: $db_lt:lifetime, @@ -62,14 +66,28 @@ macro_rules! setup_interned_struct_sans_lifetime { std::marker::PhantomData < &'static salsa::plumbing::interned::Value < $Struct > > ); + type $StructDataIdent<$db_lt> = ($($field_ty,)*); + + impl salsa::plumbing::interned::Configuration for $Struct { + const DEBUG_NAME: &'static str = stringify!($Struct); + type Data<'a> = $StructDataIdent<'a>; + type Struct<'a> = $Struct; + fn struct_from_id<'db>(id: salsa::Id) -> Self::Struct<'db> { + use salsa::plumbing::FromId; + $Struct(<$Id>::from_id(id), std::marker::PhantomData) + } + fn deref_struct(s: Self::Struct<'_>) -> salsa::Id { + use salsa::plumbing::AsId; + s.0.as_id() + } + } + const _: () = { use salsa::plumbing as $zalsa; use $zalsa::interned as $zalsa_struct; type $Configuration = $Struct; - type StructData<$db_lt> = ($($field_ty,)*); - /// Key to use during hash lookups. Each field is some type that implements `Lookup` /// for the owned type. This permits interning with an `&str` when a `String` is required and so forth. struct StructKey<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*>( @@ -77,37 +95,23 @@ macro_rules! setup_interned_struct_sans_lifetime { std::marker::PhantomData<&$db_lt ()>, ); - impl<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*> $zalsa::interned::Lookup> + impl<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*> $zalsa::interned::Lookup<$StructDataIdent<$db_lt>> for StructKey<$db_lt, $($indexed_ty),*> { fn hash(&self, h: &mut H) { $($zalsa::interned::Lookup::hash(&self.$field_index, &mut *h);)* } - fn eq(&self, data: &StructData<$db_lt>) -> bool { + fn eq(&self, data: &$StructDataIdent<$db_lt>) -> bool { ($($zalsa::interned::Lookup::eq(&self.$field_index, &data.$field_index) && )* true) } #[allow(unused_unit)] - fn into_owned(self) -> StructData<$db_lt> { + fn into_owned(self) -> $StructDataIdent<$db_lt> { ($($zalsa::interned::Lookup::into_owned(self.$field_index),)*) } } - impl $zalsa_struct::Configuration for $Configuration { - const DEBUG_NAME: &'static str = stringify!($Struct); - type Data<'a> = StructData<'a>; - type Struct<'a> = $Struct; - fn struct_from_id<'db>(id: salsa::Id) -> Self::Struct<'db> { - use $zalsa::FromId; - $Struct(<$Id>::from_id(id), std::marker::PhantomData) - } - fn deref_struct(s: Self::Struct<'_>) -> salsa::Id { - use $zalsa::AsId; - s.0.as_id() - } - } - impl $Configuration { pub fn ingredient(db: &Db) -> &$zalsa_struct::IngredientImpl where @@ -146,6 +150,9 @@ macro_rules! setup_interned_struct_sans_lifetime { } impl $zalsa::SalsaStructInDb for $Struct { + fn lookup_ingredient_index(aux: &dyn $zalsa::JarAux) -> core::option::Option<$zalsa::IngredientIndex> { + aux.lookup_jar_by_type(&<$zalsa_struct::JarImpl<$Configuration>>::default()) + } } unsafe impl $zalsa::Update for $Struct { diff --git a/components/salsa-macros/src/interned_sans_lifetime.rs b/components/salsa-macros/src/interned_sans_lifetime.rs index 384c02c5f..b2342695d 100644 --- a/components/salsa-macros/src/interned_sans_lifetime.rs +++ b/components/salsa-macros/src/interned_sans_lifetime.rs @@ -85,6 +85,7 @@ impl Macro { let attrs = &self.struct_item.attrs; let vis = &self.struct_item.vis; let struct_ident = &self.struct_item.ident; + let struct_data_ident = format_ident!("{}Data", struct_ident); let db_lt = db_lifetime::db_lifetime(&self.struct_item.generics); let new_fn = salsa_struct.constructor_name(); let field_ids = salsa_struct.field_ids(); @@ -111,6 +112,7 @@ impl Macro { attrs: [#(#attrs),*], vis: #vis, Struct: #struct_ident, + StructData: #struct_data_ident, db_lt: #db_lt, id: #id, new_fn: #new_fn, diff --git a/components/salsa-macros/src/lib.rs b/components/salsa-macros/src/lib.rs index d643a956c..688d87355 100644 --- a/components/salsa-macros/src/lib.rs +++ b/components/salsa-macros/src/lib.rs @@ -67,6 +67,50 @@ pub fn interned(args: TokenStream, input: TokenStream) -> TokenStream { interned::interned(args, input) } +/// A discouraged variant of `#[salsa::interned]`. +/// +/// `#[salsa::interned_sans_lifetime]` is intended to be used in codebases that are migrating from +/// the original Salsa to the current version of Salsa. New codebases that are just starting to use +/// Salsa should avoid using this macro and prefer `#[salsa::interned]` instead. +/// +/// `#[salsa::interned_sans_lifetime]` differs from `#[salsa::interned]` in a two key ways: +/// 1. As the name suggests, it removes the `'db` lifetime from the interned struct. This lifetime is +/// designed to meant to certain values as "salsa structs", but it also adds the desirable property +/// of misuse resistance: it is difficult to embed an `#[salsa::interned]` struct into an auxiliary +/// structures or collections collection, which can lead to subtle invalidation bugs. However, old +/// Salsa encouraged storing keys to interned values in auxiliary structures and collections, so +/// so converting all usage to Salsa's current API guidelines might not be desirable or feasible. +/// 2. `#[salsa::interned_sans_lifetime]` requires specifiying the ID. In most cases, `salsa::Id` +/// is sufficent, but in rare, performance-sensitive circumstances, it might be desireable to +/// set the Id to a type that implements `salsa::plumbing::AsId` and `salsa::plumbing::FromId`. +/// +/// ## Example +/// +/// Below is an example of a struct using `#[salsa::interned_sans_lifetime]` with a custom Id: +/// +/// ```no_compile +/// use salsa::plumbing::{AsId, FromId}; +/// +/// #[derive(Clone, Copy, Hash, Debug, PartialEq, Eq, PartialOrd, Ord)] +/// struct CustomSalsaIdWrapper(salsa::Id); +/// +/// impl AsId for CustomSalsaIdWrapper { +/// fn as_id(&self) -> salsa::Id { +/// self.0 +/// } +/// } +/// +/// impl FromId for CustomSalsaIdWrapper { +/// fn from_id(id: salsa::Id) -> Self { +/// CustomSalsaIdWrapper(id) +/// } +/// } +/// +/// #[salsa::interned_sans_lifetime(id = CustomSalsaIdWrapper)] +/// struct InternedString { +/// data: String, +/// } +/// ``` #[proc_macro_attribute] pub fn interned_sans_lifetime(args: TokenStream, input: TokenStream) -> TokenStream { interned_sans_lifetime::interned_sans_lifetime(args, input) diff --git a/src/table/memo.rs b/src/table/memo.rs index 73bd52bde..0184aecbd 100644 --- a/src/table/memo.rs +++ b/src/table/memo.rs @@ -13,7 +13,7 @@ use crate::{zalsa::MemoIngredientIndex, zalsa_local::QueryOrigin}; /// Every tracked function must take a salsa struct as its first argument /// and memo tables are attached to those salsa structs as auxiliary data. #[derive(Default)] -pub(crate) struct MemoTable { +pub struct MemoTable { memos: RwLock>, } diff --git a/src/table/sync.rs b/src/table/sync.rs index dfe78a23a..58159cfe5 100644 --- a/src/table/sync.rs +++ b/src/table/sync.rs @@ -18,7 +18,7 @@ use super::util; /// Tracks the keys that are currently being processed; used to coordinate between /// worker threads. #[derive(Default)] -pub(crate) struct SyncTable { +pub struct SyncTable { syncs: RwLock>>, } diff --git a/tests/interned-sans-lifetime.rs b/tests/interned-sans-lifetime.rs index 6ab50026f..9f2cbd6f8 100644 --- a/tests/interned-sans-lifetime.rs +++ b/tests/interned-sans-lifetime.rs @@ -1,17 +1,18 @@ use expect_test::expect; +use salsa::plumbing::{AsId, FromId}; use std::path::{Path, PathBuf}; use test_log::test; #[derive(Clone, Copy, Hash, Debug, PartialEq, Eq, PartialOrd, Ord)] struct CustomSalsaIdWrapper(salsa::Id); -impl salsa::plumbing::AsId for CustomSalsaIdWrapper { +impl AsId for CustomSalsaIdWrapper { fn as_id(&self) -> salsa::Id { self.0 } } -impl salsa::plumbing::FromId for CustomSalsaIdWrapper { +impl FromId for CustomSalsaIdWrapper { fn from_id(id: salsa::Id) -> Self { CustomSalsaIdWrapper(id) } @@ -45,7 +46,7 @@ struct InternedPathBuf { #[salsa::tracked] fn intern_stuff(db: &dyn salsa::Database) -> String { - let s1 = InternedString::new(db, "Hello, ".to_string()); + let s1 = InternedString::new(db, "Hello, "); let s2 = InternedString::new(db, "World, "); let s3 = InternedPair::new(db, (s1, s2)); @@ -70,6 +71,7 @@ fn interning_returns_equal_keys_for_equal_data() { assert_eq!(s1, s1_2); assert_eq!(s2, s2_2); } + #[test] fn interning_returns_equal_keys_for_equal_data_multi_field() { let db = salsa::DatabaseImpl::new(); @@ -127,3 +129,32 @@ fn tracked_static_query_works() { let s1 = InternedString::new(&db, "Hello, World!".to_string()); assert_eq!(length(&db, s1), 13); } + +#[test] +fn public_ingredient() { + let db = salsa::DatabaseImpl::new(); + let s = InternedString::new(&db, String::from("Hello, world!")); + let underlying_id = s.0; + + let data = InternedString::ingredient(&db).data(&db, underlying_id.as_id()); + assert_eq!(data, &(String::from("Hello, world!"),)); +} + +#[salsa::tracked] +fn intern_more_stuff(db: &dyn salsa::Database) -> (InternedString, InternedString, InternedPair) { + let s1 = InternedString::new(db, "Hello, "); + let s2 = InternedString::new(db, "World, "); + let pair = InternedPair::new(db, (s1, s2)); + (s1, s2, pair) +} + +#[test] +fn public_ingredients() { + let db = salsa::DatabaseImpl::new(); + + let (_, _, pair) = intern_more_stuff(&db); + let (interned_s1, interned_s2) = InternedPair::ingredient(&db).fields(&db, pair).0; + + assert_eq!(interned_s1.data(&db), "Hello, ".to_owned()); + assert_eq!(interned_s2.data(&db), "World, ".to_owned()); +}