-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: Panic when a TAIT exists in a RPIT #17924
Conversation
☔ The latest upstream changes (presumably #17925) made this pull request unmergeable. Please resolve the merge conflicts. |
crates/hir-ty/src/infer.rs
Outdated
for (ty, pat) in param_tys.zip(&*self.body.params) { | ||
let ty = self.insert_type_vars(ty); | ||
let ty = self.normalize_associated_types_in(ty); | ||
|
||
self.infer_top_pat(*pat, &ty); | ||
params_and_ret_tys.push(ty); | ||
tait_candidates.insert(ty); |
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.
tait_candidates.insert(ty); | |
if ty.data(Interner).flags.intersects(TypeFlags::HAS_TY_OPAQUE) { | |
tait_candidates.insert(ty); | |
} |
(just noticed the flag exists)
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.
It's strange that this makes the tests fail 🤔
failures:
---- tests::diagnostics::no_mismatches_on_atpit stdout ----
thread 'tests::diagnostics::no_mismatches_on_atpit' panicked at crates/hir-ty/src/tests/diagnostics.rs:102:5:
Unexpected type mismatches:
257..259: expected impl Sized, got ()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- tests::type_alias_impl_traits::associated_type_impl_trait stdout ----
thread 'tests::type_alias_impl_traits::associated_type_impl_trait' panicked at crates/hir-ty/src/tests/type_alias_impl_traits.rs:7:5:
Unexpected type mismatches:
203..205: expected impl Foo + ?Sized, got S1
---- tests::type_alias_impl_traits::associated_type_impl_traits_complex stdout ----
thread 'tests::type_alias_impl_traits::associated_type_impl_traits_complex' panicked at crates/hir-ty/src/tests/type_alias_impl_traits.rs:35:5:
Unexpected type mismatches:
293..295: expected impl Foo + ?Sized, got S1
585..587: expected impl Foo + ?Sized, got S1
595..597: expected impl Bar + ?Sized, got S2
failures:
tests::diagnostics::no_mismatches_on_atpit
tests::type_alias_impl_traits::associated_type_impl_trait
tests::type_alias_impl_traits::associated_type_impl_traits_complex
test result: FAILED. 783 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.67s
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.
Oh, I see. It seems that the types we are inserting have some inference vars inside and
rust-analyzer/crates/hir-ty/src/infer.rs
Line 949 in c9955bf
let ty = self.table.resolve_ty_shallow(ty); |
this line is adding opaque types when the top type doesn't have the flag HAS_TY_OPAQUE
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.
I guess we can check for either HAS_TY_OPAQUE
or HAS_TY_INFER
then? If not feel free to merge as is
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.
Reasonable alternative 👍
crates/hir-ty/src/infer.rs
Outdated
@@ -863,11 +873,16 @@ impl<'a> InferenceContext<'a> { | |||
// Functions might be defining usage sites of TAITs. | |||
// To define an TAITs, that TAIT must appear in the function's signatures. | |||
// So, it suffices to check for params and return types. | |||
params_and_ret_tys.push(self.return_ty.clone()); | |||
self.make_tait_coercion_table(params_and_ret_tys.iter()); | |||
tait_candidates.insert(self.return_ty.clone()); |
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.
tait_candidates.insert(self.return_ty.clone()); | |
if self.return_ty.data(Interner).flags.intersects(TypeFlags::HAS_TY_OPAQUE) { | |
tait_candidates.insert(self.return_ty.clone()); | |
} |
Thanks! |
✌️ @ShoyuVanilla, you can now approve this pull request! If @Veykril told you to " |
8d216c3
to
4243dbf
Compare
4243dbf
to
722f0d3
Compare
☀️ Test successful - checks-actions |
Fixes #17921
When there is a TAIT inside of a RPIT like;
while inferencing
fn foo
,insert_inference_vars_for_impl_trait
tries to substitute impl trait bounds ofBar
, i.e.Implemented(Foo)
with RPITsplaceholders
, and this causes panicrust-analyzer/crates/hir-ty/src/infer.rs
Lines 903 to 905 in fa00326