Skip to content

Check generic requirements when type checking calls to variadic generic functions #61657

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 23 additions & 1 deletion include/swift/AST/PackExpansionMatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,28 @@ class ParamPackMatcher {
bool match();
};

/// Performs a structural match of two lists of types.
///
/// The invariant is that each list must only contain at most one pack
/// expansion type. After collecting a common prefix and suffix, the
/// pack expansion on either side asborbs the remaining elements on the
/// other side.
class PackMatcher {
ArrayRef<Type> lhsTypes;
ArrayRef<Type> rhsTypes;

ASTContext &ctx;

public:
SmallVector<MatchedPair, 4> pairs;

PackMatcher(ArrayRef<Type> lhsTypes,
ArrayRef<Type> rhsTypes,
ASTContext &ctx);

bool match();
};

} // end namespace swift

#endif // SWIFT_AST_TYPE_MATCHER_H
#endif // SWIFT_AST_PACK_EXPANSION_MATCHER_H
4 changes: 4 additions & 0 deletions include/swift/Sema/Constraint.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ enum class ConstraintKind : char {
ArgumentConversion,
/// The first type is convertible to the second type, including inout.
OperatorArgumentConversion,
/// The first type must be a subclass of the second type (which is a
/// class type).
SubclassOf,
/// The first type must conform to the second type (which is a
/// protocol type).
ConformsTo,
Expand Down Expand Up @@ -669,6 +672,7 @@ class Constraint final : public llvm::ilist_node<Constraint>,
case ConstraintKind::BridgingConversion:
case ConstraintKind::ArgumentConversion:
case ConstraintKind::OperatorArgumentConversion:
case ConstraintKind::SubclassOf:
case ConstraintKind::ConformsTo:
case ConstraintKind::LiteralConformsTo:
case ConstraintKind::TransitivelyConformsTo:
Expand Down
9 changes: 9 additions & 0 deletions include/swift/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -5423,6 +5423,15 @@ class ConstraintSystem {
FunctionRefKind functionRefKind,
ConstraintLocator *locator);

/// Attempt to simplify the given superclass constraint.
///
/// \param type The type being tested.
/// \param classType The class type which the type should be a subclass of.
/// \param locator Locator describing where this constraint occurred.
SolutionKind simplifySubclassOfConstraint(Type type, Type classType,
ConstraintLocatorBuilder locator,
TypeMatchOptions flags);

/// Attempt to simplify the given conformance constraint.
///
/// \param type The type being tested.
Expand Down
91 changes: 90 additions & 1 deletion lib/AST/PackExpansionMatcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ bool ParamPackMatcher::match() {
lhsParams = lhsParams.drop_front(prefixLength).drop_back(suffixLength);
rhsParams = rhsParams.drop_front(prefixLength).drop_back(suffixLength);

// If nothing remains, we're done.
if (lhsParams.empty() && rhsParams.empty())
return false;

// If the left hand side is a single pack expansion type, bind it
// to what remains of the right hand side.
if (lhsParams.size() == 1) {
Expand Down Expand Up @@ -198,7 +202,92 @@ bool ParamPackMatcher::match() {
auto lhs = PackType::get(ctx, lhsTypes);

// FIXME: Check rhs flags
pairs.emplace_back(lhs, rhsParams[0].getPlainType(), prefixLength);
pairs.emplace_back(lhs, rhsType, prefixLength);
return false;
}
}

// Otherwise, all remaining possibilities are invalid:
// - Neither side has any pack expansions, and they have different lengths.
// - One side has a pack expansion but the other side is too short, eg
// {Int, T..., Float} vs {Int}.
// - The prefix and suffix are mismatched, so we're left with something
// like {T..., Int} vs {Float, U...}.
return true;
}

PackMatcher::PackMatcher(
ArrayRef<Type> lhsTypes,
ArrayRef<Type> rhsTypes,
ASTContext &ctx)
: lhsTypes(lhsTypes), rhsTypes(rhsTypes), ctx(ctx) {}

bool PackMatcher::match() {
unsigned minLength = std::min(lhsTypes.size(), rhsTypes.size());

// Consume the longest possible prefix where neither type in
// the pair is a pack expansion type.
unsigned prefixLength = 0;
for (unsigned i = 0; i < minLength; ++i) {
auto lhsType = lhsTypes[i];
auto rhsType = rhsTypes[i];

if (lhsType->is<PackExpansionType>() ||
rhsType->is<PackExpansionType>()) {
break;
}

pairs.emplace_back(lhsType, rhsType, i);
++prefixLength;
}

// Consume the longest possible suffix where neither type in
// the pair is a pack expansion type.
unsigned suffixLength = 0;
for (unsigned i = 0; i < minLength - prefixLength; ++i) {
auto lhsType = lhsTypes[lhsTypes.size() - i - 1];
auto rhsType = rhsTypes[rhsTypes.size() - i - 1];

if (lhsType->is<PackExpansionType>() ||
rhsType->is<PackExpansionType>()) {
break;
}

pairs.emplace_back(lhsType, rhsType, i);
++suffixLength;
}

assert(prefixLength + suffixLength <= lhsTypes.size());
assert(prefixLength + suffixLength <= rhsTypes.size());

// Drop the consumed prefix and suffix from each list of types.
lhsTypes = lhsTypes.drop_front(prefixLength).drop_back(suffixLength);
rhsTypes = rhsTypes.drop_front(prefixLength).drop_back(suffixLength);

// If nothing remains, we're done.
if (lhsTypes.empty() && rhsTypes.empty())
return false;

// If the left hand side is a single pack expansion type, bind it
// to what remains of the right hand side.
if (lhsTypes.size() == 1) {
auto lhsType = lhsTypes[0];
if (auto *lhsExpansionType = lhsType->getAs<PackExpansionType>()) {
auto rhs = PackType::get(ctx, rhsTypes);

pairs.emplace_back(lhsExpansionType->getPatternType(), rhs, prefixLength);
return false;
}
}

// If the right hand side is a single pack expansion type, bind it
// to what remains of the left hand side.
if (rhsTypes.size() == 1) {
auto rhsType = rhsTypes[0];
if (auto *rhsExpansionType = rhsType->getAs<PackExpansionType>()) {
auto lhs = PackType::get(ctx, lhsTypes);

pairs.emplace_back(lhs, rhsType, prefixLength);
return false;
}
}
Expand Down
52 changes: 28 additions & 24 deletions lib/AST/Type.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "swift/AST/GenericEnvironment.h"
#include "swift/AST/LazyResolver.h"
#include "swift/AST/Module.h"
#include "swift/AST/PackConformance.h"
#include "swift/AST/ParameterList.h"
#include "swift/AST/ProtocolConformance.h"
#include "swift/AST/SILLayout.h"
Expand Down Expand Up @@ -4365,10 +4366,6 @@ static Type getMemberForBaseType(LookupConformanceFn lookupConformances,

// Retrieve the member type with the given name.

// Tuples don't have member types.
if (substBase->is<TupleType>())
return failed();

// If we know the associated type, look in the witness table.
if (assocType) {
auto proto = assocType->getProtocol();
Expand All @@ -4377,30 +4374,37 @@ static Type getMemberForBaseType(LookupConformanceFn lookupConformances,

if (conformance.isInvalid())
return failed();
if (!conformance.isConcrete())
return failed();

// Retrieve the type witness.
auto witness =
conformance.getConcrete()->getTypeWitnessAndDecl(assocType, options);

auto witnessTy = witness.getWitnessType();
if (!witnessTy || witnessTy->hasError())
return failed();

// This is a hacky feature allowing code completion to migrate to
// using Type::subst() without changing output.
if (options & SubstFlags::DesugarMemberTypes) {
if (auto *aliasType = dyn_cast<TypeAliasType>(witnessTy.getPointer()))
witnessTy = aliasType->getSinglyDesugaredType();
Type witnessTy;

// Another hack. If the type witness is a opaque result type. They can
// only be referred using the name of the associated type.
if (witnessTy->is<OpaqueTypeArchetypeType>())
witnessTy = witness.getWitnessDecl()->getDeclaredInterfaceType();
// Retrieve the type witness.
if (conformance.isPack()) {
auto *packConformance = conformance.getPack();

witnessTy = packConformance->getAssociatedType(
assocType->getDeclaredInterfaceType());
} else if (conformance.isConcrete()) {
auto witness =
conformance.getConcrete()->getTypeWitnessAndDecl(assocType, options);

witnessTy = witness.getWitnessType();
if (!witnessTy || witnessTy->hasError())
return failed();

// This is a hacky feature allowing code completion to migrate to
// using Type::subst() without changing output.
if (options & SubstFlags::DesugarMemberTypes) {
if (auto *aliasType = dyn_cast<TypeAliasType>(witnessTy.getPointer()))
witnessTy = aliasType->getSinglyDesugaredType();

// Another hack. If the type witness is a opaque result type. They can
// only be referred using the name of the associated type.
if (witnessTy->is<OpaqueTypeArchetypeType>())
witnessTy = witness.getWitnessDecl()->getDeclaredInterfaceType();
}
}

if (witnessTy->is<ErrorType>())
if (!witnessTy || witnessTy->is<ErrorType>())
return failed();

return witnessTy;
Expand Down
3 changes: 3 additions & 0 deletions lib/AST/TypeWalker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ class Traversal : public TypeVisitor<Traversal, bool>
}

bool visitPackExpansionType(PackExpansionType *ty) {
if (doIt(ty->getCountType()))
return true;

return doIt(ty->getPatternType());
}

Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/CSBindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1281,6 +1281,7 @@ PotentialBindings::inferFromRelational(Constraint *constraint) {

switch (constraint->getKind()) {
case ConstraintKind::Subtype:
case ConstraintKind::SubclassOf:
case ConstraintKind::Conversion:
case ConstraintKind::ArgumentConversion:
case ConstraintKind::OperatorArgumentConversion: {
Expand Down Expand Up @@ -1358,6 +1359,7 @@ void PotentialBindings::infer(Constraint *constraint) {
case ConstraintKind::BindParam:
case ConstraintKind::BindToPointerType:
case ConstraintKind::Subtype:
case ConstraintKind::SubclassOf:
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 actually interesting, I wonder if we do want subclass to participate in inference because it could mean inferring a superclass too eagerly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

case ConstraintKind::Conversion:
case ConstraintKind::ArgumentConversion:
case ConstraintKind::OperatorArgumentConversion:
Expand Down
Loading