Merge pull request #17975 from rudkx/disjunction-ordering

[ConstraintSystem] Change the order in which we attempt disjunctions to be stable.

Author: rudkx

lib/Sema/CSSolver.cpp

@@ -13,14 +13,15 @@
 // This file implements the constraint solver used in the type checker.
 //
 //===----------------------------------------------------------------------===//
-#include "ConstraintSystem.h"
 #include "ConstraintGraph.h"
+#include "ConstraintSystem.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/TypeWalker.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/Compiler.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
 #include <memory>
 #include <tuple>
 
@@ -1492,13 +1493,8 @@ bool ConstraintSystem::solveRec(SmallVectorImpl<Solution> &solutions,
 
   // If we don't have more than one component, just solve the whole
   // system.
-  if (numComponents < 2) {
-    SmallVector<Constraint *, 8> disjunctions;
-    collectDisjunctions(disjunctions);
-
-    return solveSimplified(selectDisjunction(disjunctions), solutions,
-                           allowFreeTypeVariables);
-  }
+  if (numComponents < 2)
+    return solveSimplified(solutions, allowFreeTypeVariables);
 
   if (TC.Context.LangOpts.DebugConstraintSolver) {
     auto &log = getASTContext().TypeCheckerDebug->getStream();
@@ -1819,6 +1815,9 @@ static bool shouldSkipDisjunctionChoice(ConstraintSystem &cs,
 static Constraint *selectBestBindingDisjunction(
     ConstraintSystem &cs, SmallVectorImpl<Constraint *> &disjunctions) {
 
+  if (disjunctions.empty())
+    return nullptr;
+
   // Collect any disjunctions that simply attempt bindings for a
   // type variable.
   SmallVector<Constraint *, 8> bindingDisjunctions;
@@ -1885,46 +1884,40 @@ static Constraint *selectBestBindingDisjunction(
   return nullptr;
 }
 
-Constraint *ConstraintSystem::selectDisjunction(
-    SmallVectorImpl<Constraint *> &disjunctions) {
-  if (disjunctions.empty())
-    return nullptr;
+Constraint *ConstraintSystem::selectDisjunction() {
+  SmallVector<Constraint *, 4> disjunctions;
 
-  auto *disjunction =
-      selectBestBindingDisjunction(*this, disjunctions);
-  if (disjunction)
+  collectDisjunctions(disjunctions);
+  if (auto *disjunction = selectBestBindingDisjunction(*this, disjunctions))
     return disjunction;
 
-  // Pick the smallest disjunction.
-  // FIXME: This heuristic isn't great, but it helped somewhat for
-  // overload sets.
-  disjunction = disjunctions[0];
-  auto bestSize = disjunction->countActiveNestedConstraints();
-  if (bestSize > 2) {
-    for (auto contender : llvm::makeArrayRef(disjunctions).slice(1)) {
-      unsigned newSize = contender->countActiveNestedConstraints();
-      if (newSize < bestSize) {
-        bestSize = newSize;
-        disjunction = contender;
-
-        if (bestSize == 2)
-          break;
-      }
-    }
-  }
+  // Pick the disjunction with the lowest disjunction number in order
+  // to solve them in the order they were created (which should be
+  // stable within an expression).
+  auto minDisjunction =
+      std::min_element(disjunctions.begin(), disjunctions.end(),
+                       [&](Constraint *first, Constraint *second) -> bool {
+                         auto firstFound = DisjunctionNumber.find(first);
+                         auto secondFound = DisjunctionNumber.find(second);
 
-  // If there are no active constraints in the disjunction, there is
-  // no solution.
-  if (bestSize == 0)
-    return nullptr;
+                         assert(firstFound != DisjunctionNumber.end() &&
+                                secondFound != DisjunctionNumber.end());
+
+                         return firstFound->second < secondFound->second;
+                       });
 
-  return disjunction;
+  if (minDisjunction != disjunctions.end())
+    return *minDisjunction;
+
+  return nullptr;
 }
 
 bool ConstraintSystem::solveSimplified(
-    Constraint *disjunction, SmallVectorImpl<Solution> &solutions,
+    SmallVectorImpl<Solution> &solutions,
     FreeTypeVariableBinding allowFreeTypeVariables) {
 
+  auto *disjunction = selectDisjunction();
+
   auto bestBindings = determineBestBindings();
 
   // If we have a binding that does not involve type variables, and is
@@ -1937,6 +1930,8 @@ bool ConstraintSystem::solveSimplified(
                                    allowFreeTypeVariables);
   }
 
+
+
   // If there are no disjunctions we can't solve this system unless we have
   // free type variables and are allowing them in the solution.
   if (!disjunction) {

lib/Sema/Constraint.cpp

@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift.org open source project
 //
-// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Copyright (c) 2014 - 2018 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
@@ -798,6 +798,7 @@ Constraint *Constraint::createDisjunction(ConstraintSystem &cs,
   auto disjunction =  new (mem) Constraint(ConstraintKind::Disjunction,
                               cs.allocateCopy(constraints), locator, typeVars);
   disjunction->RememberChoice = (bool) rememberChoice;
+  cs.noteNewDisjunction(disjunction);
   return disjunction;
 }
 

lib/Sema/ConstraintSystem.h

@@ -902,6 +902,13 @@ class ConstraintSystem {
   /// The original CS if this CS was created as a simplification of another CS
   ConstraintSystem *baseCS = nullptr;
 
+  /// \brief The total number of disjunctions created.
+  unsigned CountDisjunctions = 0;
+
+  /// \brief Map from disjunction to the number indicating the order it
+  //         was created in.
+  llvm::DenseMap<Constraint *, unsigned> DisjunctionNumber;
+
 private:
 
   /// \brief Allocator used for all of the related constraint systems.
@@ -1852,6 +1859,11 @@ class ConstraintSystem {
                                        bool allowFixes,
                                        ConstraintLocatorBuilder locator);
 
+  void noteNewDisjunction(Constraint *constraint) {
+    assert(constraint->getKind() == ConstraintKind::Disjunction);
+    DisjunctionNumber[constraint] = CountDisjunctions++;
+  }
+
   /// \brief Add a disjunction constraint.
   void addDisjunctionConstraint(ArrayRef<Constraint *> constraints,
                                 ConstraintLocatorBuilder locator,
@@ -2920,17 +2932,13 @@ class ConstraintSystem {
   /// \brief Solve the system of constraints after it has already been
   /// simplified.
   ///
-  /// \param disjunction The disjunction to try and solve using simplified
-  /// constraint system.
-  ///
   /// \param solutions The set of solutions to this system of constraints.
   ///
   /// \param allowFreeTypeVariables How to bind free type variables in
   /// the solution.
   ///
   /// \returns true if an error occurred, false otherwise.
-  bool solveSimplified(Constraint *disjunction,
-                       SmallVectorImpl<Solution> &solutions,
+  bool solveSimplified(SmallVectorImpl<Solution> &solutions,
                        FreeTypeVariableBinding allowFreeTypeVariables);
 
   /// \brief Find reduced domains of disjunction constraints for given
@@ -2942,15 +2950,10 @@ class ConstraintSystem {
   /// \param expr The expression to find reductions for.
   void shrink(Expr *expr);
 
-  /// \brief Pick a disjunction from the given list, which,
-  /// based on the associated constraints, is the most viable to
-  /// reduce depth of the search tree.
-  ///
-  /// \param disjunctions A collection of disjunctions to examine.
+  /// \brief Pick a disjunction from the InactiveConstraints list.
   ///
-  /// \returns The disjunction with most weight relative to others, based
-  /// on the number of constraints associated with it, or nullptr otherwise.
-  Constraint *selectDisjunction(SmallVectorImpl<Constraint *> &disjunctions);
+  /// \returns The selected disjunction.
+  Constraint *selectDisjunction();
 
   bool simplifyForConstraintPropagation();
   void collectNeighboringBindOverloadDisjunctions(
@@ -3345,8 +3348,7 @@ class Component {
       llvm::SaveAndRestore<ConstraintSystem::SolverScope *>
           partialSolutionScope(cs.solverState->PartialSolutionScope, &scope);
 
-      failed = cs.solveSimplified(cs.selectDisjunction(Disjunctions), solutions,
-                                  allowFreeTypeVariables);
+      failed = cs.solveSimplified(solutions, allowFreeTypeVariables);
     }
 
     // Put the constraints back into their original bucket.

validation-test/stdlib/AnyHashable.swift.gyb

@@ -757,8 +757,9 @@ AnyHashableTests.test("AnyHashable(MinimalHashableRCSwiftError)/Hashable") {
     xs,
     equalityOracle: { $0 / 2 == $1 / 2 },
     hashEqualityOracle: { $0 / 4 == $1 / 4 })
+  let mapped = xs.map(AnyHashable.init)
   checkHashable(
-    xs.map(AnyHashable.init),
+    mapped,
     equalityOracle: { $0 / 2 == $1 / 2 },
     hashEqualityOracle: { $0 / 4 == $1 / 4 })
 }