Fix sorting of instructions in Facet Type resolution (#5664)

The sort and dedupe operations in facet type resolution explicitly work
with `ImplWitnessAccess` instructions as being a reference to an
associated constant on some entity. If only one of the instructions is
an `ImplWitnessAccess`, we still want to consider that one as such, not
get its constant value, which may be some concrete type, and use that
for comparison instead.

This makes the new test fail (which we don't want) in a consistent way
with a similar test of TypeAnd (which we also don't want to fail),
making the system more consistent, while leaving some improvements to be
done.

Avoid inconsistent orderings between instructions, by making the
comparison function into a total order. To do so, we sort
ImplWitnessAccess instructions first, and sort them by their InstId.
Non-ImplWitnessAccess instructions come second, and sort them by their
constant InstId. Thanks to jonmeow for figuring out that the function
was not producing a total order and why.

Since this means the order is no longer relative to source order, we
order the two assignments in the diagnostic by source order(ish) by
putting the lower InstId first in the diagnostic output.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/check/facet_type.cpp

@@ -259,8 +259,8 @@ auto AllocateFacetTypeImplWitness(Context& context,
 // 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
-// ordering is somewhat arbitrary but is attempted to be the order they appear
-// in the source.
+// ordering is somewhat arbitrary with `ImplWitnessAccess` instructions coming
+// first.
 static auto CompareFacetTypeConstraintValues(Context& context,
                                              SemIR::InstId lhs_id,
                                              SemIR::InstId rhs_id)
@@ -295,15 +295,23 @@ static auto CompareFacetTypeConstraintValues(Context& context,
                rhs_lookup->query_specific_interface_id.index;
       }
     }
+
+    // We do *not* want to get the evaluated result of `ImplWitnessAccess` here,
+    // we want to keep them as a reference to an associated constant for the
+    // resolution phase.
+    return lhs_id.index <=> rhs_id.index;
   }
 
-  if (context.constant_values().GetConstantInstId(lhs_id) ==
-      context.constant_values().GetConstantInstId(rhs_id)) {
-    return std::weak_ordering::equivalent;
+  // ImplWitnessAccess sorts before other instructions.
+  if (lhs_access) {
+    return std::weak_ordering::less;
   }
-  // Try to return things in the order they appear in the code by using the
-  // non-canonicalized id.
-  return lhs_id.index <=> rhs_id.index;
+  if (rhs_access) {
+    return std::weak_ordering::greater;
+  }
+
+  return context.constant_values().GetConstantInstId(lhs_id).index <=>
+         context.constant_values().GetConstantInstId(rhs_id).index;
 }
 
 // Sort and dedupe the rewrite constraints, with accesses to the same associated
@@ -482,13 +490,20 @@ auto ResolveFacetTypeRewriteConstraints(
             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, constraint.rhs_id,
-                               next.rhs_id);
+                               constraint.lhs_id, source_order1, source_order2);
       }
       constraint.rhs_id = SemIR::ErrorInst::InstId;
       next.rhs_id = SemIR::ErrorInst::InstId;

toolchain/check/testdata/facet/facet_assoc_const.carbon

@@ -178,7 +178,7 @@ library "[[@TEST_NAME]]";
 
 interface M { let X:! type; let Y:! type; }
 
-// CHECK:STDERR: fail_rewrite_conflicts_with_second_facet.carbon:[[@LINE+4]]:31: error: associated constant `.(M.Y)` given two different values `.(M.X)` and `T.(M.X)` [AssociatedConstantWithDifferentValues]
+// CHECK:STDERR: fail_rewrite_conflicts_with_second_facet.carbon:[[@LINE+4]]:31: error: associated constant `.(M.Y)` given two different values `T.(M.X)` and `.(M.X)` [AssociatedConstantWithDifferentValues]
 // CHECK:STDERR: fn F(T:! M where .X = (), U:! M where .Y = T.X and .Y = .X) {}
 // CHECK:STDERR:                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/facet/nested_facet_types.carbon

@@ -165,3 +165,19 @@ interface Z {
 // CHECK:STDERR:           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
 fn F(FF:! (((Z where .V = {}) where .T = .U) where .S = .U) & (Z where .S = {})) {}
+
+// --- fail_todo_repeated_with_value_available.carbon
+library "[[@TEST_NAME]]";
+
+interface Z { let X:! type; let Y:! type; }
+
+// Verify that the `.Y = ()` does not get applied to `.X = .Y` prematurely, as
+// that would lead to `.X = () and .X = .Y` when combined with the outer `where`
+// expression, which would be an error.
+// CHECK:STDERR: fail_todo_repeated_with_value_available.carbon:[[@LINE+4]]:10: error: associated constant `.(Z.X)` given two different values `()` and `.(Z.Y)` [AssociatedConstantWithDifferentValues]
+// CHECK:STDERR: fn F(T:! ((Z where .Y = ()) where .X = .Y) where .X = .Y) -> T.X {
+// CHECK:STDERR:          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+fn F(T:! ((Z where .Y = ()) where .X = .Y) where .X = .Y) -> T.X {
+  return ();
+}