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 panic with impl trait associated types in where clause #16830

Merged
merged 1 commit into from Mar 18, 2024

Conversation

Jesse-Bakker
Copy link
Contributor

Not sure if this is the correct fix, but the tests are green :')

Fixes #16823

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2024
@ShoyuVanilla
Copy link
Contributor

ShoyuVanilla commented Mar 17, 2024

The root cause of this is my wrong assumption in #16769 😢

if matches!(
self.impl_trait_mode,
ImplTraitLoweringState::Param(_)
| ImplTraitLoweringState::Variable(_)
) {
// Find the generic index for the target of our `bound`
let target_param_idx = self
.resolver
.where_predicates_in_scope()
.find_map(|p| match p {
WherePredicate::TypeBound {
target: WherePredicateTypeTarget::TypeOrConstParam(idx),
bound: b,
} if b == bound => Some(idx),
_ => None,
});
if let Some(target_param_idx) = target_param_idx {

In the above L1136, I thought that the predicate target T is always WherePredicateTypeTarget::TypeOrConstParam in the predicates like T: Outer<Item = impl Inner>>, but it actually is WherePredicateTypeTarget::TypeRef in this case.

My intention was to handle case when the binding.type_ref - the bound of associated type binding - is not ImplTrait by itself, but contains ImplTrait inside generic parameters, at here

if let Some(type_ref) = &binding.type_ref {
if let (TypeRef::ImplTrait(bounds), ImplTraitLoweringState::Disallowed) =
(type_ref, &self.impl_trait_mode)
{

like the case at here (it's Path - R<impl Y, T>>, not ImplTrait - impl Y)
fn foo<T>(x: impl X<Item = R<impl Y, T>>) -> T { loop {} }

and I think that the direct ImplTrait cases can be handled with preivious "redirecting lowered bounds" stretagy in these lines (and the recusive impl trait bounds inside that I intended to fix can still be handled properly as it triggers assoc_type_bindings_from_type_bound again in lower_type_bound)
for bound in bounds {
predicates.extend(
self.lower_type_bound(
bound,
TyKind::Alias(AliasTy::Projection(projection_ty.clone()))
.intern(Interner),
false,
),
);

So, I think that your PR is proper and effective 👍

@Veykril
Copy link
Member

Veykril commented Mar 18, 2024

Thanks!
@bors r=ShoyuVanilla

@bors
Copy link
Collaborator

bors commented Mar 18, 2024

📌 Commit 9582885 has been approved by ShoyuVanilla

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 18, 2024

⌛ Testing commit 9582885 with merge a71a032...

@bors
Copy link
Collaborator

bors commented Mar 18, 2024

☀️ Test successful - checks-actions
Approved by: ShoyuVanilla
Pushing a71a032 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic in chalk_ir::fold::subst
5 participants