From 389aa66bcfce63d8448eba8f55b7e44c09886538 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 3 Apr 2024 05:59:11 -0400 Subject: [PATCH 1/4] print all fields in `debug()` but ignore deps In a previous PR we added the `include_all_fields` parameter to `DebugWithDb` to allow it to not create spurious dependencies. This PR takes a different approach: we simply ignore the dependencies created during debug operations. This is risky as it can create incorrect dependencies, but it is way more convenient and seems like what users probably want. It also means that `DebugWithDb` has a simpler signature that matches the `Debug` trait again, which seems good to me. --- components/salsa-2022-macros/src/input.rs | 6 +- components/salsa-2022-macros/src/interned.rs | 6 +- .../salsa-2022-macros/src/salsa_struct.rs | 38 +---- .../salsa-2022-macros/src/tracked_struct.rs | 5 +- components/salsa-2022/src/debug.rs | 158 ++++++++---------- components/salsa-2022/src/event.rs | 50 ++---- components/salsa-2022/src/key.rs | 16 +- components/salsa-2022/src/runtime.rs | 16 ++ .../salsa-2022/src/runtime/active_query.rs | 40 +++++ .../salsa-2022/src/runtime/local_state.rs | 19 +++ examples-2022/calc/src/parser.rs | 2 +- salsa-2022-tests/tests/debug.rs | 37 +++- salsa-2022-tests/tests/input_with_ids.rs | 2 +- salsa-2022-tests/tests/singleton.rs | 2 +- .../tests/tracked_fn_read_own_specify.rs | 2 +- .../tests/warnings/needless_borrow.rs | 7 +- 16 files changed, 202 insertions(+), 204 deletions(-) diff --git a/components/salsa-2022-macros/src/input.rs b/components/salsa-2022-macros/src/input.rs index 5b83efe6..98975bc7 100644 --- a/components/salsa-2022-macros/src/input.rs +++ b/components/salsa-2022-macros/src/input.rs @@ -1,4 +1,4 @@ -use crate::salsa_struct::{SalsaField, SalsaStruct, SalsaStructKind}; +use crate::salsa_struct::{SalsaField, SalsaStruct}; use proc_macro2::{Literal, TokenStream}; /// For an entity struct `Foo` with fields `f1: T1, ..., fN: TN`, we generate... @@ -10,9 +10,7 @@ pub(crate) fn input( args: proc_macro::TokenStream, input: proc_macro::TokenStream, ) -> proc_macro::TokenStream { - match SalsaStruct::new(SalsaStructKind::Input, args, input) - .and_then(|el| InputStruct(el).generate_input()) - { + match SalsaStruct::new(args, input).and_then(|el| InputStruct(el).generate_input()) { Ok(s) => s.into(), Err(err) => err.into_compile_error().into(), } diff --git a/components/salsa-2022-macros/src/interned.rs b/components/salsa-2022-macros/src/interned.rs index a53ff5f1..59b5e7b7 100644 --- a/components/salsa-2022-macros/src/interned.rs +++ b/components/salsa-2022-macros/src/interned.rs @@ -1,4 +1,4 @@ -use crate::salsa_struct::{SalsaStruct, SalsaStructKind}; +use crate::salsa_struct::SalsaStruct; use proc_macro2::TokenStream; // #[salsa::interned(jar = Jar0, data = TyData0)] @@ -13,9 +13,7 @@ pub(crate) fn interned( args: proc_macro::TokenStream, input: proc_macro::TokenStream, ) -> proc_macro::TokenStream { - match SalsaStruct::new(SalsaStructKind::Interned, args, input) - .and_then(|el| InternedStruct(el).generate_interned()) - { + match SalsaStruct::new(args, input).and_then(|el| InternedStruct(el).generate_interned()) { Ok(s) => s.into(), Err(err) => err.into_compile_error().into(), } diff --git a/components/salsa-2022-macros/src/salsa_struct.rs b/components/salsa-2022-macros/src/salsa_struct.rs index 5b20b761..6a7ec76d 100644 --- a/components/salsa-2022-macros/src/salsa_struct.rs +++ b/components/salsa-2022-macros/src/salsa_struct.rs @@ -29,14 +29,7 @@ use crate::options::{AllowedOptions, Options}; use proc_macro2::{Ident, Span, TokenStream}; use syn::spanned::Spanned; -pub(crate) enum SalsaStructKind { - Input, - Tracked, - Interned, -} - pub(crate) struct SalsaStruct { - kind: SalsaStructKind, args: Options, struct_item: syn::ItemStruct, fields: Vec, @@ -46,23 +39,20 @@ const BANNED_FIELD_NAMES: &[&str] = &["from", "new"]; impl SalsaStruct { pub(crate) fn new( - kind: SalsaStructKind, args: proc_macro::TokenStream, input: proc_macro::TokenStream, ) -> syn::Result { let struct_item = syn::parse(input)?; - Self::with_struct(kind, args, struct_item) + Self::with_struct(args, struct_item) } pub(crate) fn with_struct( - kind: SalsaStructKind, args: proc_macro::TokenStream, struct_item: syn::ItemStruct, ) -> syn::Result { let args: Options = syn::parse(args)?; let fields = Self::extract_options(&struct_item)?; Ok(Self { - kind, args, struct_item, fields, @@ -93,13 +83,6 @@ impl SalsaStruct { self.fields.iter() } - pub(crate) fn is_identity_field(&self, field: &SalsaField) -> bool { - match self.kind { - SalsaStructKind::Input | SalsaStructKind::Tracked => field.has_id_attr, - SalsaStructKind::Interned => true, - } - } - /// Names of all fields (id and value). /// /// If this is an enum, empty vec. @@ -237,7 +220,7 @@ impl SalsaStruct { let db_type = self.db_dyn_ty(); let ident_string = ident.to_string(); - // `::salsa::debug::helper::SalsaDebug` will use `DebugWithDb` or fallbak to `Debug` + // `::salsa::debug::helper::SalsaDebug` will use `DebugWithDb` or fallback to `Debug` let fields = self .all_fields() .map(|field| -> TokenStream { @@ -245,28 +228,15 @@ impl SalsaStruct { let field_getter = field.get_name(); let field_ty = field.ty(); - let field_debug = quote_spanned! { field.field.span() => + quote_spanned! { field.field.span() => debug_struct = debug_struct.field( #field_name_string, &::salsa::debug::helper::SalsaDebug::<#field_ty, #db_type>::salsa_debug( #[allow(clippy::needless_borrow)] &self.#field_getter(_db), _db, - _include_all_fields ) ); - }; - - if self.is_identity_field(field) { - quote_spanned! { field.field.span() => - #field_debug - } - } else { - quote_spanned! { field.field.span() => - if _include_all_fields { - #field_debug - } - } } }) .collect::(); @@ -274,7 +244,7 @@ impl SalsaStruct { // `use ::salsa::debug::helper::Fallback` is needed for the fallback to `Debug` impl parse_quote_spanned! {ident.span()=> impl ::salsa::DebugWithDb<#db_type> for #ident { - fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>, _db: &#db_type, _include_all_fields: bool) -> ::std::fmt::Result { + fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>, _db: &#db_type) -> ::std::fmt::Result { #[allow(unused_imports)] use ::salsa::debug::helper::Fallback; let mut debug_struct = &mut f.debug_struct(#ident_string); diff --git a/components/salsa-2022-macros/src/tracked_struct.rs b/components/salsa-2022-macros/src/tracked_struct.rs index feb36031..9dee55b6 100644 --- a/components/salsa-2022-macros/src/tracked_struct.rs +++ b/components/salsa-2022-macros/src/tracked_struct.rs @@ -1,6 +1,6 @@ use proc_macro2::{Literal, Span, TokenStream}; -use crate::salsa_struct::{SalsaField, SalsaStruct, SalsaStructKind}; +use crate::salsa_struct::{SalsaField, SalsaStruct}; /// For an tracked struct `Foo` with fields `f1: T1, ..., fN: TN`, we generate... /// @@ -11,8 +11,7 @@ pub(crate) fn tracked( args: proc_macro::TokenStream, struct_item: syn::ItemStruct, ) -> syn::Result { - SalsaStruct::with_struct(SalsaStructKind::Tracked, args, struct_item) - .and_then(|el| TrackedStruct(el).generate_tracked()) + SalsaStruct::with_struct(args, struct_item).and_then(|el| TrackedStruct(el).generate_tracked()) } struct TrackedStruct(SalsaStruct); diff --git a/components/salsa-2022/src/debug.rs b/components/salsa-2022/src/debug.rs index 5050a227..5e1c7416 100644 --- a/components/salsa-2022/src/debug.rs +++ b/components/salsa-2022/src/debug.rs @@ -5,7 +5,16 @@ use std::{ sync::Arc, }; -pub trait DebugWithDb { +use crate::database::AsSalsaDatabase; + +/// `DebugWithDb` is a version of the traditional [`Debug`](`std::fmt::Debug`) +/// trait that gives access to the salsa database, allowing tracked +/// structs to print the values of their fields. It is typically not used +/// directly, instead you should write (e.g.) `format!("{:?}", foo.debug(db))`. +/// Implementations are automatically provided for `#[salsa::tracked]` +/// items, though you can opt-out from that if you wish to provide a manual +/// implementation. +pub trait DebugWithDb { fn debug<'me, 'db>(&'me self, db: &'me Db) -> DebugWith<'me, Db> where Self: Sized + 'me, @@ -13,32 +22,6 @@ pub trait DebugWithDb { DebugWith { value: BoxRef::Ref(self), db, - include_all_fields: false, - } - } - - fn debug_with<'me, 'db>(&'me self, db: &'me Db, include_all_fields: bool) -> DebugWith<'me, Db> - where - Self: Sized + 'me, - { - DebugWith { - value: BoxRef::Ref(self), - db, - include_all_fields, - } - } - - /// Be careful when using this method inside a tracked function, - /// because the default macro generated implementation will read all fields, - /// maybe introducing undesired dependencies. - fn debug_all<'me, 'db>(&'me self, db: &'me Db) -> DebugWith<'me, Db> - where - Self: Sized + 'me, - { - DebugWith { - value: BoxRef::Ref(self), - db, - include_all_fields: true, } } @@ -49,35 +32,20 @@ pub trait DebugWithDb { DebugWith { value: BoxRef::Box(Box::new(self)), db, - include_all_fields: false, } } - /// Be careful when using this method inside a tracked function, - /// because the default macro generated implementation will read all fields, - /// maybe introducing undesired dependencies. - fn into_debug_all<'me, 'db>(self, db: &'me Db) -> DebugWith<'me, Db> - where - Self: Sized + 'me, - { - DebugWith { - value: BoxRef::Box(Box::new(self)), - db, - include_all_fields: true, - } - } - - /// if `include_all_fields` is `false` only identity fields should be read, which means: - /// - for [#\[salsa::input\]](salsa_2022_macros::input) no fields - /// - for [#\[salsa::tracked\]](salsa_2022_macros::tracked) only fields with `#[id]` attribute - /// - for [#\[salsa::interned\]](salsa_2022_macros::interned) any field - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result; + /// Format `self` given the database `db`. + /// + /// Note: when invoked from `DebugWith`, + /// accesses to fields that occur within this method will be + /// disregarded as dependencies from the current query. + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result; } -pub struct DebugWith<'me, Db: ?Sized> { +pub struct DebugWith<'me, Db: ?Sized + AsSalsaDatabase> { value: BoxRef<'me, dyn DebugWithDb + 'me>, db: &'me Db, - include_all_fields: bool, } enum BoxRef<'me, T: ?Sized> { @@ -96,54 +64,64 @@ impl std::ops::Deref for BoxRef<'_, T> { } } -impl fmt::Debug for DebugWith<'_, D> { +impl fmt::Debug for DebugWith<'_, Db> +where + Db: AsSalsaDatabase, +{ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - DebugWithDb::fmt(&*self.value, f, self.db, self.include_all_fields) + let db = self.db.as_salsa_database(); + db.runtime() + .debug_probe(|| DebugWithDb::fmt(&*self.value, f, self.db)) } } impl DebugWithDb for &T where T: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - T::fmt(self, f, db, include_all_fields) + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + T::fmt(self, f, db) } } impl DebugWithDb for Box where T: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - T::fmt(self, f, db, include_all_fields) + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + T::fmt(self, f, db) } } impl DebugWithDb for Rc where T: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - T::fmt(self, f, db, include_all_fields) + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + T::fmt(self, f, db) } } impl DebugWithDb for Arc where T: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - T::fmt(self, f, db, include_all_fields) + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + T::fmt(self, f, db) } } impl DebugWithDb for Vec where T: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - let elements = self.iter().map(|e| e.debug_with(db, include_all_fields)); + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + let elements = self.iter().map(|e| e.debug(db)); f.debug_list().entries(elements).finish() } } @@ -151,9 +129,10 @@ where impl DebugWithDb for Option where T: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - let me = self.as_ref().map(|v| v.debug_with(db, include_all_fields)); + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + let me = self.as_ref().map(|v| v.debug(db)); fmt::Debug::fmt(&me, f) } } @@ -162,14 +141,10 @@ impl DebugWithDb for HashMap where K: DebugWithDb, V: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - let elements = self.iter().map(|(k, v)| { - ( - k.debug_with(db, include_all_fields), - v.debug_with(db, include_all_fields), - ) - }); + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + let elements = self.iter().map(|(k, v)| (k.debug(db), v.debug(db))); f.debug_map().entries(elements).finish() } } @@ -178,11 +153,12 @@ impl DebugWithDb for (A, B) where A: DebugWithDb, B: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { f.debug_tuple("") - .field(&self.0.debug_with(db, include_all_fields)) - .field(&self.1.debug_with(db, include_all_fields)) + .field(&self.0.debug(db)) + .field(&self.1.debug(db)) .finish() } } @@ -192,12 +168,13 @@ where A: DebugWithDb, B: DebugWithDb, C: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { f.debug_tuple("") - .field(&self.0.debug_with(db, include_all_fields)) - .field(&self.1.debug_with(db, include_all_fields)) - .field(&self.2.debug_with(db, include_all_fields)) + .field(&self.0.debug(db)) + .field(&self.1.debug(db)) + .field(&self.2.debug(db)) .finish() } } @@ -205,9 +182,10 @@ where impl DebugWithDb for HashSet where V: DebugWithDb, + Db: AsSalsaDatabase, { - fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db, include_all_fields: bool) -> fmt::Result { - let elements = self.iter().map(|e| e.debug_with(db, include_all_fields)); + fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result { + let elements = self.iter().map(|e| e.debug(db)); f.debug_list().entries(elements).finish() } } @@ -217,27 +195,27 @@ where /// That's the "has impl" trick (https://github.com/nvzqz/impls#how-it-works) #[doc(hidden)] pub mod helper { - use super::{DebugWith, DebugWithDb}; + use super::{AsSalsaDatabase, DebugWith, DebugWithDb}; use std::{fmt, marker::PhantomData}; pub trait Fallback { - fn salsa_debug<'a>(a: &'a T, _db: &Db, _include_all_fields: bool) -> &'a dyn fmt::Debug { + fn salsa_debug<'a>(a: &'a T, _db: &Db) -> &'a dyn fmt::Debug { a } } + impl Fallback for Everything {} + pub struct SalsaDebug(PhantomData, PhantomData); - impl, Db: ?Sized> SalsaDebug { + impl SalsaDebug + where + T: DebugWithDb, + Db: AsSalsaDatabase, + { #[allow(dead_code)] - pub fn salsa_debug<'a, 'b: 'a>( - a: &'a T, - db: &'b Db, - include_all_fields: bool, - ) -> DebugWith<'a, Db> { - a.debug_with(db, include_all_fields) + pub fn salsa_debug<'a, 'b: 'a>(a: &'a T, db: &'b Db) -> DebugWith<'a, Db> { + a.debug(db) } } - - impl Fallback for Everything {} } diff --git a/components/salsa-2022/src/event.rs b/components/salsa-2022/src/event.rs index ca2b9b68..fdbef715 100644 --- a/components/salsa-2022/src/event.rs +++ b/components/salsa-2022/src/event.rs @@ -28,15 +28,10 @@ impl DebugWithDb for Event where Db: ?Sized + Database, { - fn fmt( - &self, - f: &mut std::fmt::Formatter<'_>, - db: &Db, - include_all_fields: bool, - ) -> std::fmt::Result { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>, db: &Db) -> std::fmt::Result { f.debug_struct("Event") .field("runtime_id", &self.runtime_id) - .field("kind", &self.kind.debug_with(db, include_all_fields)) + .field("kind", &self.kind.debug(db)) .finish() } } @@ -154,19 +149,11 @@ impl DebugWithDb for EventKind where Db: ?Sized + Database, { - fn fmt( - &self, - fmt: &mut std::fmt::Formatter<'_>, - db: &Db, - include_all_fields: bool, - ) -> std::fmt::Result { + fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>, db: &Db) -> std::fmt::Result { match self { EventKind::DidValidateMemoizedValue { database_key } => fmt .debug_struct("DidValidateMemoizedValue") - .field( - "database_key", - &database_key.debug_with(db, include_all_fields), - ) + .field("database_key", &database_key.debug(db)) .finish(), EventKind::WillBlockOn { other_runtime_id, @@ -174,17 +161,11 @@ where } => fmt .debug_struct("WillBlockOn") .field("other_runtime_id", other_runtime_id) - .field( - "database_key", - &database_key.debug_with(db, include_all_fields), - ) + .field("database_key", &database_key.debug(db)) .finish(), EventKind::WillExecute { database_key } => fmt .debug_struct("WillExecute") - .field( - "database_key", - &database_key.debug_with(db, include_all_fields), - ) + .field("database_key", &database_key.debug(db)) .finish(), EventKind::WillCheckCancellation => fmt.debug_struct("WillCheckCancellation").finish(), EventKind::WillDiscardStaleOutput { @@ -192,29 +173,20 @@ where output_key, } => fmt .debug_struct("WillDiscardStaleOutput") - .field( - "execute_key", - &execute_key.debug_with(db, include_all_fields), - ) - .field("output_key", &output_key.debug_with(db, include_all_fields)) + .field("execute_key", &execute_key.debug(db)) + .field("output_key", &output_key.debug(db)) .finish(), EventKind::DidDiscard { key } => fmt .debug_struct("DidDiscard") - .field("key", &key.debug_with(db, include_all_fields)) + .field("key", &key.debug(db)) .finish(), EventKind::DidDiscardAccumulated { executor_key, accumulator, } => fmt .debug_struct("DidDiscardAccumulated") - .field( - "executor_key", - &executor_key.debug_with(db, include_all_fields), - ) - .field( - "accumulator", - &accumulator.debug_with(db, include_all_fields), - ) + .field("executor_key", &executor_key.debug(db)) + .field("accumulator", &accumulator.debug(db)) .finish(), } } diff --git a/components/salsa-2022/src/key.rs b/components/salsa-2022/src/key.rs index 8b71811d..c443b7bb 100644 --- a/components/salsa-2022/src/key.rs +++ b/components/salsa-2022/src/key.rs @@ -38,12 +38,7 @@ impl crate::debug::DebugWithDb for DependencyIndex where Db: ?Sized + Database, { - fn fmt( - &self, - f: &mut std::fmt::Formatter<'_>, - db: &Db, - _include_all_fields: bool, - ) -> std::fmt::Result { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>, db: &Db) -> std::fmt::Result { db.fmt_index(*self, f) } } @@ -73,14 +68,9 @@ impl crate::debug::DebugWithDb for DatabaseKeyIndex where Db: ?Sized + Database, { - fn fmt( - &self, - f: &mut std::fmt::Formatter<'_>, - db: &Db, - include_all_fields: bool, - ) -> std::fmt::Result { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>, db: &Db) -> std::fmt::Result { let i: DependencyIndex = (*self).into(); - DebugWithDb::fmt(&i, f, db, include_all_fields) + DebugWithDb::fmt(&i, f, db) } } diff --git a/components/salsa-2022/src/runtime.rs b/components/salsa-2022/src/runtime.rs index 87beb740..727f8342 100644 --- a/components/salsa-2022/src/runtime.rs +++ b/components/salsa-2022/src/runtime.rs @@ -104,6 +104,22 @@ impl Runtime { self.shared_state.empty_dependencies.clone() } + /// Executes `op` but ignores its effect on + /// the query dependencies; intended for use + /// by `DebugWithDb` only. + /// + /// # Danger: intended for debugging only + /// + /// This operation is intended for **debugging only**. + /// Misuse will cause Salsa to give incorrect results. + /// The expectation is that the type `R` produced will be + /// logged or printed out. **The type `R` that is produced + /// should not affect the result or other outputs + /// (such as accumulators) from the current Salsa query.** + pub fn debug_probe(&self, op: impl FnOnce() -> R) -> R { + self.local_state.debug_probe(op) + } + pub fn snapshot(&self) -> Self { if self.local_state.query_in_progress() { panic!("it is not legal to `snapshot` during a query (see salsa-rs/salsa#80)"); diff --git a/components/salsa-2022/src/runtime/active_query.rs b/components/salsa-2022/src/runtime/active_query.rs index 67ea202e..1af86f82 100644 --- a/components/salsa-2022/src/runtime/active_query.rs +++ b/components/salsa-2022/src/runtime/active_query.rs @@ -40,6 +40,26 @@ pub(super) struct ActiveQuery { pub(super) disambiguator_map: FxIndexMap, } +pub(super) struct SavedQueryState { + database_key_index: DatabaseKeyIndex, + durability: Durability, + changed_at: Revision, + input_outputs_len: usize, + untracked_read: bool, +} + +impl SavedQueryState { + fn new(query: &ActiveQuery) -> Self { + Self { + database_key_index: query.database_key_index, + durability: query.durability, + changed_at: query.changed_at, + input_outputs_len: query.input_outputs.len(), + untracked_read: query.untracked_read, + } + } +} + impl ActiveQuery { pub(super) fn new(database_key_index: DatabaseKeyIndex) -> Self { ActiveQuery { @@ -53,6 +73,26 @@ impl ActiveQuery { } } + pub(super) fn save_query_state(&self) -> SavedQueryState { + SavedQueryState::new(self) + } + + pub(super) fn restore_query_state(&mut self, state: SavedQueryState) { + assert_eq!(self.database_key_index, state.database_key_index); + + assert!(self.durability <= state.durability); + self.durability = state.durability; + + assert!(self.changed_at >= state.changed_at); + self.changed_at = state.changed_at; + + assert!(self.input_outputs.len() >= state.input_outputs_len); + self.input_outputs.truncate(state.input_outputs_len); + + assert!(self.untracked_read >= state.untracked_read); + self.untracked_read = state.untracked_read; + } + pub(super) fn add_read( &mut self, input: DependencyIndex, diff --git a/components/salsa-2022/src/runtime/local_state.rs b/components/salsa-2022/src/runtime/local_state.rs index 6405f850..2fc21cb3 100644 --- a/components/salsa-2022/src/runtime/local_state.rs +++ b/components/salsa-2022/src/runtime/local_state.rs @@ -174,6 +174,25 @@ impl LocalState { self.with_query_stack(|stack| !stack.is_empty()) } + /// Dangerous operation: executes `op` but ignores its effect on + /// the query dependencies. Useful for debugging statements, but + /// otherwise not to be toyed with! + pub(super) fn debug_probe(&self, op: impl FnOnce() -> R) -> R { + let saved_state: Option<_> = + self.with_query_stack(|stack| Some(stack.last()?.save_query_state())); + + let result = op(); + + if let Some(saved_state) = saved_state { + self.with_query_stack(|stack| { + let active_query = stack.last_mut().expect("query stack not empty"); + active_query.restore_query_state(saved_state); + }); + } + + result + } + /// Returns the index of the active query along with its *current* durability/changed-at /// information. As the query continues to execute, naturally, that information may change. pub(super) fn active_query(&self) -> Option<(DatabaseKeyIndex, StampedValue<()>)> { diff --git a/examples-2022/calc/src/parser.rs b/examples-2022/calc/src/parser.rs index bf99329a..38439c04 100644 --- a/examples-2022/calc/src/parser.rs +++ b/examples-2022/calc/src/parser.rs @@ -367,7 +367,7 @@ fn parse_string(source_text: &str) -> String { let accumulated = parse_statements::accumulated::(&db, source_program); // Format the result as a string and return it - format!("{:#?}", (statements.debug_all(&db), accumulated)) + format!("{:#?}", (statements.debug(&db), accumulated)) } // ANCHOR_END: parse_string diff --git a/salsa-2022-tests/tests/debug.rs b/salsa-2022-tests/tests/debug.rs index 204caf5f..92f9e2a3 100644 --- a/salsa-2022-tests/tests/debug.rs +++ b/salsa-2022-tests/tests/debug.rs @@ -4,7 +4,7 @@ use expect_test::expect; use salsa::DebugWithDb; #[salsa::jar(db = Db)] -struct Jar(MyInput, ComplexStruct); +struct Jar(MyInput, ComplexStruct, leak_debug_string); trait Db: salsa::DbWithJar {} @@ -44,15 +44,38 @@ fn input() { }; let complex_struct = ComplexStruct::new(&db, input, not_salsa); - // default debug only includes identity fields + // debug includes all fields let actual = format!("{:?}", complex_struct.debug(&db)); - let expected = expect!["ComplexStruct { [salsa id]: 0 }"]; - expected.assert_eq(&actual); - - // all fields - let actual = format!("{:?}", complex_struct.debug_all(&db)); let expected = expect![[ r#"ComplexStruct { [salsa id]: 0, my_input: MyInput { [salsa id]: 0, field: 22 }, not_salsa: NotSalsa { field: "it's salsa time" } }"# ]]; expected.assert_eq(&actual); } + +#[salsa::tracked] +fn leak_debug_string(db: &dyn Db, input: MyInput) -> String { + format!("{:?}", input.debug(db)) +} + +/// Test that field reads that occur as part of `Debug` are not tracked. +/// Intentionally leaks the debug string. +/// Don't try this at home, kids. +#[test] +fn untracked_dependencies() { + let mut db = Database::default(); + + let input = MyInput::new(&db, 22); + + let s = leak_debug_string(&db, input); + expect![[r#" + "MyInput { [salsa id]: 0, field: 22 }" + "#]] + .assert_debug_eq(&s); + + input.set_field(&mut db).to(23); + + // check that we reuse the cached result for debug string + // even though the dependency changed. + let s = leak_debug_string(&db, input); + assert!(s.contains(", field: 22 }")); +} diff --git a/salsa-2022-tests/tests/input_with_ids.rs b/salsa-2022-tests/tests/input_with_ids.rs index f6aacd7b..3656c8e3 100644 --- a/salsa-2022-tests/tests/input_with_ids.rs +++ b/salsa-2022-tests/tests/input_with_ids.rs @@ -38,7 +38,7 @@ fn test_debug() { let input = MyInput::new(&mut db, 50, 10, Field {}); let actual = format!("{:?}", input.debug(&db)); - let expected = expect![[r#"MyInput { [salsa id]: 0, id_one: 50, id_two: 10 }"#]]; + let expected = expect!["MyInput { [salsa id]: 0, id_one: 50, id_two: 10, field: Field }"]; expected.assert_eq(&actual); } diff --git a/salsa-2022-tests/tests/singleton.rs b/salsa-2022-tests/tests/singleton.rs index 275a2d3c..d72b19ed 100644 --- a/salsa-2022-tests/tests/singleton.rs +++ b/salsa-2022-tests/tests/singleton.rs @@ -67,6 +67,6 @@ fn debug() { let db = Database::default(); let input = MyInput::new(&db, 3, 4); let actual = format!("{:?}", input.debug(&db)); - let expected = expect![[r#"MyInput { [salsa id]: 0, id_field: 4 }"#]]; + let expected = expect!["MyInput { [salsa id]: 0, field: 3, id_field: 4 }"]; expected.assert_eq(&actual); } diff --git a/salsa-2022-tests/tests/tracked_fn_read_own_specify.rs b/salsa-2022-tests/tests/tracked_fn_read_own_specify.rs index b31c6a54..12257f64 100644 --- a/salsa-2022-tests/tests/tracked_fn_read_own_specify.rs +++ b/salsa-2022-tests/tests/tracked_fn_read_own_specify.rs @@ -55,7 +55,7 @@ fn execute() { assert_eq!(tracked_fn(&db, input), 2222); db.assert_logs(expect![[r#" [ - "tracked_fn(MyInput { [salsa id]: 0 })", + "tracked_fn(MyInput { [salsa id]: 0, field: 22 })", ]"#]]); // A "synthetic write" causes the system to act *as though* some diff --git a/salsa-2022-tests/tests/warnings/needless_borrow.rs b/salsa-2022-tests/tests/warnings/needless_borrow.rs index 92cd0677..502d23e2 100644 --- a/salsa-2022-tests/tests/warnings/needless_borrow.rs +++ b/salsa-2022-tests/tests/warnings/needless_borrow.rs @@ -7,12 +7,7 @@ struct Jar(TokenTree); enum Token {} impl salsa::DebugWithDb for Token { - fn fmt( - &self, - _f: &mut std::fmt::Formatter<'_>, - _db: &dyn Db, - _include_all_fields: bool, - ) -> std::fmt::Result { + fn fmt(&self, _f: &mut std::fmt::Formatter<'_>, _db: &dyn Db) -> std::fmt::Result { unreachable!() } } From fd15c3a60094e022428ea7a28ef57e952d2f1372 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 3 Apr 2024 06:23:43 -0400 Subject: [PATCH 2/4] support customizing the DebugWithDb impl --- .../salsa-2022-macros/src/salsa_struct.rs | 48 ++++++++++++++++--- salsa-2022-tests/tests/debug.rs | 45 ++++++++++++++++- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/components/salsa-2022-macros/src/salsa_struct.rs b/components/salsa-2022-macros/src/salsa_struct.rs index 6a7ec76d..dbdb0478 100644 --- a/components/salsa-2022-macros/src/salsa_struct.rs +++ b/components/salsa-2022-macros/src/salsa_struct.rs @@ -32,9 +32,15 @@ use syn::spanned::Spanned; pub(crate) struct SalsaStruct { args: Options, struct_item: syn::ItemStruct, + customizations: Vec, fields: Vec, } +#[derive(PartialEq, Eq, Debug, Copy, Clone)] +pub enum Customization { + DebugWithDb, +} + const BANNED_FIELD_NAMES: &[&str] = &["from", "new"]; impl SalsaStruct { @@ -51,18 +57,42 @@ impl SalsaStruct { struct_item: syn::ItemStruct, ) -> syn::Result { let args: Options = syn::parse(args)?; - let fields = Self::extract_options(&struct_item)?; + let customizations = Self::extract_customizations(&struct_item)?; + let fields = Self::extract_fields(&struct_item)?; Ok(Self { args, struct_item, + customizations, fields, }) } + fn extract_customizations(struct_item: &syn::ItemStruct) -> syn::Result> { + Ok(struct_item + .attrs + .iter() + .map(|attr| { + if attr.path.is_ident("customize") { + let args: syn::Ident = attr.parse_args()?; + if args.to_string() == "DebugWithDb" { + Ok(vec![Customization::DebugWithDb]) + } else { + Err(syn::Error::new_spanned(args, "unrecognized customization")) + } + } else { + Ok(vec![]) + } + }) + .collect::>, _>>()? + .into_iter() + .flatten() + .collect()) + } + /// Extract out the fields and their options: /// If this is a struct, it must use named fields, so we can define field accessors. /// If it is an enum, then this is not necessary. - pub(crate) fn extract_options(struct_item: &syn::ItemStruct) -> syn::Result> { + fn extract_fields(struct_item: &syn::ItemStruct) -> syn::Result> { match &struct_item.fields { syn::Fields::Named(n) => Ok(n .named @@ -146,12 +176,14 @@ impl SalsaStruct { let ident = self.id_ident(); let visibility = &self.struct_item.vis; - // Extract the attributes the user gave, but screen out derive, since we are adding our own. + // Extract the attributes the user gave, but screen out derive, since we are adding our own, + // and the customize attribute that we use for our own purposes. let attrs: Vec<_> = self .struct_item .attrs .iter() .filter(|attr| !attr.path.is_ident("derive")) + .filter(|attr| !attr.path.is_ident("customize")) .collect(); parse_quote! { @@ -214,7 +246,11 @@ impl SalsaStruct { } /// Generate `impl salsa::DebugWithDb for Foo` - pub(crate) fn as_debug_with_db_impl(&self) -> syn::ItemImpl { + pub(crate) fn as_debug_with_db_impl(&self) -> Option { + if self.customizations.contains(&Customization::DebugWithDb) { + return None; + } + let ident = self.id_ident(); let db_type = self.db_dyn_ty(); @@ -242,7 +278,7 @@ impl SalsaStruct { .collect::(); // `use ::salsa::debug::helper::Fallback` is needed for the fallback to `Debug` impl - parse_quote_spanned! {ident.span()=> + Some(parse_quote_spanned! {ident.span()=> impl ::salsa::DebugWithDb<#db_type> for #ident { fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>, _db: &#db_type) -> ::std::fmt::Result { #[allow(unused_imports)] @@ -253,7 +289,7 @@ impl SalsaStruct { debug_struct.finish() } } - } + }) } /// Disallow `#[id]` attributes on the fields of this struct. diff --git a/salsa-2022-tests/tests/debug.rs b/salsa-2022-tests/tests/debug.rs index 92f9e2a3..c173d79e 100644 --- a/salsa-2022-tests/tests/debug.rs +++ b/salsa-2022-tests/tests/debug.rs @@ -4,7 +4,13 @@ use expect_test::expect; use salsa::DebugWithDb; #[salsa::jar(db = Db)] -struct Jar(MyInput, ComplexStruct, leak_debug_string); +struct Jar( + MyInput, + ComplexStruct, + leak_debug_string, + DerivedCustom, + leak_derived_custom, +); trait Db: salsa::DbWithJar {} @@ -79,3 +85,40 @@ fn untracked_dependencies() { let s = leak_debug_string(&db, input); assert!(s.contains(", field: 22 }")); } + +#[salsa::tracked] +#[customize(DebugWithDb)] +struct DerivedCustom { + my_input: MyInput, + value: u32, +} + +impl DebugWithDb for DerivedCustom { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>, db: &dyn Db) -> std::fmt::Result { + write!( + f, + "{:?} / {:?}", + self.my_input(db).debug(db), + self.value(db) + ) + } +} + +#[salsa::tracked] +fn leak_derived_custom(db: &dyn Db, input: MyInput, value: u32) -> String { + let c = DerivedCustom::new(db, input, value); + format!("{:?}", c.debug(db)) +} + +#[test] +fn custom_debug_impl() { + let db = Database::default(); + + let input = MyInput::new(&db, 22); + + let s = leak_derived_custom(&db, input, 23); + expect![[r#" + "MyInput { [salsa id]: 0, field: 22 } / 23" + "#]] + .assert_debug_eq(&s); +} From 9dfa84b693cade23411c36ff77f3a573ed5bed6f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 3 Apr 2024 06:30:31 -0400 Subject: [PATCH 3/4] improve doc comments --- components/salsa-2022/src/debug.rs | 49 ++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/components/salsa-2022/src/debug.rs b/components/salsa-2022/src/debug.rs index 5e1c7416..1c2a3a2c 100644 --- a/components/salsa-2022/src/debug.rs +++ b/components/salsa-2022/src/debug.rs @@ -14,7 +14,28 @@ use crate::database::AsSalsaDatabase; /// Implementations are automatically provided for `#[salsa::tracked]` /// items, though you can opt-out from that if you wish to provide a manual /// implementation. +/// +/// # WARNING: Intended for debug use only! +/// +/// Debug print-outs of tracked structs include the value of all their fields, +/// but the reads of those fields are ignored by salsa. This avoids creating +/// spurious dependencies from debugging code, but if you use the resulting +/// string to influence the outputs (return value, accumulators, etc) from your +/// query, salsa's dependency tracking will be undermined. +/// +/// If for some reason you *want* to incorporate dependency output into +/// your query, do not use the `debug` or `into_debug` helpers and instead +/// invoke `fmt` manually. pub trait DebugWithDb { + /// Creates a wrapper type that implements `Debug` but which + /// uses the `DebugWithDb::fmt`. + /// + /// # WARNING: Intended for debug use only! + /// + /// The wrapper type Debug impl will access the value of all + /// fields but those accesses are ignored by salsa. This is only + /// suitable for debug output. See [`DebugWithDb`][] trait comment + /// for more details. fn debug<'me, 'db>(&'me self, db: &'me Db) -> DebugWith<'me, Db> where Self: Sized + 'me, @@ -25,6 +46,15 @@ pub trait DebugWithDb { } } + /// Creates a wrapper type that implements `Debug` but which + /// uses the `DebugWithDb::fmt`. + /// + /// # WARNING: Intended for debug use only! + /// + /// The wrapper type Debug impl will access the value of all + /// fields but those accesses are ignored by salsa. This is only + /// suitable for debug output. See [`DebugWithDb`][] trait comment + /// for more details. fn into_debug<'me, 'db>(self, db: &'me Db) -> DebugWith<'me, Db> where Self: Sized + 'me, @@ -37,12 +67,25 @@ pub trait DebugWithDb { /// Format `self` given the database `db`. /// - /// Note: when invoked from `DebugWith`, - /// accesses to fields that occur within this method will be - /// disregarded as dependencies from the current query. + /// # Dependency tracking + /// + /// When invoked manually, field accesses that occur + /// within this method are tracked by salsa. But when invoked + /// the [`DebugWith`][] value returned by the [`debug`](`Self::debug`) + /// and [`into_debug`][`Self::into_debug`] methods, + /// those accesses are ignored. fn fmt(&self, f: &mut fmt::Formatter<'_>, db: &Db) -> fmt::Result; } +/// Helper type for the [`DebugWithDb`][] trait that +/// wraps a value and implements [`std::fmt::Debug`][], +/// redirecting calls to the `fmt` method from [`DebugWithDb`][]. +/// +/// # WARNING: Intended for debug use only! +/// +/// This type intentionally ignores salsa dependencies used +/// to generate the debug output. See the [`DebugWithDb`][] trait +/// for more notes on this. pub struct DebugWith<'me, Db: ?Sized + AsSalsaDatabase> { value: BoxRef<'me, dyn DebugWithDb + 'me>, db: &'me Db, From ce2f78290a5f38794bbf2e43f46dad60bcc37b78 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 3 Apr 2024 06:33:17 -0400 Subject: [PATCH 4/4] add FIXME --- components/salsa-2022-macros/src/salsa_struct.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/salsa-2022-macros/src/salsa_struct.rs b/components/salsa-2022-macros/src/salsa_struct.rs index dbdb0478..c6f7b331 100644 --- a/components/salsa-2022-macros/src/salsa_struct.rs +++ b/components/salsa-2022-macros/src/salsa_struct.rs @@ -73,6 +73,8 @@ impl SalsaStruct { .iter() .map(|attr| { if attr.path.is_ident("customize") { + // FIXME: this should be a comma separated list but I couldn't + // be bothered to remember how syn does this. let args: syn::Ident = attr.parse_args()?; if args.to_string() == "DebugWithDb" { Ok(vec![Customization::DebugWithDb])