Merge pull request #20407 from xedin/contextual-closure-result-mismatch

[CSDiagnostics] Diagnose contextual closure result mismatches via fixes

Author: xedin

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 @@ class AssignmentFailure final : public FailureDiagnostic {
   }
 };
 
+/// 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 @@ class SkipSuperclassRequirement final : public ConstraintFix {
   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,47 +1573,30 @@ 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;
-
-  SmallVector<LocatorPathElt, 4> path;
-  auto *anchor = locator.getLocatorParts(path);
-
-  if (path.empty())
-    return;
-
-  auto &elt = path.back();
-  if (elt.getKind() != LocatorPathEltKind::TypeParameterRequirement)
-    return;
+    return nullptr;
 
   // Build simplified locator which only contains anchor and requirement info.
   ConstraintLocatorBuilder requirement(cs.getConstraintLocator(anchor));
-  auto *reqLoc = cs.getConstraintLocator(requirement.withPathElement(elt));
+  auto *reqLoc = cs.getConstraintLocator(requirement.withPathElement(req));
 
-  auto reqKind = static_cast<RequirementKind>(elt.getValue2());
+  auto reqKind = static_cast<RequirementKind>(req.getValue2());
   switch (reqKind) {
   case RequirementKind::SameType: {
-    auto *fix = SkipSameTypeRequirement::create(cs, type1, type2, reqLoc);
-    conversionsOrFixes.push_back(fix);
-    return;
+    return SkipSameTypeRequirement::create(cs, type1, type2, reqLoc);
   }
 
   case RequirementKind::Superclass: {
-    auto *fix = SkipSuperclassRequirement::create(cs, type1, type2, reqLoc);
-    conversionsOrFixes.push_back(fix);
-    return;
+    return SkipSuperclassRequirement::create(cs, type1, type2, reqLoc);
   }
 
   case RequirementKind::Conformance:
@@ -1622,6 +1605,36 @@ static void attemptToFixRequirementFailure(
   }
 }
 
+static void
+repairFailures(ConstraintSystem &cs, Type lhs, Type rhs,
+               SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
+               ConstraintLocatorBuilder locator) {
+  SmallVector<LocatorPathElt, 4> path;
+  auto *anchor = locator.getLocatorParts(path);
+
+  if (path.empty())
+    return;
+
+  auto &elt = path.back();
+  switch (elt.getKind()) {
+  case ConstraintLocator::TypeParameterRequirement: {
+    if (auto *fix = fixRequirementFailure(cs, lhs, rhs, anchor, elt))
+      conversionsOrFixes.push_back(fix);
+    break;
+  }
+
+  case ConstraintLocator::ClosureResult: {
+    auto *fix = ContextualMismatch::create(cs, lhs, rhs,
+                                           cs.getConstraintLocator(locator));
+    conversionsOrFixes.push_back(fix);
+    break;
+  }
+
+  default:
+    return;
+  }
+}
+
 ConstraintSystem::TypeMatchResult
 ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
                              TypeMatchOptions flags,
@@ -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;
   }