feat: Switch from Chalk to the next trait solver #20329
Conversation
| @@ -48,7 +48,8 @@ impl ToTokens for TrackedQuery { | |||
| quote!(#(#options),*) | |||
| }) | |||
| .into_iter() | |||
| .chain(self.lru.map(|lru| quote!(lru = #lru))); | |||
| .chain(self.lru.map(|lru| quote!(lru = #lru))) | |||
| .chain(Some(quote!(unsafe(non_update_return_type)))); | |||
There was a problem hiding this comment.
That should be changed before merge, right?
There was a problem hiding this comment.
Sure, if someone can point me to how to fix it.
| mod inhabitedness; | ||
| mod interner; | ||
| mod lower; | ||
| mod lower_nextsolver; |
There was a problem hiding this comment.
Are all those X_nextsolver modules supposed to replace their original before merge?
There was a problem hiding this comment.
I think that's pretty much impossible if we want this to land anytime soon. There is so much that relies on the non-next solver modules that goes beyond trait solving.
crates/hir-ty/src/db.rs
Outdated
| #[salsa::cycle(cycle_result = crate::drop::has_drop_glue_cycle_result)] | ||
| fn has_drop_glue(&self, ty: Ty, env: Arc<TraitEnvironment>) -> DropGlue; | ||
|
|
||
| #[salsa::invoke(crate::layout_nextsolver::layout_of_adt_query)] |
There was a problem hiding this comment.
The fully qualified names everywhere (crate::X_nextsolver::X) make the code harder to read, but I suppose we'll get rid of that once we remove the Chalk types.
There was a problem hiding this comment.
Yeah, if there are specific places that make sense to remove them, let me know. But unfortunately a lot of them are kind of necessary.
0c5be1c to
57c7b7d
Compare
|
Squashed and rebased version here: jackh726@5fa8436 Will switch this PR after a bit of review, just so people can see commits if they need. |
eb38567 to
ab8258d
Compare
93688bb to
fbf45f7
Compare
|
Regarding the remaining todos on the issue tracker itself, I think squashing commits would be good (or just cleaning them up a bit), upstreaming things I feel like can be done after merge as well so not that important. Tests I'll plan on looking at this weekend. And then I think we are mostly ready? (object if not, I have not been catching up with things here and on zulip). So one more thing we ought to do is check if there is any important unrelated bug fix we wanna get out on the last stable before this merge. |
1e93992 to
84377be
Compare
| ex Bar | ||
| ex Bar::foo() | ||
| ex bar() |
There was a problem hiding this comment.
Not really a regression by the PR and so not relevant, but all 3 of these are wrong, we shouldn't show expression completions in this position
| @@ -108,7 +108,7 @@ pub(crate) fn goto_definition( | |||
| } | |||
|
|
|||
| Some( | |||
| IdentClass::classify_node(sema, &parent)? | |||
| salsa::attach(sema.db, || IdentClass::classify_node(sema, &parent))? | |||
There was a problem hiding this comment.
We should probably have the various Analysis methods just attach the salsa db instead, that is in Analysis::with_db. That should allow us to remove all those intermediate salsa::attach calls in the ide crates (except for tests setups maybe)
There was a problem hiding this comment.
I did think about this and tried to change with_db to
let snap = &snap;
salsa::attach(snap, || {
Cancelled::catch(|| f(snap))
})
which results in Cannot change database mid-query.
There was a problem hiding this comment.
That is odd, and sounds kind of worrying ... I can take a look at that later maybe.
There was a problem hiding this comment.
Ah okay at least part of this is due to syntax highlighting building a new db for comments, so probably not that problematic. A change for later then.
e3f8237 to
9418a3f
Compare
|
I've squashed the commits. |
|
What benefits does this bring ? |
A lot!
|
|
Yes, without this, type inference failures and bugs would accumulate over time as Rust continues to evolve. |
|
I’ve opened #20422 to track the test regressions. It looks like we only have a few to fix. 😄 I might be wrong on some of them or have missed something, so feel free to edit |
To expand on "improved performance", the work that analysis-stats mostly measures correspond to things like rust-analyzer's native diagnostics and autocomplete. Features like the initial indexing at startup or code navigation are not impacted by the new trait solver. |
|
Indeed it's not startup, but it basically impacts everything else. |
|
Alright let's give this a spin! |
At this point, opening up for general review, though I don't think this is fully ready to merge.
Some checklist (non-exhaustive) of things to do before/after merge (feel free to edit):
Before:
todo!()s andFIXMEs and decide which are needed/useful to fix prior to mergewith_dbcalls toScopedDb::set_db(or decide on a better API)After (before stable):
After (any time):
chalk_irtorustc_type_irlayout_of_tyMixed:
allow(unused)