Check generic requirements when type checking calls to variadic generic functions #61657
Conversation
slavapestov
force-pushed
the
variadic-generic-function-check-requirements
branch
from
e2e4865 to
3c98e4e
Compare
Thanks to @hborla for suggesting this fix.
slavapestov
force-pushed
the
variadic-generic-function-check-requirements
branch
from
ee558fa to
52e1b40
Compare
slavapestov
force-pushed
the
variadic-generic-function-check-requirements
branch
from
52e1b40 to
c34f8d3
Compare
|
@swift-ci Please smoke test |
| // signature with a RequirementKind::Conformance requirement, so | ||
| // we must handle pack types on the left by splitting up into | ||
| // smaller constraints. | ||
| if (auto *packType = type1->getAs<PackType>()) { |
There was a problem hiding this comment.
Why isn't this handled by simplifyConformsToConstraint that accepts a Type?
There was a problem hiding this comment.
There are two overloads, one with a Type and one with a ProtocolDecl... maybe we could collapse them into one though?
There was a problem hiding this comment.
I think it makes sense to add it to the first one only, that's what is going to be called for constraint simplification.
| Type classType, | ||
| ConstraintLocatorBuilder locator, | ||
| TypeMatchOptions flags) { | ||
| if (!classType->getClassOrBoundGenericClass()) |
There was a problem hiding this comment.
To be prudent I think we should getFixedTypeRecursive for classType as well.
| if (type->getClassOrBoundGenericClass() == | ||
| classType->getClassOrBoundGenericClass()) { | ||
| auto result = matchTypes(type, classType, ConstraintKind::Bind, | ||
| flags, locator); |
There was a problem hiding this comment.
Do we want to support bi-directional inference here or should both sides be fully resolved before matching?
| for (unsigned i = 0, e = packType->getNumElements(); i < e; ++i) { | ||
| addConstraint(ConstraintKind::ConformsTo, packType->getElementType(i), | ||
| protocol->getDeclaredInterfaceType(), | ||
| locator.withPathElement(LocatorPathElt::PackElement(i))); |
There was a problem hiding this comment.
I think you can move this to the first overload and remove duplicate logic from matchExistentialTypes.
| @@ -1358,6 +1359,7 @@ void PotentialBindings::infer(Constraint *constraint) { | |||
| case ConstraintKind::BindParam: | |||
| case ConstraintKind::BindToPointerType: | |||
| case ConstraintKind::Subtype: | |||
| case ConstraintKind::SubclassOf: | |||
There was a problem hiding this comment.
This is actually interesting, I wonder if we do want subclass to participate in inference because it could mean inferring a superclass too eagerly.
There was a problem hiding this comment.
Probably! You'll notice we only generate SubclassOf when the left hand side is a pack for now, because a few tests regressed. Let's figure out how to do it all the time soon, because it will clean up diagnostics too (I saw some hacks to deal with the fact that Superclass requirements become Subtype + ConformsTo AnyObject right now)
There was a problem hiding this comment.
Hm, I was looking at matchTypes for cases where "subtype" is used between class types and it should already handle the case where both types are the same (via DeepEquality restriction) and otherwise it would add Superclass restriction that would call matchSuperclassTypes internally. Besides handling PackTypes specifically what are other reasons to add this constraint?
No description provided.