Repository: carbon-language/carbon-lang
File: toolchain/check/facet_type.cpp (C++)

Code before:

  auto Subst(SemIR::InstId& rhs_inst_id) const -> bool override {
    if (context().constant_values().Get(rhs_inst_id).is_concrete()) {
      return true;
    }

    auto rhs_access =
        TryGetImplWitnessAccessOfPeriodSelf(context(), rhs_inst_id);
    if (!rhs_access) {
      return false;
    }

    // TODO: We could consider replacing this loop, which makes for an O(N^2)
    // algorithm with something more efficient for searching, 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 : facet_type().rewrite_constraints) {
      auto search_lhs_access = TryGetImplWitnessAccessOfPeriodSelf(
          context(), search_constraint.lhs_id);
      if (!search_lhs_access) {
        continue;
      }

      if (search_lhs_access->witness_id == rhs_access->witness_id &&
          search_lhs_access->index == rhs_access->index) {
        if (IsCurrentConstraint(search_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,
                                     current_constraint_->lhs_id);
            rhs_inst_id = SemIR::ErrorInst::InstId;
          }
        } else {
          rhs_inst_id = search_constraint.rhs_id;
        }
        return true;
      }
    }

    return false;
  }

  auto Rebuild(SemIR::InstId /*orig_inst_id*/, SemIR::Inst new_inst) const
      -> SemIR::InstId override {

Review comment (suggestion):
I think this would actually be a little clearer without the indirection of a function call.

```suggestion
        if (&search_constraint == current_constraint_) {
```

Code after:
      const SemIR::FacetTypeInfo* facet_type_info,
      const SemIR::FacetTypeInfo::RewriteConstraint* substituting_constraint)
      : SubstInstCallbacks(context),
        loc_id_(loc_id),
        facet_type_info_(*facet_type_info),
        substituting_constraint_(substituting_constraint) {}

  auto Subst(SemIR::InstId& rhs_inst_id) const -> bool override {
    if (context().constant_values().Get(rhs_inst_id).is_concrete()) {
      return true;
    }

    auto rhs_access =
        TryGetImplWitnessAccessOfPeriodSelf(context(), rhs_inst_id);
    if (!rhs_access) {
      return false;
    }

    // TODO: We could consider replacing this loop, which makes for an O(N^2)
    // algorithm with something more efficient for searching, 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 : facet_type_info_.rewrite_constraints) {
      auto search_lhs_access = TryGetImplWitnessAccessOfPeriodSelf(
          context(), search_constraint.lhs_id);
      if (!search_lhs_access) {
        continue;
      }

      if (search_lhs_access->witness_id == rhs_access->witness_id &&
          search_lhs_access->index == rhs_access->index) {
        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;
        }
        return true;
      }
    }