[CS] Handle holes in mergeEquivalenceClasses

include/swift/AST/Types.h

@@ -454,11 +454,11 @@ class alignas(1 << TypeAlignInBits) TypeBase
     NumProtocols : 16
   );
 
-  SWIFT_INLINE_BITFIELD_FULL(TypeVariableType, TypeBase, 7+28,
+  SWIFT_INLINE_BITFIELD_FULL(TypeVariableType, TypeBase, 8+26,
     /// Type variable options.
-    Options : 7,
+    Options : 8,
     /// The unique number assigned to this type variable.
-    ID : 27
+    ID : 26
   );
 
   SWIFT_INLINE_BITFIELD_FULL(ErrorUnionType, TypeBase, 32,

include/swift/Sema/ConstraintLocator.h

@@ -233,6 +233,13 @@ class ConstraintLocator : public llvm::FoldingSetNode {
   /// e.g. `foo[0]` or `\Foo.[0]`
   bool isSubscriptMemberRef() const;
 
+  /// Determine whether this locator represents one of the parameter types
+  /// associated with a closure.
+  bool isClosureParameterType() const;
+
+  /// Determine whether this locator represents a closure result type.
+  bool isClosureResultType() const;
+
   /// Determine whether give locator points to the type of the
   /// key path expression.
   bool isKeyPathType() const;

include/swift/Sema/ConstraintSystem.h

@@ -318,6 +318,14 @@ enum TypeVariableOptions {
 
   /// Whether the type variable can be bound only to a pack expansion type.
   TVO_PackExpansion = 0x40,
+
+  /// Whether the hole for this type variable origates from a different
+  /// type variable in its equivalence class.
+  /// FIXME: This is an unfortunate hack due to the fact that choosing a
+  /// different representative changes binding order and regresses a bunch of
+  /// test cases. Once binding order is made more resilient we ought to be able
+  /// to remove this.
+  TVO_NonRepresentativeHole = 0x80,
 };
 
 enum class KeyPathMutability : uint8_t {
@@ -397,6 +405,12 @@ class TypeVariableType::Implementation {
   /// Whether this type variable can bind only to PackExpansionType.
   bool isPackExpansion() const { return getRawOptions() & TVO_PackExpansion; }
 
+  /// Whether the hole for this type variable origates from a different
+  /// type variable in its equivalence class.
+  bool isNonRepresentativeHole() const {
+    return getRawOptions() & TVO_NonRepresentativeHole;
+  }
+
   /// Whether this type variable prefers a subtype binding over a supertype
   /// binding.
   bool prefersSubtypeBinding() const {
@@ -565,28 +579,40 @@ class TypeVariableType::Implementation {
       otherRep->getImpl().recordBinding(*trail);
     otherRep->getImpl().ParentOrFixed = getTypeVariable();
 
+    auto &bits = getTypeVariable()->Bits.TypeVariableType;
+
     if (canBindToLValue() && !otherRep->getImpl().canBindToLValue()) {
       if (trail)
         recordBinding(*trail);
-      getTypeVariable()->Bits.TypeVariableType.Options &= ~TVO_CanBindToLValue;
+      bits.Options &= ~TVO_CanBindToLValue;
     }
 
     if (canBindToInOut() && !otherRep->getImpl().canBindToInOut()) {
       if (trail)
         recordBinding(*trail);
-      getTypeVariable()->Bits.TypeVariableType.Options &= ~TVO_CanBindToInOut;
+      bits.Options &= ~TVO_CanBindToInOut;
     }
 
     if (canBindToNoEscape() && !otherRep->getImpl().canBindToNoEscape()) {
       if (trail)
         recordBinding(*trail);
-      getTypeVariable()->Bits.TypeVariableType.Options &= ~TVO_CanBindToNoEscape;
+      bits.Options &= ~TVO_CanBindToNoEscape;
     }
 
     if (canBindToPack() && !otherRep->getImpl().canBindToPack()) {
       if (trail)
         recordBinding(*trail);
-      getTypeVariable()->Bits.TypeVariableType.Options &= ~TVO_CanBindToPack;
+      bits.Options &= ~TVO_CanBindToPack;
+    }
+
+    // If we're merging a non-hole with a hole, add the flag to allow binding
+    // to a hole. This type variable then becomes a "non-representative" hole,
+    // which is an unfortunate hack necessary to preserve binding order. See
+    // the comment on `TVO_NonRepresentativeHole` for more info.
+    if (!canBindToHole() && otherRep->getImpl().canBindToHole()) {
+      if (trail)
+        recordBinding(*trail);
+      bits.Options |= (TVO_CanBindToHole | TVO_NonRepresentativeHole);
     }
   }
 
@@ -701,6 +727,7 @@ class TypeVariableType::Implementation {
     ENTRY(TVO_PrefersSubtypeBinding, "");
     ENTRY(TVO_CanBindToPack, "pack");
     ENTRY(TVO_PackExpansion, "pack expansion");
+    ENTRY(TVO_NonRepresentativeHole, "non-rep hole");
     }
   #undef ENTRY
   }
@@ -3419,6 +3446,14 @@ class ConstraintSystem {
   getImplicitValueConversionLocator(ConstraintLocatorBuilder root,
                                     ConversionRestrictionKind restriction);
 
+  /// Retrieve the type variable that represents the originating hole in the
+  /// equivalence class for a given type variable.
+  TypeVariableType *getHoleTypeVar(TypeVariableType *tv);
+
+  /// Retrieve the locator for the hole that represents the originating hole in
+  /// the equivalence class for a given type variable.
+  ConstraintLocator *getHoleLocator(TypeVariableType *tv);
+
   /// Lookup and return parent associated with given expression.
   Expr *getParentExpr(Expr *expr) {
     if (auto result = getExprDepthAndParent(expr))

lib/Sema/CSBindings.cpp

@@ -169,7 +169,7 @@ bool BindingSet::isDelayed() const {
     return true;
 
   if (isHole()) {
-    auto *locator = TypeVar->getImpl().getLocator();
+    auto *locator = CS.getHoleLocator(TypeVar);
     assert(locator && "a hole without locator?");
 
     // Delay resolution of the code completion expression until
@@ -2745,7 +2745,7 @@ bool TypeVarBindingProducer::computeNext() {
 
 std::optional<std::pair<ConstraintFix *, unsigned>>
 TypeVariableBinding::fixForHole(ConstraintSystem &cs) const {
-  auto *dstLocator = TypeVar->getImpl().getLocator();
+  auto *dstLocator = cs.getHoleLocator(TypeVar);
   auto *srcLocator = Binding.getLocator();
 
   // FIXME: This check could be turned into an assert once
@@ -2766,7 +2766,7 @@ TypeVariableBinding::fixForHole(ConstraintSystem &cs) const {
 
   unsigned defaultImpact = 1;
 
-  if (auto *GP = TypeVar->getImpl().getGenericParameter()) {
+  if (auto *GP = dstLocator->getGenericParameter()) {
     // If it is representative for a key path root, let's emit a more
     // specific diagnostic.
     auto *keyPathRoot =
@@ -2786,12 +2786,12 @@ TypeVariableBinding::fixForHole(ConstraintSystem &cs) const {
     }
   }
 
-  if (TypeVar->getImpl().isClosureParameterType()) {
+  if (dstLocator->isClosureParameterType()) {
     ConstraintFix *fix = SpecifyClosureParameterType::create(cs, dstLocator);
     return std::make_pair(fix, defaultImpact);
   }
 
-  if (TypeVar->getImpl().isClosureResultType()) {
+  if (dstLocator->isClosureResultType()) {
     auto *closure = castToExpr<ClosureExpr>(dstLocator->getAnchor());
     // If the whole body is being ignored due to a pre-check failure,
     // let's not record a fix about result type since there is
@@ -2910,15 +2910,17 @@ static bool shouldIgnoreHoleForCodeCompletion(ConstraintSystem &cs,
                                               ConstraintLocator *srcLocator) {
   if (!cs.isForCodeCompletion())
     return false;
-
+
+  auto *holeLoc = cs.getHoleLocator(typeVar);
+
   // Don't penalize solutions with unresolved generics.
-  if (typeVar->getImpl().getGenericParameter())
+  if (holeLoc->getGenericParameter())
     return true;
 
   // Don't penalize solutions if we couldn't determine the type of the code
   // completion token. We still want to examine the surrounding types in
   // that case.
-  if (typeVar->getImpl().isCodeCompletionToken())
+  if (holeLoc->directlyAt<CodeCompletionExpr>())
     return true;
 
   // When doing completion in a result builder, we avoid solving unrelated
@@ -2946,7 +2948,7 @@ static bool shouldIgnoreHoleForCodeCompletion(ConstraintSystem &cs,
   if (cs.hasArgumentsIgnoredForCodeCompletion()) {
     // Avoid simplifying the locator if the constraint system didn't ignore
     // any arguments.
-    auto argExpr = simplifyLocatorToAnchor(typeVar->getImpl().getLocator());
+    auto argExpr = simplifyLocatorToAnchor(holeLoc);
     if (cs.isArgumentIgnoredForCodeCompletion(argExpr.dyn_cast<Expr *>())) {
       return true;
     }

lib/Sema/CSFix.cpp

@@ -939,7 +939,11 @@ DefineMemberBasedOnUse::diagnoseForAmbiguity(CommonFixesArray commonFixes) const
     const auto *solution = solutionAndFix.first;
     const auto *fix = solutionAndFix.second->getAs<DefineMemberBasedOnUse>();
 
-    auto baseType = solution->simplifyType(fix->BaseType);
+    // Ignore differences in optionality since we can look through an optional
+    // to resolve an implicit member `.foo`.
+    auto baseType = solution->simplifyType(fix->BaseType)
+                        ->getMetatypeInstanceType()
+                        ->lookThroughAllOptionalTypes();
     if (!concreteBaseType)
       concreteBaseType = baseType;
 

lib/Sema/CSSimplify.cpp

@@ -11605,22 +11605,11 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
             // `key path` constraint can't be retired until all components
             // are simplified.
             addTypeVariableConstraintsToWorkList(memberTypeVar);
-          } else if (isa<Expr *>(locator->getAnchor()) &&
-                     !getSemanticsProvidingParentExpr(
-                         getAsExpr(locator->getAnchor()))) {
-            // If there are no contextual expressions that could provide
-            // a type for the member type variable, let's default it to
-            // a placeholder eagerly so it could be propagated to the
-            // pattern if necessary.
-            recordTypeVariablesAsHoles(memberTypeVar);
-          } else if (locator->isLastElement<LocatorPathElt::PatternMatch>()) {
-            // Let's handle member patterns specifically because they use
-            // equality instead of argument application constraint, so allowing
-            // them to bind member could mean missing valid hole positions in
-            // the pattern.
-            recordTypeVariablesAsHoles(memberTypeVar);
           } else {
-            recordPotentialHole(memberTypeVar);
+            // Eagerly turn the member type variable into a hole since we know
+            // this is where the issue is and we've recorded a fix for it. This
+            // avoids producing unnecessary holes for e.g generic parameters.
+            recordTypeVariablesAsHoles(memberTypeVar);
           }
         }
 
@@ -13662,16 +13651,13 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyApplicableFnConstraint(
   // a fix, let's propagate holes to the "input" type. Doing so
   // provides more information to upcoming argument and result matching.
   if (shouldAttemptFixes()) {
-    if (auto *typeVar = type2->getAs<TypeVariableType>()) {
+    Type underlyingType = desugar2->getMetatypeInstanceType();
+    if (auto *typeVar = underlyingType->getAs<TypeVariableType>()) {
       auto *locator = typeVar->getImpl().getLocator();
       if (hasFixFor(locator)) {
         recordAnyTypeVarAsPotentialHole(func1);
       }
     }
-    Type underlyingType = desugar2;
-    while (auto *MT = underlyingType->getAs<AnyMetatypeType>()) {
-      underlyingType = MT->getInstanceType();
-    }
     underlyingType =
         getFixedTypeRecursive(underlyingType, flags, /*wantRValue=*/true);
     if (underlyingType->isPlaceholder()) {

lib/Sema/ConstraintLocator.cpp

@@ -560,6 +560,22 @@ bool ConstraintLocator::isSubscriptMemberRef() const {
   return path.back().getKind() == ConstraintLocator::SubscriptMember;
 }
 
+bool ConstraintLocator::isClosureParameterType() const {
+  if (!getAnchor())
+    return false;
+
+  return isExpr<ClosureExpr>(getAnchor()) &&
+         isLastElement<LocatorPathElt::TupleElement>();
+}
+
+bool ConstraintLocator::isClosureResultType() const {
+  if (!getAnchor())
+    return false;
+
+  return isExpr<ClosureExpr>(getAnchor()) &&
+         isLastElement<LocatorPathElt::ClosureResult>();
+}
+
 bool ConstraintLocator::isKeyPathType() const {
   auto anchor = getAnchor();
   auto path = getPath();

lib/Sema/ConstraintSystem.cpp

@@ -972,6 +972,31 @@ void ConstraintSystem::restoreType(const KeyPathExpr *KP, unsigned I, Type T) {
   }
 }
 
+TypeVariableType *ConstraintSystem::getHoleTypeVar(TypeVariableType *tv) {
+  if (!tv->getImpl().isNonRepresentativeHole())
+    return tv;
+
+  // If we have a non-representative hole, the hole type var is given by
+  // a member of its equivalence class. Pick the the one with the lowest ID to
+  // ensure consistency.
+  TypeVariableType *candidate = nullptr;
+  for (auto equiv : CG[tv].getEquivalenceClass()) {
+    auto &impl = equiv->getImpl();
+    if (equiv == tv || !impl.canBindToHole() || impl.isNonRepresentativeHole())
+      continue;
+
+    if (!candidate || candidate->getID() > equiv->getID())
+      candidate = equiv;
+  }
+  ASSERT(candidate &&
+         "Non-representative hole ought to have hole in its equivalence class");
+  return candidate;
+}
+
+ConstraintLocator *ConstraintSystem::getHoleLocator(TypeVariableType *tv) {
+  return getHoleTypeVar(tv)->getImpl().getLocator();
+}
+
 std::pair<Type, ExistentialArchetypeType *>
 ConstraintSystem::openAnyExistentialType(Type type,
                                          ConstraintLocator *locator) {
@@ -5320,7 +5345,9 @@ TypeVarBindingProducer::TypeVarBindingProducer(BindingSet &bindings)
                       bindings.getTypeVariable()->getImpl().getLocator()),
       TypeVar(bindings.getTypeVariable()), CanBeNil(bindings.canBeNil()) {
   if (bindings.isDirectHole()) {
-    auto *locator = getLocator();
+    auto *holeTV = CS.getHoleTypeVar(TypeVar);
+    auto *locator = holeTV->getImpl().getLocator();
+
     // If this type variable is associated with a code completion token
     // and it failed to infer any bindings let's adjust holes's locator
     // to point to a code completion token to avoid attempting to "fix"
@@ -5331,7 +5358,7 @@ TypeVarBindingProducer::TypeVarBindingProducer(BindingSet &bindings)
           CS.getConstraintLocator(bindings.getAssociatedCodeCompletionToken());
     }
 
-    Bindings.push_back(Binding::forHole(TypeVar, locator));
+    Bindings.push_back(Binding::forHole(holeTV, locator));
     return;
   }
 

lib/Sema/TypeCheckConstraints.cpp

@@ -117,19 +117,11 @@ bool TypeVariableType::Implementation::isTapType() const {
 }
 
 bool TypeVariableType::Implementation::isClosureParameterType() const {
-  if (!(locator && locator->getAnchor()))
-    return false;
-
-  return isExpr<ClosureExpr>(locator->getAnchor()) &&
-         locator->isLastElement<LocatorPathElt::TupleElement>();
+  return locator && locator->isClosureParameterType();
 }
 
 bool TypeVariableType::Implementation::isClosureResultType() const {
-  if (!(locator && locator->getAnchor()))
-    return false;
-
-  return isExpr<ClosureExpr>(locator->getAnchor()) &&
-         locator->isLastElement<LocatorPathElt::ClosureResult>();
+  return locator && locator->isClosureResultType();
 }
 
 bool TypeVariableType::Implementation::isKeyPathType() const {

test/Constraints/closures.swift

@@ -1362,7 +1362,7 @@ do {
     }
   }
 
-  test { // expected-error {{invalid conversion from throwing function of type '(Int) throws -> Void' to non-throwing function type '(Int) -> Void'}}
+  test { // expected-error {{invalid conversion from throwing function of type '(Int) throws -> _' to non-throwing function type '(Int) -> Void'}}
     try $0.missing // expected-error {{value of type 'Int' has no member 'missing'}}
   }
 }

test/Constraints/diagnostics.swift

@@ -1585,3 +1585,13 @@ do {
     let x: String = 0 // expected-error {{cannot convert value of type 'Int' to specified type 'String'}}
   }. // expected-error {{expected member name following '.'}}
 }
+
+func testArrayLiteralMetatypeMismatch() {
+  // Make sure we can correctly turn 'T' into a hole here.
+  func foo<T>(_ xs: T.Type, _: Int) -> T {} // expected-note {{in call to function 'foo'}}
+  func foo(_ i: Int) {
+    let x = foo([], i)
+    // expected-error@-1 {{generic parameter 'T' could not be inferred}}
+    // expected-error@-2 {{cannot convert value of type '[Any]' to expected argument type 'T.Type'}}
+  }
+}

test/Constraints/rdar45511837.swift

@@ -18,8 +18,6 @@ class Foo<Bar: NSObject> {
 
   lazy var foo: () -> Void = {
     // TODO: improve diagnostic message
-    _ = self.foobar + nil // expected-error {{'Bar' is not convertible to 'String'}}
-    // expected-note@-1 {{did you mean to use 'as!' to force downcast?}}
-    // expected-error@-2 {{'nil' is not compatible with expected argument type 'String'}}
+    _ = self.foobar + nil // expected-error {{generic parameter 'Self' could not be inferred}}
   }
 }

test/type/protocol_composition.swift

@@ -197,10 +197,7 @@ takesP1AndP2([DoesNotExist & P1 & P2]()) // expected-error {{cannot find 'DoesNo
 // expected-note@-3 2 {{overloads for '&' exist with these partially matching parameter lists}}
 // expected-error@-4 {{cannot call value of non-function type '[UInt8]'}}
 takesP1AndP2([Swift.DoesNotExist & P1 & P2]()) // expected-error {{module 'Swift' has no member named 'DoesNotExist'}}
-// expected-error@-1 {{binary operator '&' cannot be applied to operands of type 'UInt8' and '(any P2).Type'}}
-// expected-error@-2 {{binary operator '&' cannot be applied to operands of type 'UInt8' and '(any P1).Type'}}
-// expected-note@-3 2 {{overloads for '&' exist with these partially matching parameter lists}}
-// expected-error@-4 {{cannot call value of non-function type '[UInt8]'}}
+// expected-error@-1 {{cannot call value of non-function type '[Element]'}}
 
 typealias T08 = P1 & inout P2 // expected-error {{'inout' may only be used on parameters}}
 typealias T09 = P1 & __shared P2 // expected-error {{'__shared' may only be used on parameters}}