[CSDiagnostics] Diagnose contextual closure result mismatches via fixes

Let's keep track of type mismatch between type deduced
for the body of the closure vs. what is requested
contextually, it makes it much easier to diagnose
problems like:

```swift
func foo(_: () -> Int) {}
foo { "hello" }
```

Because we can pin-point problematic area of the source
when the rest of the system is consistent.

Resolves: rdar://problem/40537960

lib/Sema/CSDiag.cpp

@@ -2228,48 +2228,6 @@ static bool tryRawRepresentableFixIts(InFlightDiagnostic &diag,
   return false;
 }
 
-/// Try to add a fix-it when converting between a collection and its slice type,
-/// such as String <-> Substring or (eventually) Array <-> ArraySlice
-static bool trySequenceSubsequenceConversionFixIts(InFlightDiagnostic &diag,
-                                                   ConstraintSystem &CS,
-                                                   Type fromType, Type toType,
-                                                   Expr *expr) {
-  if (CS.TC.Context.getStdlibModule() == nullptr)
-    return false;
-
-  auto String = CS.TC.getStringType(CS.DC);
-  auto Substring = CS.TC.getSubstringType(CS.DC);
-
-  if (!String || !Substring)
-    return false;
-
-  /// FIXME: Remove this flag when void subscripts are implemented.
-  /// Make this unconditional and remove the if statement.
-  if (CS.TC.getLangOpts().FixStringToSubstringConversions) {
-    // String -> Substring conversion
-    // Add '[]' void subscript call to turn the whole String into a Substring
-    if (fromType->isEqual(String)) {
-      if (toType->isEqual(Substring)) {
-        diag.fixItInsertAfter(expr->getEndLoc (), "[]");
-        return true;
-      }
-    }
-  }
-
-  // Substring -> String conversion
-  // Wrap in String.init
-  if (fromType->isEqual(Substring)) {
-    if (toType->isEqual(String)) {
-      auto range = expr->getSourceRange();
-      diag.fixItInsert(range.Start, "String(");
-      diag.fixItInsertAfter(range.End, ")");
-      return true;
-    }
-  }
-
-  return false;
-}
-
 /// Attempts to add fix-its for these two mistakes:
 ///
 /// - Passing an integer with the right type but which is getting wrapped with a
@@ -2738,10 +2696,9 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
 
   // Try to convert between a sequence and its subsequence, notably
   // String <-> Substring.
-  if (trySequenceSubsequenceConversionFixIts(diag, CS, exprType, contextualType,
-                                             expr)) {
+  if (ContextualFailure::trySequenceSubsequenceFixIts(diag, CS, exprType,
+                                                      contextualType, expr))
     return true;
-  }
 
   // Attempt to add a fixit for the error.
   switch (CTP) {

lib/Sema/CSDiagnostics.cpp

@@ -1106,3 +1106,90 @@ Diag<StringRef> AssignmentFailure::findDeclDiagonstic(ASTContext &ctx,
 
   return diag::assignment_lhs_is_immutable_variable;
 }
+
+bool ContextualFailure::diagnoseAsError() {
+  auto *anchor = getAnchor();
+  auto path = getLocator()->getPath();
+
+  assert(!path.empty());
+
+  if (diagnoseMissingFunctionCall())
+    return true;
+
+  Diag<Type, Type> diagnostic;
+  switch (path.back().getKind()) {
+  case ConstraintLocator::ClosureResult: {
+    diagnostic = diag::cannot_convert_closure_result;
+    break;
+  }
+
+  default:
+    return false;
+  }
+
+  auto diag = emitDiagnostic(anchor->getLoc(), diagnostic, FromType, ToType);
+  diag.highlight(anchor->getSourceRange());
+
+  (void)trySequenceSubsequenceFixIts(diag, getConstraintSystem(), FromType,
+                                     ToType, anchor);
+  return true;
+}
+
+bool ContextualFailure::diagnoseMissingFunctionCall() const {
+  auto &TC = getTypeChecker();
+
+  auto *srcFT = FromType->getAs<FunctionType>();
+  if (!srcFT || !srcFT->getParams().empty())
+    return false;
+
+  if (ToType->is<AnyFunctionType>() ||
+      !TC.isConvertibleTo(srcFT->getResult(), ToType, getDC()))
+    return false;
+
+  auto *anchor = getAnchor();
+  emitDiagnostic(anchor->getLoc(), diag::missing_nullary_call,
+                 srcFT->getResult())
+      .highlight(anchor->getSourceRange())
+      .fixItInsertAfter(anchor->getEndLoc(), "()");
+  return true;
+}
+
+bool ContextualFailure::trySequenceSubsequenceFixIts(InFlightDiagnostic &diag,
+                                                     ConstraintSystem &CS,
+                                                     Type fromType, Type toType,
+                                                     Expr *expr) {
+  if (!CS.TC.Context.getStdlibModule())
+    return false;
+
+  auto String = CS.TC.getStringType(CS.DC);
+  auto Substring = CS.TC.getSubstringType(CS.DC);
+
+  if (!String || !Substring)
+    return false;
+
+  /// FIXME: Remove this flag when void subscripts are implemented.
+  /// Make this unconditional and remove the if statement.
+  if (CS.TC.getLangOpts().FixStringToSubstringConversions) {
+    // String -> Substring conversion
+    // Add '[]' void subscript call to turn the whole String into a Substring
+    if (fromType->isEqual(String)) {
+      if (toType->isEqual(Substring)) {
+        diag.fixItInsertAfter(expr->getEndLoc(), "[]");
+        return true;
+      }
+    }
+  }
+
+  // Substring -> String conversion
+  // Wrap in String.init
+  if (fromType->isEqual(Substring)) {
+    if (toType->isEqual(String)) {
+      auto range = expr->getSourceRange();
+      diag.fixItInsert(range.Start, "String(");
+      diag.fixItInsertAfter(range.End, ")");
+      return true;
+    }
+  }
+
+  return false;
+}

lib/Sema/CSDiagnostics.h

@@ -558,6 +558,40 @@ private:
   }
 };
 
+/// Intended to diagnose any possible contextual failure
+/// e.g. argument/parameter, closure result, conversions etc.
+class ContextualFailure final : public FailureDiagnostic {
+  Type FromType, ToType;
+
+public:
+  ContextualFailure(Expr *root, ConstraintSystem &cs, Type lhs, Type rhs,
+                    ConstraintLocator *locator)
+      : FailureDiagnostic(root, cs, locator), FromType(resolve(lhs)),
+        ToType(resolve(rhs)) {}
+
+  bool diagnoseAsError() override;
+
+  // If we're trying to convert something of type "() -> T" to T,
+  // then we probably meant to call the value.
+  bool diagnoseMissingFunctionCall() const;
+
+  /// Try to add a fix-it when converting between a collection and its slice
+  /// type, such as String <-> Substring or (eventually) Array <-> ArraySlice
+  static bool trySequenceSubsequenceFixIts(InFlightDiagnostic &diag,
+                                           ConstraintSystem &CS, Type fromType,
+                                           Type toType, Expr *expr);
+
+private:
+  Type resolve(Type rawType) {
+    auto type = resolveType(rawType)->getWithoutSpecifierType();
+    if (auto *BGT = type->getAs<BoundGenericType>()) {
+      if (BGT->hasUnresolvedType())
+        return BGT->getDecl()->getDeclaredInterfaceType();
+    }
+    return type;
+  }
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSFix.cpp

@@ -190,3 +190,15 @@ SkipSuperclassRequirement::create(ConstraintSystem &cs, Type lhs, Type rhs,
   return new (cs.getAllocator())
       SkipSuperclassRequirement(cs, lhs, rhs, locator);
 }
+
+bool ContextualMismatch::diagnose(Expr *root, bool asNote) const {
+  auto failure = ContextualFailure(root, getConstraintSystem(), getFromType(),
+                                   getToType(), getLocator());
+  return failure.diagnose(asNote);
+}
+
+ContextualMismatch *ContextualMismatch::create(ConstraintSystem &cs, Type lhs,
+                                               Type rhs,
+                                               ConstraintLocator *locator) {
+  return new (cs.getAllocator()) ContextualMismatch(cs, lhs, rhs, locator);
+}

lib/Sema/CSFix.h

@@ -80,6 +80,9 @@ enum class FixKind : uint8_t {
   /// and assume that types are related.
   SkipSuperclassRequirement,
 
+  /// Fix up one of the sides of conversion to make it seem
+  /// like the types are aligned.
+  ContextualMismatch,
 };
 
 class ConstraintFix {
@@ -347,6 +350,35 @@ public:
   create(ConstraintSystem &cs, Type lhs, Type rhs, ConstraintLocator *locator);
 };
 
+/// For example: Sometimes type returned from the body of the
+/// closure doesn't match expected contextual type:
+///
+/// func foo(_: () -> Int) {}
+/// foo { "ultimate question" }
+///
+/// Body of the closure produces `String` type when `Int` is expected
+/// by the context.
+class ContextualMismatch : public ConstraintFix {
+  Type LHS, RHS;
+
+protected:
+  ContextualMismatch(ConstraintSystem &cs, Type lhs, Type rhs,
+                     ConstraintLocator *locator)
+      : ConstraintFix(cs, FixKind::ContextualMismatch, locator), LHS(lhs),
+        RHS(rhs) {}
+
+public:
+  std::string getName() const override { return "fix contextual mismatch"; }
+
+  Type getFromType() const { return LHS; }
+  Type getToType() const { return RHS; }
+
+  bool diagnose(Expr *root, bool asNote = false) const override;
+
+  static ContextualMismatch *create(ConstraintSystem &cs, Type lhs, Type rhs,
+                                    ConstraintLocator *locator);
+};
+
 } // end namespace constraints
 } // end namespace swift
 

lib/Sema/CSSimplify.cpp

@@ -1573,21 +1573,42 @@ ConstraintSystem::matchTypesBindTypeVar(
   return getTypeMatchSuccess();
 }
 
-static void attemptToFixRequirementFailure(
-    ConstraintSystem &cs, Type type1, Type type2,
-    SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
-    ConstraintLocatorBuilder locator) {
-  using LocatorPathEltKind = ConstraintLocator::PathElementKind;
-
+static ConstraintFix *fixRequirementFailure(ConstraintSystem &cs, Type type1,
+                                            Type type2, Expr *anchor,
+                                            LocatorPathElt &req) {
   // Can't fix not yet properly resolved types.
   if (type1->hasTypeVariable() || type2->hasTypeVariable())
-    return;
+    return nullptr;
 
   // If dependent members are present here it's because
   // base doesn't conform to associated type's protocol.
   if (type1->hasDependentMember() || type2->hasDependentMember())
-    return;
+    return nullptr;
 
+  // Build simplified locator which only contains anchor and requirement info.
+  ConstraintLocatorBuilder requirement(cs.getConstraintLocator(anchor));
+  auto *reqLoc = cs.getConstraintLocator(requirement.withPathElement(req));
+
+  auto reqKind = static_cast<RequirementKind>(req.getValue2());
+  switch (reqKind) {
+  case RequirementKind::SameType: {
+    return SkipSameTypeRequirement::create(cs, type1, type2, reqLoc);
+  }
+
+  case RequirementKind::Superclass: {
+    return SkipSuperclassRequirement::create(cs, type1, type2, reqLoc);
+  }
+
+  case RequirementKind::Conformance:
+  case RequirementKind::Layout:
+    llvm_unreachable("conformance requirements are handled elsewhere");
+  }
+}
+
+static void
+repairFailures(ConstraintSystem &cs, Type lhs, Type rhs,
+               SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
+               ConstraintLocatorBuilder locator) {
   SmallVector<LocatorPathElt, 4> path;
   auto *anchor = locator.getLocatorParts(path);
 
@@ -1595,30 +1616,22 @@ static void attemptToFixRequirementFailure(
     return;
 
   auto &elt = path.back();
-  if (elt.getKind() != LocatorPathEltKind::TypeParameterRequirement)
-    return;
-
-  // Build simplified locator which only contains anchor and requirement info.
-  ConstraintLocatorBuilder requirement(cs.getConstraintLocator(anchor));
-  auto *reqLoc = cs.getConstraintLocator(requirement.withPathElement(elt));
-
-  auto reqKind = static_cast<RequirementKind>(elt.getValue2());
-  switch (reqKind) {
-  case RequirementKind::SameType: {
-    auto *fix = SkipSameTypeRequirement::create(cs, type1, type2, reqLoc);
-    conversionsOrFixes.push_back(fix);
-    return;
+  switch (elt.getKind()) {
+  case ConstraintLocator::TypeParameterRequirement: {
+    if (auto *fix = fixRequirementFailure(cs, lhs, rhs, anchor, elt))
+      conversionsOrFixes.push_back(fix);
+    break;
   }
 
-  case RequirementKind::Superclass: {
-    auto *fix = SkipSuperclassRequirement::create(cs, type1, type2, reqLoc);
+  case ConstraintLocator::ClosureResult: {
+    auto *fix = ContextualMismatch::create(cs, lhs, rhs,
+                                           cs.getConstraintLocator(locator));
     conversionsOrFixes.push_back(fix);
-    return;
+    break;
   }
 
-  case RequirementKind::Conformance:
-  case RequirementKind::Layout:
-    llvm_unreachable("conformance requirements are handled elsewhere");
+  default:
+    return;
   }
 }
 
@@ -2407,9 +2420,9 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
           TreatRValueAsLValue::create(*this, getConstraintLocator(locator)));
   }
 
+  // Attempt to repair any failures identifiable at this point.
   if (attemptFixes)
-    attemptToFixRequirementFailure(*this, type1, type2, conversionsOrFixes,
-                                   locator);
+    repairFailures(*this, type1, type2, conversionsOrFixes, locator);
 
   if (conversionsOrFixes.empty()) {
     // If one of the types is a type variable or member thereof, we leave this
@@ -5047,7 +5060,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
   }
 
   case FixKind::SkipSameTypeRequirement:
-  case FixKind::SkipSuperclassRequirement: {
+  case FixKind::SkipSuperclassRequirement:
+  case FixKind::ContextualMismatch: {
     return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
   }
 

test/Constraints/closures.swift

@@ -805,3 +805,29 @@ func rdar_45659733() {
     _ = (a ..< b).map { i in foo(i, i) } // Ok
   }
 }
+
+
+// rdar://problem/40537960 - Misleading diagnostic when using closure with wrong type
+
+protocol P_40537960 {}
+func rdar_40537960() {
+  struct S {
+    var v: String
+  }
+
+  struct L : P_40537960 {
+    init(_: String) {}
+  }
+
+  struct R<T : P_40537960> {
+    init(_: P_40537960) {}
+  }
+
+  struct A<T: Collection, P: P_40537960> {
+    typealias Data = T.Element
+    init(_: T, fn: (Data) -> R<P>) {}
+  }
+
+  var arr: [S] = []
+  _ = A(arr, fn: { L($0.v) }) // expected-error {{cannot convert value of type 'L' to closure result type 'R<T>'}}
+}

test/Sema/string_to_substring_conversion.swift

@@ -5,7 +5,7 @@ let ss = s[s.startIndex..<s.endIndex]
 
 // CTP_Initialization
 do {
-  let s1: Substring = { return s }() // expected-error {{cannot convert value of type 'String' to specified type 'Substring'}} {{37-37=[]}}
+  let s1: Substring = { return s }() // expected-error {{cannot convert value of type 'String' to closure result type 'Substring'}} {{33-33=[]}}
   _ = s1
 }
 

test/Sema/substring_to_string_conversion_swift4.swift

@@ -5,7 +5,7 @@ let ss = s[s.startIndex..<s.endIndex]
 
 // CTP_Initialization
 do {
-  let s1: String = { return ss }() // expected-error {{cannot convert value of type 'Substring' to specified type 'String'}} {{20-20=String(}} {{35-35=)}}
+  let s1: String = { return ss }() // expected-error {{cannot convert value of type 'Substring' to closure result type 'String'}} {{29-29=String(}} {{31-31=)}}
   _ = s1
 }
 

test/expr/closure/closures.swift

@@ -15,7 +15,7 @@ var closure3b : (Int,Int) -> (Int) -> (Int,Int) = {{ (4, 2) }} // expected-error
 var closure4 : (Int,Int) -> Int = { $0 + $1 }
 var closure5 : (Double) -> Int = {
        $0 + 1.0
-       // expected-error@+1 {{cannot convert value of type 'Double' to closure result type 'Int'}}
+       // expected-error@-1 {{cannot convert value of type 'Double' to closure result type 'Int'}}
 }
 
 var closure6 = $0  // expected-error {{anonymous closure argument not contained in a closure}}

validation-test/compiler_crashers_2_fixed/0148-rdar35773761.swift

@@ -1,4 +1,4 @@
 // RUN: %target-typecheck-verify-swift
 
 let b: () -> Void = withoutActuallyEscaping({ print("hello crash") }, do: { $0() })
-// expected-error@-1 {{cannot convert value of type '()' to specified type '() -> Void'}}
+// expected-error@-1 {{cannot convert value of type '()' to closure result type '() -> Void'}}