internal: Migrate HirDatabase away from #[query_group]; remove the latter#22831
internal: Migrate HirDatabase away from #[query_group]; remove the latter#22831ada4a wants to merge 5 commits into
HirDatabase away from #[query_group]; remove the latter#22831Conversation
- don't `push_ty_diagnostics` with an empty list - use `lookup_resolver`
The next commit would require us to `impl HirDatabase for dyn SourceDatabase + Debug`, but that wouldn't work, as trait objects can't specify multiple (non-auto) traits. Since we're going to change all the users of `dyn HirDatabase` to `dyn SourceDatabase` anyway, we can push the `Debug` bound down to the latter.
ada4a
changed the title
HirDatabase away from #[query_group]; remove the latterHirDatabase away from #[query_group]; remove the latter
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
I don't like the many places that need to explicitly coerce to &dyn SourceDatabase. I think a better solution is to add fn as_dyn(&self) -> &dyn SourceDatabase to RootDatabase and TestDB. Maybe even implement Deref, although that's probably a bad idea.
The remaining methods in the query group didn't really have a good place to migrate them to, as they were all defined on `hir-def` types. Therefore, we basically left them inside `HirDatabase`, but changed the latter from a `#[query_group]` to an extension trait on `SourceDatabase`. This means that, in order to use its queries, one needs to bring it into scope. In particular, when working with a concrete `TestDB`/`RootDatabase`, one needs to first explicitly cast it to a `&dyn SourceDatabase`, so that the compiler can then cast it to `&dyn HirDatabase` as needed. To make this more ergonomic, we introduce the `as_dyn` method on `TestDB` and `RootDatabase`.
5b54e09 to
c110819
Compare
|
Done. Though I worry that future contributors will have a hard time discovering |
| pub trait HirDatabase: SourceDatabase { | ||
| fn as_dyn_source_database(&self) -> &dyn SourceDatabase; | ||
|
|
||
| // region:mir | ||
|
|
||
| // FIXME: Collapse `mir_body_for_closure` into `mir_body` | ||
| // and `monomorphized_mir_body_for_closure` into `monomorphized_mir_body` | ||
| #[salsa::transparent] | ||
| fn mir_body(&self, def: InferBodyId) -> Result<&MirBody, MirLowerError> { | ||
| crate::mir::mir_body_query(self, def).map_err(|err| err.clone()) | ||
| let db = self.as_dyn_source_database(); | ||
| crate::mir::mir_body_query(db, def).map_err(|err| err.clone()) |
There was a problem hiding this comment.
Given we are keeping the database trait, why do we even need to swap all the trait objects from HirDatabase to SourceDatabase?
There was a problem hiding this comment.
Wouldn't doing otherwise lead to circular reasoning? mir_body_query would require the caller to be &dyn HirDatabase, but we'd want to call it inside HirDatabase::mir_body, which.. doesn't yet know that the caller implements HirDatabase, as we're in the process of implementing it.
Besides, it looks like I can't annotate functions taking db: &dyn HirDatabase with #[salsa::tracked], probably because the it's no longer a salsa db:
error[E0576]: cannot find method or associated constant `zalsa_register_downcaster` in trait `HirDatabase`
--> crates/hir-ty/src/mir/lower.rs:2285:1
|
2285 | #[salsa_macros::tracked(returns(as_ref), cycle_result = mir_body_cycle_result)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in `HirDatabase`
|
= note: this error originates in the macro `salsa::plumbing::setup_tracked_fn` which comes from the expansion of the attribute macro `salsa_macros::tracked` (in Nightly builds, run with -Z macro-backtrace for more info)
There was a problem hiding this comment.
That salsa error is because you need to annotate the trait with #[salsa::database] (or some attribute name).
I don't see a circular reasoning here. Sure, things can end up recursive in calls (with different function arguments etc) but thats the case already today. In general in Rust inside of trait definitions the Self generic argument is assumed to implement the trait itself so you can use that fact just fine.
There was a problem hiding this comment.
If I remove the let db = self.as_dyn_source_database(); line and try to pass self directly, I get back to this error:
error[E0277]: the size for values of type `Self` cannot be known at compilation time
--> crates/hir-ty/src/db.rs:48:36
|
48 | crate::mir::mir_body_query(self, def).map_err(|err| err.clone())
| ^^^^ doesn't have a size known at compile-time
|
= note: required for the cast from `&Self` to `&(dyn HirDatabase + 'static)`
help: consider further restricting `Self`
|
47 | fn mir_body(&self, def: InferBodyId) -> Result<&MirBody, MirLowerError> where Self: std::marker::Sized {
|
Do you know how I'd solve that?...
There was a problem hiding this comment.
I think we need a 'static super trait bound? That is trait HirDatabase: SourceDatabase + 'static
There was a problem hiding this comment.
But that would require the implementer to be dyn SourceDatabase + 'static, which doesn't sound right? Well, it didn't work anyway...
There was a problem hiding this comment.
It looks like I can fix this by, once again, moving the method definitions from the trait definition to the impl. But that's ugly...
4 participants
Resolves #22657 🎉
See #t-compiler/rust-analyzer > Migrating `HirDatabase` for discussion, and individual commits for details.