Merge pull request #29734 from LucianoPAlmeida/remove-diag-parameters-errors

xedin · web-flow · commit b3590c5f1dd4 · 2020-02-11T01:32:31.000-08:00
[Diagnostics] Remove `FailureDiagnosis::diagnoseParameterErrors` from CSDiag
diff --git a/lib/Sema/CSDiag.cpp b/lib/Sema/CSDiag.cpp
@@ -182,12 +182,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
     return type;
   }
 
-  /// Diagnose common failures due to applications of an argument list to an
-  /// ApplyExpr or SubscriptExpr.
-  bool diagnoseParameterErrors(CalleeCandidateInfo &CCI,
-                               Expr *fnExpr, Expr *argExpr,
-                               ArrayRef<Identifier> argLabels);
-
   /// Attempt to diagnose a specific failure from the info we've collected from
   /// the failed constraint system.
   bool diagnoseExprFailure();
@@ -918,48 +912,6 @@ static Expr *getFailedArgumentExpr(CalleeCandidateInfo CCI, Expr *argExpr) {
   }
 }
 
-/// If the candidate set has been narrowed down to a specific structural
-/// problem, e.g. that there are too few parameters specified or that argument
-/// labels don't match up, diagnose that error and return true.
-bool FailureDiagnosis::diagnoseParameterErrors(CalleeCandidateInfo &CCI,
-                                               Expr *fnExpr, Expr *argExpr,
-                                               ArrayRef<Identifier> argLabels) {
-  // If we have a failure where the candidate set differs on exactly one
-  // argument, and where we have a consistent mismatch across the candidate set
-  // (often because there is only one candidate in the set), then diagnose this
-  // as a specific problem of passing something of the wrong type into a
-  // parameter.
-  //
-  // We don't generally want to use this path to diagnose calls to
-  // symmetrically-typed binary operators because it's likely that both
-  // operands contributed to the type.
-  if ((CCI.closeness == CC_OneArgumentMismatch ||
-       CCI.closeness == CC_OneArgumentNearMismatch ||
-       CCI.closeness == CC_OneGenericArgumentMismatch ||
-       CCI.closeness == CC_OneGenericArgumentNearMismatch ||
-       CCI.closeness == CC_GenericNonsubstitutableMismatch) &&
-      CCI.failedArgument.isValid() &&
-      !isSymmetricBinaryOperator(CCI)) {
-    // Map the argument number into an argument expression.
-    TCCOptions options = TCC_ForceRecheck;
-    if (CCI.failedArgument.parameterType->is<InOutType>())
-      options |= TCC_AllowLValue;
-
-    // It could be that the argument doesn't conform to an archetype.
-    Expr *badArgExpr = getFailedArgumentExpr(CCI, argExpr);
-
-    // Re-type-check the argument with the expected type of the candidate set.
-    // This should produce a specific and tailored diagnostic saying that the
-    // type mismatches with expectations.
-    Type paramType = CCI.failedArgument.parameterType;
-    if (!typeCheckChildIndependently(badArgExpr, paramType,
-                                     CTP_CallArgument, options))
-      return true;
-  }
-  
-  return false;
-}
-
 bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
   auto *fnExpr = callExpr->getFn();
   auto fnType = CS.getType(fnExpr)->getRValueType();
@@ -997,9 +949,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
   SmallVector<Identifier, 2> argLabelsScratch;
   ArrayRef<Identifier> argLabels =
     callExpr->getArgumentLabels(argLabelsScratch);
-  if (diagnoseParameterErrors(calleeInfo, callExpr->getFn(),
-                              callExpr->getArg(), argLabels))
-    return true;
 
   Type argType;  // argument list, if known.
   if (auto FTy = fnType->getAs<AnyFunctionType>()) {
@@ -1025,10 +974,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
 
   calleeInfo.filterListArgs(decomposeArgType(CS.getType(argExpr), argLabels));
 
-  if (diagnoseParameterErrors(calleeInfo, callExpr->getFn(), argExpr,
-                              argLabels))
-    return true;
-
   // Force recheck of the arg expression because we allowed unresolved types
   // before, and that turned out not to help, and now we want any diagnoses
   // from disallowing them.
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -3072,23 +3072,16 @@ bool ConstraintSystem::repairFailures(
   auto elt = path.back();
   switch (elt.getKind()) {
   case ConstraintLocator::LValueConversion: {
-    auto CTP = getContextualTypePurpose(anchor);
-    // Special case for `CTP_CallArgument` set by CSDiag
-    // while type-checking each argument because we yet
-    // to cover argument-to-parameter conversions in the
-    // new framework.
-    if (CTP != CTP_CallArgument) {
-      // Ignore l-value conversion element since it has already
-      // played its role.
-      path.pop_back();
-      // If this is a contextual mismatch between l-value types e.g.
-      // `@lvalue String vs. @lvalue Int`, let's pretend that it's okay.
-      if (!path.empty() && path.back().is<LocatorPathElt::ContextualType>()) {
-        auto *locator = getConstraintLocator(anchor, path.back());
-        conversionsOrFixes.push_back(
-            IgnoreContextualType::create(*this, lhs, rhs, locator));
-        break;
-      }
+    // Ignore l-value conversion element since it has already
+    // played its role.
+    path.pop_back();
+    // If this is a contextual mismatch between l-value types e.g.
+    // `@lvalue String vs. @lvalue Int`, let's pretend that it's okay.
+    if (!path.empty() && path.back().is<LocatorPathElt::ContextualType>()) {
+      auto *locator = getConstraintLocator(anchor, path.back());
+      conversionsOrFixes.push_back(
+          IgnoreContextualType::create(*this, lhs, rhs, locator));
+      break;
     }
 
     LLVM_FALLTHROUGH;
@@ -3521,13 +3514,6 @@ bool ConstraintSystem::repairFailures(
     if (rhs->isExistentialType())
       break;
 
-    // TODO(diagnostics): This is a problem related to `inout` mismatch,
-    // in argument position, and we got here from CSDiag. Once
-    // argument-to-pararameter conversion failures are implemented,
-    // this check could be removed.
-    if (lhs->is<InOutType>() || rhs->is<InOutType>())
-      break;
-
     if (repairViaOptionalUnwrap(*this, lhs, rhs, matchKind, conversionsOrFixes,
                                 locator))
       break;
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -2765,6 +2765,9 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
     ParameterList,
     /// General ambiguity failure.
     General,
+    /// Argument mismatch ambiguity where each solution has the same
+    /// argument mismatch fixes for the same call.
+    ArgumentMismatch
   };
   auto ambiguityKind = AmbiguityKind::CloseMatch;
 
@@ -2775,16 +2778,52 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
       return false;
 
     if (fixes.size() > 1) {
-      ambiguityKind = (ambiguityKind == AmbiguityKind::CloseMatch ||
-                       ambiguityKind == AmbiguityKind::ParameterList) &&
+      // Attempt to disambiguite in cases where the argument matches
+      // involves tuple mismatches. e.g.
+      // func t<T, U>(_: (T, U), _: (U, T)) {}
+      // func useTuples(_ x: Int, y: Float) {
+      //     t((x, y), (x, y))
+      // }
+      // So fixes are ambiguous in all solutions.
+      if ((ambiguityKind == AmbiguityKind::CloseMatch ||
+           ambiguityKind == AmbiguityKind::ArgumentMismatch) &&
           llvm::all_of(fixes, [](const ConstraintFix *fix) -> bool {
-            auto *locator = fix->getLocator();
-            return locator->findLast<LocatorPathElt::ApplyArgument>().hasValue();
-          }) ? AmbiguityKind::ParameterList
-             : AmbiguityKind::General;
+            return fix->getKind() == FixKind::AllowTupleTypeMismatch;
+          })) {
+        ambiguityKind = AmbiguityKind::ArgumentMismatch;
+      } else {
+        ambiguityKind =
+            (ambiguityKind == AmbiguityKind::CloseMatch ||
+             ambiguityKind == AmbiguityKind::ArgumentMismatch ||
+             ambiguityKind == AmbiguityKind::ParameterList) &&
+                    llvm::all_of(
+                        fixes,
+                        [](const ConstraintFix *fix) -> bool {
+                          auto *locator = fix->getLocator();
+                          return locator
+                              ->findLast<LocatorPathElt::ApplyArgument>()
+                              .hasValue();
+                        })
+                ? AmbiguityKind::ParameterList
+                : AmbiguityKind::General;
+      }
+    }
+
+    if (fixes.size() == 1) {
+      // Attempt to disambiguite in cases where all the solutions
+      // produces the same fixes for different generic arguments e.g.
+      //   func f<T>(_: T, _: T) {}
+      //   f(Int(1), Float(1))
+      //
+      ambiguityKind =
+          ((ambiguityKind == AmbiguityKind::CloseMatch ||
+            ambiguityKind == AmbiguityKind::ArgumentMismatch) &&
+           fixes.front()->getKind() == FixKind::AllowArgumentTypeMismatch)
+              ? AmbiguityKind::ArgumentMismatch
+              : AmbiguityKind::CloseMatch;
     }
 
-    for (const auto *fix: fixes) {
+    for (const auto *fix : fixes) {
       auto *locator = fix->getLocator();
       // Assignment failures are all about the source expression,
       // because they treat destination as a contextual type.
@@ -2816,6 +2855,15 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
     return true;
   });
 
+  if (ambiguityKind == AmbiguityKind::ArgumentMismatch &&
+      viableSolutions.size() == 1) {
+    // Let's apply the solution so the contextual generic types
+    // are available in the system for diagnostics.
+    applySolution(*viableSolutions[0]);
+    solutions.front().Fixes.front()->diagnose(/*asNote*/ false);
+    return true;
+  }
+
   if (!diagnosable || viableSolutions.size() < 2)
     return false;
 
@@ -2860,6 +2908,7 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
     };
 
     switch (ambiguityKind) {
+    case AmbiguityKind::ArgumentMismatch:
     case AmbiguityKind::CloseMatch:
       // Handled below
       break;
diff --git a/test/type/opaque.swift b/test/type/opaque.swift
@@ -270,10 +270,10 @@ func associatedTypeIdentity() {
 
   sameType(cr, c.r_out())
   sameType(dr, d.r_out())
-  sameType(cr, dr) // expected-error{{}} expected-note {{}}
+  sameType(cr, dr) // expected-error {{cannot convert value of type '(some opaque.R).S' (associated type of protocol 'R') to expected argument type '(some opaque.R).S' (associated type of protocol 'R')}}
   sameType(gary(candace()).r_out(), gary(candace()).r_out())
   sameType(gary(doug()).r_out(), gary(doug()).r_out())
-  sameType(gary(doug()).r_out(), gary(candace()).r_out()) // expected-error{{}} expected-note {{}}
+  sameType(gary(doug()).r_out(), gary(candace()).r_out()) // expected-error {{cannot convert value of type 'some R' (result of 'candace()') to expected argument type 'some R' (result of 'doug()')}}
 }
 
 func redeclaration() -> some P { return 0 } // expected-note 2{{previously declared}}
