Merge pull request #36193 from xedin/followup-to-incremental-bindings

[ConstraintSystem] Delay inference until let's clear that type variable attempt is successful

Author: xedin

include/swift/Sema/ConstraintGraph.h

@@ -44,6 +44,7 @@ class Constraint;
 class ConstraintGraph;
 class ConstraintGraphScope;
 class ConstraintSystem;
+class TypeVariableBinding;
 
 /// A single node in the constraint graph, which represents a type variable.
 class ConstraintGraphNode {
@@ -135,6 +136,22 @@ class ConstraintGraphNode {
   /// equivalence class changes.
   void reintroduceToInference(Constraint *constraint, bool notifyReferencedVars);
 
+  /// Similar to \c introduceToInference(Constraint *, ...) this method is going
+  /// to notify inference that this type variable has been bound to a concrete
+  /// type.
+  ///
+  /// The reason why this can't simplify be a part of \c bindTypeVariable
+  /// is related to the fact that it's sometimes expensive to re-compute
+  /// bindings (i.e. if `DependentMemberType` is involved, because it requires
+  /// a conformance lookup), so inference has to be delayed until its clear that
+  /// type variable has been bound to a valid type and solver can make progress.
+  void introduceToInference(Type fixedType);
+
+  /// Opposite of \c introduceToInference(Type)
+  void
+  retractFromInference(Type fixedType,
+                       SmallPtrSetImpl<TypeVariableType *> &referencedVars);
+
   /// Drop all previously collected bindings and re-infer based on the
   /// current set constraints associated with this equivalence class.
   void resetBindingSet();
@@ -148,6 +165,7 @@ class ConstraintGraphNode {
   /// This is useful in situations when type variable gets bound and unbound,
   /// or equivalence class changes.
   void notifyReferencingVars() const;
+
   /// }
 
   /// The constraint graph this node belongs to.
@@ -192,6 +210,8 @@ class ConstraintGraphNode {
   void verify(ConstraintGraph &cg);
 
   friend class ConstraintGraph;
+  friend class ConstraintSystem;
+  friend class TypeVariableBinding;
 };
 
 /// A graph that describes the relationships among the various type variables

include/swift/Sema/ConstraintSystem.h

@@ -3631,6 +3631,10 @@ class ConstraintSystem {
 
     /// Indicates that we are applying a fix.
     TMF_ApplyingFix = 0x02,
+
+    /// Indicates that we are attempting a possible type for
+    /// a type variable.
+    TMF_BindingTypeVariable = 0x04,
   };
 
   /// Options that govern how type matching should proceed.
@@ -3702,10 +3706,14 @@ class ConstraintSystem {
   /// \param type The fixed type to which the type variable will be bound.
   ///
   /// \param updateState Whether to update the state based on this binding.
-  /// False when we're only assigning a type as part of reconstructing 
+  /// False when we're only assigning a type as part of reconstructing
   /// a complete solution from partial solutions.
+  ///
+  /// \param notifyBindingInference Whether to notify binding inference about
+  /// the change to this type variable.
   void assignFixedType(TypeVariableType *typeVar, Type type,
-                       bool updateState = true);
+                       bool updateState = true,
+                       bool notifyBindingInference = true);
 
   /// Determine if the type in question is an Array<T> and, if so, provide the
   /// element type of the array.

lib/Sema/CSBindings.cpp

@@ -1793,7 +1793,16 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
     type = type->reconstituteSugar(/*recursive=*/false);
   }
 
-  cs.addConstraint(ConstraintKind::Bind, TypeVar, type, srcLocator);
+  ConstraintSystem::TypeMatchOptions options;
+
+  options |= ConstraintSystem::TMF_GenerateConstraints;
+  options |= ConstraintSystem::TMF_BindingTypeVariable;
+
+  auto result =
+      cs.matchTypes(TypeVar, type, ConstraintKind::Bind, options, srcLocator);
+
+  if (result.isFailure())
+    return false;
 
   auto reportHole = [&]() {
     if (cs.isForCodeCompletion()) {
@@ -1825,5 +1834,16 @@ bool TypeVariableBinding::attempt(ConstraintSystem &cs) const {
       return true;
   }
 
-  return !cs.failedConstraint && !cs.simplify();
+  if (cs.simplify())
+    return false;
+
+  // If all of the re-activated constraints where simplified,
+  // let's notify binding inference about the fact that type
+  // variable has been bound successfully.
+  {
+    auto &CG = cs.getConstraintGraph();
+    CG[TypeVar].introduceToInference(type);
+  }
+
+  return true;
 }

lib/Sema/CSSimplify.cpp

@@ -3016,7 +3016,8 @@ ConstraintSystem::matchTypesBindTypeVar(
                : getTypeMatchFailure(locator);
   }
 
-  assignFixedType(typeVar, type);
+  assignFixedType(typeVar, type, /*updateState=*/true,
+                  /*notifyInference=*/!flags.contains(TMF_BindingTypeVariable));
 
   return getTypeMatchSuccess();
 }

lib/Sema/ConstraintGraph.cpp

@@ -275,11 +275,13 @@ inference::PotentialBindings &ConstraintGraphNode::getCurrentBindings() {
 
 static bool isUsefulForReferencedVars(Constraint *constraint) {
   switch (constraint->getKind()) {
-  // Don't attempt to propagate information about `Bind`s to referenced
-  // variables since they are adjacent through that binding already, and
-  // there is no useful information in trying to process that kind of
+  // Don't attempt to propagate information about `Bind`s and
+  // `BindOverload`s to referenced variables since they are
+  // adjacent through that binding already, and there is no
+  // useful information in trying to process that kind of
   // constraint.
   case ConstraintKind::Bind:
+  case ConstraintKind::BindOverload:
     return false;
 
   default:
@@ -335,6 +337,51 @@ void ConstraintGraphNode::reintroduceToInference(Constraint *constraint,
   introduceToInference(constraint, notifyReferencedVars);
 }
 
+void ConstraintGraphNode::introduceToInference(Type fixedType) {
+  // Notify all of the type variables that reference this one.
+  //
+  // Since this type variable has been replaced with a fixed type
+  // all of the concrete types that reference it are going to change,
+  // which means that all of the not-yet-attempted bindings should
+  // change as well.
+  notifyReferencingVars();
+
+  if (!fixedType->hasTypeVariable())
+    return;
+
+  SmallPtrSet<TypeVariableType *, 4> referencedVars;
+  fixedType->getTypeVariables(referencedVars);
+
+  for (auto *referencedVar : referencedVars) {
+    auto &node = CG[referencedVar];
+
+    // Newly referred vars need to re-introduce all constraints associated
+    // with this type variable since they are now going to be used in
+    // all of the constraints that reference bound type variable.
+    for (auto *constraint : getConstraints()) {
+      if (isUsefulForReferencedVars(constraint))
+        node.reintroduceToInference(constraint,
+                                    /*notifyReferencedVars=*/false);
+    }
+  }
+}
+
+void ConstraintGraphNode::retractFromInference(
+    Type fixedType, SmallPtrSetImpl<TypeVariableType *> &referencedVars) {
+  // Notify referencing variables (just like in bound case) that this
+  // type variable has been modified.
+  notifyReferencingVars();
+
+  // TODO: This might be an overkill but it's (currently)
+  // the simpliest way to reliably ensure that all of the
+  // no longer related constraints have been retracted.
+  for (auto *referencedVar : referencedVars) {
+    auto &node = CG[referencedVar];
+    if (node.forRepresentativeVar())
+      node.resetBindingSet();
+  }
+}
+
 void ConstraintGraphNode::resetBindingSet() {
   assert(forRepresentativeVar());
 
@@ -541,65 +588,34 @@ void ConstraintGraph::bindTypeVariable(TypeVariableType *typeVar, Type fixed) {
 
   auto &node = (*this)[typeVar];
 
-  // Notify all of the type variables that reference this one.
-  //
-  // Since this type variable has been replaced with a fixed type
-  // all of the concrete types that reference it are going to change,
-  // which means that all of the not-yet-attempted bindings should
-  // change as well.
-  node.notifyReferencingVars();
+  llvm::SmallPtrSet<TypeVariableType *, 4> referencedVars;
+  fixed->getTypeVariables(referencedVars);
 
-  if (!fixed->hasTypeVariable())
-    return;
-
-  llvm::SmallPtrSet<TypeVariableType *, 4> typeVars;
-  fixed->getTypeVariables(typeVars);
-
-  for (auto otherTypeVar : typeVars) {
+  for (auto otherTypeVar : referencedVars) {
     if (typeVar == otherTypeVar)
       continue;
 
     auto &otherNode = (*this)[otherTypeVar];
 
     otherNode.addReferencedBy(typeVar);
     node.addReferencedVar(otherTypeVar);
-
-    // Newly referred vars need to re-introduce all constraints associated
-    // with this type variable since they are now going to be used in
-    // all of the constraints that reference bound type variable.
-    for (auto *constraint : (*this)[typeVar].getConstraints()) {
-      if (isUsefulForReferencedVars(constraint))
-        otherNode.reintroduceToInference(constraint,
-                                         /*notifyReferencedVars=*/false);
-    }
   }
 }
 
 void ConstraintGraph::unbindTypeVariable(TypeVariableType *typeVar, Type fixed) {
   auto &node = (*this)[typeVar];
 
-  // Notify referencing variables (just like in bound case) that this
-  // type variable has been modified.
-  node.notifyReferencingVars();
-
-  if (!fixed->hasTypeVariable())
-    return;
-
-  llvm::SmallPtrSet<TypeVariableType *, 4> typeVars;
-  fixed->getTypeVariables(typeVars);
+  llvm::SmallPtrSet<TypeVariableType *, 4> referencedVars;
+  fixed->getTypeVariables(referencedVars);
 
-  for (auto otherTypeVar : typeVars) {
+  for (auto otherTypeVar : referencedVars) {
     auto &otherNode = (*this)[otherTypeVar];
 
     otherNode.removeReferencedBy(typeVar);
     node.removeReference(otherTypeVar);
-
-    // TODO: This might be an overkill but it's (currently)
-    // the simpliest way to reliably ensure that all of the
-    // no longer related constraints have been retracted.
-    if (otherNode.forRepresentativeVar())
-      otherNode.resetBindingSet();
   }
+
+  node.retractFromInference(fixed, referencedVars);
 }
 
 #pragma mark Algorithms

lib/Sema/ConstraintSystem.cpp

@@ -155,7 +155,8 @@ bool ConstraintSystem::typeVarOccursInType(TypeVariableType *typeVar,
 }
 
 void ConstraintSystem::assignFixedType(TypeVariableType *typeVar, Type type,
-                                       bool updateState) {
+                                       bool updateState,
+                                       bool notifyBindingInference) {
   assert(!type->hasError() &&
          "Should not be assigning a type involving ErrorType!");
 
@@ -167,41 +168,41 @@ void ConstraintSystem::assignFixedType(TypeVariableType *typeVar, Type type,
   if (!type->isTypeVariableOrMember()) {
     // If this type variable represents a literal, check whether we picked the
     // default literal type. First, find the corresponding protocol.
-    ProtocolDecl *literalProtocol = nullptr;
+    //
     // If we have the constraint graph, we can check all type variables in
     // the equivalence class. This is the More Correct path.
     // FIXME: Eliminate the less-correct path.
     auto typeVarRep = getRepresentative(typeVar);
-    for (auto tv : CG[typeVarRep].getEquivalenceClass()) {
+    for (auto *tv : CG[typeVarRep].getEquivalenceClass()) {
       auto locator = tv->getImpl().getLocator();
-      if (!locator || !locator->getPath().empty())
-        continue;
+      if (!(locator && (locator->directlyAt<CollectionExpr>() ||
+                        locator->directlyAt<LiteralExpr>())))
+          continue;
 
-      auto *anchor = getAsExpr(locator->getAnchor());
-      if (!anchor)
+      auto *literalProtocol = TypeChecker::getLiteralProtocol(
+          getASTContext(), castToExpr(locator->getAnchor()));
+      if (!literalProtocol)
         continue;
 
-      literalProtocol =
-          TypeChecker::getLiteralProtocol(getASTContext(), anchor);
-      if (literalProtocol)
-        break;
-    }
-
-    // If the protocol has a default type, check it.
-    if (literalProtocol) {
+      // If the protocol has a default type, check it.
       if (auto defaultType = TypeChecker::getDefaultType(literalProtocol, DC)) {
         // Check whether the nominal types match. This makes sure that we
         // properly handle Array vs. Array<T>.
         if (defaultType->getAnyNominal() != type->getAnyNominal()) {
           increaseScore(SK_NonDefaultLiteral);
         }
       }
+
+      break;
     }
   }
 
   // Notify the constraint graph.
   CG.bindTypeVariable(typeVar, type);
   addTypeVariableConstraintsToWorkList(typeVar);
+
+  if (notifyBindingInference)
+    CG[typeVar].introduceToInference(type);
 }
 
 void ConstraintSystem::addTypeVariableConstraintsToWorkList(