[CS] Handle holes in mergeEquivalenceClasses

Make sure we preserve `TVO_CanBindToHole`. This unfortunately requires
introducing the concept of a "non-representative" hole for
representatives where the actual hole is given by another type variable
in the equivalence class. This avoids disturbing binding order and
regressing diagnostics. Once binding is less sensitive to ordering we
ought to be able to always choose the hole as the representative.

Author: hamishknight

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/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/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;
   }
 

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}}
   }
 }

validation-test/Sema/SwiftUI/rdar88256059.swift

@@ -9,7 +9,8 @@ struct MyView: View {
 
   var body: some View {
     Table(self.data) {
-      // expected-error@-1 {{expected expression of type 'Columns' in result builder 'TableColumnBuilder'}} {{23-23=<#T##Columns#>}}
+      // expected-error@-1 {{expected expression of type 'Column' in result builder 'TableColumnBuilder'}} {{23-23=<#T##Column#>}}
+      // expected-error@-2 {{generic parameter 'Column' could not be inferred}}
     }
   }
 }