[Constraint solver] Rework solveImpl(Expr*) to return SolutionResult.

DougGregor · DougGregor · commit d752f414d337 · 2020-01-26T12:05:43.000-08:00
SolutionResult better captures what can happen when solving a constraint
system for the given expression occurs than the ad hoc SolutionKind return
&amp; small vector of Solution results. Also, try to make this logic less
convoluted.
diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp
@@ -7507,6 +7507,11 @@ ArrayRef<Solution> SolutionResult::getAmbiguousSolutions() const {
   return makeArrayRef(solutions, numSolutions);
 }
 
+MutableArrayRef<Solution> SolutionResult::takeAmbiguousSolutions() && {
+  assert(getKind() == Ambiguous);
+  return MutableArrayRef<Solution>(solutions, numSolutions);
+}
+
 llvm::PointerUnion<Expr *, Stmt *> SolutionApplicationTarget::walk(
     ASTWalker &walker) {
   switch (kind) {
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -9076,8 +9076,9 @@ void ConstraintSystem::addContextualConversionConstraint(
     case CTP_ReturnStmt:
     case CTP_ReturnSingleExpr:
     case CTP_Initialization:
+      // FIXME: This option should not be global!
       if (Options.contains(
-          ConstraintSystemFlags::UnderlyingTypeForOpaqueReturnType))
+            ConstraintSystemFlags::UnderlyingTypeForOpaqueReturnType))
         constraintKind = ConstraintKind::OpaqueUnderlyingType;
       break;
 
diff --git a/lib/Sema/CSSolver.cpp b/lib/Sema/CSSolver.cpp
@@ -1115,80 +1115,93 @@ bool ConstraintSystem::solve(Expr *&expr,
       getASTContext().TypeCheckerOpts.DebugConstraintSolver,
       debugConstraintSolverForExpr(getASTContext(), expr));
 
-  // Attempt to solve the constraint system.
-  auto solution = solveImpl(expr,
-                            convertType,
-                            listener,
-                            solutions,
-                            allowFreeTypeVariables);
-
-  // The constraint system has failed
-  if (solution == SolutionKind::Error)
-    return true;
-
-  // If the system is unsolved or there are multiple solutions present but
-  // type checker options do not allow unresolved types, let's try to salvage
-  if (solution == SolutionKind::Unsolved ||
-      (solutions.size() != 1 &&
-       !Options.contains(
-           ConstraintSystemFlags::AllowUnresolvedTypeVariables))) {
-    if (shouldSuppressDiagnostics())
-      return true;
+  /// Dump solutions for debugging purposes.
+  auto dumpSolutions = [&] {
+    // Debug-print the set of solutions.
+    if (getASTContext().TypeCheckerOpts.DebugConstraintSolver) {
+      auto &log = getASTContext().TypeCheckerDebug->getStream();
+      if (solutions.size() == 1) {
+        log << "---Solution---\n";
+        solutions[0].dump(log);
+      } else {
+        for (unsigned i = 0, e = solutions.size(); i != e; ++i) {
+          log << "--- Solution #" << i << " ---\n";
+          solutions[i].dump(log);
+        }
+      }
+    }
+  };
 
-    // Try to fix the system or provide a decent diagnostic.
-    auto salvagedResult = salvage();
-    switch (salvagedResult.getKind()) {
-    case SolutionResult::Kind::Success:
+  // Take up to two attempts at solving the system. The first attempts to
+  // solve a system that is expected to be well-formed, the second kicks in
+  // when there is an error and attempts to salvage an ill-formed expression.
+  for (unsigned stage = 0; stage != 2; ++stage) {
+    auto solution = (stage == 0)
+        ? solveImpl(expr, convertType, listener, allowFreeTypeVariables)
+        : salvage();
+
+    switch (solution.getKind()) {
+    case SolutionResult::Success:
+      // Return the successful solution.
       solutions.clear();
-      solutions.push_back(std::move(salvagedResult).takeSolution());
-      break;
-
-    case SolutionResult::Kind::Error:
-    case SolutionResult::Kind::Ambiguous:
-      return true;
+      solutions.push_back(std::move(solution).takeSolution());
+      dumpSolutions();
+      return false;
 
-    case SolutionResult::Kind::UndiagnosedError:
-      diagnoseFailureFor(expr);
-      salvagedResult.markAsDiagnosed();
+    case SolutionResult::Error:
       return true;
 
-    case SolutionResult::Kind::TooComplex:
+    case SolutionResult::TooComplex:
       getASTContext().Diags.diagnose(expr->getLoc(), diag::expression_too_complex)
-        .highlight(expr->getSourceRange());
-      salvagedResult.markAsDiagnosed();
+          .highlight(expr->getSourceRange());
+      solution.markAsDiagnosed();
       return true;
-    }
 
-    // The system was salvaged; continue on as if nothing happened.
-  }
+    case SolutionResult::Ambiguous:
+      // If salvaging produced an ambiguous result, it has already been
+      // diagnosed.
+      if (stage == 1) {
+        solution.markAsDiagnosed();
+        return true;
+      }
 
-  if (getExpressionTooComplex(solutions)) {
-    getASTContext().Diags.diagnose(expr->getLoc(), diag::expression_too_complex)
-      .highlight(expr->getSourceRange());
-    return true;
-  }
+      if (Options.contains(
+            ConstraintSystemFlags::AllowUnresolvedTypeVariables)) {
+        auto ambiguousSolutions = std::move(solution).takeAmbiguousSolutions();
+        solutions.assign(std::make_move_iterator(ambiguousSolutions.begin()),
+                         std::make_move_iterator(ambiguousSolutions.end()));
+        dumpSolutions();
+        solution.markAsDiagnosed();
+        return false;
+      }
 
-  if (getASTContext().TypeCheckerOpts.DebugConstraintSolver) {
-    auto &log = getASTContext().TypeCheckerDebug->getStream();
-    if (solutions.size() == 1) {
-      log << "---Solution---\n";
-      solutions[0].dump(log);
-    } else {
-      for (unsigned i = 0, e = solutions.size(); i != e; ++i) {
-        log << "--- Solution #" << i << " ---\n";
-        solutions[i].dump(log);
+      LLVM_FALLTHROUGH;
+
+    case SolutionResult::UndiagnosedError:
+      if (shouldSuppressDiagnostics()) {
+        solution.markAsDiagnosed();
+        return true;
+      }
+
+      if (stage == 1) {
+        diagnoseFailureFor(expr);
+        solution.markAsDiagnosed();
+        return true;
       }
+
+      // Loop again to try to salvage.
+      solution.markAsDiagnosed();
+      continue;
     }
   }
 
-  return false;
+  llvm_unreachable("Loop always returns");
 }
 
-ConstraintSystem::SolutionKind
+SolutionResult
 ConstraintSystem::solveImpl(Expr *&expr,
                             Type convertType,
                             ExprTypeCheckListener *listener,
-                            SmallVectorImpl<Solution> &solutions,
                             FreeTypeVariableBinding allowFreeTypeVariables) {
   if (getASTContext().TypeCheckerOpts.DebugConstraintSolver) {
     auto &log = getASTContext().TypeCheckerDebug->getStream();
@@ -1220,7 +1233,7 @@ ConstraintSystem::solveImpl(Expr *&expr,
   else {
     if (listener)
       listener->constraintGenerationFailed(expr);
-    return SolutionKind::Error;
+    return SolutionResult::forError();
   }
 
   // If there is a type that we're expected to convert to, add the conversion
@@ -1247,7 +1260,7 @@ ConstraintSystem::solveImpl(Expr *&expr,
 
   // Notify the listener that we've built the constraint system.
   if (listener && listener->builtConstraints(*this, expr)) {
-    return SolutionKind::Error;
+    return SolutionResult::forError();
   }
 
   if (getASTContext().TypeCheckerOpts.DebugConstraintSolver) {
@@ -1259,11 +1272,22 @@ ConstraintSystem::solveImpl(Expr *&expr,
   }
 
   // Try to solve the constraint system using computed suggestions.
+  SmallVector<Solution, 4> solutions;
   solve(solutions, allowFreeTypeVariables);
 
-  // If there are no solutions let's mark system as unsolved,
-  // and solved otherwise even if there are multiple solutions still present.
-  return solutions.empty() ? SolutionKind::Unsolved : SolutionKind::Solved;
+  if (getExpressionTooComplex(solutions))
+    return SolutionResult::forTooComplex();
+
+  switch (solutions.size()) {
+  case 0:
+    return SolutionResult::forUndiagnosedError();
+
+  case 1:
+    return SolutionResult::forSolved(std::move(solutions.front()));
+
+  default:
+    return SolutionResult::forAmbiguous(solutions);
+  }
 }
 
 bool ConstraintSystem::solve(SmallVectorImpl<Solution> &solutions,
diff --git a/lib/Sema/ConstraintSystem.h b/lib/Sema/ConstraintSystem.h
@@ -4040,18 +4040,13 @@ class ConstraintSystem {
   /// \param expr The expression to generate constraints from.
   /// \param convertType The expected type of the expression.
   /// \param listener The callback to check solving progress.
-  /// \param solutions The set of solutions to the system of constraints.
   /// \param allowFreeTypeVariables How to bind free type variables in
   /// the solution.
-  ///
-  /// \returns Error is an error occurred, Solved is system is consistent
-  /// and solutions were found, Unsolved otherwise.
-  SolutionKind solveImpl(Expr *&expr,
-                         Type convertType,
-                         ExprTypeCheckListener *listener,
-                         SmallVectorImpl<Solution> &solutions,
-                         FreeTypeVariableBinding allowFreeTypeVariables
-                          = FreeTypeVariableBinding::Disallow);
+  SolutionResult solveImpl(Expr *&expr,
+                           Type convertType,
+                           ExprTypeCheckListener *listener,
+                           FreeTypeVariableBinding allowFreeTypeVariables
+                             = FreeTypeVariableBinding::Disallow);
 
 public:
   /// Pre-check the expression, validating any types that occur in the
diff --git a/lib/Sema/SolutionResult.h b/lib/Sema/SolutionResult.h
@@ -120,6 +120,9 @@ class SolutionResult {
   /// Retrieve the set of solutions when there is an ambiguity.
   ArrayRef<Solution> getAmbiguousSolutions() const;
 
+  /// Take the set of solutions when there is an ambiguity.
+  MutableArrayRef<Solution> takeAmbiguousSolutions() &&;
+
   /// Whether this solution requires the client to produce a diagnostic.
   bool requiresDiagnostic() const {
     switch (kind) {
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
@@ -153,7 +153,7 @@ enum ContextualTypePurpose {
 
 
 
-/// Flags that can be used to control name lookup.
+/// Flags that can be used to control type checking.
 enum class TypeCheckExprFlags {
   /// Whether we know that the result of the expression is discarded.  This
   /// disables constraints forcing an lvalue result to be loadable.
diff --git a/test/Constraints/keypath.swift b/test/Constraints/keypath.swift
@@ -41,10 +41,9 @@ func testFunc() {
   let _: (S) -> Int = \.i
   _ = ([S]()).map(\.i)
 
-  // FIXME: A terrible error, but the same as the pre-existing key path
-  // error in the similar situation: 'let _ = \S.init'.
   _ = ([S]()).map(\.init)
-  // expected-error@-1 {{type of expression is ambiguous without more context}}
+  // expected-error@-1 {{key path cannot refer to static member 'init()'}}
+  // expected-error@-2 {{key path value type '() -> S' cannot be converted to contextual type '_'}}
 
   let kp = \S.i
   let _: KeyPath<S, Int> = kp // works, because type defaults to KeyPath nominal
