Merge pull request #6586 from xedin/SR_3520

Generic function taking closure with inout parameter returns erroneous results

Author: slavapestov

lib/Sema/CSDiag.cpp

@@ -6330,15 +6330,14 @@ visitRebindSelfInConstructorExpr(RebindSelfInConstructorExpr *E) {
 
 
 bool FailureDiagnosis::visitClosureExpr(ClosureExpr *CE) {
+  auto contextualType = CS->getContextualType();
   Type expectedResultType;
-  
+
   // If we have a contextual type available for this closure, apply it to the
   // ParamDecls in our parameter list.  This ensures that any uses of them get
   // appropriate types.
-  if (CS->getContextualType() &&
-      CS->getContextualType()->is<AnyFunctionType>()) {
-
-    auto fnType = CS->getContextualType()->castTo<AnyFunctionType>();
+  if (contextualType && contextualType->is<AnyFunctionType>()) {
+    auto fnType = contextualType->getAs<AnyFunctionType>();
     auto *params = CE->getParameters();
     Type inferredArgType = fnType->getInput();
 
@@ -6393,12 +6392,24 @@ bool FailureDiagnosis::visitClosureExpr(ClosureExpr *CE) {
       return true;
     }
 
+    // Coerce parameter types here only if there are no unresolved
     if (CS->TC.coerceParameterListToType(params, CE, fnType))
       return true;
 
+    for (auto param : *params) {
+      auto paramType = param->getType();
+      // If this is unresolved 'inout' parameter, it's better to drop
+      // 'inout' from type but keep 'mutability' classifier because that
+      // might help to diagnose actual problem e.g. type inference and
+      // doesn't give us much information anyway.
+      if (paramType->is<InOutType>() && paramType->hasUnresolvedType()) {
+        param->setType(CS->getASTContext().TheUnresolvedType);
+        param->setInterfaceType(param->getType());
+      }
+    }
+
     expectedResultType = fnType->getResult();
   } else {
-    
     // Defend against type variables from our constraint system leaking into
     // recursive constraints systems formed when checking the body of the
     // closure.  These typevars come into them when the body does name
@@ -6417,36 +6428,67 @@ bool FailureDiagnosis::visitClosureExpr(ClosureExpr *CE) {
   if (!CE->hasSingleExpressionBody())
     return false;
 
-  // If the closure had an expected result type, use it.
-  if (CE->hasExplicitResultType())
-    expectedResultType = CE->getExplicitResultTypeLoc().getType();
-
   // When we're type checking a single-expression closure, we need to reset the
   // DeclContext to this closure for the recursive type checking.  Otherwise,
   // if there is a closure in the subexpression, we can violate invariants.
   {
     llvm::SaveAndRestore<DeclContext*> SavedDC(CS->DC, CE);
-    
-    auto CTP = expectedResultType ? CTP_ClosureResult : CTP_Unused;
 
     // Explicitly disallow to produce solutions with unresolved type variables,
     // because there is no auxiliary logic which would handle that and it's
     // better to allow failure diagnosis to run directly on the closure body.
     // Note that presence of contextual type implicitly forbids such solutions,
     // but it's not always reset.
+
+    if (expectedResultType && !CE->hasExplicitResultType()) {
+      ExprCleaner cleaner(CE);
+
+      auto closure = CE->getSingleExpressionBody();
+      ConcreteDeclRef decl = nullptr;
+      // Let's try to compute result type without mutating AST and
+      // using expected (contextual) result type, that's going to help
+      // diagnose situations where contextual type expected one result
+      // type but actual closure produces a different one without explicitly
+      // declaring it (e.g. by using anonymous parameters).
+      auto type = CS->TC.getTypeOfExpressionWithoutApplying(
+          closure, CS->DC, decl, FreeTypeVariableBinding::Disallow);
+
+      Type resultType = type.hasValue() ? *type : Type();
+
+      // Following situations are possible:
+      // * No result type - possible structurable problem in the body;
+      // * Function result type - possible use of function without calling it,
+      //   which is properly diagnosed by actual type-check call.
+      if (resultType && !resultType->getRValueType()->is<AnyFunctionType>()) {
+        if (!resultType->isEqual(expectedResultType)) {
+          diagnose(closure->getEndLoc(), diag::cannot_convert_closure_result,
+                   resultType, expectedResultType);
+          return true;
+        }
+      }
+    }
+
+    // If the closure had an expected result type, use it.
+    if (CE->hasExplicitResultType())
+      expectedResultType = CE->getExplicitResultTypeLoc().getType();
+
+    // If we couldn't diagnose anything related to the contextual result type
+    // let's run proper type-check with expected type and try to verify it.
+
+    auto CTP = expectedResultType ? CTP_ClosureResult : CTP_Unused;
     if (!typeCheckChildIndependently(CE->getSingleExpressionBody(),
                                      expectedResultType, CTP, TCCOptions(),
                                      nullptr, false))
       return true;
   }
-  
+
   // If the body of the closure looked ok, then look for a contextual type
   // error.  This is necessary because FailureDiagnosis::diagnoseExprFailure
   // doesn't do this for closures.
-  if (CS->getContextualType() &&
-      !CS->getContextualType()->isEqual(CE->getType())) {
-
-    auto fnType = CS->getContextualType()->getAs<AnyFunctionType>();
+  if (contextualType) {
+    auto fnType = contextualType->getAs<AnyFunctionType>();
+    if (!fnType || fnType->isEqual(CE->getType()))
+      return false;
 
     // If the closure had an explicitly written return type incompatible with
     // the contextual type, diagnose that.

lib/Sema/CSSimplify.cpp

@@ -1448,6 +1448,40 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
       SWIFT_FALLTHROUGH;
 
     case ConstraintKind::Subtype:
+      // Subtype constraints are subject for edge contraction,
+      // which is inappropriate in this case, because it's going to
+      // erase/lose 'inout' modifier after merging equivalence classes
+      // (if inout containts type var, see ConstraintGraph::contractEdges()),
+      // since right-hand side type variable must not be materializable
+      // it can simply get left-hand side as a fixed binding, otherwise fail.
+      if (type1->is<InOutType>() &&
+          type1->getInOutObjectType()->isTypeVariableOrMember() && typeVar2) {
+        // Left-hand side type is not materializable, so we need to
+        // check if it's even appropriate to have such a constraint
+        // between these two types, or fail early otherwise if right-hand
+        // side must be materializable.
+        if (typeVar2->getImpl().mustBeMaterializable())
+          return SolutionKind::Error;
+
+        // Constriants like `inout T0 subtype T1` where (T0 must be
+        // materializable) are created when closures are part of the generic
+        // function parameters e.g. `func foo<T>(_ t: T, (inout T) -> Void) {}`
+        // so when such function gets called e.g.
+        // ```
+        //  var x = 42
+        //  foo(x) { $0 = 0 }
+        // ```
+        // it's going to try and map closure parameters type (inout T0), where
+        // T0 is opened generic parameter T, to argument type (T1), which can
+        // be 'inout' but it's uncertain at this stage, but since closure
+        // 'declaration' `{ $0 = 0 }` is wrapped inside of a function call,
+        // it has to 'map' parameters to arguments instead of converting them,
+        // see `ConstraintSystem::matchFunctionTypes`.
+        assignFixedType(typeVar2, type1);
+        return SolutionKind::Solved;
+      }
+      SWIFT_FALLTHROUGH;
+
     case ConstraintKind::ArgumentConversion:
     case ConstraintKind::OperatorArgumentTupleConversion:
     case ConstraintKind::OperatorArgumentConversion:

test/Constraints/closures.swift

@@ -395,3 +395,45 @@ let mismatchInClosureResultType : (String) -> ((Int) -> Void) = {
     return { }
     // expected-error@-1 {{contextual type for closure argument list expects 1 argument, which cannot be implicitly ignored}}
 }
+
+// SR-3520: Generic function taking closure with inout parameter can result in a variety of compiler errors or EXC_BAD_ACCESS
+func sr3520_1<T>(_ g: (inout T) -> Int) {}
+sr3520_1 { $0 = 1 } // expected-error {{cannot convert value of type '()' to closure result type 'Int'}}
+
+func sr3520_2<T>(_ item: T, _ update: (inout T) -> Void) {
+  var x = item
+  update(&x)
+}
+var sr3250_arg = 42
+sr3520_2(sr3250_arg) { $0 += 3 } // ok
+
+// This test makes sure that having closure with inout argument doesn't crash with member lookup
+struct S_3520 {
+  var number1: Int
+}
+func sr3520_set_via_closure<S, T>(_ closure: (inout S, T) -> ()) {}
+sr3520_set_via_closure({ $0.number1 = $1 }) // expected-error {{type of expression is ambiguous without more context}}
+
+// SR-1976/SR-3073: Inference of inout
+func sr1976<T>(_ closure: (inout T) -> Void) {}
+sr1976({ $0 += 2 }) // ok
+
+// SR-3073: UnresolvedDotExpr in single expression closure
+
+struct SR3073Lense<Whole, Part> {
+  let set: (inout Whole, Part) -> ()
+}
+struct SR3073 {
+  var number1: Int
+  func lenses() {
+    let _: SR3073Lense<SR3073, Int> = SR3073Lense(
+      set: { $0.number1 = $1 } // ok
+    )
+  }
+}
+
+// SR-3479: Segmentation fault and other error for closure with inout parameter
+func sr3497_unfold<A, B>(_ a0: A, next: (inout A) -> B) {}
+func sr3497() {
+  let _ = sr3497_unfold((0, 0)) { s in 0 } // ok
+}