Skip to content
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

Idea: salsa::supertype and upcasting on Id #578

Open
nikomatsakis opened this issue Sep 26, 2024 · 2 comments
Open

Idea: salsa::supertype and upcasting on Id #578

nikomatsakis opened this issue Sep 26, 2024 · 2 comments
Assignees
Labels
bikeshed 🚴‍♀️ Debating API details and the like good first issue Good for newcomers
Milestone

Comments

@nikomatsakis
Copy link
Member

I've noticed that I often have an enum whose variants are all different salsa structs:

enum Item<'db> {
    Struct(Struct<'db>),
    Function(Function<'db>),
    ...
}

I was thinking about how this enum will use 2 words and not 1 and I realized that, given that each variant is just storing an Id, and that we can go from an Id to an IngredientIndex (it's stored on the page), we could have a proc-macro that takes such an enum and returns a special struct that just stores a single Id. It could then be converted into an enum readily enough. You also downcast it.

I'm imagining the following:

First, add two methods to salsa::Id, fn is<S>(self, db: &dyn Database) -> bool and fn downcast<S>(self, db: &dyn Database) -> Option<S>, where S: SalsaStructInDb<'_>. These methods can lookup the id in db.table, identify the ingredient index on the from the Page, compare that to the ingredient index for S, and then do the appropriate thing. (We might also want to tweak the FromId or other traits in a similar-ish way.)

Next, create a proc macro named supertype (bikeshed) which expects an enum with one lifetime argument and where each variant has a single argument:

#[salsa::supertype]
enum Foo<'db> {
    A(Bar<'db>),
    ...
    Z(Baz<'db>),
}

we generate the following:

// A struct to store the id itself
#[derive(Copy, Clone, ...)]
struct Foo<'db> { id: Id }

// Original enum, but with a tweaked name (should be configurable)
enum FooVariants<'db> {
    A(Bar<'db>),
    ...
    Z(Baz<'db>),
}

// From impls to "upcast" into the struct; these just take the `id`
impl<'db> From<Bar<'db>> for Foo<'db> { ... }

// From impls to "upcast" into the enum; these pick the appropriate variant
impl<'db> From<Bar<'db>> for FooVariants<'db> { ... }

// From impls from the variants struct
impl<'db> From<FooVariants<'db>> for Foo<'db> { ... }

impl Foo<'db> {
    // A method to convert from the struct into the variants 
    fn variants(self, db: &dyn salsa::Database) -> FooVariants<'db> {
        if let Some(v) = self.id.downcast::<Foo<'db>>(db) {
            return v.into();
        }

        // ...

        if let Some(v) = self.id.downcast::<Bar<'db>>(db) {
            return v.into();
        }

        unreachable!()
    }

    // Convenient downcast methods for each of the options
    fn a(self) -> Option<Foo<'db>> {
        self.id.downcast()
    }

    fn z(self) -> Option<Bar<'db>> {
        self.id.downcast()
    }
}
@nikomatsakis nikomatsakis added good first issue Good for newcomers bikeshed 🚴‍♀️ Debating API details and the like labels Sep 26, 2024
@nikomatsakis nikomatsakis added this to the Salsa 3.0 milestone Sep 26, 2024
@nikomatsakis
Copy link
Member Author

Mentoring instructions:

Methods on id

  • Add a method to TablePage trait to access the ingredient index. The impl can just return this field -- remove the #[allow(dead_code] annotation while you're at it.
  • We will need to add an accessor get_ingredient(&self, id: Id) -> IngredientIndex to Table, kind of like get, which gets the ingredient index for a given Id. It should call split_id and just take the first page (the page). It can then check self.pages.get(id) -- note that it should not call self.page::<T> because we don't know the T -- and invoke the method you added in the previous step.
  • Then we add inherent methods to Id, is and downcast. This is the trickiest part because I think we want to tweak the traits. I would start by modifying SalsaStructInDb -- probably rename it to SalsaStruct<'db> and add some methods like fn ingredient_index(db: &'db dyn salsa::Database) -> IngredientIndex and fn new(db: &'db dyn salsa::Database, id: Id) -> Self. We'll need to tweak the implementations created in the various macros in salsa-macro-rules and get everything to build. The ingredient_index method should invoke the existing ingredient method, which already has caching. The new method should probably assert! (maybe debug_assert!) that the ingredient for id matches Self::ingredient_index.
  • Once the previous step is done, is can be implemented by comparing S::ingredient_index() with the result of db.table.ingredient_index(id). Downcasting can be implemented by checking is and then invoking new.
  • Alternatively, you could name the methods in SalsaStructInDb something like is and downcast and just have salsa::Id forward to them.

Proc macro

  • Add a proc macro to salsa-macros crate, sort of like this one
    • That proc macro should parse the enum, check various criteria, and generate a salsa::plumbing::setup_supertype! macro invocation.
  • Add setup_supertype! to the salsa-macro-rules crate, sort of like this. That will generate the actual code.

@davidbarsky davidbarsky self-assigned this Nov 29, 2024
@davidbarsky
Copy link
Contributor

I've assigned this issue to myself; I'll need this functionality in Salsa in order to migrate rust-analyzer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bikeshed 🚴‍♀️ Debating API details and the like good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants