Handle FacetAccessType as the self type in symbolic impl lookups (#5200)

It is possible to construct a symbolic impl lookup query that, when
evaluated against a specific, will have a self type that is:
- A facet value instruction with a symbolic constant value
- That constant value is rewritten to a FacetValue pointing through a
FacetAccessType to a symbolic facet value.

Impl lookup looks through the FacetValue to the type inside since
FacetValue will reduce the number of interfaces available to match the
minimum deduced requirements.

Impl lookup also unwraps FacetAccessType in the self type of the query
and the impl, so that queries on FacetAccessType and on facet values can
both compare against the impl's self type with a simple constant value
equality check.

We were unwrapping FacetAccessType on the way into impl lookup, and then
assumed that meant it would never be a FacetAccessType in the symbolic
impl lookup instruction. However, as we can see, the query self
instruction can be symbolic and its value can be rewritten. And in that
case it can contain or become a FacetAccessType.

So we need to also unwrap the FacetAccessType when doing a symbolic impl
lookup.

Closes #5187

toolchain/check/impl_lookup.cpp

@@ -169,6 +169,21 @@ static auto FindAndDiagnoseImplLookupCycle(
   return false;
 }
 
+// If the constant value is a FacetAccessType instruction, this returns the
+// value of the facet value it points to instead.
+static auto UnwrapFacetAccessType(Context& context, SemIR::ConstantId id)
+    -> SemIR::ConstantId {
+  // If the self type is a FacetAccessType, work with the facet value directly,
+  // which gives us the potential witnesses to avoid looking for impl
+  // declarations. We will do the same for the impl declarations we try to match
+  // so that we can compare the self constant values.
+  if (auto access = context.insts().TryGetAs<SemIR::FacetAccessType>(
+          context.constant_values().GetInstId(id))) {
+    return context.constant_values().Get(access->facet_value_inst_id);
+  }
+  return id;
+}
+
 // Gets the set of `SpecificInterface`s that are required by a facet type
 // (as a constant value).
 static auto GetInterfacesFromConstantId(
@@ -224,11 +239,7 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
   // will not be the same constant value as a query facet value. We move through
   // to the facet value here, and if the query was a FacetAccessType we did the
   // same there so they still match.
-  if (auto access = context.insts().TryGetAs<SemIR::FacetAccessType>(
-          context.constant_values().GetInstId(deduced_self_const_id))) {
-    deduced_self_const_id =
-        context.constant_values().Get(access->facet_value_inst_id);
-  }
+  deduced_self_const_id = UnwrapFacetAccessType(context, deduced_self_const_id);
   if (query_self_const_id != deduced_self_const_id) {
     return EvalImplLookupResult::MakeNone();
   }
@@ -388,11 +399,7 @@ auto LookupImplWitness(Context& context, SemIR::LocId loc_id,
   // which gives us the potential witnesses to avoid looking for impl
   // declarations. We will do the same for the impl declarations we try to match
   // so that we can compare the self constant values.
-  if (auto access = context.insts().TryGetAs<SemIR::FacetAccessType>(
-          context.constant_values().GetInstId(query_self_const_id))) {
-    query_self_const_id =
-        context.constant_values().Get(access->facet_value_inst_id);
-  }
+  query_self_const_id = UnwrapFacetAccessType(context, query_self_const_id);
 
   if (FindAndDiagnoseImplLookupCycle(context, context.impl_lookup_stack(),
                                      loc_id, query_self_const_id,
@@ -575,6 +582,9 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
           context.constant_values().GetInstId(query_self_const_id))) {
     query_self_const_id =
         context.constant_values().Get(facet_value->type_inst_id);
+    // If the FacetValue points to a FacetAccessType, we need to unwrap that for
+    // comparison with the impl's self type.
+    query_self_const_id = UnwrapFacetAccessType(context, query_self_const_id);
   }
 
   auto query_type_structure = BuildTypeStructure(

toolchain/check/testdata/impl/lookup/min_prelude/symbolic_lookup.carbon

@@ -0,0 +1,55 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// INCLUDE-FILE: toolchain/testing/min_prelude/facet_types.carbon
+// EXTRA-ARGS: --no-dump-sem-ir --custom-core
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/impl/lookup/min_prelude/symbolic_lookup.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/lookup/min_prelude/symbolic_lookup.carbon
+
+// --- self_type_facet_value_of_facet_access_type.carbon
+library "[[@TEST_NAME]]";
+
+interface X {}
+interface Y {}
+interface Z {}
+
+impl forall [T:! Y] T as X {}
+impl forall [T:! Z] T as X {}
+
+class C(T:! X) {}
+
+// The `adapt C(T)` line produces a symbolic impl lookup that `T` impls `X`:
+// - At first the impl lookup query in the eval block has self as
+//   `BindSymbolicName(T:! Y)`.
+//
+// The call to construct `D` from in `Test()` makes a specific for `D`:
+// - The value of `T` in `D` is deduced to be a `FacetValue` of type `Y & Z`
+//   pointing to the `BindSymbolicName(T:! Y & Z)` from `Test()`. But
+//   `FacetValue` needs an instruction of type `TypeType` so the
+//   `BindSymbolicName` is wrapped in `FacetAccessType`, meaning it's
+//   `FacetValue(FacetAccessType(BindSymbolicName(T:! Y & Z)))`.
+// - That facet value is written to the specific of `D` constructed from
+//   `Test()`.
+// - The eval block of `D` is run with the above specific. This runs the
+//   symbolic impl lookup that `T` impls `X`. But with the `T` rewritten by the
+//   specific to the `FacetValue(FacetAccessType(BindSymbolicName(T:! Y & Z)))`.
+//
+// The impl lookup unwraps the `FacetValue` to get the underlying type as it may
+// have access to a more constrained FacetType (with access to more interfaces)
+// than the `FacetValue` itself.
+//
+// In this case, the unwrap finds a `FacetAccessType`. Impl lookup must handle
+// the presence or absence of `FacetAccessType` in the query self type
+// consistently, while comparing the self type (after deduction) to the impl's
+// self type. This verifies we handle the case of the query not having a
+// `FacetAccessType` in the initial query that makes the symbolic lookup
+// instruction (`LookupImplWitness`), but then the self type being replaced
+// with a `FacetAccessType` when evaluating the instruction.
+class D(T:! Y) { adapt C(T); }
+
+fn Test(T:! Y & Z, d: D(T)) {}