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

Conversation

samscott89
Copy link

I had a return type like Result<Tracked<'db>, Error> and the error message was complaining about 'db not being 'static. It took me a while to figure out that the 'db wasn't the problem, it was that my Error type doesn't implement Update.

Since tracking down where the 'static bound comes it's fairly obvious why, but for future people this might provide a slightly easier debugging experience.

Copy link

netlify bot commented Jun 3, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit d812631
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/683f2de40dec6c00088cccdf

@samscott89 samscott89 changed the title Sam/compile err non update ref Skip update fallback if return type has non-static lifetime Jun 3, 2025
src/update.rs Outdated
@@ -494,6 +494,7 @@ fallback_impl! { compact_str::CompactString, }

macro_rules! tuple_impl {
($($t:ident),*; $($u:ident),*) => {
#[diagnostic::do_not_recommend]
Copy link
Author

Choose a reason for hiding this comment

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

This also seemed like a nice quality of life improvement. The error message just lists all the tuple types otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, not supported in 1.80 though

Copy link
Author

Choose a reason for hiding this comment

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

Removed d812631

Copy link
Member

Choose a reason for hiding this comment

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

You can bring this back, we bumped the MSRV now (needs a rebase then)

Copy link
Author

Choose a reason for hiding this comment

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

ah nice, will do then

Copy link

codspeed-hq bot commented Jun 3, 2025

CodSpeed Performance Report

Merging #896 will not alter performance

Comparing samscott89:sam/compile-err-non-update-ref (d812631) with master (79c2eac)

Summary

✅ 12 untouched benchmarks

Comment on lines +332 to +339
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;
}
}
}
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants