Find cycles in rewrite constraints without performing the full exponential expansion of the RHS (#5673)

Make Subst perform "recursion" on the RHS instructions as they are
replaced, effectively doing a depth-first traversal through the rewrite
constraints doing replacements. This allows us to fully compute
individual associated constants in the minimal amount of work, and cache
the results so they can be reused cheaply in cases where the rewrite
constraints generate an exponential number of references to associated
constants.

Fixes https://github.com/carbon-language/carbon-lang/issues/5672

toolchain/check/eval.cpp

@@ -640,8 +640,10 @@ static auto GetConstantFacetTypeInfo(EvalContext& eval_context,
   // GetConstantValueIgnoringPeriodSelf() which will update the phase
   // accordingly.
   info.rewrite_constraints = orig.rewrite_constraints;
-  ResolveFacetTypeRewriteConstraints(eval_context.context(), loc_id,
-                                     info.rewrite_constraints);
+  if (!ResolveFacetTypeRewriteConstraints(eval_context.context(), loc_id,
+                                          info.rewrite_constraints)) {
+    *phase = Phase::UnknownDueToError;
+  }
 
   for (auto& rewrite : info.rewrite_constraints) {
     // `where` requirements using `.Self` should not be considered symbolic.
@@ -1582,18 +1584,10 @@ static auto MakeConstantForBuiltinCall(EvalContext& eval_context,
       auto combined_info = SemIR::FacetTypeInfo::Combine(
           context.facet_types().Get(lhs_facet_type_id),
           context.facet_types().Get(rhs_facet_type_id));
-      // TODO: The instructions in the rewrite constraints have already been
-      // canonicalized before coming here, and it leads to incorrect diagnostics
-      // in resolve when assigning the same thing to an associated constant
-      // in both facet types being combined.
-      ResolveFacetTypeRewriteConstraints(eval_context.context(), loc_id,
-                                         combined_info.rewrite_constraints);
-      for (auto& rewrite : combined_info.rewrite_constraints) {
-        if (rewrite.lhs_id == SemIR::ErrorInst::InstId ||
-            rewrite.rhs_id == SemIR::ErrorInst::InstId) {
-          phase = Phase::UnknownDueToError;
-          break;
-        }
+      if (!ResolveFacetTypeRewriteConstraints(
+              eval_context.context(), loc_id,
+              combined_info.rewrite_constraints)) {
+        phase = Phase::UnknownDueToError;
       }
       combined_info.Canonicalize();
       return MakeFacetTypeResult(eval_context.context(), combined_info, phase);

toolchain/check/facet_type.cpp

@@ -7,6 +7,7 @@
 #include <compare>
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
 #include "toolchain/check/convert.h"
 #include "toolchain/check/diagnostic_helpers.h"
 #include "toolchain/check/generic.h"
@@ -256,6 +257,47 @@ auto AllocateFacetTypeImplWitness(Context& context,
   context.inst_blocks().ReplacePlaceholder(witness_id, empty_table);
 }
 
+namespace {
+struct FacetTypeConstraintValue {
+  SemIR::EntityNameId entity_name_id;
+  SemIR::ElementIndex access_index;
+  SemIR::SpecificInterfaceId specific_interface_id;
+
+  friend auto operator<=>(const FacetTypeConstraintValue& lhs,
+                          const FacetTypeConstraintValue& rhs)
+      -> std::weak_ordering {
+    if (lhs.entity_name_id != rhs.entity_name_id) {
+      return lhs.entity_name_id.index <=> rhs.entity_name_id.index;
+    }
+    if (lhs.access_index != rhs.access_index) {
+      return lhs.access_index.index <=> rhs.access_index.index;
+    }
+    return lhs.specific_interface_id.index <=> rhs.specific_interface_id.index;
+  }
+
+  friend auto operator==(const FacetTypeConstraintValue& lhs,
+                         const FacetTypeConstraintValue& rhs) -> bool = default;
+};
+}  // namespace
+
+static auto GetFacetTypeConstraintValue(Context& context,
+                                        SemIR::ImplWitnessAccess access)
+    -> std::optional<FacetTypeConstraintValue> {
+  auto lookup =
+      context.insts().TryGetAs<SemIR::LookupImplWitness>(access.witness_id);
+  if (lookup) {
+    auto self = context.insts().TryGetAs<SemIR::BindSymbolicName>(
+        context.constant_values().GetConstantInstId(
+            lookup->query_self_inst_id));
+    if (self) {
+      return {{.entity_name_id = self->entity_name_id,
+               .access_index = access.index,
+               .specific_interface_id = lookup->query_specific_interface_id}};
+    }
+  }
+  return std::nullopt;
+}
+
 // Returns an ordering between two values in a rewrite constraint. Two
 // `ImplWitnessAccess` instructions that refer to the same associated constant
 // through the same facet value are treated as equivalent. Otherwise, the
@@ -272,28 +314,10 @@ static auto CompareFacetTypeConstraintValues(Context& context,
   auto lhs_access = context.insts().TryGetAs<SemIR::ImplWitnessAccess>(lhs_id);
   auto rhs_access = context.insts().TryGetAs<SemIR::ImplWitnessAccess>(rhs_id);
   if (lhs_access && rhs_access) {
-    auto lhs_lookup = context.insts().TryGetAs<SemIR::LookupImplWitness>(
-        lhs_access->witness_id);
-    auto rhs_lookup = context.insts().TryGetAs<SemIR::LookupImplWitness>(
-        rhs_access->witness_id);
-    if (lhs_lookup && rhs_lookup) {
-      auto lhs_self = context.insts().TryGetAs<SemIR::BindSymbolicName>(
-          context.constant_values().GetConstantInstId(
-              lhs_lookup->query_self_inst_id));
-      auto rhs_self = context.insts().TryGetAs<SemIR::BindSymbolicName>(
-          context.constant_values().GetConstantInstId(
-              rhs_lookup->query_self_inst_id));
-      if (lhs_self && rhs_self) {
-        if (lhs_self->entity_name_id != rhs_self->entity_name_id) {
-          return lhs_self->entity_name_id.index <=>
-                 rhs_self->entity_name_id.index;
-        }
-        if (lhs_access->index != rhs_access->index) {
-          return lhs_access->index <=> rhs_access->index;
-        }
-        return lhs_lookup->query_specific_interface_id.index <=>
-               rhs_lookup->query_specific_interface_id.index;
-      }
+    auto lhs_access_value = GetFacetTypeConstraintValue(context, *lhs_access);
+    auto rhs_access_value = GetFacetTypeConstraintValue(context, *rhs_access);
+    if (lhs_access_value && rhs_access_value) {
+      return *lhs_access_value <=> *rhs_access_value;
     }
 
     // We do *not* want to get the evaluated result of `ImplWitnessAccess` here,
@@ -342,6 +366,105 @@ static auto SortAndDedupeRewriteConstraints(
   rewrites.erase(llvm::unique(rewrites, eq), rewrites.end());
 }
 
+// A mapping of each associated constant (represented as `ImplWitnessAccess`) to
+// its value (represented as an `InstId`). Used to track rewrite constraints,
+// with the LHS mapping to the resolved value of the RHS.
+class AccessRewriteValues {
+ public:
+  enum State {
+    NotRewritten,
+    BeingRewritten,
+    FullyRewritten,
+  };
+  struct Value {
+    State state;
+    SemIR::InstId inst_id;
+  };
+
+  auto InsertNotRewritten(Context& context, SemIR::ImplWitnessAccess access,
+                          SemIR::InstId inst_id) -> void {
+    map_.insert({GetKey(context, access), {NotRewritten, inst_id}});
+  }
+
+  // Finds and returns a pointer into the cache for a given ImplWitnessAccess.
+  // The pointer will be invalidated by mutating the cache. Returns `nullptr`
+  // if `access` is not found.
+  auto FindRef(Context& context, SemIR::ImplWitnessAccess access) -> Value* {
+    auto it = map_.find(GetKey(context, access));
+    if (it == map_.end()) {
+      return nullptr;
+    }
+    return &it->second;
+  }
+
+  auto SetBeingRewritten(Value& value) -> void {
+    if (value.state == NotRewritten) {
+      value.state = BeingRewritten;
+    }
+  }
+
+  auto SetFullyRewritten(Value& value, SemIR::InstId rewritten_to_inst_id)
+      -> void {
+    // TODO: If state == FullyRewrtten and the inst id is different (according
+    // to `CompareFacetTypeConstraintValues`), we can diagnose writing two
+    // different values for the same associated constant immediately?
+    //
+    // TODO: Once the above is done, we don't need to do the SortAndDedupe
+    // step in ResolveFacetTypeRewriteConstraints()? We can just convert this
+    // `map_` into a new set of `RewriteConstraint`s which will already be
+    // deduped.
+    if (value.state == BeingRewritten) {
+      value = {FullyRewritten, rewritten_to_inst_id};
+    }
+  }
+
+ private:
+  using Key = FacetTypeConstraintValue;
+  struct KeyInfo {
+    static auto getEmptyKey() -> Key {
+      return {
+          .entity_name_id = SemIR::EntityNameId::None,
+          .access_index = SemIR::ElementIndex(-1),
+          .specific_interface_id = SemIR::SpecificInterfaceId::None,
+      };
+    }
+    static auto getTombstoneKey() -> Key {
+      return {
+          .entity_name_id = SemIR::EntityNameId::None,
+          .access_index = SemIR::ElementIndex(-2),
+          .specific_interface_id = SemIR::SpecificInterfaceId::None,
+      };
+    }
+    static auto getHashValue(Key key) -> unsigned {
+      // This hash produces the same value if two ImplWitnessAccess are
+      // pointing to the same associated constant, even if they are different
+      // instruction ids.
+      //
+      // TODO: This truncates the high bits of the hash code which does not
+      // make for a good hash function.
+      return static_cast<unsigned>(static_cast<uint64_t>(HashValue(key)));
+    }
+    static auto isEqual(Key lhs, Key rhs) -> bool {
+      // This comparison is true if the two ImplWitnessAccess are pointing to
+      // the same associated constant, even if they are different instruction
+      // ids.
+      return lhs == rhs;
+    }
+  };
+
+  auto GetKey(Context& context, SemIR::ImplWitnessAccess access) -> Key {
+    return {*GetFacetTypeConstraintValue(context, access)};
+  }
+
+  // Try avoid heap allocations in the common case where there are a small
+  // number of rewrite rules referring to each other by keeping up to 16 on
+  // the stack.
+  //
+  // TODO: Revisit if 16 is an appropriate number when we can measure how deep
+  // rewrite constraint chains go in practice.
+  llvm::SmallDenseMap<Key, Value, 16, KeyInfo> map_;
+};
+
 // To be used for substituting into the RHS of a rewrite constraint.
 //
 // It will substitute any `ImplWitnessAccess` into `.Self` (a reference to an
@@ -359,119 +482,189 @@ static auto SortAndDedupeRewriteConstraints(
 //
 // This additionally diagnoses cycles when the `ImplWitnessAccess` is reading
 // from the same rewrite constraint, and is thus assigning to the associated
-// constant a value that refers to the same associated constant, such as with
-// `Z where .X = C(.X)`. In the event of a cycle, the `ImplWitnessAccess` is
+// constant a value that refers to the same associated constant, such as with `Z
+// where .X = C(.X)`. In the event of a cycle, the `ImplWitnessAccess` is
 // replaced with `ErrorInst` so that further evaluation of the
 // `ImplWitnessAccess` will not loop infinitely.
+//
+// The `rewrite_values` given to the constructor must be set up initially with
+// each rewrite rule of an associated constant inserted with its unresolved
+// value via `InsertNotRewritten`. Then for each rewrite rule of an associated
+// constant, the LHS access should be set as being rewritten with its state
+// changed to `BeingRewritten` in order to detect cycles before performing
+// SubstInst. The result of SubstInst should be preserved afterward by changing
+// the state and value for the LHS to `FullyRewritten` and the subst output
+// instruction, respectively, to avoid duplicating work.
 class SubstImplWitnessAccessCallbacks : public SubstInstCallbacks {
  public:
-  // The `rewrites` is the set of rewrite constraints that are being
-  // substituted, and where it looks for rewritten values to substitute from.
-  //
-  // The `substituting_constraint` is the rewrite constraint for which the RHS
-  // is being substituted with the value from another rewrite constraint, if
-  // possible. That is, `.Y = .X` in the example in the class docs.
-  explicit SubstImplWitnessAccessCallbacks(
-      Context* context, SemIR::LocId loc_id,
-      llvm::ArrayRef<SemIR::FacetTypeInfo::RewriteConstraint> rewrites,
-      const SemIR::FacetTypeInfo::RewriteConstraint* substituting_constraint)
+  explicit SubstImplWitnessAccessCallbacks(Context* context,
+                                           SemIR::LocId loc_id,
+                                           AccessRewriteValues* rewrite_values)
       : SubstInstCallbacks(context),
         loc_id_(loc_id),
-        rewrites_(rewrites),
-        substituting_constraint_(substituting_constraint) {}
+        rewrite_values_(rewrite_values) {}
+
+  auto Subst(SemIR::InstId& rhs_inst_id) -> SubstResult override {
+    auto rhs_access =
+        context().insts().TryGetAs<SemIR::ImplWitnessAccess>(rhs_inst_id);
+    if (!rhs_access) {
+      // We only want to substitute ImplWitnessAccesses written directly on the
+      // RHS of the rewrite constraint, not when they are nested inside facet
+      // types that are part of the RHS, like `.X = C as (I where .Y = {})`.
+      if (context().insts().Is<SemIR::FacetType>(rhs_inst_id)) {
+        return SubstResult::FullySubstituted;
+      }
+      if (context().constant_values().Get(rhs_inst_id).is_concrete()) {
+        // There's no ImplWitnessAccess that we care about inside this
+        // instruction.
+        return SubstResult::FullySubstituted;
+      } else {
+        // SubstOperands will result in a Rebuild or ReuseUnchanged callback, so
+        // push the non-ImplWitnessAccess to get proper bracketing, allowing us
+        // to pop it in the paired callback.
+        substs_in_progress_.push_back(rhs_inst_id);
+        return SubstResult::SubstOperands;
+      }
+    }
 
-  auto Subst(SemIR::InstId& rhs_inst_id) const -> bool override {
-    if (!context().insts().Is<SemIR::ImplWitnessAccess>(rhs_inst_id)) {
-      return context().constant_values().Get(rhs_inst_id).is_concrete();
+    auto* rewrite_value = rewrite_values_->FindRef(context(), *rhs_access);
+    if (!rewrite_value) {
+      // The RHS refers to an associated constant for which there is no rewrite
+      // rule.
+      return SubstResult::FullySubstituted;
     }
 
-    // TODO: We could consider something better than linear search here, such as
-    // a map. However that would probably require heap allocations which may be
-    // worse overall since the number of rewrite constraints is generally low.
-    for (const auto& search_constraint : rewrites_) {
-      if (CompareFacetTypeConstraintValues(context(), search_constraint.lhs_id,
-                                           rhs_inst_id) ==
-          std::weak_ordering::equivalent) {
-        if (&search_constraint == substituting_constraint_) {
-          if (search_constraint.rhs_id != SemIR::ErrorInst::InstId) {
-            CARBON_DIAGNOSTIC(FacetTypeConstraintCycle, Error,
-                              "found cycle in facet type constraint for {0}",
-                              InstIdAsConstant);
-            // TODO: It would be nice to note the places where the values are
-            // assigned but rewrite constraint instructions are from canonical
-            // constant values, and have no locations. We'd need to store a
-            // location along with them in the rewrite constraints, and track
-            // propagation of locations here, which may imply heap allocations.
-            context().emitter().Emit(loc_id_, FacetTypeConstraintCycle,
-                                     substituting_constraint_->lhs_id);
-            rhs_inst_id = SemIR::ErrorInst::InstId;
-          }
-        } else {
-          rhs_inst_id = search_constraint.rhs_id;
-        }
-      }
+    // Diagnose a cycle if the RHS refers to something that depends on the value
+    // of the RHS.
+    if (rewrite_value->state == AccessRewriteValues::BeingRewritten) {
+      CARBON_DIAGNOSTIC(FacetTypeConstraintCycle, Error,
+                        "found cycle in facet type constraint for {0}",
+                        InstIdAsConstant);
+      // TODO: It would be nice to note the places where the values are
+      // assigned but rewrite constraint instructions are from canonical
+      // constant values, and have no locations. We'd need to store a location
+      // along with them in the rewrite constraints, and track propagation of
+      // locations here, which may imply heap allocations.
+      context().emitter().Emit(loc_id_, FacetTypeConstraintCycle, rhs_inst_id);
+      rhs_inst_id = SemIR::ErrorInst::InstId;
+      return SubstResult::FullySubstituted;
+    } else if (rewrite_value->state == AccessRewriteValues::FullyRewritten) {
+      rhs_inst_id = rewrite_value->inst_id;
+      return SubstResult::FullySubstituted;
     }
 
-    // Never recurse into ImplWitnessAccess, we don't want to substitute into
-    // FacetTypes found within. We only substitute ImplWitnessAccesses that
-    // appear directly on the RHS.
-    return true;
+    // We have a non-rewritten RHS. We need to recurse on rewriting it. Reuse
+    // the previous lookup by mutating it in place.
+    rewrite_values_->SetBeingRewritten(*rewrite_value);
+
+    // The ImplWitnessAccess was replaced with some other instruction, which may
+    // contain or be another ImplWitnessAccess. Keep track of the associated
+    // constant we are now computing the value of.
+    substs_in_progress_.push_back(rhs_inst_id);
+    rhs_inst_id = rewrite_value->inst_id;
+    return SubstResult::SubstAgain;
   }
 
-  auto Rebuild(SemIR::InstId /*orig_inst_id*/, SemIR::Inst new_inst) const
+  auto Rebuild(SemIR::InstId /*orig_inst_id*/, SemIR::Inst new_inst)
       -> SemIR::InstId override {
-    return RebuildNewInst(loc_id_, new_inst);
+    auto inst_id = RebuildNewInst(loc_id_, new_inst);
+    auto subst_inst_id = substs_in_progress_.pop_back_val();
+    if (auto access = context().insts().TryGetAs<SemIR::ImplWitnessAccess>(
+            subst_inst_id)) {
+      auto* rewrite_value = rewrite_values_->FindRef(context(), *access);
+      CARBON_CHECK(rewrite_value);
+      rewrite_values_->SetFullyRewritten(*rewrite_value, inst_id);
+    }
+    return inst_id;
+  }
+
+  auto ReuseUnchanged(SemIR::InstId orig_inst_id) -> SemIR::InstId override {
+    auto subst_inst_id = substs_in_progress_.pop_back_val();
+    if (auto access = context().insts().TryGetAs<SemIR::ImplWitnessAccess>(
+            subst_inst_id)) {
+      auto* rewrite_value = rewrite_values_->FindRef(context(), *access);
+      CARBON_CHECK(rewrite_value);
+      rewrite_values_->SetFullyRewritten(*rewrite_value, orig_inst_id);
+    }
+    return orig_inst_id;
   }
 
  private:
+  struct SubstInProgress {
+    // The associated constant whose value is being determined, represented as
+    // an ImplWitnessAccess. Or another instruction that we are recursing
+    // through.
+    SemIR::InstId inst_id;
+  };
+
+  // The location of the rewrite constraints as a whole.
   SemIR::LocId loc_id_;
-  llvm::ArrayRef<SemIR::FacetTypeInfo::RewriteConstraint> rewrites_;
-  const SemIR::FacetTypeInfo::RewriteConstraint* substituting_constraint_;
+  // Tracks the resolved value of each rewrite constraint, keyed by the
+  // `ImplWitnessAccess` of the associated constant on the LHS of the
+  // constraint. The value of each associated constant may be changed during
+  // substitution, replaced with a fully resolved value for the RHS. This allows
+  // us to cache work; when a value for an associated constant is found once it
+  // can be reused cheaply, avoiding exponential runtime when rewrite rules
+  // refer to each other in ways that create exponential references.
+  AccessRewriteValues* rewrite_values_;
+  // A stack of instructions being replaced in Subst(). When it's an associated
+  // constant, then it represents the constant value is being determined,
+  // represented as an ImplWitnessAccess.
+  //
+  // Avoid heap allocations in common cases, if there are chains of instructions
+  // in associated constants with a depth at most 16.
+  llvm::SmallVector<SemIR::InstId, 16> substs_in_progress_;
 };
 
 auto ResolveFacetTypeRewriteConstraints(
     Context& context, SemIR::LocId loc_id,
     llvm::SmallVector<SemIR::FacetTypeInfo::RewriteConstraint>& rewrites)
-    -> void {
+    -> bool {
   if (rewrites.empty()) {
-    return;
+    return true;
   }
 
-  while (true) {
-    bool applied_rewrite = false;
-
-    for (auto& constraint : rewrites) {
-      if (constraint.lhs_id == SemIR::ErrorInst::InstId ||
-          constraint.rhs_id == SemIR::ErrorInst::InstId) {
-        continue;
-      }
-
-      auto lhs_access =
-          context.insts().TryGetAs<SemIR::ImplWitnessAccess>(constraint.lhs_id);
-      if (!lhs_access) {
-        continue;
-      }
+  // Apply rewrite constraints to each other, so that for example:
+  // `.X = Y and .Y = ()` becomes `.X = () and .Y = ()`.
+  AccessRewriteValues rewrite_values;
+  for (auto& constraint : rewrites) {
+    if (constraint.lhs_id == SemIR::ErrorInst::InstId ||
+        constraint.rhs_id == SemIR::ErrorInst::InstId) {
+      continue;
+    }
+    auto lhs_access =
+        context.insts().TryGetAs<SemIR::ImplWitnessAccess>(constraint.lhs_id);
+    if (!lhs_access) {
+      continue;
+    }
 
-      // Replace any `ImplWitnessAccess` in the RHS of this constraint with the
-      // RHS of another constraint that sets the value of the associated
-      // constant being accessed in the RHS.
-      auto subst_inst_id =
-          SubstInst(context, constraint.rhs_id,
-                    SubstImplWitnessAccessCallbacks(&context, loc_id, rewrites,
-                                                    &constraint));
-      if (subst_inst_id != constraint.rhs_id) {
-        constraint.rhs_id = subst_inst_id;
-        if (constraint.rhs_id != SemIR::ErrorInst::InstId) {
-          // If the RHS is replaced with a non-error value, we need to do
-          // another pass so that the new RHS value can continue to propagate.
-          applied_rewrite = true;
-        }
-      }
+    rewrite_values.InsertNotRewritten(context, *lhs_access, constraint.rhs_id);
+  }
+  for (auto& constraint : rewrites) {
+    if (constraint.lhs_id == SemIR::ErrorInst::InstId ||
+        constraint.rhs_id == SemIR::ErrorInst::InstId) {
+      continue;
+    }
+    auto lhs_access =
+        context.insts().TryGetAs<SemIR::ImplWitnessAccess>(constraint.lhs_id);
+    if (!lhs_access) {
+      continue;
     }
 
-    if (!applied_rewrite) {
-      break;
+    auto* lhs_rewrite_value = rewrite_values.FindRef(context, *lhs_access);
+    CARBON_CHECK(lhs_rewrite_value);
+    rewrite_values.SetBeingRewritten(*lhs_rewrite_value);
+
+    auto replace_witness_callbacks =
+        SubstImplWitnessAccessCallbacks(&context, loc_id, &rewrite_values);
+    auto subst_inst_id =
+        SubstInst(context, constraint.rhs_id, replace_witness_callbacks);
+    constraint.rhs_id = subst_inst_id;
+    if (constraint.rhs_id == SemIR::ErrorInst::InstId) {
+      return false;
     }
+
+    rewrite_values.SetFullyRewritten(*lhs_rewrite_value, subst_inst_id);
   }
 
   // We sort the constraints so that we can find different values being written
@@ -542,8 +735,11 @@ auto ResolveFacetTypeRewriteConstraints(
       }
       constraint.rhs_id = SemIR::ErrorInst::InstId;
       next.rhs_id = SemIR::ErrorInst::InstId;
+      return false;
     }
   }
+
+  return true;
 }
 
 }  // namespace Carbon::Check

toolchain/check/facet_type.h

@@ -77,10 +77,13 @@ auto AllocateFacetTypeImplWitness(Context& context,
 //
 // The rewrite constraints in `rewrites` are modified in place and may be
 // reordered, with `ErrorInst` inserted when diagnosing errors.
+//
+// Returns false if resolve failed due to diagnosing an error. The resulting
+// value of the facet type should be an error constant.
 auto ResolveFacetTypeRewriteConstraints(
     Context& context, SemIR::LocId loc_id,
     llvm::SmallVector<SemIR::FacetTypeInfo::RewriteConstraint>& rewrites)
-    -> void;
+    -> bool;
 
 }  // namespace Carbon::Check
 

toolchain/check/generic.cpp

@@ -86,7 +86,7 @@ class RebuildGenericConstantInEvalBlockCallbacks : public SubstInstCallbacks {
 
   // Check for instructions for which we already have a mapping into the eval
   // block, and substitute them with the instructions in the eval block.
-  auto Subst(SemIR::InstId& inst_id) const -> bool override {
+  auto Subst(SemIR::InstId& inst_id) -> SubstResult override {
     auto const_id = context().constant_values().Get(inst_id);
     if (!const_id.has_value()) {
       // An unloaded import ref should never contain anything we need to
@@ -95,12 +95,12 @@ class RebuildGenericConstantInEvalBlockCallbacks : public SubstInstCallbacks {
           context().insts().Is<SemIR::ImportRefUnloaded>(inst_id),
           "Substituting into instruction with invalid constant ID: {0}",
           context().insts().Get(inst_id));
-      return true;
+      return SubstResult::FullySubstituted;
     }
     if (!context().constant_values().DependsOnGenericParameter(const_id)) {
       // This instruction doesn't have a symbolic constant value, so can't
       // contain any bindings that need to be substituted.
-      return true;
+      return SubstResult::FullySubstituted;
     }
 
     // If this constant value has a defining instruction in the eval block,
@@ -109,15 +109,15 @@ class RebuildGenericConstantInEvalBlockCallbacks : public SubstInstCallbacks {
     if (auto result = constants_in_generic_.Lookup(
             context().constant_values().GetInstId(const_id))) {
       inst_id = result.value();
-      return true;
+      return SubstResult::FullySubstituted;
     }
 
-    return false;
+    return SubstResult::SubstOperands;
   }
 
   // Build a new instruction in the eval block corresponding to the given
   // constant.
-  auto Rebuild(SemIR::InstId orig_inst_id, SemIR::Inst new_inst) const
+  auto Rebuild(SemIR::InstId orig_inst_id, SemIR::Inst new_inst)
       -> SemIR::InstId override {
     auto& orig_symbolic_const = context().constant_values().GetSymbolicConstant(
         context().constant_values().Get(orig_inst_id));
@@ -142,8 +142,7 @@ class RebuildGenericConstantInEvalBlockCallbacks : public SubstInstCallbacks {
     return result.value();
   }
 
-  auto ReuseUnchanged(SemIR::InstId orig_inst_id) const
-      -> SemIR::InstId override {
+  auto ReuseUnchanged(SemIR::InstId orig_inst_id) -> SemIR::InstId override {
     auto inst = context().insts().Get(orig_inst_id);
     CARBON_CHECK(
         inst.Is<SemIR::BindSymbolicName>() ||
@@ -173,7 +172,7 @@ class RebuildTemplateActionInEvalBlockCallbacks final
       : RebuildGenericConstantInEvalBlockCallbacks(context, loc_id),
         action_inst_id_(action_inst_id) {}
 
-  auto Rebuild(SemIR::InstId orig_inst_id, SemIR::Inst new_inst) const
+  auto Rebuild(SemIR::InstId orig_inst_id, SemIR::Inst new_inst)
       -> SemIR::InstId override {
     if (orig_inst_id == action_inst_id_) {
       // TODO: We want to ReplaceInstPreservingConstantValue here, but don't
@@ -185,8 +184,7 @@ class RebuildTemplateActionInEvalBlockCallbacks final
                                                                new_inst);
   }
 
-  auto ReuseUnchanged(SemIR::InstId orig_inst_id) const
-      -> SemIR::InstId override {
+  auto ReuseUnchanged(SemIR::InstId orig_inst_id) -> SemIR::InstId override {
     if (orig_inst_id == action_inst_id_) {
       return orig_inst_id;
     }
@@ -206,9 +204,10 @@ static auto AddGenericTypeToEvalBlock(Context& context, SemIR::LocId loc_id,
                                       SemIR::TypeId type_id) -> SemIR::TypeId {
   // Substitute into the type's constant instruction and rebuild it in the eval
   // block.
-  auto type_inst_id =
-      SubstInst(context, context.types().GetInstId(type_id),
-                RebuildGenericConstantInEvalBlockCallbacks(&context, loc_id));
+  auto rebuild_generic_constant_callbacks =
+      RebuildGenericConstantInEvalBlockCallbacks(&context, loc_id);
+  auto type_inst_id = SubstInst(context, context.types().GetInstId(type_id),
+                                rebuild_generic_constant_callbacks);
   return context.types().GetTypeIdForTypeConstantId(
       context.constant_values().GetAttached(type_inst_id));
 }
@@ -244,9 +243,11 @@ static auto AddGenericConstantToEvalBlock(Context& context,
 static auto AddTemplateActionToEvalBlock(Context& context,
                                          SemIR::InstId inst_id) -> void {
   // Substitute into the constant value and rebuild it in the eval block.
-  auto new_inst_id = SubstInst(context, inst_id,
-                               RebuildTemplateActionInEvalBlockCallbacks(
-                                   &context, SemIR::LocId(inst_id), inst_id));
+  auto rebuild_template_action_callbacks =
+      RebuildTemplateActionInEvalBlockCallbacks(&context, SemIR::LocId(inst_id),
+                                                inst_id);
+  auto new_inst_id =
+      SubstInst(context, inst_id, rebuild_template_action_callbacks);
   CARBON_CHECK(new_inst_id == inst_id,
                "Substitution changed InstId of template action");
   context.generic_region_stack().PeekConstantsInGenericMap().Insert(inst_id,

toolchain/check/subst.cpp

@@ -39,6 +39,8 @@ struct WorklistItem {
   SemIR::InstId inst_id;
   // Whether the operands of this instruction have been added to the worklist.
   bool is_expanded : 1;
+  // Whether the instruction was subst'd and re-added to the worklist.
+  bool is_repeated : 1;
   // The index of the worklist item to process after we finish updating this
   // one. For the final child of an instruction, this is the parent. For any
   // other child, this is the index of the next child of the parent. For the
@@ -51,8 +53,10 @@ struct WorklistItem {
 class Worklist {
  public:
   explicit Worklist(SemIR::InstId root_id) {
-    worklist_.push_back(
-        {.inst_id = root_id, .is_expanded = false, .next_index = -1});
+    worklist_.push_back({.inst_id = root_id,
+                         .is_expanded = false,
+                         .is_repeated = false,
+                         .next_index = -1});
   }
 
   auto operator[](int index) -> WorklistItem& { return worklist_[index]; }
@@ -63,6 +67,7 @@ class Worklist {
     CARBON_CHECK(inst_id.has_value());
     worklist_.push_back({.inst_id = inst_id,
                          .is_expanded = false,
+                         .is_repeated = false,
                          .next_index = static_cast<int>(worklist_.size() + 1)});
     CARBON_CHECK(worklist_.back().next_index > 0, "Constant too large.");
   }
@@ -267,7 +272,7 @@ static auto PopOperand(Context& context, Worklist& worklist,
 // Pops the operands of the specified instruction off the worklist and rebuilds
 // the instruction with the updated operands if it has changed.
 static auto Rebuild(Context& context, Worklist& worklist, SemIR::InstId inst_id,
-                    const SubstInstCallbacks& callbacks) -> SemIR::InstId {
+                    SubstInstCallbacks& callbacks) -> SemIR::InstId {
   auto inst = context.insts().Get(inst_id);
 
   // Note that we pop in reverse order because we pushed them in forwards order.
@@ -288,7 +293,7 @@ static auto Rebuild(Context& context, Worklist& worklist, SemIR::InstId inst_id,
 }
 
 auto SubstInst(Context& context, SemIR::InstId inst_id,
-               const SubstInstCallbacks& callbacks) -> SemIR::InstId {
+               SubstInstCallbacks& callbacks) -> SemIR::InstId {
   Worklist worklist(inst_id);
 
   // For each instruction that forms part of the constant, we will visit it
@@ -307,6 +312,14 @@ auto SubstInst(Context& context, SemIR::InstId inst_id,
   while (index != -1) {
     auto& item = worklist[index];
 
+    if (item.is_repeated) {
+      // Pop the copy of the repeated item when we get back to the repeated
+      // item, and steal any work that was done there so we don't have to
+      // Subst() the repeated item again. The pop does not reallocate the
+      // worklist so does not invalidate `item`.
+      item.inst_id = worklist.Pop();
+    }
+
     if (item.is_expanded) {
       // Rebuild this item if necessary. Note that this might pop items from the
       // worklist but does not reallocate, so does not invalidate `item`.
@@ -315,17 +328,37 @@ auto SubstInst(Context& context, SemIR::InstId inst_id,
       continue;
     }
 
-    if (callbacks.Subst(item.inst_id)) {
-      // If any instruction is an ErrorInst, combining it into another
-      // instruction will also produce an ErrorInst, so shortcut out here to
-      // save wasted work.
-      if (item.inst_id == SemIR::ErrorInst::InstId) {
-        return SemIR::ErrorInst::InstId;
-      }
+    if (item.is_repeated) {
+      // When Subst returns SubstAgain, we must call back to Rebuild or
+      // ReuseUnchanged for that work item.
+      item.inst_id = callbacks.ReuseUnchanged(item.inst_id);
       index = item.next_index;
       continue;
     }
 
+    switch (callbacks.Subst(item.inst_id)) {
+      case SubstInstCallbacks::SubstResult::FullySubstituted:
+        // If any instruction is an ErrorInst, combining it into another
+        // instruction will also produce an ErrorInst, so shortcut out here to
+        // save wasted work.
+        if (item.inst_id == SemIR::ErrorInst::InstId) {
+          return SemIR::ErrorInst::InstId;
+        }
+        index = item.next_index;
+        continue;
+      case SubstInstCallbacks::SubstResult::SubstAgain: {
+        item.is_repeated = true;
+
+        // This modifies `worklist` which invalidates `item`.
+        worklist.Push(item.inst_id);
+        worklist.back().next_index = index;
+        index = worklist.size() - 1;
+        continue;
+      }
+      case SubstInstCallbacks::SubstResult::SubstOperands:
+        break;
+    }
+
     // Extract the operands of this item into the worklist. Note that this
     // modifies the worklist, so it's not safe to use `item` after
     // `ExpandOperands` returns.
@@ -342,6 +375,7 @@ auto SubstInst(Context& context, SemIR::InstId inst_id,
     } else {
       // No need to rebuild this instruction: its operands can't be changed by
       // substitution because it has none.
+      item.inst_id = callbacks.ReuseUnchanged(item.inst_id);
       index = next_index;
     }
   }
@@ -352,7 +386,7 @@ auto SubstInst(Context& context, SemIR::InstId inst_id,
 }
 
 auto SubstInst(Context& context, SemIR::TypeInstId inst_id,
-               const SubstInstCallbacks& callbacks) -> SemIR::TypeInstId {
+               SubstInstCallbacks& callbacks) -> SemIR::TypeInstId {
   return context.types().GetAsTypeInstId(
       SubstInst(context, static_cast<SemIR::InstId>(inst_id), callbacks));
 }
@@ -371,11 +405,11 @@ class SubstConstantCallbacks final : public SubstInstCallbacks {
 
   // Applies the given Substitutions to an instruction, in order to replace
   // BindSymbolicName instructions with the value of the binding.
-  auto Subst(SemIR::InstId& inst_id) const -> bool override {
+  auto Subst(SemIR::InstId& inst_id) -> SubstResult override {
     if (context().constant_values().Get(inst_id).is_concrete()) {
       // This instruction is a concrete constant, so can't contain any
       // bindings that need to be substituted.
-      return true;
+      return SubstResult::FullySubstituted;
     }
 
     auto entity_name_id = SemIR::EntityNameId::None;
@@ -387,7 +421,7 @@ class SubstConstantCallbacks final : public SubstInstCallbacks {
                        inst_id)) {
       entity_name_id = bind->entity_name_id;
     } else {
-      return false;
+      return SubstResult::SubstOperands;
     }
 
     // This is a symbolic binding. Check if we're substituting it.
@@ -398,18 +432,18 @@ class SubstConstantCallbacks final : public SubstInstCallbacks {
           bind_index) {
         // This is the binding we're replacing. Perform substitution.
         inst_id = context().constant_values().GetInstId(replacement_id);
-        return true;
+        return SubstResult::FullySubstituted;
       }
     }
 
     // If it's not being substituted, we still need to look through it, as we
     // may need to substitute into its type (a `FacetType`, with one or more
     // `SpecificInterfaces` within).
-    return false;
+    return SubstResult::SubstOperands;
   }
 
   // Rebuilds an instruction by building a new constant.
-  auto Rebuild(SemIR::InstId /*old_inst_id*/, SemIR::Inst new_inst) const
+  auto Rebuild(SemIR::InstId /*old_inst_id*/, SemIR::Inst new_inst)
       -> SemIR::InstId override {
     return RebuildNewInst(loc_id_, new_inst);
   }
@@ -435,9 +469,9 @@ auto SubstConstant(Context& context, SemIR::LocId loc_id,
     return const_id;
   }
 
-  auto subst_inst_id =
-      SubstInst(context, context.constant_values().GetInstId(const_id),
-                SubstConstantCallbacks(&context, loc_id, substitutions));
+  auto callbacks = SubstConstantCallbacks(&context, loc_id, substitutions);
+  auto subst_inst_id = SubstInst(
+      context, context.constant_values().GetInstId(const_id), callbacks);
   return context.constant_values().Get(subst_inst_id);
 }
 

toolchain/check/subst.h

@@ -18,11 +18,31 @@ class SubstInstCallbacks {
 
   auto context() const -> Context& { return *context_; }
 
+  // How further substitution should or should not be applied to the instruction
+  // after Subst is done.
+  //
+  // Rebuild or ReuseUnchaged will always be called when SubstAgain or
+  // SubstOperands is returned, after processing anything inside the instruction
+  // after Subst.
+  enum SubstResult {
+    // Don't substitute into the operands of the instruction.
+    FullySubstituted,
+    // Attempt to substitute into the operands of the instruction.
+    SubstOperands,
+    // Attempt to substitute again on the resulting instruction, acting like
+    // recursion on the instruction itself.
+    SubstAgain,
+  };
+
   // Performs any needed substitution into an instruction. The instruction ID
   // should be updated as necessary to represent the new instruction. Returns
-  // true if the resulting instruction ID is fully-substituted, or false if
-  // substitution may be needed into operands of the instruction.
-  virtual auto Subst(SemIR::InstId& inst_id) const -> bool = 0;
+  // FullySubstituted if the resulting instruction ID is fully-substituted.
+  // Return SubstOperands if substitution may be needed into operands of the
+  // instruction, or SubstAgain if the replaced instruction itself should have
+  // substitution applied to it again. When SubstOperands or SubstAgain is
+  // returned, it results in a call back to Rebuild or ReuseUnchanged when that
+  // instruction is done being substituted.
+  virtual auto Subst(SemIR::InstId& inst_id) -> SubstResult = 0;
 
   // Rebuilds the type of an instruction from the substituted type instruction.
   // By default this builds the unattached type described by the given type ID.
@@ -33,15 +53,14 @@ class SubstInstCallbacks {
   // `orig_inst_id` is the instruction prior to substitution, and `new_inst` is
   // the substituted instruction. Returns the new instruction ID to use to refer
   // to `new_inst`.
-  virtual auto Rebuild(SemIR::InstId orig_inst_id, SemIR::Inst new_inst) const
+  virtual auto Rebuild(SemIR::InstId orig_inst_id, SemIR::Inst new_inst)
       -> SemIR::InstId = 0;
 
   // Performs any work needed when no substitutions were performed into an
   // instruction for which `Subst` returned `false`. Provides an opportunity to
   // perform any necessary updates to the instruction beyond updating its
   // operands. Returns the new instruction ID to use to refer to `orig_inst_id`.
-  virtual auto ReuseUnchanged(SemIR::InstId orig_inst_id) const
-      -> SemIR::InstId {
+  virtual auto ReuseUnchanged(SemIR::InstId orig_inst_id) -> SemIR::InstId {
     return orig_inst_id;
   }
 
@@ -63,9 +82,9 @@ class SubstInstCallbacks {
 // which are substituted recursively, and if any of them change then `Rebuild`
 // is used to build a new instruction with the substituted operands.
 auto SubstInst(Context& context, SemIR::InstId inst_id,
-               const SubstInstCallbacks& callbacks) -> SemIR::InstId;
+               SubstInstCallbacks& callbacks) -> SemIR::InstId;
 auto SubstInst(Context& context, SemIR::TypeInstId inst_id,
-               const SubstInstCallbacks& callbacks) -> SemIR::TypeInstId;
+               SubstInstCallbacks& callbacks) -> SemIR::TypeInstId;
 
 // A substitution that is being performed.
 struct Substitution {

toolchain/check/testdata/facet/facet_assoc_const.carbon

@@ -206,6 +206,20 @@ fn G(T:! M where .X = () and .Y = ()) {
   F(T);
 }
 
+// --- repeated_concrete_value_and_associated.carbon
+library "[[@TEST_NAME]]";
+
+interface M { let X:! type; let Y:! type; }
+
+fn F1(T:! M where .X = () and .Y = .X and .X = .Y) {}
+
+fn F2(T:! M where .X = () and .X = .X) {}
+
+fn G(T:! M where .X = () and .Y = ()) {
+  F1(T);
+  F2(T);
+}
+
 // --- repeated_with_bitand.carbon
 library "[[@TEST_NAME]]";
 
@@ -235,12 +249,23 @@ library "[[@TEST_NAME]]";
 
 interface M { let X:! type; }
 
+// This fails because it resolves to `.X = .X` which is cyclical.
+//
 // CHECK:STDERR: fail_cycle_single.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(M.X)` [FacetTypeConstraintCycle]
 // CHECK:STDERR: fn F(T:! M where .X = .X) {}
 // CHECK:STDERR:          ^~~~~~~~~~~~~~~
 // CHECK:STDERR:
 fn F(T:! M where .X = .X) {}
 
+// Even though `.X = ()` is specified, the rewrites are resolved left to right
+// and a cycle `.X = .X` is found first.
+//
+// CHECK:STDERR: fail_cycle_single.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(M.X)` [FacetTypeConstraintCycle]
+// CHECK:STDERR: fn G(T:! M where .X = .X and .X = ()) {}
+// CHECK:STDERR:          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+fn G(T:! M where .X = .X and .X = ()) {}
+
 // --- fail_cycle.carbon
 library "[[@TEST_NAME]]";
 
@@ -270,7 +295,7 @@ interface J {
 
 // This fails because it resolves to `.X1 = .X1` which is cyclical.
 //
-// CHECK:STDERR: fail_cycle_between_interfaces.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(I.X2)` [FacetTypeConstraintCycle]
+// CHECK:STDERR: fail_cycle_between_interfaces.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(I.X1)` [FacetTypeConstraintCycle]
 // CHECK:STDERR: fn G(T:! I & J where .X1 = .X3 and .X2 = .X1 and .X3 = .X2) {}
 // CHECK:STDERR:          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
@@ -321,6 +346,78 @@ class C(T:! type, U:! type);
 // CHECK:STDERR:
 fn F(T:! I where .X1 = C(.X2, .X3) and .X2 = C(.X3, .X1));
 
+// --- exponential_large.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let T0:! type;
+  let T1:! type;
+  let T2:! type;
+  let T3:! type;
+  let T4:! type;
+  let T5:! type;
+  let T6:! type;
+  let T7:! type;
+  let T8:! type;
+  let T9:! type;
+}
+
+// A naive attempt to resolve the rewrite rules will run take minutes to
+// complete, since the resulting RHS values are exponential in size, and a naive
+// approach can recursively rebuild the RHS values from the ground up
+// repeatedly.
+fn F(
+    T:! Z where
+      .T0 = (.T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1) and
+      .T1 = (.T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2) and
+      .T2 = (.T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3) and
+      .T3 = (.T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4) and
+      .T4 = (.T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5) and
+      .T5 = (.T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6) and
+      .T6 = (.T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7) and
+      .T7 = (.T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8) and
+      .T8 = (.T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9) and
+      .T9 = ()
+    );
+
+// --- fail_exponential_large_cycle.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let T0:! type;
+  let T1:! type;
+  let T2:! type;
+  let T3:! type;
+  let T4:! type;
+  let T5:! type;
+  let T6:! type;
+  let T7:! type;
+  let T8:! type;
+  let T9:! type;
+}
+
+// A naive attempt to resolve the rewrite rules will run take minutes to
+// complete, since the resulting RHS values are exponential in size, and a naive
+// approach can recursively rebuild the RHS values from the ground up
+// repeatedly.
+fn F(
+    // CHECK:STDERR: fail_exponential_large_cycle.carbon:[[@LINE+4]]:9: error: found cycle in facet type constraint for `.(Z.T9)` [FacetTypeConstraintCycle]
+    // CHECK:STDERR:     T:! Z where
+    // CHECK:STDERR:         ^~~~~~~
+    // CHECK:STDERR:
+    T:! Z where
+      .T9 = .T0 and
+      .T8 = (.T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9, .T9) and
+      .T7 = (.T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8, .T8) and
+      .T6 = (.T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7, .T7) and
+      .T5 = (.T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6, .T6) and
+      .T4 = (.T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5, .T5) and
+      .T3 = (.T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4, .T4) and
+      .T2 = (.T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3, .T3) and
+      .T1 = (.T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2, .T2) and
+      .T0 = (.T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1, .T1)
+    );
+
 // --- non-type.carbon
 library "[[@TEST_NAME]]";
 
@@ -426,6 +523,84 @@ fn F(A:! Z where .T = .V.U and .V = .Self and .U = ()) -> A.T {
   return ();
 }
 
+// --- fail_cycle_with_unrelated_associated_constant.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let T0:! type;
+  let T1:! type;
+  let T2:! type;
+  let T3:! type;
+}
+
+// CHECK:STDERR: fail_cycle_with_unrelated_associated_constant.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(Z.T0)` [FacetTypeConstraintCycle]
+// CHECK:STDERR: fn F(T:! Z where .T0 = .T1 and .T1 = .T0 and .T2 = .T3) {}
+// CHECK:STDERR:          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+fn F(T:! Z where .T0 = .T1 and .T1 = .T0 and .T2 = .T3) {}
+
+// --- fail_cycle_with_branching_in_rhs.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let T0:! type;
+  let T1:! type;
+  let T2:! type;
+  let T3:! type;
+  let T4:! type;
+}
+
+// TODO: There should only be one diagnostic here.
+//
+// CHECK:STDERR: fail_cycle_with_branching_in_rhs.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(Z.T1)` [FacetTypeConstraintCycle]
+// CHECK:STDERR: fn F(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T3 = .T1) {}
+// CHECK:STDERR:          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+fn F(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T3 = .T1) {}
+
+// CHECK:STDERR: fail_cycle_with_branching_in_rhs.carbon:[[@LINE+4]]:10: error: found cycle in facet type constraint for `.(Z.T0)` [FacetTypeConstraintCycle]
+// CHECK:STDERR: fn G(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T3 = .T0) {}
+// CHECK:STDERR:          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+fn G(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T3 = .T0) {}
+
+// --- no_cycle_with_branching_in_rhs.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let T0:! type;
+  let T1:! type;
+  let T2:! type;
+  let T3:! type;
+  let T4:! type;
+  let T5:! type;
+}
+
+// These create misdiagnostics if the resolving algorithms messes up tracking
+// its stack during replacements by leaving either of .T2 or .T3 on the stack
+// (from the RHS of .T1) while resolving the other. Or it can fail to apply the
+// () up the chain correctly.
+
+fn F(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T4 = () and .T3 = .T2) -> T.T0 {
+  return ((), ());
+}
+
+fn G(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T4 = .T5 and .T5 = () and .T3 = .T2) -> T.T0 {
+  return ((), ());
+}
+
+fn H(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = (.T4, ()) and .T3 = .T2 and .T4 = {}) -> T.T0 {
+  return (({}, ()), ({}, ()));
+}
+
+fn I(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T3 = .T2 and .T4 = ()) -> T.T0 {
+  return ((), ());
+}
+
+fn J(T:! Z where .T0 = .T1 and .T1 = (.T2, .T3) and .T2 = .T4 and .T4 = .T5 and .T3 = .T2 and .T5 = ()) -> T.T0 {
+  return ((), ());
+}
+
 // CHECK:STDOUT: --- fail_cycle.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {

toolchain/check/testdata/impl/impl_assoc_const.carbon

@@ -204,6 +204,15 @@ interface M { let X:! type; let Y:! type; }
 
 impl () as M where .X = .Y and .X = .Y and .Y = () {}
 
+// --- repeated_concrete_value_and_associated.carbon
+library "[[@TEST_NAME]]";
+
+interface M { let X:! type; let Y:! type; }
+
+impl () as M where .X = () and .Y = .X and .X = .Y {}
+
+impl {} as M where .X = () and .X = .X and .Y = () {}
+
 // --- fail_repeated_and_different.carbon
 library "[[@TEST_NAME]]";
 
@@ -228,6 +237,15 @@ interface M { let X:! type; }
 // CHECK:STDERR:
 impl () as M where .X = .X {}
 
+// Even though `.X = ()` is specified, the rewrites are resolved left to right
+// and a cycle `.X = .X` is found first.
+//
+// CHECK:STDERR: fail_cycle_single.carbon:[[@LINE+4]]:12: error: found cycle in facet type constraint for `.(M.X)` [FacetTypeConstraintCycle]
+// CHECK:STDERR: impl () as M where .X = .X and .X = () {}
+// CHECK:STDERR:            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+impl () as M where .X = .X and .X = () {}
+
 // --- fail_cycle.carbon
 library "[[@TEST_NAME]]";