Skip to content

Emit self ty for query debug name of assoc function queries #927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Veykril merged 1 commit into from
Jun 27, 2025
Merged

Emit self ty for query debug name of assoc function queries #927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions components/salsa-macro-rules/src/setup_tracked_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ macro_rules! setup_tracked_fn {

assert_return_type_is_update: {$($assert_return_type_is_update:tt)*},

$(self_ty: $self_ty:ty,)?

// Annoyingly macro-rules hygiene does not extend to items defined in the macro.
// We have the procedural macro generate names for those items that are
// not used elsewhere in the user's code.
Expand Down Expand Up @@ -139,7 +141,7 @@ macro_rules! setup_tracked_fn {
file: file!(),
line: line!(),
};
const DEBUG_NAME: &'static str = concat!(stringify!($fn_name), "::interned_arguments");
const DEBUG_NAME: &'static str = concat!($(stringify!($self_ty), "::",)? stringify!($fn_name), "::interned_arguments");

type Fields<$db_lt> = ($($interned_input_ty),*);

Expand Down Expand Up @@ -185,7 +187,7 @@ macro_rules! setup_tracked_fn {
file: file!(),
line: line!(),
};
const DEBUG_NAME: &'static str = stringify!($fn_name);
const DEBUG_NAME: &'static str = concat!($(stringify!($self_ty), "::", )? stringify!($fn_name));

type DbView = dyn $Db;

Expand Down
1 change: 1 addition & 0 deletions components/salsa-macros/src/accumulator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ impl AllowedOptions for Accumulator {
const ID: bool = false;
const REVISIONS: bool = false;
const HEAP_SIZE: bool = false;
const SELF_TY: bool = false;
}

struct StructMacro {
Expand Down
2 changes: 2 additions & 0 deletions components/salsa-macros/src/input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ impl crate::options::AllowedOptions for InputStruct {
const REVISIONS: bool = false;

const HEAP_SIZE: bool = false;

const SELF_TY: bool = false;
}

impl SalsaStructAllowedOptions for InputStruct {
Expand Down
2 changes: 2 additions & 0 deletions components/salsa-macros/src/interned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ impl crate::options::AllowedOptions for InternedStruct {
const REVISIONS: bool = true;

const HEAP_SIZE: bool = false;

const SELF_TY: bool = false;
}

impl SalsaStructAllowedOptions for InternedStruct {
Expand Down
101 changes: 101 additions & 0 deletions components/salsa-macros/src/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ pub(crate) struct Options<A: AllowedOptions> {
/// If this is `Some`, the value is the provided `heap_size` function.
pub heap_size_fn: Option<syn::Path>,

/// The `self_ty = <Ty>` option is used to set the the self type of the tracked impl for tracked
/// functions. This is merely used to refine the query name.
pub self_ty: Option<syn::Type>,

/// Remember the `A` parameter, which plays no role after parsing.
phantom: PhantomData<A>,
}
Expand All @@ -130,6 +134,7 @@ impl<A: AllowedOptions> Default for Options<A> {
id: Default::default(),
revisions: Default::default(),
heap_size_fn: Default::default(),
self_ty: Default::default(),
}
}
}
Expand All @@ -153,6 +158,7 @@ pub(crate) trait AllowedOptions {
const ID: bool;
const REVISIONS: bool;
const HEAP_SIZE: bool;
const SELF_TY: bool;
}

type Equals = syn::Token![=];
Expand Down Expand Up @@ -416,6 +422,22 @@ impl<A: AllowedOptions> syn::parse::Parse for Options<A> {
"`heap_size` option not allowed here",
));
}
} else if ident == "self_ty" {
if A::SELF_TY {
let _eq = Equals::parse(input)?;
let ty = syn::Type::parse(input)?;
if let Some(old) = options.self_ty.replace(ty) {
return Err(syn::Error::new(
old.span(),
"option `self_ty` provided twice",
));
}
} else {
return Err(syn::Error::new(
ident.span(),
"`self_ty` option not allowed here",
));
}
} else {
return Err(syn::Error::new(
ident.span(),
Expand All @@ -433,3 +455,82 @@ impl<A: AllowedOptions> syn::parse::Parse for Options<A> {
Ok(options)
}
}
impl<A: AllowedOptions> quote::ToTokens for Options<A> {
fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) {
Copy link
Contributor

@MichaReiser MichaReiser Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read the diff to closely but what's this used for (should it replace some existing code?)

Copy link
Member Author

@Veykril Veykril Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to inject arguments into the #[tracked] invocation supplied by the user, see https://github.com/salsa-rs/salsa/pull/927/files#diff-7091e579afd9cfe8ee883486f5951094096ebf6419e4e5fc5d1d9be926a858fdR86-R99

Copy link
Contributor

@MichaReiser MichaReiser Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this goes beyond what I understand about macros 😅

let Self {
returns,
no_eq,
debug,
no_lifetime,
singleton,
specify,
non_update_return_type,
db_path,
cycle_fn,
cycle_initial,
cycle_result,
data,
lru,
constructor_name,
id,
revisions,
heap_size_fn,
self_ty,
phantom: _,
} = self;
if let Some(returns) = returns {
tokens.extend(quote::quote! { returns(#returns), });
};
if no_eq.is_some() {
tokens.extend(quote::quote! { no_eq, });
}
if debug.is_some() {
tokens.extend(quote::quote! { debug, });
}
if no_lifetime.is_some() {
tokens.extend(quote::quote! { no_lifetime, });
}
if singleton.is_some() {
tokens.extend(quote::quote! { singleton, });
}
if specify.is_some() {
tokens.extend(quote::quote! { specify, });
}
if non_update_return_type.is_some() {
tokens.extend(quote::quote! { unsafe(non_update_return_type), });
}
if let Some(db_path) = db_path {
tokens.extend(quote::quote! { db = #db_path, });
}
if let Some(cycle_fn) = cycle_fn {
tokens.extend(quote::quote! { cycle_fn = #cycle_fn, });
}
if let Some(cycle_initial) = cycle_initial {
tokens.extend(quote::quote! { cycle_initial = #cycle_initial, });
}
if let Some(cycle_result) = cycle_result {
tokens.extend(quote::quote! { cycle_result = #cycle_result, });
}
if let Some(data) = data {
tokens.extend(quote::quote! { data = #data, });
}
if let Some(lru) = lru {
tokens.extend(quote::quote! { lru = #lru, });
}
if let Some(constructor_name) = constructor_name {
tokens.extend(quote::quote! { constructor = #constructor_name, });
}
if let Some(id) = id {
tokens.extend(quote::quote! { id = #id, });
}
if let Some(revisions) = revisions {
tokens.extend(quote::quote! { revisions = #revisions, });
}
if let Some(heap_size_fn) = heap_size_fn {
tokens.extend(quote::quote! { heap_size_fn = #heap_size_fn, });
}
if let Some(self_ty) = self_ty {
tokens.extend(quote::quote! { self_ty = #self_ty, });
}
}
}
7 changes: 7 additions & 0 deletions components/salsa-macros/src/tracked_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ impl crate::options::AllowedOptions for TrackedFn {
const REVISIONS: bool = false;

const HEAP_SIZE: bool = true;

const SELF_TY: bool = true;
}

struct Macro {
Expand Down Expand Up @@ -199,6 +201,10 @@ impl Macro {
} else {
quote! {}
};
let self_ty = match &self.args.self_ty {
Some(ty) => quote! { self_ty: #ty, },
None => quote! {},
};

Ok(crate::debug::dump_tokens(
fn_name,
Expand All @@ -224,6 +230,7 @@ impl Macro {
lru: #lru,
return_mode: #return_mode,
assert_return_type_is_update: { #assert_return_type_is_update },
#self_ty
unused_names: [
#zalsa,
#Configuration,
Expand Down
15 changes: 12 additions & 3 deletions components/salsa-macros/src/tracked_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl Macro {
return Ok(());
};

let self_ty = &impl_item.self_ty;
let self_ty = &*impl_item.self_ty;

let Some(tracked_attr_index) = fn_item.attrs.iter().position(|a| self.is_tracked_attr(a))
else {
Expand All @@ -83,11 +83,20 @@ impl Macro {
let mut change = ChangeSelfPath::new(self_ty, trait_);
change.visit_impl_item_fn_mut(fn_item);

let salsa_tracked_attr = fn_item.attrs.remove(tracked_attr_index);
let args: FnArgs = match &salsa_tracked_attr.meta {
let mut salsa_tracked_attr = fn_item.attrs.remove(tracked_attr_index);
let mut args: FnArgs = match &salsa_tracked_attr.meta {
syn::Meta::Path(..) => Default::default(),
_ => salsa_tracked_attr.parse_args()?,
};
if args.self_ty.is_none() {
// If the user did not specify a self_ty, we use the impl's self_ty
args.self_ty = Some(self_ty.clone());
Copy link
Contributor

@MichaReiser MichaReiser Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason or use case where a user needs to provide a self type? Can you add a test for it (or remove the attribute)

Copy link
Member Author

@Veykril Veykril Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user isn't really supposed to use this, this is solely needed for tracked impls to pass through their self type to the assoc tracked fn so that it can adjust the debug name appropriately

}
salsa_tracked_attr.meta = syn::Meta::List(syn::MetaList {
path: salsa_tracked_attr.path().clone(),
delimiter: syn::MacroDelimiter::Paren(syn::token::Paren::default()),
tokens: quote! {#args},
});

let InnerTrait = self.hygiene.ident("InnerTrait");
let inner_fn_name = self.hygiene.ident(&fn_item.sig.ident.to_string());
Expand Down
2 changes: 2 additions & 0 deletions components/salsa-macros/src/tracked_struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ impl crate::options::AllowedOptions for TrackedStruct {
const REVISIONS: bool = false;

const HEAP_SIZE: bool = false;

const SELF_TY: bool = false;
}

impl SalsaStructAllowedOptions for TrackedStruct {
Expand Down
2 changes: 1 addition & 1 deletion tests/tracked_assoc_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,6 @@ fn debug_name() {
assert_eq!(output.field(&db), 88);
db.assert_logs(expect![[r#"
[
"salsa_event(WillExecute { database_key: tracked_trait_fn_(Id(0)) })",
"salsa_event(WillExecute { database_key: MyOutput < 'db >::tracked_trait_fn_(Id(0)) })",
Copy link
Member Author

@Veykril Veykril Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting here is a bit unfortunate, but we the only other options are to either hand format these (I'd rather not) instead of relying on stringify! behavior or pulling in something like syn pretty (forgot the exact name). I think this is fine (and either way an improvement over the status quo)

Copy link
Contributor

@MichaReiser MichaReiser Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean https://github.com/dtolnay/prettyplease

I don't think it's worth the dependency, given that this is for debugging only.

]"#]]);
}
2 changes: 1 addition & 1 deletion tests/tracked_method.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,6 @@ fn debug_name() {
assert_eq!(object.tracked_trait_fn(&db), 88);
db.assert_logs(expect![[r#"
[
"salsa_event(WillExecute { database_key: tracked_trait_fn_(Id(0)) })",
"salsa_event(WillExecute { database_key: MyInput::tracked_trait_fn_(Id(0)) })",
]"#]]);
}