salsa-rs / salsa

A generic framework for on-demand, incrementalized computation. Inspired by adapton, glimmer, and rustc's query system.
https://salsa-rs.netlify.app/
Apache License 2.0
2.13k stars 152 forks source link

Missing `#[salsa::db]` on impl block should fail compilation #613

Open MichaReiser opened 3 days ago

MichaReiser commented 3 days ago

The following example misses a #[salsa::db] on the impl Db for Pimpl but it only fails at runtime with a panic in as_view.

Ideally, we would catch this error at compile time. If this isn't possible, improve the error message to guide users towards the proper fix.

use salsa::Accumulator;

#[salsa::db]
trait Db: salsa::Database {}

#[salsa::db]
pub(crate) struct Pimpl {
    storage: salsa::Storage<Self>,
}

#[salsa::db]
impl salsa::Database for Pimpl {
    fn salsa_event(&self, _event: &dyn Fn() -> salsa::Event) {}
}

// #[salsa::db] <-----MISSING
impl Db for Pimpl {
}

#[salsa::input]
struct File {
    #[return_ref]
    contents: String,
}

#[salsa::tracked]
struct Element<'db> {}

#[salsa::accumulator]
struct Diagnostic(&'static str);

#[salsa::tracked]
fn parse_single_file(db: &dyn Db, file: File) -> Option<Element<'_>> {
    Diagnostic("argh").accumulate(db);
    None
}

impl Pimpl {
    pub(crate) fn new() -> Self {
        Self {
            storage: Default::default(),
        }
    }

}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn it_works() {
        let mut pimpl = Pimpl::new();
        let this = &mut pimpl;
        let file = File::new(this, "Hello World!".to_string());
        let node = parse_single_file(this, file);
        // FIXME: This panics with 'Unwrap on None value'
        parse_single_file::accumulated::<Diagnostic>(this, file);
    }
}
NickNick commented 2 days ago

Without the #[salsa::db], a function zalsa_db() is required. Perhaps it would be possible to add another function fn you_forgot_salsa_db()?

Coming from someone who hasn't spent a lot of time with macros, but I thought that could be a quick first step towards making this a little more clear.