[CSSolver] Don't re-run solver to diagnose ambiguities

lib/Sema/CSSolver.cpp

@@ -1448,18 +1448,23 @@ ConstraintSystem::solve(SyntacticElementTarget &target,
       return None;
     }
 
-    case SolutionResult::Ambiguous:
-      // If salvaging produced an ambiguous result, it has already been
-      // diagnosed.
+    case SolutionResult::Ambiguous: {
       // If we have found an ambiguous solution in the first stage, salvaging
       // won't produce more solutions, so we can inform the solution callback
       // about the current ambiguous solutions straight away.
-      if (stage == 1 || Context.SolutionCallback) {
+      if (Context.SolutionCallback) {
         reportSolutionsToSolutionCallback(solution);
         solution.markAsDiagnosed();
         return None;
       }
 
+      // If salvaging produced an ambiguous result, it has already been
+      // diagnosed.
+      if (stage == 1) {
+        solution.markAsDiagnosed();
+        return None;
+      }
+
       if (Options.contains(
             ConstraintSystemFlags::AllowUnresolvedTypeVariables)) {
         dumpSolutions(solution);
@@ -1470,7 +1475,25 @@ ConstraintSystem::solve(SyntacticElementTarget &target,
         return std::move(result);
       }
 
+      auto solutionsRef = std::move(solution).takeAmbiguousSolutions();
+      SmallVector<Solution, 4> ambiguity(
+          std::make_move_iterator(solutionsRef.begin()),
+          std::make_move_iterator(solutionsRef.end()));
+
+      {
+        SolverState state(*this, FreeTypeVariableBinding::Disallow);
+
+        // Constraint generator is allowed to introduce fixes to the
+        // contraint system.
+        if (diagnoseAmbiguityWithFixes(ambiguity) ||
+            diagnoseAmbiguity(ambiguity)) {
+          solution.markAsDiagnosed();
+          return None;
+        }
+      }
+
       LLVM_FALLTHROUGH;
+    }
 
     case SolutionResult::UndiagnosedError:
       /// If we have a SolutionCallback, we are inspecting constraint system
@@ -1563,7 +1586,7 @@ bool ConstraintSystem::solve(SmallVectorImpl<Solution> &solutions,
   // Filter deduced solutions, try to figure out if there is
   // a single best solution to use, if not explicitly disabled
   // by constraint system options.
-  filterSolutions(solutions);
+  filterSolutions(solutions, /*minimize=*/true);
 
   // We fail if there is no solution or the expression was too complex.
   return solutions.empty() || isTooComplex(solutions);

lib/Sema/ConstraintSystem.cpp

@@ -5457,28 +5457,37 @@ bool ConstraintSystem::diagnoseAmbiguity(ArrayRef<Solution> solutions) {
     // FIXME: We would prefer to emit the name as written, but that information
     // is not sufficiently centralized in the AST.
     DeclNameRef name(getOverloadChoiceName(overload.choices));
+
+    // A location to attach ambiguity diagnostic to.
+    SourceLoc diagLoc;
+
+    // Some of the locations do not simplify all the way to anchor,
+    // for example - key path components and dynamic member references
+    // are not represented via ASTNode,
     auto anchor = simplifyLocatorToAnchor(overload.locator);
-    if (!anchor) {
-      // It's not clear that this is actually valid. Just use the overload's
-      // anchor for release builds, but assert so we can properly diagnose
-      // this case if it happens to be hit. Note that the overload will
-      // *always* be anchored, otherwise everything would be broken, ie. this
-      // assertion would be the least of our worries.
-      anchor = overload.locator->getAnchor();
-      assert(false && "locator could not be simplified to anchor");
+    if (anchor) {
+      diagLoc = getLoc(anchor);
+    } else if (auto keyPathComponent = overload.locator->getFirstElementAs<
+                                       LocatorPathElt::KeyPathComponent>()) {
+      auto *KPE = castToExpr<KeyPathExpr>(overload.locator->getAnchor());
+      diagLoc = KPE->getComponents()[keyPathComponent->getIndex()].getLoc();
+    } else {
+      diagLoc = getLoc(overload.locator->getAnchor());
     }
 
     // Emit the ambiguity diagnostic.
     auto &DE = getASTContext().Diags;
-    DE.diagnose(getLoc(anchor),
+    DE.diagnose(diagLoc,
                 name.isOperator() ? diag::ambiguous_operator_ref
                                   : diag::ambiguous_decl_ref,
                 name);
 
-    TrailingClosureAmbiguityFailure failure(solutions, anchor,
-                                            overload.choices);
-    if (failure.diagnoseAsNote())
-      return true;
+    if (anchor) {
+      TrailingClosureAmbiguityFailure failure(solutions, anchor,
+                                              overload.choices);
+      if (failure.diagnoseAsNote())
+        return true;
+    }
 
     // Emit candidates.  Use a SmallPtrSet to make sure only emit a particular
     // candidate once.  FIXME: Why is one candidate getting into the overload