Merge pull request swiftlang#77152 from slavapestov/fix-issue-77008

Sema: Give constraints a more dignified retirement

Author: slavapestov

include/swift/Sema/CSTrail.def

@@ -69,7 +69,6 @@ CLOSURE_CHANGE(RecordedPreconcurrencyClosure)
 CONSTRAINT_CHANGE(DisabledConstraint)
 CONSTRAINT_CHANGE(FavoredConstraint)
 CONSTRAINT_CHANGE(GeneratedConstraint)
-CONSTRAINT_CHANGE(RetiredConstraint)
 
 GRAPH_NODE_CHANGE(AddedConstraint)
 GRAPH_NODE_CHANGE(RemovedConstraint)
@@ -97,8 +96,9 @@ CHANGE(RecordedCaseLabelItemInfo)
 CHANGE(RecordedPotentialThrowSite)
 CHANGE(RecordedIsolatedParam)
 CHANGE(RecordedKeyPath)
+CHANGE(RetiredConstraint)
 
-LAST_CHANGE(RecordedKeyPath)
+LAST_CHANGE(RetiredConstraint)
 
 #undef LOCATOR_CHANGE
 #undef EXPR_CHANGE

include/swift/Sema/CSTrail.h

@@ -20,6 +20,8 @@
 #include "swift/AST/AnyFunctionRef.h"
 #include "swift/AST/Type.h"
 #include "swift/AST/Types.h"
+#include "swift/Sema/Constraint.h"
+#include "llvm/ADT/ilist.h"
 #include <vector>
 
 namespace llvm {
@@ -103,7 +105,6 @@ class SolverTrail {
         Type DstType;
       } Restriction;
 
-
       struct {
         GenericTypeParamType *GP;
         Type ReqTy;
@@ -119,6 +120,14 @@ class SolverTrail {
         Type OldType;
       } KeyPath;
 
+      struct {
+        /// It's former position in the inactive constraints list.
+        llvm::ilist<Constraint>::iterator Where;
+
+        /// The constraint.
+        Constraint *Constraint;
+      } Retiree;
+
       ConstraintFix *TheFix;
       ConstraintLocator *TheLocator;
       PackExpansionType *TheExpansion;
@@ -213,6 +222,10 @@ class SolverTrail {
     /// Create a change that recorded a key path expression.
     static Change RecordedKeyPath(KeyPathExpr *expr);
 
+    /// Create a change that removed a constraint from the inactive constraint list.
+    static Change RetiredConstraint(llvm::ilist<Constraint>::iterator where,
+                                    Constraint *constraint);
+
     /// Undo this change, reverting the constraint graph to the state it
     /// had prior to this change.
     ///

include/swift/Sema/ConstraintSystem.h

@@ -2538,13 +2538,6 @@ class ConstraintSystem {
     #define CS_STATISTIC(Name, Description) unsigned Name = 0;
     #include "ConstraintSolverStats.def"
 
-    /// Mark given constraint as retired along current solver path.
-    ///
-    /// \param constraint The constraint to retire temporarily.
-    void retireConstraint(Constraint *constraint) {
-      Trail.recordChange(SolverTrail::Change::RetiredConstraint(constraint));
-    }
-
     /// Add new "generated" constraint along the current solver path.
     ///
     /// \param constraint The newly generated constraint.
@@ -3924,7 +3917,10 @@ class ConstraintSystem {
   /// Remove an inactive constraint from the current constraint graph.
   void removeInactiveConstraint(Constraint *constraint) {
     CG.removeConstraint(constraint);
-    InactiveConstraints.erase(constraint);
+
+    auto where = InactiveConstraints.erase(constraint);
+    if (solverState)
+      recordChange(SolverTrail::Change::RetiredConstraint(where, constraint));
 
     if (isDebugMode() && getPhase() == ConstraintSystemPhase::Solving) {
       auto &log = llvm::errs();
@@ -3934,9 +3930,6 @@ class ConstraintSystem {
                         solverState->getCurrentIndent() + 4);
       log << ")\n";
     }
-
-    if (solverState)
-      solverState->retireConstraint(constraint);
   }
 
   /// Transfer given constraint from to active list

lib/Sema/CSStep.cpp

@@ -425,13 +425,17 @@ StepResult ComponentStep::take(bool prevFailed) {
     case StepKind::Binding:
       return suspend(
           std::make_unique<TypeVariableStep>(*bestBindings, Solutions));
-    case StepKind::Disjunction:
+    case StepKind::Disjunction: {
+      CS.retireConstraint(disjunction);
       return suspend(
           std::make_unique<DisjunctionStep>(CS, disjunction, Solutions));
-    case StepKind::Conjunction:
+    }
+    case StepKind::Conjunction: {
+      CS.retireConstraint(conjunction);
       return suspend(
           std::make_unique<ConjunctionStep>(CS, conjunction, Solutions));
     }
+    }
     llvm_unreachable("Unhandled case in switch!");
   }
 

lib/Sema/CSStep.h

@@ -202,18 +202,6 @@ class SolverStep {
     return StepResult::unsolved(followup);
   }
 
-  /// Erase constraint from the constraint system (include constraint graph)
-  /// and return the constraint which follows it.
-  ConstraintList::iterator erase(Constraint *constraint) {
-    CS.CG.removeConstraint(constraint);
-    return CS.InactiveConstraints.erase(constraint);
-  }
-
-  void restore(ConstraintList::iterator &iterator, Constraint *constraint) {
-    CS.InactiveConstraints.insert(iterator, constraint);
-    CS.CG.addConstraint(constraint);
-  }
-
   void recordDisjunctionChoice(ConstraintLocator *disjunctionLocator,
                                unsigned index) const {
     CS.recordDisjunctionChoice(disjunctionLocator, index);
@@ -684,16 +672,14 @@ class TypeVariableStep final : public BindingStep<TypeVarBindingProducer> {
 class DisjunctionStep final : public BindingStep<DisjunctionChoiceProducer> {
   Constraint *Disjunction;
   SmallVector<Constraint *, 4> DisabledChoices;
-  ConstraintList::iterator AfterDisjunction;
 
   std::optional<Score> BestNonGenericScore;
   std::optional<std::pair<Constraint *, Score>> LastSolvedChoice;
 
 public:
   DisjunctionStep(ConstraintSystem &cs, Constraint *disjunction,
                   SmallVectorImpl<Solution> &solutions)
-      : BindingStep(cs, {cs, disjunction}, solutions), Disjunction(disjunction),
-        AfterDisjunction(erase(disjunction)) {
+      : BindingStep(cs, {cs, disjunction}, solutions), Disjunction(disjunction) {
     assert(Disjunction->getKind() == ConstraintKind::Disjunction);
     pruneOverloadSet(Disjunction);
     ++cs.solverState->NumDisjunctions;
@@ -702,8 +688,6 @@ class DisjunctionStep final : public BindingStep<DisjunctionChoiceProducer> {
   ~DisjunctionStep() override {
     // Rewind back any changes left after attempting last choice.
     ActiveChoice.reset();
-    // Return disjunction constraint back to the system.
-    restore(AfterDisjunction, Disjunction);
     // Re-enable previously disabled overload choices.
     for (auto *choice : DisabledChoices)
       choice->setEnabled();
@@ -919,10 +903,6 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
 
   /// Conjunction constraint associated with this step.
   Constraint *Conjunction;
-  /// Position of the conjunction in the inactive constraints
-  /// list which is required to re-instate it to the system
-  /// after this step is done.
-  ConstraintList::iterator AfterConjunction;
 
   /// Indicates that one of the elements failed inference.
   bool HadFailure = false;
@@ -950,7 +930,7 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
                     conjunction->isIsolated() ? IsolatedSolutions : solutions),
         BestScore(getBestScore()),
         OuterScopeCount(cs.CountScopes, 0), Conjunction(conjunction),
-        AfterConjunction(erase(conjunction)), OuterSolutions(solutions) {
+        OuterSolutions(solutions) {
     assert(conjunction->getKind() == ConstraintKind::Conjunction);
 
     // Make a snapshot of the constraint system state before conjunction.
@@ -969,9 +949,6 @@ class ConjunctionStep : public BindingStep<ConjunctionElementProducer> {
     // Return all of the type variables and constraints back.
     Snapshot.reset();
 
-    // Restore conjunction constraint.
-    restore(AfterConjunction, Conjunction);
-
     // Restore best score only if conjunction fails because
     // successful outcome should keep a score set by `restoreOuterState`.
     if (HadFailure)

lib/Sema/CSTrail.cpp

@@ -296,6 +296,16 @@ SolverTrail::Change::RecordedKeyPath(KeyPathExpr *expr) {
   return result;
 }
 
+SolverTrail::Change
+SolverTrail::Change::RetiredConstraint(ConstraintList::iterator where,
+                                       Constraint *constraint) {
+  Change result;
+  result.Kind = ChangeKind::RetiredConstraint;
+  result.Retiree.Where = where;
+  result.Retiree.Constraint = constraint;
+  return result;
+}
+
 SyntacticElementTargetKey
 SolverTrail::Change::getSyntacticElementTargetKey() const {
   ASSERT(Kind == ChangeKind::RecordedTarget);
@@ -481,7 +491,8 @@ void SolverTrail::Change::undo(ConstraintSystem &cs) const {
   }
 
   case ChangeKind::RetiredConstraint:
-    cs.InactiveConstraints.push_back(TheConstraint.Constraint);
+    cs.InactiveConstraints.insert(Retiree.Where,
+                                  Retiree.Constraint);
     break;
   }
 }
@@ -674,6 +685,13 @@ void SolverTrail::Change::dump(llvm::raw_ostream &out,
     simple_display(out, KeyPath.Expr);
     out << ")\n";
     break;
+
+  case ChangeKind::RetiredConstraint:
+    out << "(RetiredConstraint ";
+    TheConstraint.Constraint->print(out, &cs.getASTContext().SourceMgr,
+                                    indent + 2);
+    out << ")\n";
+    break;
   }
 }
 

test/Constraints/issue-77008.swift

@@ -0,0 +1,5 @@
+// RUN: not %target-swift-frontend -typecheck %s
+
+// Just don't crash.
+
+Binding<Bool>Binding<Bool>

validation-test/Sema/type_checker_perf/fast/borderline_flat_map_operator_mix.swift

@@ -1,5 +1,7 @@
 // RUN: %target-typecheck-verify-swift -solver-expression-time-threshold=10
 
+// REQUIRES: no_asan
+
 struct S {
     var t: Double
 

validation-test/Sema/type_checker_perf/slow/coerce_literal_linear_combo.swift.gyb

@@ -1,5 +1,7 @@
 // RUN: %scale-test --invert-result --begin 2 --end 5 --step 1 --select NumLeafScopes %s
 
+// REQUIRES: no_asan
+
 func g<T: Equatable>(_: T, _: T) {}
 
 let base: UInt64 = 0

validation-test/Sema/type_checker_perf/slow/simd_scalar_multiple.swift.gyb

@@ -1,5 +1,7 @@
 // RUN: %scale-test --invert-result --begin 1 --end 5 --step 1 --select NumLeafScopes %s
 
+// REQUIRES: no_asan
+
 func f(c: Float, a: SIMD2<Float>) -> SIMD2<Float> {
   return (c * a)
 %for i in range(1, N):