[Diagnostics] Tailored diagnostics for reference equality operator mismatches

xedin · xedin · commit b898eaf326ce · 2019-09-13T22:35:52.000-07:00
diff --git a/lib/Sema/CSDiag.cpp b/lib/Sema/CSDiag.cpp
@@ -462,12 +462,6 @@ class FailureDiagnosis :public ASTVisitor<FailureDiagnosis, /*exprresult*/bool>{
   /// exact expression kind).
   bool diagnoseGeneralConversionFailure(Constraint *constraint);
 
-  /// Produce a diagnostic for binary comparisons of the nil literal
-  /// to other values.
-  bool diagnoseNilLiteralComparison(Expr *lhsExpr, Expr *rhsExpr,
-                                    CalleeCandidateInfo &calleeInfo,
-                                    SourceLoc applyLoc);
-
   bool diagnoseMemberFailures(
       Expr *E, Expr *baseEpxr, ConstraintKind lookupKind, DeclName memberName,
       FunctionRefKind funcRefKind, ConstraintLocator *locator,
@@ -3004,60 +2998,6 @@ static bool isNameOfStandardComparisonOperator(StringRef opName) {
          opName == "<="  || opName == ">=";
 }
 
-bool FailureDiagnosis::diagnoseNilLiteralComparison(
-    Expr *lhsExpr, Expr *rhsExpr, CalleeCandidateInfo &calleeInfo,
-    SourceLoc applyLoc) {
-
-  auto overloadName = calleeInfo.declName;
-
-  // Only diagnose for comparison operators.
-  if (!isNameOfStandardComparisonOperator(overloadName))
-    return false;
-
-  Expr *otherExpr = lhsExpr;
-  Expr *nilExpr = rhsExpr;
-
-  // Swap if we picked the wrong side as the nil literal.
-  if (!isa<NilLiteralExpr>(nilExpr->getValueProvidingExpr()))
-    std::swap(otherExpr, nilExpr);
-
-  // Bail if neither side is a nil literal.
-  if (!isa<NilLiteralExpr>(nilExpr->getValueProvidingExpr()))
-    return false;
-
-  // Bail if both sides are a nil literal.
-  if (isa<NilLiteralExpr>(otherExpr->getValueProvidingExpr()))
-    return false;
-
-  auto otherType = CS.getType(otherExpr)->getRValueType();
-
-  // Bail if we were unable to determine the other type.
-  if (isUnresolvedOrTypeVarType(otherType))
-    return false;
-
-  // Regardless of whether the type has reference or value semantics,
-  // comparison with nil is illegal, albeit for different reasons spelled
-  // out by the diagnosis.
-  if (otherType->getOptionalObjectType() &&
-      (overloadName == "!==" || overloadName == "===")) {
-    auto revisedName = overloadName;
-    revisedName.pop_back();
-
-    // If we made it here, then we're trying to perform a comparison with
-    // reference semantics rather than value semantics.  The fixit will
-    // lop off the extra '=' in the operator.
-    diagnose(applyLoc,
-             diag::value_type_comparison_with_nil_illegal_did_you_mean,
-             otherType)
-        .fixItReplace(applyLoc, revisedName);
-  } else {
-    diagnose(applyLoc, diag::value_type_comparison_with_nil_illegal, otherType)
-        .highlight(otherExpr->getSourceRange());
-  }
-
-  return true;
-}
-
 static bool diagnoseClosureExplicitParameterMismatch(
     ConstraintSystem &CS, SourceLoc loc,
     ArrayRef<AnyFunctionType::Param> params,
@@ -3709,11 +3649,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
     auto lhsType = CS.getType(lhsExpr)->getRValueType();
     auto rhsType = CS.getType(rhsExpr)->getRValueType();
 
-    // Diagnose any comparisons with the nil literal.
-    if (diagnoseNilLiteralComparison(lhsExpr, rhsExpr, calleeInfo,
-                                     callExpr->getLoc()))
-      return true;
-
     // TODO(diagnostics): There are still cases not yet handled by new
     // diagnostics framework e.g.
     //
@@ -3732,18 +3667,6 @@ bool FailureDiagnosis::visitApplyExpr(ApplyExpr *callExpr) {
       return failure.diagnosePatternMatchingMismatch();
     }
 
-    // Diagnose attempts to compare reference equality of certain types.
-    if (overloadName == "===" || overloadName == "!==") {
-      // Functions.
-      if (lhsType->is<AnyFunctionType>() || rhsType->is<AnyFunctionType>()) {
-        diagnose(callExpr->getLoc(), diag::cannot_reference_compare_types,
-                 overloadName, lhsType, rhsType)
-          .highlight(lhsExpr->getSourceRange())
-          .highlight(rhsExpr->getSourceRange());
-        return true;
-      }
-    }
-
     if (isContextualConversionFailure(argTuple))
       return false;
 
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -4526,6 +4526,9 @@ bool ArgumentMismatchFailure::diagnoseAsError() {
   if (diagnosePatternMatchingMismatch())
     return true;
 
+  if (diagnoseUseOfReferenceEqualityOperator())
+    return true;
+
   auto argType = getFromType();
   auto paramType = getToType();
 
@@ -4568,6 +4571,71 @@ bool ArgumentMismatchFailure::diagnoseAsNote() {
   return false;
 }
 
+bool ArgumentMismatchFailure::diagnoseUseOfReferenceEqualityOperator() const {
+  auto *locator = getLocator();
+
+  if (!isArgumentOfReferenceEqualityOperator(locator))
+    return false;
+
+  auto *binaryOp = cast<BinaryExpr>(getRawAnchor());
+  auto *lhs = binaryOp->getArg()->getElement(0);
+  auto *rhs = binaryOp->getArg()->getElement(1);
+
+  auto name = *getOperatorName(binaryOp->getFn());
+
+  auto lhsType = getType(lhs)->getRValueType();
+  auto rhsType = getType(rhs)->getRValueType();
+
+  // If both arguments where incorrect e.g. both are function types,
+  // let's avoid producing a diagnostic second time, because first
+  // one would cover both arguments.
+  if (getAnchor() == rhs && rhsType->is<FunctionType>()) {
+    auto &cs = getConstraintSystem();
+    if (cs.hasFixFor(cs.getConstraintLocator(
+            binaryOp, {ConstraintLocator::ApplyArgument,
+                       LocatorPathElt::ApplyArgToParam(0, 0)})))
+      return true;
+  }
+
+  // Regardless of whether the type has reference or value semantics,
+  // comparison with nil is illegal, albeit for different reasons spelled
+  // out by the diagnosis.
+  if (isa<NilLiteralExpr>(lhs) || isa<NilLiteralExpr>(rhs)) {
+    std::string revisedName = name.str();
+    revisedName.pop_back();
+
+    auto loc = binaryOp->getLoc();
+    auto nonNilType = isa<NilLiteralExpr>(lhs) ? rhsType : lhsType;
+    auto nonNilExpr = isa<NilLiteralExpr>(lhs) ? rhs : lhs;
+
+    // If we made it here, then we're trying to perform a comparison with
+    // reference semantics rather than value semantics. The fixit will
+    // lop off the extra '=' in the operator.
+    if (nonNilType->getOptionalObjectType()) {
+      emitDiagnostic(loc,
+                     diag::value_type_comparison_with_nil_illegal_did_you_mean,
+                     nonNilType)
+          .fixItReplace(loc, revisedName);
+    } else {
+      emitDiagnostic(loc, diag::value_type_comparison_with_nil_illegal,
+                     nonNilType)
+          .highlight(nonNilExpr->getSourceRange());
+    }
+
+    return true;
+  }
+
+  if (lhsType->is<FunctionType>() || rhsType->is<FunctionType>()) {
+    emitDiagnostic(binaryOp->getLoc(), diag::cannot_reference_compare_types,
+                   name.str(), lhsType, rhsType)
+        .highlight(lhs->getSourceRange())
+        .highlight(rhs->getSourceRange());
+    return true;
+  }
+
+  return false;
+}
+
 bool ArgumentMismatchFailure::diagnosePatternMatchingMismatch() const {
   if (!isArgumentOfPatternMatchingOperator(getLocator()))
     return false;
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -1613,6 +1613,10 @@ class ArgumentMismatchFailure : public ContextualFailure {
   /// Tailored diagnostic for pattern matching with `~=` operator.
   bool diagnosePatternMatchingMismatch() const;
 
+  /// Tailored diagnostics for argument mismatches associated with
+  /// reference equality operators `===` and `!==`.
+  bool diagnoseUseOfReferenceEqualityOperator() const;
+
 protected:
   SourceLoc getLoc() const { return getAnchor()->getLoc(); }
 
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -2433,6 +2433,15 @@ bool ConstraintSystem::repairFailures(
         repairViaBridgingCast(*this, lhs, rhs, conversionsOrFixes, locator))
       break;
 
+    // If this is an argument to `===` or `!==` there are tailored
+    // diagnostics available for it as part of argument-to-parameter
+    // conversion fix, so let's not try any restrictions or other fixes.
+    if (isArgumentOfReferenceEqualityOperator(loc)) {
+      conversionsOrFixes.push_back(
+          AllowArgumentMismatch::create(*this, lhs, rhs, loc));
+      break;
+    }
+
     // Argument is a r-value but parameter expects an l-value e.g.
     //
     // func foo(_ x: inout Int) {}
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -2960,37 +2960,48 @@ bool constraints::isKnownKeyPathDecl(ASTContext &ctx, ValueDecl *decl) {
          decl == ctx.getPartialKeyPathDecl() || decl == ctx.getAnyKeyPathDecl();
 }
 
-bool constraints::isPatternMatchingOperator(Expr *expr) {
+static bool isOperator(Expr *expr, StringRef expectedName) {
+  auto name = getOperatorName(expr);
+  return name ? name->is(expectedName) : false;
+}
+
+Optional<Identifier> constraints::getOperatorName(Expr *expr) {
   ValueDecl *choice = nullptr;
   if (auto *ODRE = dyn_cast_or_null<OverloadedDeclRefExpr>(expr)) {
     choice = ODRE->getDecls().front();
   } else if (auto *DRE = dyn_cast_or_null<DeclRefExpr>(expr)) {
     choice = DRE->getDecl();
   } else {
-    return false;
+    return None;
   }
 
-  if (auto *FD = dyn_cast_or_null<AbstractFunctionDecl>(choice)) {
-    auto name = FD->getBaseName().userFacingName();
-    return name == "~=";
-  }
+  if (auto *FD = dyn_cast_or_null<AbstractFunctionDecl>(choice))
+    return FD->getBaseName().getIdentifier();
 
-  return false;
+  return None;
+}
+
+bool constraints::isPatternMatchingOperator(Expr *expr) {
+  return isOperator(expr, "~=");
 }
 
 bool constraints::isArgumentOfPatternMatchingOperator(
     ConstraintLocator *locator) {
-  Expr *anchor = nullptr;
-  if (locator->findLast<LocatorPathElt::ApplyArgToParam>()) {
-    anchor = locator->getAnchor();
-  } else {
+  auto *binaryOp = dyn_cast_or_null<BinaryExpr>(locator->getAnchor());
+  if (!(binaryOp && binaryOp->isImplicit()))
     return false;
-  }
+  return isPatternMatchingOperator(binaryOp->getFn());
+}
 
-  auto *binaryOp = dyn_cast_or_null<BinaryExpr>(anchor);
-  if (!(binaryOp && binaryOp->isImplicit()))
+bool constraints::isArgumentOfReferenceEqualityOperator(
+    ConstraintLocator *locator) {
+  if (!locator->findLast<LocatorPathElt::ApplyArgToParam>())
     return false;
 
-  auto *fnExpr = binaryOp->getFn()->getSemanticsProvidingExpr();
-  return isPatternMatchingOperator(fnExpr);
+  if (auto *binaryOp = dyn_cast_or_null<BinaryExpr>(locator->getAnchor())) {
+    auto *fnExpr = binaryOp->getFn();
+    return isOperator(fnExpr, "===") || isOperator(fnExpr, "!==");
+  }
+
+  return false;
 }
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
@@ -3991,10 +3991,18 @@ Expr *getArgumentExpr(Expr *expr, unsigned index);
 /// associated with implicit `~=` (pattern-matching) operator
 bool isArgumentOfPatternMatchingOperator(ConstraintLocator *locator);
 
+/// Determine whether given locator points to one of the arguments
+/// associated with `===` and `!==` operators.
+bool isArgumentOfReferenceEqualityOperator(ConstraintLocator *locator);
+
 /// Determine whether given expression is a reference to a
 /// pattern-matching operator `~=`
 bool isPatternMatchingOperator(Expr *expr);
 
+/// If given expression references operator overlaod(s)
+/// extract and produce name of the operator.
+Optional<Identifier> getOperatorName(Expr *expr);
+
 // Check whether argument of the call at given position refers to
 // parameter marked as `@autoclosure`. This function is used to
 // maintain source compatibility with Swift versions < 5,
