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

fix: silence mismatches involving unresolved projections #16968

Merged
merged 3 commits into from
Apr 1, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/hir-ty/src/chalk_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub trait TyExt {
fn contains_unknown(&self) -> bool;
fn is_ty_var(&self) -> bool;
fn is_union(&self) -> bool;
fn is_projection(&self) -> bool;

fn as_adt(&self) -> Option<(hir_def::AdtId, &Substitution)>;
fn as_builtin(&self) -> Option<BuiltinType>;
Expand Down Expand Up @@ -101,6 +102,13 @@ impl TyExt for Ty {
matches!(self.adt_id(Interner), Some(AdtId(hir_def::AdtId::UnionId(_))))
}

fn is_projection(&self) -> bool {
matches!(
self.kind(Interner),
TyKind::Alias(AliasTy::Projection(_)) | TyKind::AssociatedType(_, _)
)
}

fn as_adt(&self) -> Option<(hir_def::AdtId, &Substitution)> {
match self.kind(Interner) {
TyKind::Adt(AdtId(adt), parameters) => Some((*adt, parameters)),
Expand Down
51 changes: 38 additions & 13 deletions crates/hir-ty/src/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,8 @@ pub struct InferenceResult {
/// Type of the result of `.into_iter()` on the for. `ExprId` is the one of the whole for loop.
pub type_of_for_iterator: FxHashMap<ExprId, Ty>,
type_mismatches: FxHashMap<ExprOrPatId, TypeMismatch>,
/// Whether there are any type-mismatching errors in the result.
pub(crate) has_errors: bool,
/// Interned common types to return references to.
standard_types: InternedStandardTypes,
/// Stores the types which were implicitly dereferenced in pattern binding modes.
Expand Down Expand Up @@ -654,6 +656,7 @@ impl<'a> InferenceContext<'a> {
type_of_rpit,
type_of_for_iterator,
type_mismatches,
has_errors,
standard_types: _,
pat_adjustments,
binding_modes: _,
Expand Down Expand Up @@ -695,16 +698,33 @@ impl<'a> InferenceContext<'a> {
for ty in type_of_for_iterator.values_mut() {
*ty = table.resolve_completely(ty.clone());
}

*has_errors = !type_mismatches.is_empty();

type_mismatches.retain(|_, mismatch| {
mismatch.expected = table.resolve_completely(mismatch.expected.clone());
mismatch.actual = table.resolve_completely(mismatch.actual.clone());
chalk_ir::zip::Zip::zip_with(
&mut UnknownMismatch(self.db),
Variance::Invariant,
&mismatch.expected,
&mismatch.actual,
)
.is_ok()
let unresolved_ty_mismatch = || {
chalk_ir::zip::Zip::zip_with(
&mut UnknownMismatch(self.db, |ty| matches!(ty.kind(Interner), TyKind::Error)),
Variance::Invariant,
&mismatch.expected,
&mismatch.actual,
)
.is_ok()
};

let unresolved_projections_mismatch = || {
chalk_ir::zip::Zip::zip_with(
&mut UnknownMismatch(self.db, |ty| ty.contains_unknown() && ty.is_projection()),
chalk_ir::Variance::Invariant,
&mismatch.expected,
&mismatch.actual,
)
.is_ok()
};

unresolved_ty_mismatch() && unresolved_projections_mismatch()
});
diagnostics.retain_mut(|diagnostic| {
use InferenceDiagnostic::*;
Expand Down Expand Up @@ -1646,11 +1666,16 @@ impl std::ops::BitOrAssign for Diverges {
*self = *self | other;
}
}
/// A zipper that checks for unequal `{unknown}` occurrences in the two types. Used to filter out
/// mismatch diagnostics that only differ in `{unknown}`. These mismatches are usually not helpful.
/// As the cause is usually an underlying name resolution problem.
struct UnknownMismatch<'db>(&'db dyn HirDatabase);
impl chalk_ir::zip::Zipper<Interner> for UnknownMismatch<'_> {
/// A zipper that checks for unequal `{unknown}` occurrences in the two types.
/// Types that have different constructors are filtered out and tested by the
/// provided closure `F`. Commonly used to filter out mismatch diagnostics that
/// only differ in `{unknown}`. These mismatches are usually not helpful, as the
/// cause is usually an underlying name resolution problem.
///
/// E.g. when F is `|ty| matches!(ty.kind(Interer), TyKind::Unknown)`, the zipper
/// will skip over all mismatches that only differ in `{unknown}`.
struct UnknownMismatch<'db, F: Fn(&Ty) -> bool>(&'db dyn HirDatabase, F);
impl<F: Fn(&Ty) -> bool> chalk_ir::zip::Zipper<Interner> for UnknownMismatch<'_, F> {
fn zip_tys(&mut self, variance: Variance, a: &Ty, b: &Ty) -> chalk_ir::Fallible<()> {
let zip_substs = |this: &mut Self,
variances,
Expand Down Expand Up @@ -1721,7 +1746,7 @@ impl chalk_ir::zip::Zipper<Interner> for UnknownMismatch<'_> {
zip_substs(self, None, &fn_ptr_a.substitution.0, &fn_ptr_b.substitution.0)?
}
(TyKind::Error, TyKind::Error) => (),
(TyKind::Error, _) | (_, TyKind::Error) => return Err(chalk_ir::NoSolution),
_ if (self.1)(a) || (self.1)(b) => return Err(chalk_ir::NoSolution),
Copy link
Member

@Veykril Veykril Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_ if (self.1)(a) || (self.1)(b) => return Err(chalk_ir::NoSolution),
(TyKind::Error, _) | (_, TyKind::Error) | (TyKind::Alias(AliasTy::Projection(_)) | TyKind::AssociatedType(_, _), _) | (_, TyKind::Alias(AliasTy::Projection(_)) | TyKind::AssociatedType(_, _)) => return Err(chalk_ir::NoSolution),

No need for the closures and zipping twice, we can just inline things here. It's very unlikely that we want to reuse this zipper for other things, so keeping it simple is better here. (Also adjust the comment again)

The contains_unknown is also false btw, the entire idea of this zipper is to strip away unknowns that mismatch the constructor, but otherwise we want to render unknowns in mismatches still (ex. Foo<i32> vs Foo<{unknown}> should be removed, but not Foo<i32> vs Foo<Bar<{Unknown}>>)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, previously I chose to skip mismatches in diagnostics, so I reused UnknownMismatch, and I didn't change it after using has_error field.

_ => (),
}

Expand Down
17 changes: 10 additions & 7 deletions crates/hir-ty/src/mir/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ pub enum MirLowerError {
UnresolvedField,
UnsizedTemporary(Ty),
MissingFunctionDefinition(DefWithBodyId, ExprId),
TypeMismatch(TypeMismatch),
TypeMismatch(Option<TypeMismatch>),
/// This should never happen. Type mismatch should catch everything.
TypeError(&'static str),
NotSupported(String),
Expand Down Expand Up @@ -170,14 +170,15 @@ impl MirLowerError {
body.pretty_print_expr(db.upcast(), *owner, *it)
)?;
}
MirLowerError::TypeMismatch(e) => {
writeln!(
MirLowerError::TypeMismatch(e) => match e {
Some(e) => writeln!(
f,
"Type mismatch: Expected {}, found {}",
e.expected.display(db),
e.actual.display(db),
)?;
}
)?,
None => writeln!(f, "Type mismatch: types mismatch with {{unknown}}",)?,
},
MirLowerError::GenericArgNotProvided(id, subst) => {
let parent = id.parent;
let param = &db.generic_params(parent).type_or_consts[id.local_id];
Expand Down Expand Up @@ -2152,8 +2153,10 @@ pub fn lower_to_mir(
// need to take this input explicitly.
root_expr: ExprId,
) -> Result<MirBody> {
if let Some((_, it)) = infer.type_mismatches().next() {
return Err(MirLowerError::TypeMismatch(it.clone()));
if infer.has_errors {
return Err(MirLowerError::TypeMismatch(
infer.type_mismatches().next().map(|(_, it)| it.clone()),
));
}
let mut ctx = MirLowerCtx::new(db, owner, body, infer);
// 0 is return local
Expand Down
17 changes: 17 additions & 0 deletions crates/hir-ty/src/tests/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,20 @@ impl Trait for () {
"#,
);
}

#[test]
fn no_mismatches_with_unresolved_projections() {
check_no_mismatches(
r#"
// Thing is {unknown}
fn create() -> Option<(i32, Thing)> {
Some((69420, Thing))
}

fn consume() -> Option<()> {
let (number, thing) = create()?;
Some(())
}
"#,
);
}
1 change: 1 addition & 0 deletions crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1678,6 +1678,7 @@ impl DefWithBody {
for d in &infer.diagnostics {
acc.extend(AnyDiagnostic::inference_diagnostic(db, self.into(), d, &source_map));
}

for (pat_or_expr, mismatch) in infer.type_mismatches() {
let expr_or_pat = match pat_or_expr {
ExprOrPatId::ExprId(expr) => source_map.expr_syntax(expr).map(Either::Left),
Expand Down