Skip to content

[CS] Allow binding closure param to pack expansion #80569

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

Open
jameesbrown wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[CS] Allow binding closure param to pack expansion #80569

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
20 changes: 14 additions & 6 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3115,6 +3115,8 @@ ConstraintSystem::TypeMatchResult
ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
ConstraintKind kind, TypeMatchOptions flags,
ConstraintLocatorBuilder locator) {
func1 = simplifyType(func1)->castTo<FunctionType>();
func2 = simplifyType(func2)->castTo<FunctionType>();
// Match the 'throws' effect.
TypeMatchResult throwsResult =
matchFunctionThrowing(*this, func1, func2, kind, flags, locator);
Expand Down Expand Up @@ -12071,11 +12073,6 @@ bool ConstraintSystem::resolveClosure(TypeVariableType *typeVar,
if (contextualTy->isTypeVariableOrMember())
return false;

// Cannot propagate pack expansion type from context,
// it has to be handled by type matching logic.
if (isPackExpansionType(contextualTy))
return false;

// If contextual type has an error, let's wait for inference,
// otherwise contextual would interfere with diagnostics.
if (contextualTy->hasError())
Expand Down Expand Up @@ -12241,8 +12238,19 @@ bool ConstraintSystem::resolveClosure(TypeVariableType *typeVar,
// choices in the body.
if (auto contextualParam = getContextualParamAt(i)) {
auto paramTy = simplifyType(contextualParam->getOldType());
if (isSuitableContextualType(paramTy))
auto isSuitable = isSuitableContextualType(paramTy);
if (isSuitable && !paramTy->is<PackExpansionType>())
addConstraint(ConstraintKind::Bind, externalType, paramTy, paramLoc);
else if (isSuitable) {
// If we have an unbound type variable and the contextual parameter
// type is a pack expansion, we must bind it here. Otherwise, it will
// never get bound.
Copy link
Contributor

@xedin xedin Apr 8, 2025

Choose a reason for hiding this comment

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

I think the important question here is why doesn't it get bound, is this an inference problem?

Copy link
Contributor Author

@jameesbrown jameesbrown Apr 8, 2025

Choose a reason for hiding this comment

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

If we start with a plain type variable that doesn't have TVO_PackExpansion, it's not allowed to be bound to a PackExpansionType (there's a comment about this in matchTypesBindTypeVar). In some places we do the:

auto *pack = PackType::get(Ctx, tyVar)
// bind this to the pack expansion
auto *expansion = PackExpansionType::get(pack, pack)

But that always fails when the pack expansion we are binding to has a pattern type that's a PackArchetypeType because we'll try to match the reduced shape of Pack{$T1} to a PackArchetypeType.

What we are expecting is to have opened the pack expansion type via

swift/lib/Sema/TypeOfReference.cpp

Line 296 in 52ebd4a

Type ConstraintSystem::openPackExpansionType(PackExpansionType *expansion,
and use that as our type variable, but at this point it seems to late. The only place I can think of is when we initially infer the closure type, but there are problems with that too. Is there a way we can say "forget about this type variable" and replace it?

Copy link
Contributor

@xedin xedin Apr 8, 2025

Choose a reason for hiding this comment

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

Okay, so I think what we need to do instead of assigning is to flip the flag on parameter type variable to allow it to be bound to a pack expansion.

Copy link
Contributor

@xedin xedin Apr 8, 2025

Choose a reason for hiding this comment

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

This might be okay to do by default in inferClosureType since we already set TVO_CanBindInOut there.

Copy link
Contributor Author

@jameesbrown jameesbrown Apr 8, 2025

Choose a reason for hiding this comment

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

Okay I like that way better too, thank you! Going to try that.

Copy link
Contributor

@xedin xedin May 30, 2025

Choose a reason for hiding this comment

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

@jameesbrown are you still planning to work on this?

if (auto externalTypeVar =
getFixedTypeRecursive(externalType, /*wantRValue=*/true)
->getAs<TypeVariableType>()) {
assignFixedType(externalTypeVar, paramTy);
}
}
}

addConstraint(
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/TypeOfReference.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ Type ConstraintSystem::getUnopenedTypeOfReference(

requestedType =
requestedType->getWithoutSpecifierType()->getReferenceStorageReferent();
requestedType = simplifyType(requestedType);

// Strip pack expansion types off of pack references.
if (auto *expansion = requestedType->getAs<PackExpansionType>())
Expand Down
10 changes: 10 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 @@ -798,3 +798,13 @@ do {
}
}
}

// https://github.com/swiftlang/swift/issues/78426
func test_pack_param_inference_closure<each T>(_: repeat each T) {
func takesGeneric<U>(_: U) {}
let _: (repeat each T) -> Void = { x in }
let _: (repeat each T) -> Void = { repeat takesGeneric(each $0) }
let _: (Any, repeat each T) -> Void = { _,x in repeat takesGeneric(each x) }
let _ = { x in repeat takesGeneric(each x) }
// expected-error@-1{{cannot infer type of closure parameter 'x' without a type annotation}}
}