Merge pull request #18935 from xedin/properly-diagnose-ambiguities-with-fixes

[ConstraintSystem] Diagnose ambiguities related to solutions with fixes

Author: xedin

include/swift/AST/DiagnosticsSema.def

@@ -58,6 +58,8 @@ NOTE(extended_type_declared_here,none,
 
 ERROR(ambiguous_member_overload_set,none,
       "ambiguous reference to member %0", (DeclName))
+ERROR(ambiguous_reference_to_decl,none,
+      "ambiguous reference to %0 %1", (DescriptiveDeclKind, DeclName))
 
 ERROR(ambiguous_subscript,none,
       "ambiguous subscript with base type %0 and index type %1",
@@ -1539,6 +1541,10 @@ NOTE(where_requirement_failure_both_subst,none,
      "where %0 = %1, %2 = %3", (Type, Type, Type, Type))
 NOTE(requirement_implied_by_conditional_conformance,none,
      "requirement from conditional conformance of %0 to %1", (Type, Type))
+NOTE(candidate_types_conformance_requirement,none,
+     "candidate requires that %0 conform to %1 "
+     "(requirement specified as %2 == %3%4)",
+     (Type, Type, Type, Type, StringRef))
 NOTE(candidate_types_equal_requirement,none,
      "candidate requires that the types %0 and %1 be equivalent "
      "(requirement specified as %2 == %3%4)",

lib/Sema/CSDiag.cpp

@@ -14,19 +14,20 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "ConstraintSystem.h"
 #include "CSDiag.h"
 #include "CalleeCandidateInfo.h"
+#include "ConstraintSystem.h"
 #include "MiscDiagnostics.h"
 #include "TypeCheckAvailability.h"
 #include "TypoCorrection.h"
 #include "swift/AST/ASTWalker.h"
+#include "swift/AST/DiagnosticEngine.h"
 #include "swift/AST/GenericEnvironment.h"
 #include "swift/AST/Initializer.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/ProtocolConformance.h"
-#include "swift/AST/TypeWalker.h"
 #include "swift/AST/TypeMatcher.h"
+#include "swift/AST/TypeWalker.h"
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/StringExtras.h"
 #include "llvm/ADT/DenseSet.h"
@@ -455,9 +456,91 @@ tryDiagnoseTrailingClosureAmbiguity(TypeChecker &tc,
   return true;
 }
 
-static bool diagnoseAmbiguity(ConstraintSystem &cs,
-                              ArrayRef<Solution> solutions,
-                              Expr *expr) {
+bool ConstraintSystem::diagnoseAmbiguityWithFixes(
+    Expr *expr, ArrayRef<Solution> solutions) {
+  if (solutions.empty())
+    return false;
+
+  auto getOverloadDecl = [&](SelectedOverload &overload) -> ValueDecl * {
+    auto &choice = overload.choice;
+    return choice.isDecl() ? choice.getDecl() : nullptr;
+  };
+
+  // Problems related to fixes forming ambiguous solution set
+  // could only be diagnosed (at the moment), if all of the fixes
+  // are attached to the same anchor, which means they fix
+  // different overloads of the same declaration.
+  Expr *commonAnchor = nullptr;
+  SmallPtrSet<ValueDecl *, 4> distinctChoices;
+  SmallVector<std::pair<const Solution *, const ConstraintFix *>, 4>
+      viableSolutions;
+
+  bool diagnosable = llvm::all_of(solutions, [&](const Solution &solution) {
+    ArrayRef<ConstraintFix *> fixes = solution.Fixes;
+
+    // Currently only support a single fix in a solution,
+    // but ultimately should be able to deal with multiple.
+    if (fixes.size() != 1)
+      return false;
+
+    const auto *fix = fixes.front();
+    if (commonAnchor && commonAnchor != fix->getAnchor())
+      return false;
+
+    commonAnchor = fix->getAnchor();
+
+    SmallVector<SelectedOverload, 2> overloads;
+    solution.getOverloadChoices(commonAnchor, overloads);
+    // There is unfortunately no way, at the moment, to figure out
+    // what declaration the fix is attached to, so we have to make
+    // sure that there is only one declaration associated with common
+    // anchor to be sure that the right problem is being diagnosed.
+    if (overloads.size() != 1)
+      return false;
+
+    auto *decl = getOverloadDecl(overloads.front());
+    if (!decl)
+      return false;
+
+    // If this declaration is distinct, let's record this solution
+    // as viable, otherwise we'd produce the same diagnostic multiple
+    // times, which means that actual problem is elsewhere.
+    if (distinctChoices.insert(decl).second)
+      viableSolutions.push_back({&solution, fix});
+    return true;
+  });
+
+  if (!diagnosable || viableSolutions.size() < 2)
+    return false;
+
+  auto *decl = *distinctChoices.begin();
+  assert(solverState);
+
+  bool diagnosed = true;
+  {
+    DiagnosticTransaction transaction(TC.Diags);
+
+    TC.diagnose(commonAnchor->getLoc(), diag::ambiguous_reference_to_decl,
+                decl->getDescriptiveKind(), decl->getFullName());
+
+    for (const auto &viable : viableSolutions) {
+      // Create scope so each applied solution is rolled back.
+      ConstraintSystem::SolverScope scope(*this);
+      applySolution(*viable.first);
+      // All of the solutions supposed to produce a "candidate" note.
+      diagnosed &= viable.second->diagnose(expr, /*asNote*/ true);
+    }
+
+    // If not all of the fixes produced a note, we can't diagnose this.
+    if (!diagnosed)
+      transaction.abort();
+  }
+
+  return diagnosed;
+}
+
+bool ConstraintSystem::diagnoseAmbiguity(Expr *expr,
+                                         ArrayRef<Solution> solutions) {
   // Produce a diff of the solutions.
   SolutionDiff diff(solutions);
 
@@ -484,7 +567,7 @@ static bool diagnoseAmbiguity(ConstraintSystem &cs,
 
     // If we can't resolve the locator to an anchor expression with no path,
     // we can't diagnose this well.
-    auto *anchor = simplifyLocatorToAnchor(cs, overload.locator);
+    auto *anchor = simplifyLocatorToAnchor(*this, overload.locator);
     if (!anchor)
       continue;
     auto it = indexMap.find(anchor);
@@ -524,10 +607,10 @@ static bool diagnoseAmbiguity(ConstraintSystem &cs,
   if (bestOverload) {
     auto &overload = diff.overloads[*bestOverload];
     auto name = getOverloadChoiceName(overload.choices);
-    auto anchor = simplifyLocatorToAnchor(cs, overload.locator);
+    auto anchor = simplifyLocatorToAnchor(*this, overload.locator);
 
     // Emit the ambiguity diagnostic.
-    auto &tc = cs.getTypeChecker();
+    auto &tc = getTypeChecker();
     tc.diagnose(anchor->getLoc(),
                 name.isOperator() ? diag::ambiguous_operator_ref
                                   : diag::ambiguous_decl_ref,
@@ -9021,6 +9104,11 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
     // FIXME: If we were able to actually fix things along the way,
     // we may have to hunt for the best solution. For now, we don't care.
 
+    // Before removing any "fixed" solutions, let's check
+    // if ambiguity is caused by fixes and diagnose if possible.
+    if (diagnoseAmbiguityWithFixes(expr, viable))
+      return true;
+
     // Remove solutions that require fixes; the fixes in those systems should
     // be diagnosed rather than any ambiguity.
     auto hasFixes = [](const Solution &sol) { return !sol.Fixes.empty(); };
@@ -9039,9 +9127,9 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
           solution.dump(log);
           log << "\n";
         }
-      }        
+      }
 
-      if (diagnoseAmbiguity(*this, viable, expr)) {
+      if (diagnoseAmbiguity(expr, viable)) {
         return true;
       }
     }

lib/Sema/CSDiag.h

@@ -18,8 +18,16 @@
 #ifndef SWIFT_SEMA_CSDIAG_H
 #define SWIFT_SEMA_CSDIAG_H
 
+#include "ConstraintSystem.h"
+#include "swift/AST/DiagnosticEngine.h"
+#include "llvm/ADT/StringRef.h"
+#include <string>
+
 namespace swift {
-  
+  class Expr;
+  class Type;
+  class SourceLoc;
+
   std::string getTypeListString(Type type);
 
   /// Rewrite any type variables & archetypes in the specified type with

lib/Sema/CSDiagnostics.cpp

@@ -29,6 +29,14 @@ using namespace constraints;
 
 FailureDiagnostic::~FailureDiagnostic() {}
 
+bool FailureDiagnostic::diagnose(bool asNote) {
+  return asNote ? diagnoseAsNote() : diagnoseAsError();
+}
+
+bool FailureDiagnostic::diagnoseAsNote() {
+  return false;
+}
+
 std::pair<Expr *, bool> FailureDiagnostic::computeAnchor() const {
   auto &cs = getConstraintSystem();
 
@@ -116,7 +124,7 @@ const DeclContext *RequirementFailure::getRequirementDC() const {
   return AffectedDecl->getAsGenericContext();
 }
 
-bool RequirementFailure::diagnose() {
+bool RequirementFailure::diagnoseAsError() {
   if (!canDiagnoseFailure())
     return false;
 
@@ -140,6 +148,15 @@ bool RequirementFailure::diagnose() {
   return true;
 }
 
+bool RequirementFailure::diagnoseAsNote() {
+  const auto &req = getRequirement();
+  const auto *reqDC = getRequirementDC();
+
+  emitDiagnostic(reqDC->getAsDecl(), getDiagnosticAsNote(), getLHS(), getRHS(),
+                 req.getFirstType(), req.getSecondType(), "");
+  return true;
+}
+
 void RequirementFailure::emitRequirementNote(const Decl *anchor) const {
   auto &req = getRequirement();
 
@@ -159,7 +176,7 @@ void RequirementFailure::emitRequirementNote(const Decl *anchor) const {
                  req.getFirstType(), getLHS(), req.getSecondType(), getRHS());
 }
 
-bool MissingConformanceFailure::diagnose() {
+bool MissingConformanceFailure::diagnoseAsError() {
   if (!canDiagnoseFailure())
     return false;
 
@@ -220,18 +237,18 @@ bool MissingConformanceFailure::diagnose() {
 
   // If none of the special cases could be diagnosed,
   // let's fallback to the most general diagnostic.
-  return RequirementFailure::diagnose();
+  return RequirementFailure::diagnoseAsError();
 }
 
-bool LabelingFailure::diagnose() {
+bool LabelingFailure::diagnoseAsError() {
   auto &cs = getConstraintSystem();
   auto *call = cast<CallExpr>(getAnchor());
   return diagnoseArgumentLabelError(cs.getASTContext(), call->getArg(),
                                     CorrectLabels,
                                     isa<SubscriptExpr>(call->getFn()));
 }
 
-bool NoEscapeFuncToTypeConversionFailure::diagnose() {
+bool NoEscapeFuncToTypeConversionFailure::diagnoseAsError() {
   auto *anchor = getAnchor();
 
   if (ConvertTo) {
@@ -254,7 +271,7 @@ bool NoEscapeFuncToTypeConversionFailure::diagnose() {
   return true;
 }
 
-bool MissingForcedDowncastFailure::diagnose() {
+bool MissingForcedDowncastFailure::diagnoseAsError() {
   if (hasComplexLocator())
     return false;
 
@@ -295,7 +312,7 @@ bool MissingForcedDowncastFailure::diagnose() {
   }
 }
 
-bool MissingAddressOfFailure::diagnose() {
+bool MissingAddressOfFailure::diagnoseAsError() {
   if (hasComplexLocator())
     return false;
 
@@ -306,7 +323,7 @@ bool MissingAddressOfFailure::diagnose() {
   return true;
 }
 
-bool MissingExplicitConversionFailure::diagnose() {
+bool MissingExplicitConversionFailure::diagnoseAsError() {
   if (hasComplexLocator())
     return false;
 
@@ -363,7 +380,7 @@ bool MissingExplicitConversionFailure::diagnose() {
   return true;
 }
 
-bool MemberAccessOnOptionalBaseFailure::diagnose() {
+bool MemberAccessOnOptionalBaseFailure::diagnoseAsError() {
   if (hasComplexLocator())
     return false;
 
@@ -516,7 +533,7 @@ static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type type) {
   return true;
 }
 
-bool MissingOptionalUnwrapFailure::diagnose() {
+bool MissingOptionalUnwrapFailure::diagnoseAsError() {
   if (hasComplexLocator())
     return false;
 
@@ -533,7 +550,7 @@ bool MissingOptionalUnwrapFailure::diagnose() {
   return true;
 }
 
-bool RValueTreatedAsLValueFailure::diagnose() {
+bool RValueTreatedAsLValueFailure::diagnoseAsError() {
   Diag<StringRef> subElementDiagID;
   Diag<Type> rvalueDiagID;
   Expr *diagExpr = getLocator()->getAnchor();