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

Code before:
    }

    // This loop moves `i` to the last position with the same LHS value, so that
    // we don't diagnose more than once within the same contiguous range of
    // assignments to a single LHS value.
    for (; i < rewrites.size() - 1; ++i) {
      auto& next = rewrites[i + 1];
      auto next_lhs_access =
          context.insts().TryGetAs<SemIR::ImplWitnessAccess>(next.lhs_id);
      if (!next_lhs_access) {
        break;
      }

      if (CompareFacetTypeConstraintValues(context, constraint.lhs_id,
                                           next.lhs_id) !=
          std::weak_ordering::equivalent) {
        break;
      }

      if (constraint.rhs_id != SemIR::ErrorInst::InstId &&
          next.rhs_id != SemIR::ErrorInst::InstId) {
        CARBON_DIAGNOSTIC(
            AssociatedConstantWithDifferentValues, Error,
            "associated constant {0} given two different values {1} and {2}",
            InstIdAsConstant, InstIdAsConstant, 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.
        auto source_order1 = constraint.rhs_id.index < next.rhs_id.index
                                 ? constraint.rhs_id
                                 : next.rhs_id;
        auto source_order2 = constraint.rhs_id.index >= next.rhs_id.index
                                 ? constraint.rhs_id
                                 : next.rhs_id;
        context.emitter().Emit(loc_id, AssociatedConstantWithDifferentValues,
                               constraint.lhs_id, source_order1, source_order2);
      }
      constraint.rhs_id = SemIR::ErrorInst::InstId;
      next.rhs_id = SemIR::ErrorInst::InstId;
    }
  }

  while (true) {
    bool applied_rewrite = false;

    for (auto& constraint : rewrites) {
      if (constraint.lhs_id == SemIR::ErrorInst::InstId ||
          constraint.rhs_id == SemIR::ErrorInst::InstId) {
        continue;
      }

Review comment (suggestion):
Maybe add a comment about why the reordering here? Also that's it's a loose approximation (I bet there are things we could manufacture out-of-order; canonical would be to get the absolute location, but not sure it's worthwhile)

Code after:
    // This loop moves `i` to the last position with the same LHS value, so that
    // we don't diagnose more than once within the same contiguous range of
    // assignments to a single LHS value.
    for (; i < rewrites.size() - 1; ++i) {
      auto& next = rewrites[i + 1];
      auto next_lhs_access =
          context.insts().TryGetAs<SemIR::ImplWitnessAccess>(next.lhs_id);
      if (!next_lhs_access) {
        break;
      }

      if (CompareFacetTypeConstraintValues(context, constraint.lhs_id,
                                           next.lhs_id) !=
          std::weak_ordering::equivalent) {
        break;
      }

      if (constraint.rhs_id != SemIR::ErrorInst::InstId &&
          next.rhs_id != SemIR::ErrorInst::InstId) {
        CARBON_DIAGNOSTIC(
            AssociatedConstantWithDifferentValues, Error,
            "associated constant {0} given two different values {1} and {2}",
            InstIdAsConstant, InstIdAsConstant, InstIdAsConstant);
        // Use inst id ordering as a simple proxy for source ordering, to try
        // to name the values in the same order they appear in the facet type.
        auto source_order1 = constraint.rhs_id.index < next.rhs_id.index
                                 ? constraint.rhs_id
                                 : next.rhs_id;
        auto source_order2 = constraint.rhs_id.index >= next.rhs_id.index
                                 ? constraint.rhs_id
                                 : next.rhs_id;
        // 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.
        context.emitter().Emit(loc_id, AssociatedConstantWithDifferentValues,
                               constraint.lhs_id, source_order1, source_order2);
      }
      constraint.rhs_id = SemIR::ErrorInst::InstId;
      next.rhs_id = SemIR::ErrorInst::InstId;
    }
  }

  while (true) {
    bool applied_rewrite = false;

    for (auto& constraint : rewrites) {
      if (constraint.lhs_id == SemIR::ErrorInst::InstId ||
          constraint.rhs_id == SemIR::ErrorInst::InstId) {
        continue;
      }