Allow lifetime annotations in database_storage! and query_group!

[fabianschuiki]

Extend the database_storage! macro such that it allows for lifetimes to be added to the generated struct. This is necessary if the database itself has lifetime parameters that need to be threaded through to the queries being invoked.

The main goal is to allow arena allocation to be used in your database struct, and to pass those references around as query inputs and outputs.

Not sure this is the best solution -- I'd love to hear your thoughts.

[fabianschuiki]

I tried to adjust the query_group! macro, since lifetimes need to be annotated there as well. Turns out this is quite tricky and makes the entire macro absolutely unreadable. The main cause of trouble are nested repetitions. For example here:

$($trait_attr)* $v trait $query_trait <$($lts)*>:
    $(salsa::plumbing::GetQueryTable<$QueryType> +)* $($header)* { ... }

We're enumerating the different $QueryTypes, but for each you would need to attach the list of lifetime parameters: $QueryType<$($lts)*)>. That sadly doesn't work because I'm already repeating the $QueryType. Some sort of outer product would be nice here, but AFAIK there's no such thing in today's rust.

My usual go-to approach in this situation is to use recursion to perform the outer product. In this case however I can't get the parser to expand a macro in bounds position, e.g.

$($trait_attr)* $v trait $query_trait <$($lts)*>:
    query_group!(@trait_bounds ...) $($header)* { ... }
               ^
error: expected one of `(`, `+`, `::`, `<`, `=`, `where`, or `{`, found `!`

I'm pretty much stuck on this. The only option I see would be to have the user type the query type twice (as in type MyQuery, MyQuery<'t>;), once as name for the struct and once as type (including lifetimes) for use in bounds.

[fabianschuiki]

Added the complementary changes to query_group! and added a test case. Also dropped the 'static bound from Query. Not sure if this will cause future grief though.

Some future work:

• Make macros parametric over multiple lifetimes (Did this where I could, but in certain places I couldn't get the outer product between $QueryTypes and $lts to work.)
• In database_storage! the user currently has to annotate the lifetime on the query type, whereas in query_group! they must not. Would be better if the former implicitly attached the lifetime to the query type automatically, but outer product troubles me again.

[fabianschuiki]

Explored this a little bit further. In the implementation of QueryFunction<DB> for the different queries, we're currently passing &DB to the query function. This makes it difficult to embed an arena into the database:

struct DatabaseImpl<'a> {
    arena: typed_arena::Arena<SomeType<'a>>,
}

trait Database<'a>: salsa::Database {
    fn alloc(&'a self, x: SomeType<'a>) -> &'a SomeType<'a>;
}

impl<'a> Database<'a> for DatabaseImpl<'a> { ... }
query_group! { ... }
database_storage! { ... }

fn my_query<'a>(db: &impl Database<'a>) {
    db.alloc( ... ); // borrow checker sadness here
}

The call to alloc does not check out as it requires &'a impl Database<'a>, but we're only passing &impl Database<'a>.

One way around this would be to not have the arena be part of the database, but rather keep a reference to it and have the arena survive the database. But then in the case that we want to keep a per-database arena for temporary values, e.g. for snapshots, we would want the arena embedded within the database.

Maybe we could add an associated type Handle that we then pass by value into the query:

fn my_query<'a>(db: Database<'a>::Handle) { ... }

Not sure if that would help though.

Edit: Maybe we could add an argument to the two macros that allows an explicit lifetime to be annotated on database refs. This would allow the user to pick one of the lifetimes they annotated and declare that the "database lifetime". Or maybe we could make the database lifetime explicit somewhere in salsa::Database and then try to get the compiler to match up the lifetime with the query fn somehow?

[fabianschuiki]

Just bumped into the drop checker when trying to allocate into an arena held by the database, since it cannot guarantee that refs held in other fields of the database struct are dropped before the arena field.

Not sure how to work around this... looks like the salsa::Runtime is not trivially droppable. Needs further investigation. Will hold the arena outside the database struct for now.

[nikomatsakis]

Nice! The main thing that gives me pause is the need to add PhantomData to QueryType, but I don't see an immediate way around it. It's probably worth merging this (perhaps generalized to >1 lifetime parameter) for now, as it is not very disruptive, and then re-considering the query_mut(X) structure to see if there is another way (at minimum, you could certainly imagine query_mut::<X>(), which in a way might be more clear).

[nikomatsakis]

I wonder if, while we're at it, we want to support other sorts of parameters? Regardless, this should probably be something like $(<$($lt:lifetime,)*>)?, so that one can use the syntax <'a, 'b> (I forget, is ? stable yet..?)

[fabianschuiki]

That was stabilized end of Nov (rust-lang/rust#48075), but hasn't made it into 1.31.1.

[nikomatsakis]

Similarly here I would use a phantomdata like:

PhantomData<($(&$lt (),)*)>

which then also works regardless of how many lifetimes were provided.

But this is going to be a drag: some of the APIs really rely on being able to type Query as a value and have it work. Hmm.

Good reason to push harder on rust-lang/rfcs#2357 =)

[nikomatsakis]

Or, alternatively, perhaps a reason to rejigger some of those APIs..

[nikomatsakis]

e.g., db.query_mut(Foo).set(...)

[fabianschuiki]

Yeah I don't like this either... I don't see why the query struct couldn't be independent of the database's lifetime, after all it's just a means of naming the query. But my brain hasn't been able to come up with anything better 😞

[nikomatsakis]

Just bumped into the drop checker when trying to allocate into an arena held by the database, since it cannot guarantee that refs held in other fields of the database struct are dropped before the arena field.

I would expect the arena to be outside the database, yes. This is the general pattern in Rust when using an arena, for better or worse.

[nikomatsakis]

Turns out this is quite tricky and makes the entire macro absolutely unreadable.

Oh, I just saw this. I guess this is why it's currently handling at most one lifetime etc. Hmm. Maybe this argues for the move to procedural macros =)

[nikomatsakis]

I should really have read all your comments before reviewing, I guess =)

[fabianschuiki]

I should really have read all your comments before reviewing, I guess =)

No worries, I dumped a lot of text 🙈

[fabianschuiki]

Maybe this argues for the move to procedural macros =)

Yes that might actually be a good solution. Could also help in unifying the two macros probably?

[fabianschuiki]

I think the problem regarding lifetimes on query structs is the following. Assume we have a query Uppercase that we don't want to annotate a lifetime with:

#[derive(Debug, Default)]
struct Uppercase;

impl<'a, DB> salsa::Query<DB> for Uppercase
where
    DB: Database<'a>,
{
    type Key = String;
    type Value = &'a str;
    type Storage = salsa::plumbing::MemoizedStorage<DB, Self>;
}

impl<'a, DB> salsa::plumbing::QueryFunction<DB> for Uppercase
where
    DB: Database<'a>,
{
    fn execute(db: &DB, key: String) -> &'a str {
        uppercase(db, key)
    }
}

This fails with

error[E0207]: the lifetime parameter `'a` is not constrained by the impl trait, self type, or predicates       
  --> examples/arena/main.rs:34:6                                                                              
   |                                                                                                           
34 | impl<'a, DB> salsa::Query<DB> for Uppercase                                                               
   |      ^^ unconstrained lifetime parameter                                                                  

So we would need something that for any DB: Database could construct a type for Value, and thus hide the 'a.

[nikomatsakis]

I'm going to close this for now, since we are trying to implement the new procedural macro based approach first.