Sema: Don't create new ConstraintGraphNode during active undo

This messes up the bookkeeping for the trail.

Author: slavapestov

Diff for: include/swift/Sema/CSTrail.h

@@ -133,6 +133,8 @@ class SolverTrail {
   SolverTrail(const SolverTrail &) = delete;
   SolverTrail &operator=(const SolverTrail &) = delete;
 
+  bool isUndoActive() const { return UndoActive; }
+
   void recordChange(Change change);
 
   void dumpActiveScopeChanges(llvm::raw_ostream &out,

Diff for: include/swift/Sema/ConstraintSystem.h

@@ -521,7 +521,7 @@ class TypeVariableType::Implementation {
       impl = &nextTV->getImpl();
     }
 
-    if (impl == this || !trail)
+    if (impl == this || !trail || trail->isUndoActive())
       return result;
 
     // Perform path compression.
@@ -2659,10 +2659,6 @@ class ConstraintSystem {
       favoredConstraints.push_back(constraint);
     }
 
-    void recordChange(SolverTrail::Change change) {
-      Trail.recordChange(change);
-    }
-
   private:
     /// The list of constraints that have been retired along the
     /// current path, this list is used in LIFO fashion when
@@ -2798,6 +2794,14 @@ class ConstraintSystem {
   /// we're exploring.
   SolverState *solverState = nullptr;
 
+  bool isRecordingChanges() const {
+    return solverState && !solverState->Trail.isUndoActive();
+  }
+
+  void recordChange(SolverTrail::Change change) {
+    solverState->Trail.recordChange(change);
+  }
+
   /// Form a locator that can be used to retrieve argument information cached in
   /// the constraint system for the callee described by the anchor of the
   /// passed locator.

Diff for: lib/Sema/CSTrail.cpp

@@ -194,9 +194,7 @@ void SolverTrail::Change::dump(llvm::raw_ostream &out,
 }
 
 void SolverTrail::recordChange(Change change) {
-  if (UndoActive)
-    return;
-
+  ASSERT(!UndoActive);
   Changes.push_back(change);
 }
 

Diff for: lib/Sema/ConstraintGraph.cpp

@@ -79,9 +79,12 @@ ConstraintGraph::lookupNode(TypeVariableType *typeVar) {
   // Record this type variable.
   TypeVariables.push_back(typeVar);
 
-  // Record the change, if there are active scopes.
+  // Record the change, if there are active scopes. Note that we specifically
+  // check CS.solverState and not CS.isRecordingChanges(), because we want
+  // recordChange() to assert if there's an active undo. It is not valid to
+  // create new nodes during an undo.
   if (CS.solverState)
-    CS.solverState->recordChange(SolverTrail::Change::addedTypeVariable(typeVar));
+    CS.recordChange(SolverTrail::Change::addedTypeVariable(typeVar));
 
   // If this type variable is not the representative of its equivalence class,
   // add it to its representative's set of equivalences.
@@ -475,8 +478,8 @@ void ConstraintGraph::addConstraint(Constraint *constraint) {
   }
 
   // Record the change, if there are active scopes.
-  if (CS.solverState)
-    CS.solverState->recordChange(SolverTrail::Change::addedConstraint(constraint));
+  if (CS.isRecordingChanges())
+    CS.recordChange(SolverTrail::Change::addedConstraint(constraint));
 }
 
 void ConstraintGraph::removeConstraint(Constraint *constraint) {
@@ -501,8 +504,8 @@ void ConstraintGraph::removeConstraint(Constraint *constraint) {
   }
 
   // Record the change, if there are active scopes.
-  if (CS.solverState)
-    CS.solverState->recordChange(SolverTrail::Change::removedConstraint(constraint));
+  if (CS.isRecordingChanges())
+    CS.recordChange(SolverTrail::Change::removedConstraint(constraint));
 }
 
 void ConstraintGraph::mergeNodes(TypeVariableType *typeVar1, 
@@ -520,8 +523,8 @@ void ConstraintGraph::mergeNodes(TypeVariableType *typeVar1,
   auto typeVarNonRep = typeVar1 == typeVarRep? typeVar2 : typeVar1;
 
   // Record the change, if there are active scopes.
-  if (CS.solverState) {
-    CS.solverState->recordChange(
+  if (CS.isRecordingChanges()) {
+    CS.recordChange(
       SolverTrail::Change::extendedEquivalenceClass(
                         typeVarRep,
                         repNode.getEquivalenceClass().size()));
@@ -536,12 +539,6 @@ void ConstraintGraph::bindTypeVariable(TypeVariableType *typeVar, Type fixed) {
   assert(!fixed->is<TypeVariableType>() &&
          "Cannot bind to type variable; merge equivalence classes instead");
 
-  // Record the change, if there are active scopes.
-  if (CS.solverState) {
-    CS.solverState->recordChange(
-      SolverTrail::Change::boundTypeVariable(typeVar, fixed));
-  }
-
   auto &node = (*this)[typeVar];
 
   llvm::SmallPtrSet<TypeVariableType *, 4> referencedVars;
@@ -556,6 +553,10 @@ void ConstraintGraph::bindTypeVariable(TypeVariableType *typeVar, Type fixed) {
     otherNode.addReferencedBy(typeVar);
     node.addReferencedVar(otherTypeVar);
   }
+
+  // Record the change, if there are active scopes.
+  if (CS.isRecordingChanges())
+    CS.recordChange(SolverTrail::Change::boundTypeVariable(typeVar, fixed));
 }
 
 void ConstraintGraph::unbindTypeVariable(TypeVariableType *typeVar, Type fixed) {