Sema: Push reconciliation down into applySolution() to strengthen invariants

Now, we assert if you try to record the same change twice in any
other code path.

Author: slavapestov

include/swift/Sema/ConstraintSystem.h

@@ -4422,7 +4422,8 @@ class ConstraintSystem {
   /// Record the set of opened types for the given locator.
   void recordOpenedTypes(
          ConstraintLocatorBuilder locator,
-         const OpenedTypeMap &replacements);
+         const OpenedTypeMap &replacements,
+         bool fixmeAllowDuplicates=false);
 
   /// Check whether the given type conforms to the given protocol and if
   /// so return a valid conformance reference.

lib/Sema/CSSimplify.cpp

@@ -14927,10 +14927,10 @@ void ConstraintSystem::recordMatchCallArgumentResult(
     ConstraintLocator *locator, MatchCallArgumentResult result) {
   assert(locator->isLastElement<LocatorPathElt::ApplyArgument>());
   bool inserted = argumentMatchingChoices.insert({locator, result}).second;
-  if (inserted) {
-    if (isRecordingChanges())
-      recordChange(SolverTrail::Change::recordedMatchCallArgumentResult(locator));
-  }
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedMatchCallArgumentResult(locator));
 }
 
 void ConstraintSystem::recordCallAsFunction(UnresolvedDotExpr *root,

lib/Sema/CSSolver.cpp

@@ -304,17 +304,20 @@ void ConstraintSystem::applySolution(const Solution &solution) {
 
   // Register the solution's disjunction choices.
   for (auto &choice : solution.DisjunctionChoices) {
-    recordDisjunctionChoice(choice.first, choice.second);
+    if (DisjunctionChoices.count(choice.first) == 0)
+      recordDisjunctionChoice(choice.first, choice.second);
   }
 
   // Register the solution's applied disjunctions.
   for (auto &choice : solution.AppliedDisjunctions) {
-    recordAppliedDisjunction(choice.first, choice.second);
+    if (AppliedDisjunctions.count(choice.first) == 0)
+      recordAppliedDisjunction(choice.first, choice.second);
   }
 
   // Remember all of the argument/parameter matching choices we made.
   for (auto &argumentMatch : solution.argumentMatchingChoices) {
-    recordMatchCallArgumentResult(argumentMatch.first, argumentMatch.second);
+    if (argumentMatchingChoices.count(argumentMatch.first) == 0)
+      recordMatchCallArgumentResult(argumentMatch.first, argumentMatch.second);
   }
 
   // Remember implied results.
@@ -323,32 +326,40 @@ void ConstraintSystem::applySolution(const Solution &solution) {
 
   // Register the solution's opened types.
   for (const auto &opened : solution.OpenedTypes) {
-    recordOpenedType(opened.first, opened.second);
+    if (OpenedTypes.count(opened.first) == 0)
+      recordOpenedType(opened.first, opened.second);
   }
 
   // Register the solution's opened existential types.
   for (const auto &openedExistential : solution.OpenedExistentialTypes) {
-    recordOpenedExistentialType(openedExistential.first, openedExistential.second);
+    if (OpenedExistentialTypes.count(openedExistential.first) == 0) {
+      recordOpenedExistentialType(openedExistential.first,
+                                  openedExistential.second);
+    }
   }
 
   // Register the solution's opened pack expansion types.
   for (const auto &expansion : solution.OpenedPackExpansionTypes) {
-    recordOpenedPackExpansionType(expansion.first, expansion.second);
+    if (OpenedPackExpansionTypes.count(expansion.first) == 0)
+      recordOpenedPackExpansionType(expansion.first, expansion.second);
   }
 
   // Register the solutions's pack expansion environments.
   for (const auto &expansion : solution.PackExpansionEnvironments) {
-    recordPackExpansionEnvironment(expansion.first, expansion.second);
+    if (PackExpansionEnvironments.count(expansion.first) == 0)
+      recordPackExpansionEnvironment(expansion.first, expansion.second);
   }
 
   // Register the solutions's pack environments.
   for (auto &packEnvironment : solution.PackEnvironments) {
-    addPackEnvironment(packEnvironment.first, packEnvironment.second);
+    if (PackEnvironments.count(packEnvironment.first) == 0)
+      addPackEnvironment(packEnvironment.first, packEnvironment.second);
   }
 
   // Register the defaulted type variables.
-  for (auto *locator : solution.DefaultedConstraints)
+  for (auto *locator : solution.DefaultedConstraints) {
     recordDefaultedConstraint(locator);
+  }
 
   // Add the node types back.
   for (auto &nodeType : solution.nodeTypes) {
@@ -424,12 +435,17 @@ void ConstraintSystem::applySolution(const Solution &solution) {
   }
 
   // Register any fixes produced along this path.
-  for (auto *fix : solution.Fixes)
-    addFix(fix);
+  for (auto *fix : solution.Fixes) {
+    if (Fixes.count(fix) == 0)
+      addFix(fix);
+  }
 
   // Register fixed requirements.
-  for (auto fix : solution.FixedRequirements)
-    recordFixedRequirement(std::get<0>(fix), std::get<1>(fix), std::get<2>(fix));
+  for (auto fix : solution.FixedRequirements) {
+    recordFixedRequirement(std::get<0>(fix),
+                           std::get<1>(fix),
+                           std::get<2>(fix));
+  }
 }
 bool ConstraintSystem::simplify() {
   // While we have a constraint in the worklist, process it.

lib/Sema/ConstraintSystem.cpp

@@ -281,8 +281,7 @@ void ConstraintSystem::removeConversionRestriction(
 
 void ConstraintSystem::addFix(ConstraintFix *fix) {
   bool inserted = Fixes.insert(fix);
-  if (!inserted)
-    return;
+  ASSERT(inserted);
 
   if (solverState)
     recordChange(SolverTrail::Change::addedFix(fix));
@@ -294,32 +293,23 @@ void ConstraintSystem::removeFix(ConstraintFix *fix) {
 }
 
 void ConstraintSystem::recordDisjunctionChoice(
-    ConstraintLocator *locator,
-    unsigned index) {
-  // We shouldn't ever register disjunction choices multiple times.
-  auto inserted = DisjunctionChoices.insert(
-      std::make_pair(locator, index));
-  if (!inserted.second) {
-    ASSERT(inserted.first->second == index);
-    return;
-  }
+    ConstraintLocator *locator, unsigned index) {
+  bool inserted = DisjunctionChoices.insert({locator, index}).second;
+  ASSERT(inserted);
 
-  if (solverState) {
-    recordChange(SolverTrail::Change::recordedDisjunctionChoice(
-      locator, index));
-  }
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedDisjunctionChoice(locator, index));
 }
 
 void ConstraintSystem::recordAppliedDisjunction(
     ConstraintLocator *locator, FunctionType *fnType) {
   // We shouldn't ever register disjunction choices multiple times.
-  auto inserted = AppliedDisjunctions.insert(
-      std::make_pair(locator, fnType));
-  if (inserted.second) {
-    if (solverState) {
-      recordChange(SolverTrail::Change::recordedAppliedDisjunction(locator));
-    }
-  }
+  bool inserted = AppliedDisjunctions.insert(
+      std::make_pair(locator, fnType)).second;
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedAppliedDisjunction(locator));
 }
 
 /// Retrieve a dynamic result signature for the given declaration.
@@ -853,10 +843,10 @@ std::pair<Type, OpenedArchetypeType *> ConstraintSystem::openExistentialType(
 void ConstraintSystem::recordOpenedExistentialType(
     ConstraintLocator *locator, OpenedArchetypeType *opened) {
   bool inserted = OpenedExistentialTypes.insert({locator, opened}).second;
-  if (inserted) {
-    if (solverState)
-      recordChange(SolverTrail::Change::recordedOpenedExistentialType(locator));
-  }
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedOpenedExistentialType(locator));
 }
 
 GenericEnvironment *
@@ -894,12 +884,10 @@ ConstraintSystem::getPackElementEnvironment(ConstraintLocator *locator,
 void ConstraintSystem::recordPackExpansionEnvironment(
     ConstraintLocator *locator, std::pair<UUID, Type> uuidAndShape) {
   bool inserted = PackExpansionEnvironments.insert({locator, uuidAndShape}).second;
-  if (inserted) {
-    if (solverState) {
-      recordChange(
-        SolverTrail::Change::recordedPackExpansionEnvironment(locator));
-    }
-  }
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedPackExpansionEnvironment(locator));
 }
 
 PackExpansionExpr *
@@ -910,12 +898,11 @@ ConstraintSystem::getPackEnvironment(PackElementExpr *packElement) const {
 
 void ConstraintSystem::addPackEnvironment(PackElementExpr *packElement,
                                           PackExpansionExpr *packExpansion) {
-  bool inserted =
-      PackEnvironments.insert({packElement, packExpansion}).second;
-  if (inserted) {
-    if (solverState)
-      recordChange(SolverTrail::Change::recordedPackEnvironment(packElement));
-  }
+  bool inserted = PackEnvironments.insert({packElement, packExpansion}).second;
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedPackEnvironment(packElement));
 }
 
 /// Extend the given depth map by adding depths for all of the subexpressions
@@ -1028,7 +1015,8 @@ Type ConstraintSystem::openUnboundGenericType(GenericTypeDecl *decl,
   openGeneric(decl->getDeclContext(), decl->getGenericSignature(), locator,
               replacements);
 
-  recordOpenedTypes(locator, replacements);
+  // FIXME: Get rid of fixmeAllowDuplicates.
+  recordOpenedTypes(locator, replacements, /*fixmeAllowDuplicates=*/true);
 
   if (parentTy) {
     const auto parentTyInContext =
@@ -1278,10 +1266,10 @@ Type ConstraintSystem::openPackExpansionType(PackExpansionType *expansion,
 void ConstraintSystem::recordOpenedPackExpansionType(PackExpansionType *expansion,
                                                      TypeVariableType *expansionVar) {
   bool inserted = OpenedPackExpansionTypes.insert({expansion, expansionVar}).second;
-  if (inserted) {
-    if (solverState)
-      recordChange(SolverTrail::Change::recordedOpenedPackExpansionType(expansion));
-  }
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedOpenedPackExpansionType(expansion));
 }
 
 Type ConstraintSystem::openOpaqueType(OpaqueTypeArchetypeType *opaque,
@@ -1687,10 +1675,10 @@ Type ConstraintSystem::getUnopenedTypeOfReference(
 void ConstraintSystem::recordOpenedType(
     ConstraintLocator *locator, ArrayRef<OpenedType> openedTypes) {
   bool inserted = OpenedTypes.insert({locator, openedTypes}).second;
-  if (inserted) {
-    if (solverState)
-      recordChange(SolverTrail::Change::recordedOpenedTypes(locator));
-  }
+  ASSERT(inserted);
+
+  if (solverState)
+    recordChange(SolverTrail::Change::recordedOpenedTypes(locator));
 }
 
 void ConstraintSystem::removeOpenedType(ConstraintLocator *locator) {
@@ -1700,7 +1688,8 @@ void ConstraintSystem::removeOpenedType(ConstraintLocator *locator) {
 
 void ConstraintSystem::recordOpenedTypes(
        ConstraintLocatorBuilder locator,
-       const OpenedTypeMap &replacements) {
+       const OpenedTypeMap &replacements,
+       bool fixmeAllowDuplicates) {
   if (replacements.empty())
     return;
 
@@ -1721,7 +1710,10 @@ void ConstraintSystem::recordOpenedTypes(
   OpenedType* openedTypes
     = Allocator.Allocate<OpenedType>(replacements.size());
   std::copy(replacements.begin(), replacements.end(), openedTypes);
-  recordOpenedType(
+
+  // FIXME: Get rid of fixmeAllowDuplicates.
+  if (!fixmeAllowDuplicates || OpenedTypes.count(locatorPtr) == 0)
+    recordOpenedType(
       locatorPtr, llvm::ArrayRef(openedTypes, replacements.size()));
 }