-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: Wrong closure kind inferences for closures with predicates #16472
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
//! Unification and canonicalization logic. | ||
|
||
use std::{fmt, iter, mem}; | ||
use std::{cmp, fmt, iter, mem}; | ||
|
||
use chalk_ir::{ | ||
cast::Cast, fold::TypeFoldable, interner::HasInterner, zip::Zip, CanonicalVarKind, FloatTy, | ||
IntTy, TyVariableKind, UniverseIndex, | ||
IntTy, TyVariableKind, UniverseIndex, WhereClause, | ||
}; | ||
use chalk_solve::infer::ParameterEnaVariableExt; | ||
use either::Either; | ||
|
@@ -14,11 +14,12 @@ use triomphe::Arc; | |
|
||
use super::{InferOk, InferResult, InferenceContext, TypeError}; | ||
use crate::{ | ||
consteval::unknown_const, db::HirDatabase, fold_tys_and_consts, static_lifetime, | ||
to_chalk_trait_id, traits::FnTrait, AliasEq, AliasTy, BoundVar, Canonical, Const, ConstValue, | ||
DebruijnIndex, GenericArg, GenericArgData, Goal, Guidance, InEnvironment, InferenceVar, | ||
Interner, Lifetime, ParamKind, ProjectionTy, ProjectionTyExt, Scalar, Solution, Substitution, | ||
TraitEnvironment, Ty, TyBuilder, TyExt, TyKind, VariableKind, | ||
chalk_db::TraitId, consteval::unknown_const, db::HirDatabase, fold_tys_and_consts, | ||
static_lifetime, to_chalk_trait_id, traits::FnTrait, AliasEq, AliasTy, BoundVar, Canonical, | ||
ClosureId, Const, ConstValue, DebruijnIndex, DomainGoal, GenericArg, GenericArgData, Goal, | ||
GoalData, Guidance, InEnvironment, InferenceVar, Interner, Lifetime, ParamKind, ProjectionTy, | ||
ProjectionTyExt, Scalar, Solution, Substitution, TraitEnvironment, Ty, TyBuilder, TyExt, | ||
TyKind, VariableKind, | ||
}; | ||
|
||
impl InferenceContext<'_> { | ||
|
@@ -181,6 +182,8 @@ pub(crate) struct InferenceTable<'a> { | |
/// Double buffer used in [`Self::resolve_obligations_as_possible`] to cut down on | ||
/// temporary allocations. | ||
resolve_obligations_buffer: Vec<Canonicalized<InEnvironment<Goal>>>, | ||
fn_trait_predicates: Vec<(Ty, FnTrait)>, | ||
cached_fn_trait_ids: Option<CachedFnTraitIds>, | ||
Comment on lines
+185
to
+186
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not too happy with this, can't we just iterate through the pending obligations when necessary? (as is what rustc does) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The chalk resolves obligations like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I should look into it more, but I'm not sure that it is intended chalk functionality or not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, I'm gonna take a deep look into the chalk closure inferences. I might fix this from upstream or maybe RA is asking chalk without some details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Veykril I had a closer into RA and chalk, and I think that we couldn't use pending obligations like rustc does. When the chalk has to solve a goal that whether some closure implements some 1. It asks So, solving whether the given closure implements But since RA's Giving the "correct" kind of a closure doesn't work for this because;
I've tried just giving So, I have concluded that we would have to modify lots of RA inferencing code to do this as rustc does, and this was why I wrote somewhat sad codes for similar thing 😢 (When I wrote it, I hadn't debugged the chalk much, but tried using pending obligations and it was always empty; so I had similar conclusion without much evidence then 😅 ) |
||
} | ||
|
||
pub(crate) struct InferenceTableSnapshot { | ||
|
@@ -189,15 +192,34 @@ pub(crate) struct InferenceTableSnapshot { | |
type_variable_table_snapshot: Vec<TypeVariableFlags>, | ||
} | ||
|
||
#[derive(Clone)] | ||
struct CachedFnTraitIds { | ||
fn_trait: TraitId, | ||
fn_mut_trait: TraitId, | ||
fn_once_trait: TraitId, | ||
} | ||
|
||
impl CachedFnTraitIds { | ||
fn new(db: &dyn HirDatabase, trait_env: &Arc<TraitEnvironment>) -> Option<Self> { | ||
let fn_trait = FnTrait::Fn.get_id(db, trait_env.krate).map(to_chalk_trait_id)?; | ||
let fn_mut_trait = FnTrait::FnMut.get_id(db, trait_env.krate).map(to_chalk_trait_id)?; | ||
let fn_once_trait = FnTrait::FnOnce.get_id(db, trait_env.krate).map(to_chalk_trait_id)?; | ||
Some(Self { fn_trait, fn_mut_trait, fn_once_trait }) | ||
} | ||
} | ||
|
||
impl<'a> InferenceTable<'a> { | ||
pub(crate) fn new(db: &'a dyn HirDatabase, trait_env: Arc<TraitEnvironment>) -> Self { | ||
let cached_fn_trait_ids = CachedFnTraitIds::new(db, &trait_env); | ||
InferenceTable { | ||
db, | ||
trait_env, | ||
var_unification_table: ChalkInferenceTable::new(), | ||
type_variable_table: Vec::new(), | ||
pending_obligations: Vec::new(), | ||
resolve_obligations_buffer: Vec::new(), | ||
fn_trait_predicates: Vec::new(), | ||
cached_fn_trait_ids, | ||
} | ||
} | ||
|
||
|
@@ -547,6 +569,22 @@ impl<'a> InferenceTable<'a> { | |
} | ||
|
||
fn register_obligation_in_env(&mut self, goal: InEnvironment<Goal>) { | ||
if let Some(fn_trait_ids) = &self.cached_fn_trait_ids { | ||
if let GoalData::DomainGoal(DomainGoal::Holds(WhereClause::Implemented(trait_ref))) = | ||
goal.goal.data(Interner) | ||
{ | ||
if let Some(ty) = trait_ref.substitution.type_parameters(Interner).next() { | ||
if trait_ref.trait_id == fn_trait_ids.fn_trait { | ||
self.fn_trait_predicates.push((ty, FnTrait::Fn)); | ||
} else if trait_ref.trait_id == fn_trait_ids.fn_mut_trait { | ||
self.fn_trait_predicates.push((ty, FnTrait::FnMut)); | ||
} else if trait_ref.trait_id == fn_trait_ids.fn_once_trait { | ||
self.fn_trait_predicates.push((ty, FnTrait::FnOnce)); | ||
} | ||
} | ||
} | ||
} | ||
|
||
let canonicalized = self.canonicalize(goal); | ||
let solution = self.try_resolve_obligation(&canonicalized); | ||
if matches!(solution, Some(Solution::Ambig(_))) { | ||
|
@@ -838,6 +876,23 @@ impl<'a> InferenceTable<'a> { | |
_ => c, | ||
} | ||
} | ||
|
||
pub(super) fn get_closure_fn_trait_predicate( | ||
&mut self, | ||
closure_id: ClosureId, | ||
) -> Option<FnTrait> { | ||
let predicates = mem::take(&mut self.fn_trait_predicates); | ||
let res = predicates.iter().filter_map(|(ty, fn_trait)| { | ||
if matches!(self.resolve_completely(ty.clone()).kind(Interner), TyKind::Closure(c, ..) if *c == closure_id) { | ||
Some(*fn_trait) | ||
} else { | ||
None | ||
} | ||
}).fold(None, |acc, x| Some(cmp::max(acc.unwrap_or(FnTrait::FnOnce), x))); | ||
self.fn_trait_predicates = predicates; | ||
|
||
res | ||
} | ||
} | ||
|
||
impl fmt::Debug for InferenceTable<'_> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use
unwrap_or_else(|| self.closure_kind_from_capture())
for lazyinessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to implement a diagnostic here as the comments above says. I'll try it and will convert into
unwrap_or_else
if it is not possible