[Constraint system] Capture "salvage" result in a self-contained data structure

Rework the interface to ConstraintSystem::salvage() to (a) not require
an existing set of solutions, which it overwrites anyway, (b) not
depend on having a single expression as input, and (c) be clear with
its client about whether the operation has already emitted a
diagnostic vs. the client being expected to produce a diagnostic.

Author: DougGregor

lib/Sema/CSApply.cpp

@@ -20,6 +20,7 @@
 #include "CodeSynthesis.h"
 #include "CSDiagnostics.h"
 #include "MiscDiagnostics.h"
+#include "SolutionResult.h"
 #include "TypeCheckProtocol.h"
 #include "swift/AST/ASTVisitor.h"
 #include "swift/AST/ASTWalker.h"
@@ -7531,3 +7532,50 @@ void Solution::setExprTypes(Expr *expr) const {
   SetExprTypes SET(*this);
   expr->walk(SET);
 }
+
+/// MARK: SolutionResult implementation.
+
+SolutionResult SolutionResult::forSolved(Solution &&solution) {
+  SolutionResult result(Kind::Success);
+  result.solutions = new Solution(std::move(solution));
+  result.numSolutions = 1;
+  return result;
+}
+
+SolutionResult SolutionResult::forAmbiguous(
+    MutableArrayRef<Solution> solutions) {
+  assert(solutions.size() > 1 && "Not actually ambiguous");
+  SolutionResult result(Kind::Ambiguous);
+  result.solutions =
+      (Solution *)malloc(sizeof(Solution) * solutions.size());
+  result.numSolutions = solutions.size();
+  std::uninitialized_copy(std::make_move_iterator(solutions.begin()),
+                          std::make_move_iterator(solutions.end()),
+                          result.solutions);
+  return result;
+}
+
+SolutionResult::~SolutionResult() {
+  assert((!requiresDiagnostic() || emittedDiagnostic) &&
+         "SolutionResult was destroyed without emitting a diagnostic");
+
+  for (unsigned i : range(numSolutions)) {
+    solutions[i].~Solution();
+  }
+  free(solutions);
+}
+
+const Solution &SolutionResult::getSolution() const {
+  assert(numSolutions == 1 && "Wrong number of solutions");
+  return solutions[0];
+}
+
+Solution &&SolutionResult::takeSolution() && {
+  assert(numSolutions == 1 && "Wrong number of solutions");
+  return std::move(solutions[0]);
+}
+
+ArrayRef<Solution> SolutionResult::getAmbiguousSolutions() const {
+  assert(getKind() == Ambiguous);
+  return makeArrayRef(solutions, numSolutions);
+}

lib/Sema/CSSolver.cpp

@@ -16,6 +16,7 @@
 #include "CSStep.h"
 #include "ConstraintGraph.h"
 #include "ConstraintSystem.h"
+#include "SolutionResult.h"
 #include "TypeCheckType.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/TypeWalker.h"
@@ -1121,10 +1122,27 @@ bool ConstraintSystem::solve(Expr *&expr,
     if (shouldSuppressDiagnostics())
       return true;
 
-    // Try to provide a decent diagnostic.
-    if (salvage(solutions, expr)) {
-      // If salvage produced an error message, then it failed to salvage the
-      // expression, just bail out having reported the error.
+    // Try to fix the system or provide a decent diagnostic.
+    auto salvagedResult = salvage();
+    switch (salvagedResult.getKind()) {
+    case SolutionResult::Kind::Success:
+      solutions.clear();
+      solutions.push_back(std::move(salvagedResult).takeSolution());
+      break;
+
+    case SolutionResult::Kind::Error:
+    case SolutionResult::Kind::Ambiguous:
+      return true;
+
+    case SolutionResult::Kind::UndiagnosedError:
+      diagnoseFailureForExpr(expr);
+      salvagedResult.markAsDiagnosed();
+      return true;
+
+    case SolutionResult::Kind::TooComplex:
+      getASTContext().Diags.diagnose(expr->getLoc(), diag::expression_too_complex)
+        .highlight(expr->getSourceRange());
+      salvagedResult.markAsDiagnosed();
       return true;
     }
 

lib/Sema/ConstraintSystem.cpp

@@ -19,6 +19,7 @@
 #include "ConstraintGraph.h"
 #include "CSDiagnostics.h"
 #include "CSFix.h"
+#include "SolutionResult.h"
 #include "TypeCheckType.h"
 #include "swift/AST/Initializer.h"
 #include "swift/AST/GenericEnvironment.h"
@@ -2444,7 +2445,7 @@ bool OverloadChoice::isImplicitlyUnwrappedValueOrReturnValue() const {
   llvm_unreachable("unhandled kind");
 }
 
-bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
+SolutionResult ConstraintSystem::salvage() {
   auto &ctx = getASTContext();
   if (ctx.TypeCheckerOpts.DebugConstraintSolver) {
     auto &log = ctx.TypeCheckerDebug->getStream();
@@ -2456,6 +2457,7 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
   //
   // FIXME: can this be removed?  We need to arrange for recordFixes to be
   // eliminated.
+  SmallVector<Solution, 2> viable;
   viable.clear();
 
   {
@@ -2472,13 +2474,13 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
       if (*best != 0)
         viable[0] = std::move(viable[*best]);
       viable.erase(viable.begin() + 1, viable.end());
-      return false;
+      return SolutionResult::forSolved(std::move(viable[0]));
     }
 
     // Before removing any "fixed" solutions, let's check
     // if ambiguity is caused by fixes and diagnose if possible.
     if (diagnoseAmbiguityWithFixes(viable))
-      return true;
+      return SolutionResult::forAmbiguous(viable);
 
     // 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.
@@ -2504,23 +2506,18 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
       }
 
       if (diagnoseAmbiguity(viable)) {
-        return true;
+        return SolutionResult::forAmbiguous(viable);
       }
     }
 
     // Fall through to produce diagnostics.
   }
 
-  if (getExpressionTooComplex(viable)) {
-    ctx.Diags.diagnose(expr->getLoc(), diag::expression_too_complex)
-                .highlight(expr->getSourceRange());
-    return true;
-  }
+  if (getExpressionTooComplex(viable))
+    return SolutionResult::forTooComplex();
 
-  // If all else fails, diagnose the failure by looking through the system's
-  // constraints.
-  diagnoseFailureForExpr(expr);
-  return true;
+  // Could not produce a specific diagnostic; punt to the client.
+  return SolutionResult::forUndiagnosedError();
 }
 
 static void diagnoseOperatorAmbiguity(ConstraintSystem &cs,

lib/Sema/ConstraintSystem.h

@@ -2081,17 +2081,8 @@ class ConstraintSystem {
   ValueDecl *findResolvedMemberRef(ConstraintLocator *locator);
 
   /// Try to salvage the constraint system by applying (speculative)
-  /// fixes to the underlying expression.
-  ///
-  /// \param viable the set of viable solutions produced by the initial
-  /// solution attempt.
-  ///
-  /// \param expr the expression we're trying to salvage.
-  ///
-  /// \returns false if we were able to salvage the system, in which case
-  /// \c viable[0] contains the resulting solution. Otherwise, emits a
-  /// diagnostic and returns true.
-  bool salvage(SmallVectorImpl<Solution> &viable, Expr *expr);
+  /// fixes.
+  SolutionResult salvage();
 
   /// Mine the active and inactive constraints in the constraint
   /// system to generate a plausible diagnosis of why the system could not be

lib/Sema/SolutionResult.h

@@ -0,0 +1,145 @@
+//===--- SolutionResult.h - Constraint System Solution ----------*- C++ -*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2019 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines the SolutionResult class.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef SWIFT_TYPECHECK_SOLUTION_RESULT_H
+#define SWIFT_TYPECHECK_SOLUTION_RESULT_H
+
+#include "llvm/ADT/ArrayRef.h"
+
+namespace swift {
+
+using llvm::ArrayRef;
+using llvm::makeArrayRef;
+
+namespace constraints {
+
+class Solution;
+
+/// Describes the result of solving a constraint system, after
+/// potentially taking various corrective actions.
+class SolutionResult {
+public:
+  enum Kind : unsigned char {
+    /// The constraint system was successfully solved, and one can
+    /// retrieve the resulting solution.
+    Success,
+    /// The constraint system had multiple solutions, none of which
+    /// was better than the others.
+    Ambiguous,
+    /// The constraint system had no solution, and a diagnostic has
+    /// already been emitted.
+    Error,
+    /// The constraint system had no solution, but no diagnostic has
+    /// been emitted yet.
+    UndiagnosedError,
+    /// The constraint system was too complex to solve, but no
+    /// diagnostic has been emitted yet.
+    TooComplex,
+  };
+
+private:
+  /// The kind of solution result.
+  Kind kind;
+
+  /// Whether the client has emitted a diagnostic.
+  unsigned emittedDiagnostic : 1;
+
+  /// The number of solutions owned by this result.
+  unsigned numSolutions = 0;
+
+  /// A pointer to the set of solutions, of which there are
+  /// \c numSolutions entries.
+  Solution *solutions = nullptr;
+
+  /// General constructor for the named constructors.
+  SolutionResult(Kind kind) : kind(kind) {
+    emittedDiagnostic = false;
+  }
+
+public:
+  SolutionResult(const SolutionResult &other) = delete;
+
+  SolutionResult(SolutionResult &&other)
+      : kind(other.kind), numSolutions(other.numSolutions),
+        solutions(other.solutions) {
+    emittedDiagnostic = false;
+    other.kind = Error;
+    other.numSolutions = 0;
+    other.solutions = nullptr;
+  }
+
+  SolutionResult &operator=(const SolutionResult &other) = delete;
+  SolutionResult &operator=(SolutionResult &&other) = delete;
+
+  ~SolutionResult();
+
+  /// Produce a "solved" result, embedding the given solution.
+  static SolutionResult forSolved(Solution &&solution);
+
+  /// Produce an "ambiguous" result, providing the set of
+  /// potential solutions.
+  static SolutionResult forAmbiguous(MutableArrayRef<Solution> solutions);
+
+  /// Produce a "too complex" failure, which was not yet been
+  /// diagnosed.
+  static SolutionResult forTooComplex() {
+    return SolutionResult(TooComplex);
+  }
+
+  /// Produce a failure that has already been diagnosed.
+  static SolutionResult forError() {
+    return SolutionResult(Error);
+  }
+
+  /// Produce a failure that has not yet been diagnosed.
+  static SolutionResult forUndiagnosedError() {
+    return SolutionResult(UndiagnosedError);
+  }
+
+  Kind getKind() const{ return kind; }
+
+  /// Retrieve the solution, where there is one.
+  const Solution &getSolution() const;
+
+  /// Retrieve the solution, where there is one.
+  Solution &&takeSolution() &&;
+
+  /// Retrieve the set of solutions when there is an ambiguity.
+  ArrayRef<Solution> getAmbiguousSolutions() const;
+
+  /// Whether this solution requires the client to produce a diagnostic.
+  bool requiresDiagnostic() const {
+    switch (kind) {
+    case Success:
+    case Ambiguous:
+    case Error:
+      return false;
+
+    case UndiagnosedError:
+    case TooComplex:
+      return true;
+    }
+  }
+
+  /// Note that the failure has been diagnosed.
+  void markAsDiagnosed() {
+    emittedDiagnostic = true;
+  }
+};
+
+} }
+
+#endif /* SWIFT_TYPECHECK_SOLUTION_RESULT_H */

lib/Sema/TypeCheckConstraints.cpp

@@ -18,6 +18,7 @@
 
 #include "ConstraintSystem.h"
 #include "MiscDiagnostics.h"
+#include "SolutionResult.h"
 #include "TypeChecker.h"
 #include "TypeCheckType.h"
 #include "TypoCorrection.h"
@@ -3465,9 +3466,30 @@ bool TypeChecker::convertToType(Expr *&expr, Type type, DeclContext *dc,
 
   // Attempt to solve the constraint system.
   SmallVector<Solution, 4> viable;
-  if ((cs.solve(viable) || viable.size() != 1) &&
-      cs.salvage(viable, expr)) {
-    return true;
+  if ((cs.solve(viable) || viable.size() != 1)) {
+    // Try to fix the system or provide a decent diagnostic.
+    auto salvagedResult = cs.salvage();
+    switch (salvagedResult.getKind()) {
+    case SolutionResult::Kind::Success:
+      viable.clear();
+      viable.push_back(std::move(salvagedResult).takeSolution());
+      break;
+
+    case SolutionResult::Kind::Error:
+    case SolutionResult::Kind::Ambiguous:
+      return true;
+
+    case SolutionResult::Kind::UndiagnosedError:
+      cs.diagnoseFailureForExpr(expr);
+      salvagedResult.markAsDiagnosed();
+      return true;
+
+    case SolutionResult::Kind::TooComplex:
+      Context.Diags.diagnose(expr->getLoc(), diag::expression_too_complex)
+        .highlight(expr->getSourceRange());
+      salvagedResult.markAsDiagnosed();
+      return true;
+    }
   }
 
   auto &solution = viable[0];

lib/Sema/TypeChecker.h

@@ -50,6 +50,7 @@ namespace constraints {
   enum class SolutionKind : char;
   class ConstraintSystem;
   class Solution;
+  class SolutionResult;
 }
 
 /// A mapping from substitutable types to the protocol-conformance