-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[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.
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
base: main
Are you sure you want to change the base?
Conversation
lib/Sema/CSGen.cpp
Outdated
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 think this should go into ConstraintSystem::resolveClosure because that's the spot where we know that there is a contextual type.
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.
Thanks! I actually started with ConstraintSystem::resolveClosure, but the solution didn’t quite fall into place as cleanly, which led me to try this approach. I definitely missed something though and will try that way again.
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.
This approach would only work for cases were contextual type is known in advance but if closure is passed as an argument to an overloaded function there is no contextual type available upfront, it becomes available once an overload of that function is bound.
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.
That makes sense, thanks for pointing it out, I was having trouble thinking of how we'd not have the contextual type other than the most basic version: let _ = { x in }.
162a751 to
f20e3bc
Compare
| 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. |
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 think the important question here is why doesn't it get bound, is this an inference problem?
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.
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, |
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.
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.
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.
This might be okay to do by default in inferClosureType since we already set TVO_CanBindInOut there.
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.
Okay I like that way better too, thank you! Going to try that.
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.
@jameesbrown are you still planning to work on this?
In
resolveClosureTypeallow binding a closure parameter (when it's a free type variable) to a contextual type that is a pack expansion and otherwise valid.Resolves: #78426