Turn a few generics into dynamic dispatch. #592
Conversation
There was a problem hiding this comment.
There's a couple different bit here:
- The changes to
CoherenceSolverandWfSolver, I need to think more about and look at the goals we're generating. We can remove the generics and just take a&dyn Fn"into_solver" with a return ofBox<dyn Solver>. - The switch from
impl Fnto&dyn Fnto me feels like a necessary evil. It obviously a little less ergonomic, overall the changes are fine.
chalk-solve/src/coherence/solve.rs
Outdated
| @@ -131,7 +130,7 @@ impl<I: Interner, S: Solver<I>, SC: Into<S> + Copy> CoherenceSolver<'_, I, S, SC | |||
| .negate(interner); | |||
|
|
|||
| let canonical_goal = &goal.into_closed_goal(interner); | |||
| let solution = self.solver_choice.into().solve(self.db, canonical_goal); | |||
| let solution = self.solver.solve(self.db, canonical_goal); | |||
There was a problem hiding this comment.
So...this is not the same things semantically. Before, we use a fresh solver for each goal. Now, we use a single solver for all goals. Importantly, the solver can (and does) keep some state between goals. It's a bit surprising to me that nothing fails here. (Really, I think the only thing we have to be careful about is placeholders). I think this needs to stay as creating a new solver for each goal, but with some convincing (from myself or otherwise), this could be changed now or later.
|
Updated to make coherence and wf solvers create fresh inner solver every time. |
chalk-solve/src/coherence.rs
Outdated
| @@ -11,11 +11,10 @@ use std::sync::Arc; | |||
| pub mod orphan; | |||
| mod solve; | |||
|
|
|||
| pub struct CoherenceSolver<'db, I: Interner, S, SC: Into<S> + Copy> { | |||
| pub struct CoherenceSolver<'db, 'sb, I: Interner> { | |||
There was a problem hiding this comment.
I think this (and WfSolver) could just have a single 'a lifetime.
chalk-solve/src/coherence.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn specialization_priorities( | ||
| &self, | ||
| &mut self, |
There was a problem hiding this comment.
Why do these need &mut self now?
There was a problem hiding this comment.
It's no longer needed now. I have changed them back.
chalk-integration/src/query.rs
Outdated
| @@ -132,12 +132,9 @@ fn orphan_check(db: &dyn LoweringDatabase) -> Result<(), ChalkError> { | |||
|
|
|||
| tls::set_current_program(&program, || -> Result<(), ChalkError> { | |||
| let local_impls = program.local_impl_ids(); | |||
| let mut solver = db.solver_choice().into_solver(); | |||
There was a problem hiding this comment.
So to be semantically the same, this should be inside the loop (and then you could just pass the solver by value). But I think we only generate a LocalImplAllowed goal with no placeholders, so this should be fine to reuse.
There was a problem hiding this comment.
I've moved it inside the loop for consistency.
chalk-solve/src/coherence/solve.rs
Outdated
| pub(super) fn visit_specializations_of_trait( | ||
| &self, | ||
| &mut self, |
crlf0710
force-pushed
the
trait_object_solver
branch
from
580f89e to
7adf0dc
Compare
crlf0710
force-pushed
the
trait_object_solver
branch
from
7adf0dc to
251026c
Compare
|
Thanks! @bors r+ |
|
📌 Commit 251026c has been approved by |
|
☀️ Test successful - checks-actions |
Fixes #562.
r? @jackh726
cc @nikomatsakis