✅ Deploy Preview for salsa-rs canceled.

CodSpeed Performance Report

Merging #632 will not alter performance

_{Comparing davidbarsky:davidbarsky/add-interned-sans-lifetime-macro (6d23f3b) with master (0ac5c1c)}

Summary

✅ 9 untouched benchmarks

@davidbarsky ....

---- components/salsa-macros/src/lib.rs - interned_sans_lifetime (line 91) stdout ----
error[E0433]: failed to resolve: use of undeclared crate or module `salsa`
  --> components/salsa-macros/src/lib.rs:107:3
   |
18 | #[salsa::interned_sans_lifetime(id = CustomSalsaIdWrapper)]
   |   ^^^^^ use of undeclared crate or module `salsa`

We discussed that we want to discourage this pattern but it would also be nice if we can avoid duplicating the interned macro. Would it be possible to gate the functionality behind a feature? E.g. that the interned macro only supports a no_lifetime argument if salsa_macros is used with the compat feature.

Given the goal is for this to only ease the transition for r-a it should be fine (and honestly preferred in my eyes) to gate this behind a feature until r-a no longer needs it (at which point we can remove it again).

To expand as to why this is a separate macro instead of something like #[salsa::interned(no_lifetime)], there are two reasons:

1. I wrote this for expediency's sake to confirm whether this is something that I even want/need in rust-analyzer. Copying is pretty easy, after all!
2. I didn't know how to make $db_lt optional in a macro_rules-based macro: namely, it's not clear to me how I can do control flow as I'd do in a proc macro, but this might be my inexperience with macro_rules :).
   1. If it's not possible to do control flow in a macro_rules-based macro; a default lifetime parameter a la https://internals.rust-lang.org/t/default-lifetime-parameters/12073 would've been great, but, unfortunately, that doesn't exist today.

It's hard to tell for me what the difference is between the two macros but would it be possible to change $db_lt to either be <'db> (so that it includes the <..> or an empty string ifno_lifetime` is used.

Or you might be able to do something similar to clone and no_clone

https://github.com/MichaReiser/salsa/blob/e68679b3a9c2b5cfd8eab92de89edf4073b03601/components/salsa-macro-rules/src/maybe_clone.rs#L5-L21

It's hard to tell for me what the difference is between the two macros

I do too, but difftastic helped here. I don't think it's a particularly useful paste, but if you have it handy, difft components/salsa-macro-rules/src/setup_interned_struct.rs components/salsa-macro-rules/src/setup_interned_struct_sans_lifetime.rs will show the difference. The primary differences are:

1. The addition of a StructData parameter to the macro. I didn't want pull in paste, so I just made the ident in the proc macro.
2. The addition of the $Id parameter, to allow the ID overriding behavior we need in rust-analyzer.
3. Moving salsa::plumbing::interned::Configuration out of the const _: () = {...} block in order to get the data through the underlying ID alone.
4. The removal of lifetimes and a few select additions of 'static, as needed.

I honestly recommend cloning this PR and diffing via difftastic.

would it be possible to change $db_lt to either be <'db> (so that it includes the <..>) or an empty string if no_lifetime is used?

Not that I'm aware of, at least with macro_rules!-based macros. I don't think we can invoke maybe_*-style macros in the type position. From a "how can we unify these macros", I think it'd require moving things away from salsa-macro-rules into a (mostly) pure quote!-based approach.

I fixed the rustdoc failures, but I can't reproduce the test failure w.r.t to cancellation on my Mac. That's a bit concerning, but I think I'll try and debug this tomorrow.