From 09a5fddff892d0eb7151ae16775504de8a814d8d Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Fri, 13 Jun 2025 14:01:39 -0400 Subject: [PATCH] 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 --- toolchain/check/facet_type.cpp | 35 +++++++++++++------ .../testdata/facet/facet_assoc_const.carbon | 2 +- .../testdata/facet/nested_facet_types.carbon | 16 +++++++++ 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/toolchain/check/facet_type.cpp b/toolchain/check/facet_type.cpp index 08b40ffc0289..c6d0b333605d 100644 --- a/toolchain/check/facet_type.cpp +++ b/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; diff --git a/toolchain/check/testdata/facet/facet_assoc_const.carbon b/toolchain/check/testdata/facet/facet_assoc_const.carbon index 484ec821f41a..387241dd0d70 100644 --- a/toolchain/check/testdata/facet/facet_assoc_const.carbon +++ b/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: diff --git a/toolchain/check/testdata/facet/nested_facet_types.carbon b/toolchain/check/testdata/facet/nested_facet_types.carbon index 9f3adf92752f..e648f0119aa4 100644 --- a/toolchain/check/testdata/facet/nested_facet_types.carbon +++ b/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 (); +}