Merge pull request #12072 from xedin/rdar-33429010

[ConstraintGraph] Don't try to contract edge of parameter bindings with `inout` attribute

lib/Sema/CSBindings.cpp

@@ -188,16 +188,9 @@ ConstraintSystem::getPotentialBindings(TypeVariableType *typeVar) {
       continue;
 
     switch (constraint->getKind()) {
-    case ConstraintKind::BindParam:
-      if (simplifyType(constraint->getSecondType())
-              ->getAs<TypeVariableType>() == typeVar) {
-        result.IsRHSOfBindParam = true;
-      }
-
-      LLVM_FALLTHROUGH;
-
     case ConstraintKind::Bind:
     case ConstraintKind::Equal:
+    case ConstraintKind::BindParam:
     case ConstraintKind::BindToPointerType:
     case ConstraintKind::Subtype:
     case ConstraintKind::Conversion:

lib/Sema/CSSimplify.cpp

@@ -1681,11 +1681,27 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
         if (!isBindable(typeVar1, type2))
           return formUnsolvedResult();
 
+        // If the right-hand side of the BindParam constraint
+        // is `lvalue` type, we'll have to make sure that
+        // left-hand side is bound to type variable which
+        // is wrapped in `inout` type to preserve inout/lvalue pairing.
         if (auto *lvt = type2->getAs<LValueType>()) {
-          assignFixedType(typeVar1, InOutType::get(lvt->getObjectType()));
-        } else {
-          assignFixedType(typeVar1, type2);
+          auto *tv = createTypeVariable(typeVar1->getImpl().getLocator(),
+                                        /*options=*/0);
+          assignFixedType(typeVar1, InOutType::get(tv));
+
+          typeVar1 = tv;
+          type2 = lvt->getObjectType();
         }
+
+        // If we have a binding for the right-hand side
+        // (argument type) don't try to bind it to the left-hand
+        // side (parameter type) directly, because their
+        // relationship is contravariant and the actual
+        // binding can only come from the left-hand side.
+        addUnsolvedConstraint(
+            Constraint::create(*this, ConstraintKind::ArgumentConversion, type2,
+                               typeVar1, getConstraintLocator(locator)));
         return SolutionKind::Solved;
       }
 

lib/Sema/CSSolver.cpp

@@ -75,6 +75,17 @@ Optional<Type> ConstraintSystem::checkTypeOfBinding(TypeVariableType *typeVar,
   if (count(referencedTypeVars, typeVar))
     return None;
 
+  // If type variable is not allowed to bind to `lvalue`,
+  // let's check if type of potential binding has any
+  // type variables, which are allowed to bind to `lvalue`,
+  // and postpone such type from consideration.
+  if (!typeVar->getImpl().canBindToLValue()) {
+    for (auto *typeVar : referencedTypeVars) {
+      if (typeVar->getImpl().canBindToLValue())
+        return None;
+    }
+  }
+
   // If the type is a type variable itself, don't permit the binding.
   if (auto bindingTypeVar = type->getRValueType()->getAs<TypeVariableType>()) {
     if (isNilLiteral) {

lib/Sema/ConstraintGraph.cpp

@@ -648,25 +648,6 @@ static bool shouldContractEdge(ConstraintKind kind) {
   }
 }
 
-/// We use this function to determine if a subtype constraint is set
-/// between two (possibly sugared) type variables, one of which is wrapped
-/// in an inout type.
-static bool isStrictInoutSubtypeConstraint(Constraint *constraint) {
-  if (constraint->getKind() != ConstraintKind::Subtype)
-    return false;
-
-  auto t1 = constraint->getFirstType()->getDesugaredType();
-
-  if (auto tt = t1->getAs<TupleType>()) {
-    if (tt->getNumElements() != 1)
-      return false;
-
-    t1 = tt->getElementType(0).getPointer();
-  }
-
-  return t1->is<InOutType>();
-}
-
 bool ConstraintGraph::contractEdges() {
   llvm::SetVector<std::pair<TypeVariableType *,
                             TypeVariableType *>> contractions;
@@ -694,20 +675,13 @@ bool ConstraintGraph::contractEdges() {
 
         auto isParamBindingConstraint = kind == ConstraintKind::BindParam;
 
-        // We need to take special care not to directly contract parameter
-        // binding constraints if there is an inout subtype constraint on the
-        // type variable. The constraint solver depends on multiple constraints
-        // being present in this case, so it can generate the appropriate lvalue
-        // wrapper for the argument type.
-        if (isParamBindingConstraint) {
-          auto *node = tyvar1->getImpl().getGraphNode();
-          auto constraints = node->getConstraints();
-          if (llvm::any_of(constraints, [](Constraint *constraint) {
-                            return isStrictInoutSubtypeConstraint(constraint);
-                          })) {
-            continue;
-          }
-        }
+        // If the parameter is allowed to bind to `inout` let's not
+        // try to contract the edge connecting parameter declaration to
+        // it's use in the body. If parameter declaration is bound to
+        // `inout` it's use has to be bound to `l-value`, which can't
+        // happen once equivalence classes of parameter and argument are merged.
+        if (isParamBindingConstraint && tyvar1->getImpl().canBindToInOut())
+          continue;
 
         auto rep1 = CS.getRepresentative(tyvar1);
         auto rep2 = CS.getRepresentative(tyvar2);
@@ -740,23 +714,30 @@ bool ConstraintGraph::contractEdges() {
 }
 
 void ConstraintGraph::removeEdge(Constraint *constraint) {
+  bool isExistingConstraint = false;
 
   for (auto &active : CS.ActiveConstraints) {
     if (&active == constraint) {
       CS.ActiveConstraints.erase(constraint);
+      isExistingConstraint = true;
       break;
     }
   }
 
   for (auto &inactive : CS.InactiveConstraints) {
     if (&inactive == constraint) {
       CS.InactiveConstraints.erase(constraint);
+      isExistingConstraint = true;
       break;
     }
   }
 
-  if (CS.solverState)
-    CS.solverState->removeGeneratedConstraint(constraint);
+  if (CS.solverState) {
+    if (isExistingConstraint)
+      CS.solverState->retireConstraint(constraint);
+    else
+      CS.solverState->removeGeneratedConstraint(constraint);
+  }
 
   removeConstraint(constraint);
 }

lib/Sema/ConstraintSystem.h

@@ -2551,8 +2551,8 @@ class ConstraintSystem {
   };
 
   struct PotentialBindings {
-    typedef std::tuple<bool, bool, bool, bool, bool,
-                       unsigned char, unsigned int> BindingScore;
+    typedef std::tuple<bool, bool, bool, bool, unsigned char, unsigned int>
+        BindingScore;
 
     TypeVariableType *TypeVar;
 
@@ -2574,9 +2574,6 @@ class ConstraintSystem {
     /// The number of defaultable bindings.
     unsigned NumDefaultableBindings = 0;
 
-    /// Is this type variable on the RHS of a BindParam constraint?
-    bool IsRHSOfBindParam = false;
-
     /// Tracks the position of the last known supertype in the group.
     Optional<unsigned> lastSupertypeIndex;
 
@@ -2593,7 +2590,6 @@ class ConstraintSystem {
     static BindingScore formBindingScore(const PotentialBindings &b) {
       return std::make_tuple(!b.hasNonDefaultableBindings(),
                              b.FullyBound,
-                             b.IsRHSOfBindParam,
                              b.SubtypeOfExistentialType,
                              b.InvolvesTypeVariables,
                              static_cast<unsigned char>(b.LiteralBinding),

test/Constraints/closures.swift

@@ -603,3 +603,45 @@ sr3520_2(sr3250_arg) { $0 += 3 } // ok
 // SR-1976/SR-3073: Inference of inout
 func sr1976<T>(_ closure: (inout T) -> Void) {}
 sr1976({ $0 += 2 }) // ok
+
+// rdar://problem/33429010
+
+struct I_33429010 : IteratorProtocol {
+    func next() -> Int? {
+        fatalError()
+    }
+}
+
+extension Sequence {
+    public func rdar33429010<Result>(into initialResult: Result,
+                                     _ nextPartialResult: (_ partialResult: inout Result, Iterator.Element) throws -> ()
+        ) rethrows -> Result {
+        return initialResult
+    }
+}
+
+extension Int {
+   public mutating func rdar33429010_incr(_ inc: Int) {
+     self += inc
+   }
+}
+
+func rdar33429010_2() {
+  let iter = I_33429010()
+  var acc: Int = 0 // expected-warning {{}}
+  let _: Int = AnySequence { iter }.rdar33429010(into: acc, { $0 + $1 })
+  // expected-warning@-1 {{result of operator '+' is unused}}
+  let _: Int = AnySequence { iter }.rdar33429010(into: acc, { $0.rdar33429010_incr($1) })
+}
+
+class P_33429010 {
+  var name: String = "foo"
+}
+
+class C_33429010 : P_33429010 {
+}
+
+func rdar33429010_3() {
+ let arr = [C_33429010()]
+ let _ = arr.map({ ($0.name, $0 as P_33429010) }) // Ok
+}

test/Sema/immutability.swift

@@ -628,8 +628,7 @@ func sr4214() {
     return f(x)
   }
 
-  // expected-error@+1 {{expression type '(inout MutableSubscripts) -> ()' is ambiguous without more context}}
-  let closure = { val in val.x = 7 } as (inout MutableSubscripts) -> ()
+  let closure = { val in val.x = 7 } as (inout MutableSubscripts) -> () // Ok
   var v = MutableSubscripts()
   closure(&v)
   // FIXME: This diagnostic isn't really all that much better

validation-test/Sema/type_checker_perf/fast/rdar22282851.swift → validation-test/Sema/type_checker_perf/slow/rdar22282851.swift

@@ -7,5 +7,6 @@ struct S {
 
 func rdar22282851(_ a: [S]) -> [S] {
   let result = a.sorted{ $0.s < $1.s || ($0.s == $1.s && $0.s < $1.s) }
+  // expected-error@-1 {{expression was too complex to be solved in reasonable time; consider breaking up the expression into distinct sub-expressions}}
   return result
 }