Skip to content

[CS] Use simplifyType in isDependentMemberTypeWithBaseThatContainsUnresolvedPackExpansions #84875

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 15, 2025
Merged

[CS] Use simplifyType in isDependentMemberTypeWithBaseThatContainsUnresolvedPackExpansions #84875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7104,8 +7104,11 @@ static bool isDependentMemberTypeWithBaseThatContainsUnresolvedPackExpansions(
if (!type->is<DependentMemberType>())
return false;

auto baseTy = cs.getFixedTypeRecursive(type->getDependentMemberRoot(),
/*wantRValue=*/true);
// FIXME: It's really unfortunate we need to use `simplifyType` here since
// this is called from `matchTypes`. We need to completely simplify the type
// though since pack expansions can be present in fixed types for nested
// type vars.
auto baseTy = cs.simplifyType(type->getDependentMemberRoot());
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a hotspot, an alternate implementation is to call getFixedTypeRecursive() and recurse on each type variable that appears therein

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really unfortunate, I was trying to avoid it…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is a hotspot, an alternate implementation is to call getFixedTypeRecursive() and recurse on each type variable that appears therein

Happy to try that instead

Copy link
Contributor Author

@hamishknight hamishknight Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually I wonder if we could fix this by moving this logic into the TypeSimplifier itself, I didn't actually realize that getFixedTypeRecursive is calling into simplifyType for DependentMemberType, seems like we ought to be able to change type simplification to decline to simplify a DMT base with an unresolved pack expansion

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that we delay matching only if the base is a type variable at the moment, that’s why this and the other check for tuples are directly in matchTypes, in this case the base is not a type variable. I think fix that you’d that to change the meaning of isTypeVariableOrMember()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not sure I follow, for context this is the follow-up I'm planning on doing hamishknight@536304f, which seems to work fine

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried almost exactly that to fix the issue in #84729. I'm talking about check in - https://github.com/swiftlang/swift/blob/main/lib/Sema/CSSimplify.cpp#L7371-L7374, in the example I added the "base" isn't actually a type variable so even though the dependent member didn't get simplified we end up still attempting to match it against the other type and failing that's why I didn't end up going with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I think the problem with my change might have been that it still produced a dependent member type with "newBase" instead of returning the original type... I'm a bit worried if we'd be in a situation that does have a partially resolved dependent member base type at some point though and that would cause too aggressive simplification in matchTypes...

llvm::SmallPtrSet<TypeVariableType *, 2> typeVars;
baseTy->getTypeVariables(typeVars);
return llvm::any_of(typeVars, [](const TypeVariableType *typeVar) {
Expand Down
20 changes: 20 additions & 0 deletions test/Constraints/pack-expansion-expressions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,3 +833,23 @@ func test_dependent_members() {
return Variadic.f(c1, c2) // Ok
}
}

protocol P2 {
associatedtype X
}

extension P2 {
func foo() where X == Bool {}
func foo() where X == String {}
}

do {
struct S<each E>: P2 {
typealias X = String
init(_ fn: () -> (repeat each E)) {}
}

func foo(_ x: Int) {
S { x }.foo() // Make sure we can pick the right 'foo' here.
}
}