Skip to content

Skip update fallback if return type has non-static lifetime #896

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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 36 additions & 5 deletions components/salsa-macros/src/tracked_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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<T: #zalsa::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 {
Expand Down Expand Up @@ -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;
}
}
}
Comment on lines +332 to +339
Copy link
Member

Choose a reason for hiding this comment

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

This unfortunately breaks down with macro calls in the type, -> foo!() might expand to something introducing an elided lifetime. Likewise a user could use a Foo<'_> but without specifying the <'_> which warns post 2018 edition, but not error.

Copy link
Author

Choose a reason for hiding this comment

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

Both of those seem like reasonable gaps to me, since it will still fallback to the current behaviour. Although I could see the argument that it might create even more confusing behaviour.

What do you think the best path forward is? I have no problems with closing this PR if it doesn't seem worth it.


#[derive(Debug, PartialEq, Eq, Hash)]
enum FunctionType {
Constant,
Expand Down
60 changes: 48 additions & 12 deletions tests/compile-fail/tracked_fn_return_ref.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<tracked_fn_return_ref::Configuration_ as salsa::function::Configuration>::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`:
()
(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 `<tracked_fn_return_struct_containing_ref::Configuration_ as salsa::function::Configuration>::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`:
()
(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 `<tracked_fn_return_struct_containing_ref_elided_explicit::Configuration_ as salsa::function::Configuration>::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)
27 changes: 27 additions & 0 deletions tests/tracked_fn_return_static.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//! 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 {}

#[derive(Clone, PartialEq)]
struct NotUpdate<'a> {
_marker: std::marker::PhantomData<&'a ()>,
}

#[salsa::tracked]
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);
test(db, input);
})
}