From f2fbc8ed264e08804d48e5c183d92b4c5a693a8d Mon Sep 17 00:00:00 2001 From: Sam Scott Date: Tue, 3 Jun 2025 12:01:02 -0500 Subject: [PATCH 1/4] Don't attempt fallback if return type has non-static lifetime --- components/salsa-macros/src/tracked_fn.rs | 41 +++++++++++-- src/update.rs | 1 + .../compile-fail/tracked_fn_return_ref.stderr | 60 +++++++++++++++---- 3 files changed, 85 insertions(+), 17 deletions(-) diff --git a/components/salsa-macros/src/tracked_fn.rs b/components/salsa-macros/src/tracked_fn.rs index cb892b45b..98cac26cd 100644 --- a/components/salsa-macros/src/tracked_fn.rs +++ b/components/salsa-macros/src/tracked_fn.rs @@ -91,6 +91,12 @@ impl Macro { ty }); let output_ty = self.output_ty(&db_lt, &item)?; + let mut has_lifetime_visitor = HasLifetimeVisitor { + has_lifetime: false, + }; + syn::visit::visit_type(&mut has_lifetime_visitor, &output_ty); + let has_lifetime = has_lifetime_visitor.has_lifetime; + let (cycle_recovery_fn, cycle_recovery_initial, cycle_recovery_strategy) = self.cycle_recovery()?; let is_specifiable = self.args.specify.is_some(); @@ -184,11 +190,23 @@ impl Macro { UpdateDispatch::<#output_ty>::maybe_update }; let assert_return_type_is_update = if requires_update { - quote! { - #[allow(clippy::all, warnings)] - fn _assert_return_type_is_update<#db_lt>() { - use #zalsa::{UpdateFallback, UpdateDispatch}; - #maybe_update_path; + if has_lifetime { + // don't import the `UpdateFallback` if we have non-static + // lifetimes to avoid confusing errors + quote! { + #[allow(clippy::all, warnings)] + fn _assert_return_type_is_update<#db_lt>() { + fn _assert_is_update() { } + _assert_is_update::<#output_ty>(); + } + } + } else { + quote! { + #[allow(clippy::all, warnings)] + fn _assert_return_type_is_update<#db_lt>() { + use #zalsa::{UpdateFallback, UpdateDispatch}; + #maybe_update_path; + } } } } else { @@ -307,6 +325,19 @@ impl syn::visit_mut::VisitMut for ToDbLifetimeVisitor { } } +struct HasLifetimeVisitor { + has_lifetime: bool, +} + +impl<'ast> syn::visit::Visit<'ast> for HasLifetimeVisitor { + fn visit_lifetime(&mut self, l: &'ast syn::Lifetime) { + // We don't consider `'static` to be a lifetime in this context. + if l.ident != "static" { + self.has_lifetime = true; + } + } +} + #[derive(Debug, PartialEq, Eq, Hash)] enum FunctionType { Constant, diff --git a/src/update.rs b/src/update.rs index d95bd13a9..9e32632d6 100644 --- a/src/update.rs +++ b/src/update.rs @@ -494,6 +494,7 @@ fallback_impl! { compact_str::CompactString, } macro_rules! tuple_impl { ($($t:ident),*; $($u:ident),*) => { + #[diagnostic::do_not_recommend] unsafe impl<$($t),*> Update for ($($t,)*) where $($t: Update,)* diff --git a/tests/compile-fail/tracked_fn_return_ref.stderr b/tests/compile-fail/tracked_fn_return_ref.stderr index ee8595edc..6b97e1c36 100644 --- a/tests/compile-fail/tracked_fn_return_ref.stderr +++ b/tests/compile-fail/tracked_fn_return_ref.stderr @@ -29,26 +29,62 @@ warning: elided lifetime has a name 43 | ) -> ContainsRef<'_> { | ^^ this elided lifetime gets resolved as `'db` -error: lifetime may not live long enough +error[E0277]: the trait bound `&'db str: Update` is not satisfied --> tests/compile-fail/tracked_fn_return_ref.rs:15:67 | 15 | fn tracked_fn_return_ref<'db>(db: &'db dyn Db, input: MyInput) -> &'db str { - | --- lifetime `'db` defined here ^ requires that `'db` must outlive `'static` + | ^^^^^^^^ the trait `Update` is not implemented for `&'db str` + | + = help: the trait `Update` is implemented for `String` +note: required by a bound in `::execute::_assert_return_type_is_update::_assert_is_update` + --> tests/compile-fail/tracked_fn_return_ref.rs:14:1 + | +14 | #[salsa::tracked] + | ^^^^^^^^^^^^^^^^^ required by this bound in `_assert_is_update` + = note: this error originates in the attribute macro `salsa::tracked` (in Nightly builds, run with -Z macro-backtrace for more info) -error: lifetime may not live long enough +error[E0277]: the trait bound `ContainsRef<'db>: Update` is not satisfied --> tests/compile-fail/tracked_fn_return_ref.rs:23:6 | -20 | fn tracked_fn_return_struct_containing_ref<'db>( - | --- lifetime `'db` defined here -... 23 | ) -> ContainsRef<'db> { - | ^^^^^^^^^^^ requires that `'db` must outlive `'static` + | ^^^^^^^^^^^^^^^^ the trait `Update` is not implemented for `ContainsRef<'db>` + | + = help: the following other types implement trait `Update`: + Arc + BTreeMap + BTreeSet + Box + Box<[T]> + HashMap + HashSet + Infallible + and $N others +note: required by a bound in `::execute::_assert_return_type_is_update::_assert_is_update` + --> tests/compile-fail/tracked_fn_return_ref.rs:19:1 + | +19 | #[salsa::tracked] + | ^^^^^^^^^^^^^^^^^ required by this bound in `_assert_is_update` + = note: this error originates in the attribute macro `salsa::tracked` (in Nightly builds, run with -Z macro-backtrace for more info) -error: lifetime may not live long enough +error[E0277]: the trait bound `ContainsRef<'db>: Update` is not satisfied --> tests/compile-fail/tracked_fn_return_ref.rs:43:6 | -40 | fn tracked_fn_return_struct_containing_ref_elided_explicit<'db>( - | --- lifetime `'db` defined here -... 43 | ) -> ContainsRef<'_> { - | ^^^^^^^^^^^ requires that `'db` must outlive `'static` + | ^^^^^^^^^^^^^^^ the trait `Update` is not implemented for `ContainsRef<'db>` + | + = help: the following other types implement trait `Update`: + Arc + BTreeMap + BTreeSet + Box + Box<[T]> + HashMap + HashSet + Infallible + and $N others +note: required by a bound in `::execute::_assert_return_type_is_update::_assert_is_update` + --> tests/compile-fail/tracked_fn_return_ref.rs:39:1 + | +39 | #[salsa::tracked] + | ^^^^^^^^^^^^^^^^^ required by this bound in `_assert_is_update` + = note: this error originates in the attribute macro `salsa::tracked` (in Nightly builds, run with -Z macro-backtrace for more info) From 38e20bb59659a90342c1b0a3e3b5211db72adb07 Mon Sep 17 00:00:00 2001 From: Sam Scott Date: Tue, 3 Jun 2025 12:06:19 -0500 Subject: [PATCH 2/4] Add test that statics still work --- tests/tracked_fn_return_static.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 tests/tracked_fn_return_static.rs diff --git a/tests/tracked_fn_return_static.rs b/tests/tracked_fn_return_static.rs new file mode 100644 index 000000000..39c044e2b --- /dev/null +++ b/tests/tracked_fn_return_static.rs @@ -0,0 +1,21 @@ +//! Test that a `tracked` function can return a +//! non-`Update` type, so long as it has `'static` lifetime. + +use salsa::Database; + +#[salsa::input] +struct Input {} + +#[salsa::tracked] +fn test(_db: &dyn salsa::Database, _: Input) -> &'static str { + "test" +} + +#[test] +fn invoke() { + salsa::DatabaseImpl::new().attach(|db| { + let input = Input::new(db); + let x: &str = test(db, input); + assert_eq!(x, "test"); + }) +} From 9364f15dad14369d35f77a0aad21120038666b4c Mon Sep 17 00:00:00 2001 From: Sam Scott Date: Tue, 3 Jun 2025 12:08:56 -0500 Subject: [PATCH 3/4] Use a custom test in case `&'static str` later implements Update --- tests/tracked_fn_return_static.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/tracked_fn_return_static.rs b/tests/tracked_fn_return_static.rs index 39c044e2b..5c2f88e7f 100644 --- a/tests/tracked_fn_return_static.rs +++ b/tests/tracked_fn_return_static.rs @@ -6,16 +6,22 @@ use salsa::Database; #[salsa::input] struct Input {} +#[derive(Clone, PartialEq)] +struct NotUpdate<'a> { + _marker: std::marker::PhantomData<&'a ()>, +} + #[salsa::tracked] -fn test(_db: &dyn salsa::Database, _: Input) -> &'static str { - "test" +fn test(_db: &dyn salsa::Database, _: Input) -> NotUpdate<'static> { + NotUpdate { + _marker: std::marker::PhantomData, + } } #[test] fn invoke() { salsa::DatabaseImpl::new().attach(|db| { let input = Input::new(db); - let x: &str = test(db, input); - assert_eq!(x, "test"); + test(db, input); }) } From d812631d157a94f1f22d307a256d5bd575832694 Mon Sep 17 00:00:00 2001 From: Sam Scott Date: Tue, 3 Jun 2025 12:16:09 -0500 Subject: [PATCH 4/4] Remove unsupported attribute --- src/update.rs | 1 - .../compile-fail/tracked_fn_return_ref.stderr | 32 +++++++++---------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/update.rs b/src/update.rs index 9e32632d6..d95bd13a9 100644 --- a/src/update.rs +++ b/src/update.rs @@ -494,7 +494,6 @@ fallback_impl! { compact_str::CompactString, } macro_rules! tuple_impl { ($($t:ident),*; $($u:ident),*) => { - #[diagnostic::do_not_recommend] unsafe impl<$($t),*> Update for ($($t,)*) where $($t: Update,)* diff --git a/tests/compile-fail/tracked_fn_return_ref.stderr b/tests/compile-fail/tracked_fn_return_ref.stderr index 6b97e1c36..d08702199 100644 --- a/tests/compile-fail/tracked_fn_return_ref.stderr +++ b/tests/compile-fail/tracked_fn_return_ref.stderr @@ -50,14 +50,14 @@ error[E0277]: the trait bound `ContainsRef<'db>: Update` is not satisfied | ^^^^^^^^^^^^^^^^ the trait `Update` is not implemented for `ContainsRef<'db>` | = help: the following other types implement trait `Update`: - Arc - BTreeMap - BTreeSet - Box - Box<[T]> - HashMap - HashSet - Infallible + () + (A, B) + (A, B, C) + (A, B, C, D) + (A, B, C, D, E) + (A, B, C, D, E, F) + (A, B, C, D, E, F, G) + (A, B, C, D, E, F, G, H) and $N others note: required by a bound in `::execute::_assert_return_type_is_update::_assert_is_update` --> tests/compile-fail/tracked_fn_return_ref.rs:19:1 @@ -73,14 +73,14 @@ error[E0277]: the trait bound `ContainsRef<'db>: Update` is not satisfied | ^^^^^^^^^^^^^^^ the trait `Update` is not implemented for `ContainsRef<'db>` | = help: the following other types implement trait `Update`: - Arc - BTreeMap - BTreeSet - Box - Box<[T]> - HashMap - HashSet - Infallible + () + (A, B) + (A, B, C) + (A, B, C, D) + (A, B, C, D, E) + (A, B, C, D, E, F) + (A, B, C, D, E, F, G) + (A, B, C, D, E, F, G, H) and $N others note: required by a bound in `::execute::_assert_return_type_is_update::_assert_is_update` --> tests/compile-fail/tracked_fn_return_ref.rs:39:1