Make the GetCanonicalFacetOrTypeValue operation more crisp (#6157)

Previously it performed two kinds of operations, with a boolean
parameter to control whether it would unwrap FacetValue or not. This
made the function hard to explain as "canonicalization".

Now the contract of GetCanonicalFacetOrTypeValue is as follows:
1. For a facet value expression, it returns the canonical value of the
facet value.
2. For a `<facet value> as type` it returns the canonical value of the
`<facet value>`.
3. For other type expressions, it returns the canonical value of the
type.

1 and 2 together collapse together two representations of a facet value
(as a FacetType or as a TypeType) into a single canonical value, which
is important for constant comparison of facet values where the `as type`
is not meant to change the result. This is the case in impl lookups and
`.Self` comparisons.

The step of unwrapping `FacetValue` is only useful in the constant
evaluation of `LookupImplWitness` and is used to collapse *symbolic*
queries on `FacetValue(T)` and on `T` down to a single canonical value,
since they produce the same result later when `T` is replaced with a
facet value or type that can provide a concrete witness. This is now
extensively documented in the constant evaluation of
`LookupImplWitness`.

This change came out of a request/discussion in #6115 (see comment
#6115 (comment)).

Author: danakj

toolchain/check/check_unit.cpp

@@ -524,10 +524,9 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
   auto poisoned_queries =
       std::exchange(context_.poisoned_concrete_impl_lookup_queries(), {});
   for (const auto& poison : poisoned_queries) {
-    auto witness_result =
-        EvalLookupSingleImplWitness(context_, poison.loc_id, poison.query,
-                                    poison.non_canonical_query_self_inst_id,
-                                    /*poison_concrete_results=*/false);
+    auto witness_result = EvalLookupSingleImplWitness(
+        context_, poison.loc_id, poison.query, poison.query.query_self_inst_id,
+        /*poison_concrete_results=*/false);
     CARBON_CHECK(witness_result.has_concrete_value());
     auto found_witness_id = witness_result.concrete_witness();
     if (found_witness_id != poison.impl_witness) {

toolchain/check/context.h

@@ -216,13 +216,14 @@ class Context {
     return impl_lookup_stack_;
   }
 
-  // A concrete impl lookup query and its result.
+  // An impl lookup query that resulted in a concrete witness from finding an
+  // `impl` declaration (not though a facet value), and its result. Used to look
+  // for conflicting `impl` declarations.
   struct PoisonedConcreteImplLookupQuery {
     // The location the LookupImplWitness originated from.
     SemIR::LocId loc_id;
     // The query for a witness of an impl for an interface.
     SemIR::LookupImplWitness query;
-    SemIR::InstId non_canonical_query_self_inst_id;
     // The resulting ImplWitness.
     SemIR::InstId impl_witness;
   };

toolchain/check/convert.cpp

@@ -828,29 +828,21 @@ static auto CanRemoveQualifiers(SemIR::TypeQualifiers quals,
 static auto DiagnoseConversionFailureToConstraintValue(
     Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
     SemIR::TypeId target_type_id) -> void {
-  CARBON_DCHECK(target_type_id == SemIR::TypeType::TypeId ||
-                context.types().Is<SemIR::FacetType>(target_type_id));
-
-  auto type_of_expr_id = context.insts().Get(expr_id).type_id();
-  CARBON_CHECK(context.types().IsFacetType(type_of_expr_id));
-  // If the source type is/has a facet value, then we can include its
-  // FacetType in the diagnostic to help explain what interfaces the
-  // source type implements.
-  auto facet_value_inst_id = SemIR::InstId::None;
-  if (auto facet_access_type =
-          context.insts().TryGetAs<SemIR::FacetAccessType>(expr_id)) {
-    facet_value_inst_id = facet_access_type->facet_value_inst_id;
-  } else if (context.types().Is<SemIR::FacetType>(type_of_expr_id)) {
-    facet_value_inst_id = expr_id;
-  }
+  CARBON_CHECK(context.types().IsFacetType(target_type_id));
+
+  // If the source type is/has a facet value (converted with `as type` or
+  // otherwise), then we can include its `FacetType` in the diagnostic to help
+  // explain what interfaces the source type implements.
+  auto const_expr_id = GetCanonicalFacetOrTypeValue(context, expr_id);
+  auto const_expr_type_id = context.insts().Get(const_expr_id).type_id();
 
-  if (facet_value_inst_id.has_value()) {
+  if (context.types().Is<SemIR::FacetType>(const_expr_type_id)) {
     CARBON_DIAGNOSTIC(ConversionFailureFacetToFacet, Error,
                       "cannot convert type {0} that implements {1} into type "
                       "implementing {2}",
-                      InstIdAsType, TypeOfInstId, SemIR::TypeId);
+                      InstIdAsType, SemIR::TypeId, SemIR::TypeId);
     context.emitter().Emit(loc_id, ConversionFailureFacetToFacet, expr_id,
-                           facet_value_inst_id, target_type_id);
+                           const_expr_type_id, target_type_id);
   } else {
     CARBON_DIAGNOSTIC(ConversionFailureTypeToFacet, Error,
                       "cannot convert type {0} into type implementing {1}",

toolchain/check/eval.cpp

@@ -2173,14 +2173,13 @@ auto TryEvalTypedInst<SemIR::BindSymbolicName>(EvalContext& eval_context,
 
 // Returns whether `const_id` is the same constant facet value as
 // `facet_value_inst_id`.
+//
+// Compares with the canonical facet value of `const_id`, dropping any `as type`
+// conversions.
 static auto IsSameFacetValue(Context& context, SemIR::ConstantId const_id,
                              SemIR::InstId facet_value_inst_id) -> bool {
-  if (auto facet_access_type = context.insts().TryGetAs<SemIR::FacetAccessType>(
-          context.constant_values().GetInstId(const_id))) {
-    const_id =
-        context.constant_values().Get(facet_access_type->facet_value_inst_id);
-  }
-  return const_id == context.constant_values().Get(facet_value_inst_id);
+  auto canon_const_id = GetCanonicalFacetOrTypeValue(context, const_id);
+  return canon_const_id == context.constant_values().Get(facet_value_inst_id);
 }
 
 // TODO: Convert this to an EvalConstantInst function. This will require

toolchain/check/eval_inst.cpp

@@ -241,27 +241,48 @@ auto EvalConstantInst(Context& /*context*/, SemIR::FunctionDecl inst)
 auto EvalConstantInst(Context& context, SemIR::InstId inst_id,
                       SemIR::LookupImplWitness inst) -> ConstantEvalResult {
   // The self value is canonicalized in order to produce a canonical
-  // LookupImplWitness instruction. We save the non-canonical instruction as it
-  // may be a concrete `FacetValue` that contains a concrete witness.
-  auto non_canonical_query_self_inst_id = inst.query_self_inst_id;
-  inst.query_self_inst_id =
-      GetCanonicalizedFacetOrTypeValue(context, inst.query_self_inst_id);
-
-  auto result = EvalLookupSingleImplWitness(
-      context, SemIR::LocId(inst_id), inst, non_canonical_query_self_inst_id,
-      /*poison_concrete_results=*/true);
+  // LookupImplWitness instruction, avoiding multiple constant values for
+  // `<facet value>` and `<facet value>` as type, which always have the same
+  // lookup result.
+  auto self_facet_value_inst_id =
+      GetCanonicalFacetOrTypeValue(context, inst.query_self_inst_id);
+
+  // When we look for a witness in the (facet) type of self, we may get a
+  // concrete witness from a `FacetValue` (which is `self_facet_value_inst_id`)
+  // in which case this instruction evaluates to that witness.
+  //
+  // If we only get a symbolic witness result though, then this instruction
+  // evaluates to a `LookupImplWitness`. Since there was no concrete result in
+  // the `FacetValue`, we don't need to preserve it. By looking through the
+  // `FacetValue` at the type value it wraps to generate a more canonical value
+  // for a symbolic `LookupImplWitness`. This makes us produce the same constant
+  // value for symbolic lookups in `FacetValue(T)` and `T`, since they will
+  // always have the same lookup result later, when `T` is replaced in a
+  // specific by something that can provide a concrete witness.
+  if (auto facet_value = context.insts().TryGetAs<SemIR::FacetValue>(
+          self_facet_value_inst_id)) {
+    inst.query_self_inst_id =
+        GetCanonicalFacetOrTypeValue(context, facet_value->type_inst_id);
+  } else {
+    inst.query_self_inst_id = self_facet_value_inst_id;
+  }
+
+  auto result = EvalLookupSingleImplWitness(context, SemIR::LocId(inst_id),
+                                            inst, self_facet_value_inst_id,
+                                            /*poison_concrete_results=*/true);
   if (!result.has_value()) {
     // We use NotConstant to communicate back to impl lookup that the lookup
     // failed. This can not happen for a deferred symbolic lookup in a generic
     // eval block, since we only add the deferred lookup instruction (being
     // evaluated here) to the SemIR if the lookup succeeds.
     return ConstantEvalResult::NotConstant;
   }
-  if (!result.has_concrete_value()) {
-    return ConstantEvalResult::NewSamePhase(inst);
+  if (result.has_concrete_value()) {
+    return ConstantEvalResult::Existing(
+        context.constant_values().Get(result.concrete_witness()));
   }
-  return ConstantEvalResult::Existing(
-      context.constant_values().Get(result.concrete_witness()));
+
+  return ConstantEvalResult::NewSamePhase(inst);
 }
 
 auto EvalConstantInst(Context& context, SemIR::InstId inst_id,

toolchain/check/impl_lookup.cpp

@@ -239,7 +239,7 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
   // to the facet value here, and if the query was a FacetAccessType we did the
   // same there so they still match.
   deduced_self_const_id =
-      GetCanonicalizedFacetOrTypeValue(context, deduced_self_const_id);
+      GetCanonicalFacetOrTypeValue(context, deduced_self_const_id);
   if (query_self_const_id != deduced_self_const_id) {
     return EvalImplLookupResult::MakeNone();
   }
@@ -299,34 +299,15 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
   }
 }
 
-// Unwraps a FacetAccessType to move from a value of type `TypeType` to a facet
-// value of type `FacetType` if possible.
-//
-// Generally `GetCanonicalizedFacetOrTypeValue()` is what you want to call
-// instead, as this only does part of that operation, potentially returning a
-// non-canonical facet value.
-static auto UnwrapFacetAccessType(Context& context, SemIR::InstId inst_id)
-    -> SemIR::InstId {
-  if (auto access = context.insts().TryGetAs<SemIR::FacetAccessType>(inst_id)) {
-    return access->facet_value_inst_id;
-  }
-  return inst_id;
-}
-
 // Finds a lookup result from `query_self_inst_id` if it is a facet value that
 // names the query interface in its facet type. Note that `query_self_inst_id`
 // is allowed to be a non-canonical facet value in order to find a concrete
 // witness, so it's not referenced as a constant value.
 static auto LookupImplWitnessInSelfFacetValue(
-    Context& context, SemIR::InstId query_self_inst_id,
+    Context& context, SemIR::InstId self_facet_value_inst_id,
     SemIR::SpecificInterface query_specific_interface) -> EvalImplLookupResult {
-  // Unwrap FacetAccessType without getting the canonical facet value from the
-  // self value, as we want to preserve the non-canonical `FacetValue`
-  // instruction which can contain the concrete witness.
-  query_self_inst_id = UnwrapFacetAccessType(context, query_self_inst_id);
-
   auto facet_type = context.types().TryGetAs<SemIR::FacetType>(
-      context.insts().Get(query_self_inst_id).type_id());
+      context.insts().Get(self_facet_value_inst_id).type_id());
   if (!facet_type) {
     return EvalImplLookupResult::MakeNone();
   }
@@ -346,8 +327,8 @@ static auto LookupImplWitnessInSelfFacetValue(
   }
   auto index = (*it).index();
 
-  if (auto facet_value =
-          context.insts().TryGetAs<SemIR::FacetValue>(query_self_inst_id)) {
+  if (auto facet_value = context.insts().TryGetAs<SemIR::FacetValue>(
+          self_facet_value_inst_id)) {
     auto witness_id =
         context.inst_blocks().Get(facet_value->witnesses_block_id)[index];
     if (context.insts().Is<SemIR::ImplWitness>(witness_id)) {
@@ -546,7 +527,7 @@ static auto GetOrAddLookupImplWitness(Context& context, SemIR::LocId loc_id,
 static auto TypeCanDestroy(Context& context,
                            SemIR::ConstantId query_self_const_id) -> bool {
   auto inst = context.insts().Get(context.constant_values().GetInstId(
-      GetCanonicalizedFacetOrTypeValue(context, query_self_const_id)));
+      GetCanonicalFacetOrTypeValue(context, query_self_const_id)));
 
   // For facet values, look if the FacetType provides the same.
   if (auto facet_type =
@@ -865,14 +846,14 @@ static auto CollectCandidateImplsForQuery(
 
 auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
                                  SemIR::LookupImplWitness eval_query,
-                                 SemIR::InstId non_canonical_query_self_inst_id,
+                                 SemIR::InstId self_facet_value_inst_id,
                                  bool poison_concrete_results)
     -> EvalImplLookupResult {
   auto query_specific_interface =
       context.specific_interfaces().Get(eval_query.query_specific_interface_id);
 
   auto facet_lookup_result = LookupImplWitnessInSelfFacetValue(
-      context, non_canonical_query_self_inst_id, query_specific_interface);
+      context, self_facet_value_inst_id, query_specific_interface);
   if (facet_lookup_result.has_concrete_value()) {
     return facet_lookup_result;
   }
@@ -955,8 +936,6 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
         context.poisoned_concrete_impl_lookup_queries().push_back(
             {.loc_id = loc_id,
              .query = eval_query,
-             .non_canonical_query_self_inst_id =
-                 non_canonical_query_self_inst_id,
              .impl_witness = result.concrete_witness()});
       }
       return result;

toolchain/check/impl_lookup.h

@@ -104,7 +104,7 @@ class [[nodiscard]] EvalImplLookupResult {
 // have found that and not caused us to defer lookup to here.
 auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
                                  SemIR::LookupImplWitness eval_query,
-                                 SemIR::InstId non_canonical_query_self_inst_id,
+                                 SemIR::InstId self_facet_value_inst_id,
                                  bool poison_concrete_results)
     -> EvalImplLookupResult;
 

toolchain/check/type.cpp

@@ -238,32 +238,29 @@ auto GetUnboundElementType(Context& context, SemIR::TypeInstId class_type_id,
                                                 element_type_id);
 }
 
-auto GetCanonicalizedFacetOrTypeValue(Context& context, SemIR::InstId inst_id)
+auto GetCanonicalFacetOrTypeValue(Context& context, SemIR::InstId inst_id)
     -> SemIR::InstId {
-  // We can have FacetAccessType of a FacetValue, and a FacetValue of a
-  // FacetAccessType, but they don't nest indefinitely.
-  if (auto access = context.insts().TryGetAs<SemIR::FacetAccessType>(inst_id)) {
-    inst_id = access->facet_value_inst_id;
+  if (inst_id == SemIR::ErrorInst::InstId) {
+    return inst_id;
   }
-  if (auto value = context.insts().TryGetAs<SemIR::FacetValue>(inst_id)) {
-    inst_id = value->type_inst_id;
 
-    if (auto access =
-            context.insts().TryGetAs<SemIR::FacetAccessType>(inst_id)) {
-      inst_id = access->facet_value_inst_id;
-    }
-  }
+  CARBON_CHECK(
+      context.types().IsFacetType(context.insts().Get(inst_id).type_id()),
+      "{0}", context.insts().Get(inst_id).type_id());
 
-  CARBON_CHECK(!context.insts().Is<SemIR::FacetAccessType>(inst_id));
-  CARBON_CHECK(!context.insts().Is<SemIR::FacetValue>(inst_id));
+  auto const_inst_id = context.constant_values().GetConstantInstId(inst_id);
+
+  if (auto access =
+          context.insts().TryGetAs<SemIR::FacetAccessType>(const_inst_id)) {
+    return access->facet_value_inst_id;
+  }
 
-  return context.constant_values().GetConstantInstId(inst_id);
+  return const_inst_id;
 }
 
-auto GetCanonicalizedFacetOrTypeValue(Context& context,
-                                      SemIR::ConstantId const_id)
+auto GetCanonicalFacetOrTypeValue(Context& context, SemIR::ConstantId const_id)
     -> SemIR::ConstantId {
-  return context.constant_values().Get(GetCanonicalizedFacetOrTypeValue(
+  return context.constant_values().Get(GetCanonicalFacetOrTypeValue(
       context, context.constant_values().GetInstId(const_id)));
 }
 

toolchain/check/type.h

@@ -107,18 +107,26 @@ auto GetPatternType(Context& context, SemIR::TypeId scrutinee_type_id)
 auto GetUnboundElementType(Context& context, SemIR::TypeInstId class_type_id,
                            SemIR::TypeInstId element_type_id) -> SemIR::TypeId;
 
-// Convert a facet value or type value instruction to a canonical facet or type
-// value instruction.
+// Given a facet value or a type value, get the canonical facet value if
+// possible, or return the canonical value of the input type expression if it
+// has no canonical facet value.
 //
-// Type values are already canonical and are returned unchanged, except for
-// `FacetAccessType` which is unwrapped to find the facet value it refers to.
+// A facet value can be appear in two ways: as a facet value of type
+// `FacetType`, or through an `as type` conversion which has type `TypeType` but
+// still refers to the original facet value. While both have canonical values of
+// their own, in cases that want to work with the facet value when possible,
+// this collapses the two cases back together by undoing the `as type`
+// conversion.
 //
-// For facet values, unwraps `FacetValue` instructions to get to an underlying
-// canonical type instruction.
-auto GetCanonicalizedFacetOrTypeValue(Context& context, SemIR::InstId inst_id)
+// This extra canonicalization step is important for constant comparison of
+// facet values, when the `as type` conversion is not required to compare as a
+// different value.
+//
+// For type expressions other than `<facet value> as type`, the canonical type
+// value is returned.
+auto GetCanonicalFacetOrTypeValue(Context& context, SemIR::InstId inst_id)
     -> SemIR::InstId;
-auto GetCanonicalizedFacetOrTypeValue(Context& context,
-                                      SemIR::ConstantId const_id)
+auto GetCanonicalFacetOrTypeValue(Context& context, SemIR::ConstantId const_id)
     -> SemIR::ConstantId;
 
 }  // namespace Carbon::Check