[ConstraintSystem] Diagnose ambiguities related to solutions with fixes

If all of the solutions in the set have a single fix, which points
to the common anchor, attempt to diagnose the failure as an
ambiguity with a list of candidates and their related problems as notes.

Having richer message like that helps to understand why something is
ambiguous e.g. if there are two overloads, one requires conformance
to some protocol and another has a same-type requirement on some type,
but neither matched exactly, having both candidates in the diagnostic
message with associated errors, instead of simplify pointing to related
declarations, helps tremendously.

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",

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/CSDiagnostics.cpp

@@ -234,6 +234,9 @@ bool MissingConformanceFailure::diagnose(bool asNote) {
 }
 
 bool LabelingFailure::diagnose(bool asNote) {
+  if (asNote)
+    return false;
+
   auto &cs = getConstraintSystem();
   auto *call = cast<CallExpr>(getAnchor());
   return diagnoseArgumentLabelError(cs.getASTContext(), call->getArg(),

lib/Sema/ConstraintSystem.h

@@ -707,6 +707,16 @@ class Solution {
     return None;
   }
 
+  /// Retrieve overload choices associated with given expression.
+  void getOverloadChoices(Expr *anchor,
+                          SmallVectorImpl<SelectedOverload> &overloads) const {
+    for (auto &e : overloadChoices) {
+      auto *locator = e.first;
+      if (locator->getAnchor() == anchor)
+        overloads.push_back(e.second);
+    }
+  }
+
   LLVM_ATTRIBUTE_DEPRECATED(
       void dump() const LLVM_ATTRIBUTE_USED,
       "only for use within the debugger");
@@ -1778,6 +1788,9 @@ class ConstraintSystem {
   /// emits an error message.
   void diagnoseFailureForExpr(Expr *expr);
 
+  bool diagnoseAmbiguity(Expr *expr, ArrayRef<Solution> solutions);
+  bool diagnoseAmbiguityWithFixes(Expr *expr, ArrayRef<Solution> solutions);
+
   /// \brief Give the deprecation warning for referring to a global function
   /// when there's a method from a conditional conformance in a smaller/closer
   /// scope.

test/decl/ext/protocol.swift

@@ -180,7 +180,9 @@ extension S1 {
 // Protocol extensions with additional requirements
 // ----------------------------------------------------------------------------
 extension P4 where Self.AssocP4 : P1 {
-  func extP4a() { // expected-note 2 {{found this candidate}}
+// expected-note@-1 {{candidate requires that 'Int' conform to 'P1' (requirement specified as 'Self.AssocP4' == 'P1')}}
+// expected-note@-2 {{candidate requires that 'S4aHelper' conform to 'P1' (requirement specified as 'Self.AssocP4' == 'P1')}}
+  func extP4a() {
     acceptsP1(reqP4a())
   }
 }
@@ -211,17 +213,19 @@ extension P4 where Self.AssocP4 == Int {
 }
 
 extension P4 where Self.AssocP4 == Bool {
-  func extP4a() -> Bool { return reqP4a() } // expected-note 2 {{found this candidate}}
+// expected-note@-1 {{candidate requires that the types 'Int' and 'Bool' be equivalent (requirement specified as 'Self.AssocP4' == 'Bool')}}
+// expected-note@-2 {{candidate requires that the types 'S4aHelper' and 'Bool' be equivalent (requirement specified as 'Self.AssocP4' == 'Bool')}}
+  func extP4a() -> Bool { return reqP4a() }
 }
 
 func testP4(_ s4a: S4a, s4b: S4b, s4c: S4c, s4d: S4d) {
   // FIXME: Both of the 'ambiguous' examples below are indeed ambiguous,
   //        because they don't match on conformance and same-type
   //        requirement of different overloads, but diagnostic
   //        could be improved to point out exactly what is missing in each case.
-  s4a.extP4a() // expected-error{{ambiguous reference to member 'extP4a()'}}
+  s4a.extP4a() // expected-error{{ambiguous reference to instance method 'extP4a()'}}
   s4b.extP4a() // ok
-  s4c.extP4a() // expected-error{{ambiguous reference to member 'extP4a()'}}
+  s4c.extP4a() // expected-error{{ambiguous reference to instance method 'extP4a()'}}
   s4c.extP4Int() // okay
   var b1 = s4d.extP4a() // okay, "Bool" version
   b1 = true // checks type above