fix: silence mismatches involving unresolved projections #16968
Conversation
rustbot
added
the
S-waiting-on-review
crates/hir-ty/src/infer.rs
Outdated
| @@ -699,7 +699,7 @@ impl<'a> InferenceContext<'a> { | |||
| 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), | |||
| &mut UnknownMismatch(self.db, |ty| matches!(ty.kind(Interner), TyKind::Error)), | |||
There was a problem hiding this comment.
Not sure, but it feels like we should be checking here for projections?
There was a problem hiding this comment.
I have tried doing this at first time, but it seems to cause some errors in MIR (as shown below). I suspect that MIR uses type_mismatch in borrowck to avoid certain paths, so I only removed the corresponding error in the diagnostics. However, I haven't studied this part (MIR) carefully yet 🙊. If necessary, I can further investigate this error in MIR to ensure it's not another potential bug.
thread 'Worker' panicked at crates/hir-ty/src/mir.rs:185:21:
internal error: entered unreachable code: Only tuple has tuple field
There was a problem hiding this comment.
No, you're probably right, MIR is not resilient to code that doesn't typecheck.
There was a problem hiding this comment.
We should probably add a flag the type check results whether there were errors or other problems and have mir rely on that for exiting out early. Since us stripping diagnostics could cause MIR lowering to try lowering an non-successfully typed body.
There was a problem hiding this comment.
Agree. I'll try to implement it.
There was a problem hiding this comment.
I added an ignore_in_diagnostics field to the TypeMismatch struct, enabling us to retain type-mismatches for MIR lowering while removing them from diagnostics. It's more elegant than the previous solution.
There was a problem hiding this comment.
Hmmm, I found that after adding the field, we need to modify some code about tests and statistics to filter out ignored diagnostics. (done)
|
r? @Veykril |
roife
force-pushed
the
fix-issue-16801
branch
2 times, most recently
from
66043cf
to
6bb49ca
Compare
crates/hir-ty/src/infer.rs
Outdated
| @@ -258,6 +258,7 @@ pub enum InferenceDiagnostic { | |||
| pub struct TypeMismatch { | |||
| pub expected: Ty, | |||
| pub actual: Ty, | |||
| pub ignored_in_diagnostics: bool, | |||
There was a problem hiding this comment.
Instead of this, let's just add a has_errors field to InferenceResult and set that to true if there are any type mismatches pre filtering, then change mir lowering to error out if that flag is true
crates/hir-ty/src/infer.rs
Outdated
| @@ -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), | |||
There was a problem hiding this comment.
| _ 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}>>)
There was a problem hiding this comment.
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.
|
Thanks! |
|
☀️ Test successful - checks-actions |
fix #16801