Look for final impl when accessing associated constant in facet (#5269)

While facets may come with a rewrite for an associated constant, they
are symbolic. A final impl has the ability to provide a concrete value
instead, which allows generic code to use the concrete value in place of
the associated constant's (fully qualified) name.

For instance, instead of `I.Type`, the concrete type `()` can be used if
there is an `impl final [T:! type] T as I where .Type = ()` impl.

This does not yet cache the result of the lookups.

Depends on https://github.com/carbon-language/carbon-lang/pull/5255

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

toolchain/check/BUILD

@@ -29,6 +29,7 @@ cc_library(
         "function.cpp",
         "generic.cpp",
         "global_init.cpp",
+        "impl.cpp",
         "impl_lookup.cpp",
         "import.cpp",
         "import_cpp.cpp",
@@ -68,6 +69,7 @@ cc_library(
         "function.h",
         "generic.h",
         "global_init.h",
+        "impl.h",
         "impl_lookup.h",
         "import.h",
         "import_cpp.h",
@@ -166,7 +168,6 @@ cc_library(
         ":context",
         ":diagnostic_emitter",
         ":dump",
-        ":impl",
         ":pointer_dereference",
         "//common:check",
         "//common:error",
@@ -223,21 +224,6 @@ cc_library(
     ],
 )
 
-cc_library(
-    name = "impl",
-    srcs = ["impl.cpp"],
-    hdrs = ["impl.h"],
-    deps = [
-        ":context",
-        "//common:check",
-        "//toolchain/base:kind_switch",
-        "//toolchain/diagnostics:diagnostic_emitter",
-        "//toolchain/sem_ir:file",
-        "//toolchain/sem_ir:inst",
-        "//toolchain/sem_ir:typed_insts",
-    ],
-)
-
 cc_library(
     name = "node_stack",
     srcs = ["node_stack.cpp"],

toolchain/check/handle_impl.cpp

@@ -364,11 +364,12 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
 
   // Process modifiers.
   // TODO: Should we somehow permit access specifiers on `impl`s?
-  // TODO: Handle `final` modifier.
   auto introducer =
       context.decl_introducer_state_stack().Pop<Lex::TokenKind::Impl>();
   LimitModifiersOnDecl(context, introducer, KeywordModifierSet::ImplDecl);
 
+  bool is_final = introducer.modifier_set.HasAnyOf(KeywordModifierSet::Final);
+
   // Finish processing the name, which should be empty, but might have
   // parameters.
   auto name_context = context.decl_name_stack().FinishImplName();
@@ -387,7 +388,8 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
                            {.self_id = self_inst_id,
                             .constraint_id = constraint_inst_id,
                             .interface = CheckConstraintIsInterface(
-                                context, impl_decl_id, constraint_inst_id)}};
+                                context, impl_decl_id, constraint_inst_id),
+                            .is_final = is_final}};
   // Add the impl declaration.
   bool invalid_redeclaration = false;
   auto lookup_bucket_ref = context.impls().GetOrAddLookupBucket(impl_info);

toolchain/check/impl.cpp

@@ -249,4 +249,10 @@ auto FillImplWitnessWithErrors(Context& context, SemIR::Impl& impl) -> void {
   }
 }
 
+auto IsImplEffectivelyFinal(Context& context, const SemIR::Impl& impl) -> bool {
+  return impl.is_final ||
+         (context.constant_values().Get(impl.self_id).is_concrete() &&
+          context.constant_values().Get(impl.constraint_id).is_concrete());
+}
+
 }  // namespace Carbon::Check

toolchain/check/impl.h

@@ -26,6 +26,10 @@ auto FinishImplWitness(Context& context, SemIR::Impl& impl) -> void;
 // Sets all unset members of the witness for `impl` to the error instruction.
 auto FillImplWitnessWithErrors(Context& context, SemIR::Impl& impl) -> void;
 
+// Returns whether the impl is either `final` explicitly, or implicitly due to
+// being concrete.
+auto IsImplEffectivelyFinal(Context& context, const SemIR::Impl& impl) -> bool;
+
 }  // namespace Carbon::Check
 
 #endif  // CARBON_TOOLCHAIN_CHECK_IMPL_H_

toolchain/check/impl_lookup.cpp

@@ -12,6 +12,7 @@
 #include "toolchain/check/diagnostic_helpers.h"
 #include "toolchain/check/eval.h"
 #include "toolchain/check/generic.h"
+#include "toolchain/check/impl.h"
 #include "toolchain/check/import_ref.h"
 #include "toolchain/check/inst.h"
 #include "toolchain/check/type.h"
@@ -220,6 +221,7 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
     // DeduceImplArguments can import new impls which can invalidate any
     // pointers into `context.impls()`.
     const SemIR::Impl& impl = context.impls().Get(impl_id);
+
     if (impl.generic_id.has_value()) {
       specific_id =
           DeduceImplArguments(context, loc_id,
@@ -290,11 +292,12 @@ static auto GetWitnessIdForImpl(Context& context, SemIR::LocId loc_id,
     ResolveSpecificDefinition(context, loc_id, specific_id);
   }
 
-  bool impl_is_effectively_final =
-      // TODO: impl.is_final ||
-      (context.constant_values().Get(impl.self_id).is_concrete() &&
-       context.constant_values().Get(impl.constraint_id).is_concrete());
-  if (query_is_concrete || impl_is_effectively_final) {
+  if (query_is_concrete || IsImplEffectivelyFinal(context, impl)) {
+    // TODO: These final results should be cached somehow. Positive (non-None)
+    // results could be cached globally, as they can not change. But
+    // negative results can change after a final impl is written, so
+    // they can only be cached in a limited way, or the cache needs to
+    // be invalidated by writing a final impl that would match.
     return EvalImplLookupResult::MakeFinal(
         context.constant_values().GetInstId(SemIR::GetConstantValueInSpecific(
             context.sem_ir(), specific_id, impl.witness_id)));
@@ -340,9 +343,9 @@ static auto FindWitnessInFacet(
 // if not, it will evaluate to itself as a symbolic witness to be further
 // evaluated with a more specific query when building a specific for the generic
 // context the query came from.
-static auto FindWitnessInImpls(Context& context, SemIR::LocId loc_id,
-                               SemIR::ConstantId query_self_const_id,
-                               SemIR::SpecificInterface interface)
+static auto GetOrAddLookupImplWitness(Context& context, SemIR::LocId loc_id,
+                                      SemIR::ConstantId query_self_const_id,
+                                      SemIR::SpecificInterface interface)
     -> SemIR::InstId {
   auto witness_const_id = EvalOrAddInst(
       context, loc_id.ToImplicit(),
@@ -434,9 +437,12 @@ auto LookupImplWitness(Context& context, SemIR::LocId loc_id,
     // do an O(N+M) merge instead of O(N*M) nested loops.
     auto result_witness_id =
         FindWitnessInFacet(context, loc_id, query_self_const_id, interface);
+    // TODO: If the impl lookup finds a final impl, it should take precedence
+    // over the witness from the facet value. See the test:
+    // fail_todo_final_impl_precidence_over_facet_value.carbon.
     if (!result_witness_id.has_value()) {
-      result_witness_id =
-          FindWitnessInImpls(context, loc_id, query_self_const_id, interface);
+      result_witness_id = GetOrAddLookupImplWitness(
+          context, loc_id, query_self_const_id, interface);
     }
     if (result_witness_id.has_value()) {
       result_witness_ids.push_back(result_witness_id);
@@ -491,11 +497,16 @@ struct CandidateImpl {
 
 // Returns the list of candidates impls for lookup to select from.
 static auto CollectCandidateImplsForQuery(
-    Context& context, const TypeStructure& query_type_structure,
+    Context& context, bool final_only,
+    const TypeStructure& query_type_structure,
     SemIR::SpecificInterface& query_specific_interface)
     -> llvm::SmallVector<CandidateImpl> {
   llvm::SmallVector<CandidateImpl> candidate_impls;
   for (auto [id, impl] : context.impls().enumerate()) {
+    if (final_only && !IsImplEffectivelyFinal(context, impl)) {
+      continue;
+    }
+
     // If the impl's interface_id differs from the query, then this impl can
     // not possibly provide the queried interface.
     if (impl.interface.interface_id != query_specific_interface.interface_id) {
@@ -593,7 +604,8 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
       QueryIsConcrete(context, query_self_const_id, query_specific_interface);
 
   auto candidate_impls = CollectCandidateImplsForQuery(
-      context, query_type_structure, query_specific_interface);
+      context, /*final_only=*/false, query_type_structure,
+      query_specific_interface);
 
   for (const auto& candidate : candidate_impls) {
     // In deferred lookup for a symbolic impl witness, while building a
@@ -619,4 +631,48 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
   return EvalImplLookupResult::MakeNone();
 }
 
+auto LookupFinalImplWitnessForSpecificInterface(
+    Context& context, SemIR::LocId loc_id,
+    SemIR::ConstantId query_self_const_id,
+    SemIR::SpecificInterface query_specific_interface) -> SemIR::InstId {
+  // This would mean we need to UnwrapFacetAccessType(query_self_const_id), but
+  // it's already done by member access, which is the one use of this function.
+  CARBON_DCHECK(!context.insts().Is<SemIR::FacetAccessType>(
+      context.constant_values().GetInstId(query_self_const_id)));
+
+  auto query_type_structure = BuildTypeStructure(
+      context, context.constant_values().GetInstId(query_self_const_id),
+      query_specific_interface);
+  bool query_is_concrete =
+      QueryIsConcrete(context, query_self_const_id, query_specific_interface);
+
+  auto candidate_impls = CollectCandidateImplsForQuery(
+      context, /*final_only=*/true, query_type_structure,
+      query_specific_interface);
+
+  for (const auto& candidate : candidate_impls) {
+    // In deferred lookup for a symbolic impl witness, while building a
+    // specific, there may be no stack yet as this may be the first lookup. If
+    // further lookups are started as a result in deduce, they will build the
+    // stack.
+    //
+    // NOTE: Don't retain a reference into the stack, it may be invalidated if
+    // we do further impl lookups when GetWitnessIdForImpl() does deduction.
+    if (!context.impl_lookup_stack().empty()) {
+      context.impl_lookup_stack().back().impl_loc = candidate.loc_inst_id;
+    }
+
+    // NOTE: GetWitnessIdForImpl() does deduction, which can cause new impls
+    // to be imported, invalidating any pointer into `context.impls()`.
+    auto result = GetWitnessIdForImpl(
+        context, loc_id, query_is_concrete, query_self_const_id,
+        query_specific_interface, candidate.impl_id);
+    if (result.has_value()) {
+      CARBON_CHECK(result.has_concrete_value());
+      return result.concrete_witness();
+    }
+  }
+  return SemIR::InstId::None;
+}
+
 }  // namespace Carbon::Check

toolchain/check/impl_lookup.h

@@ -97,6 +97,18 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
                                  SemIR::LookupImplWitness eval_query)
     -> EvalImplLookupResult;
 
+// Looks for a witness of a _final_ impl declaration. Since only final impls are
+// returned, it always returns a concrete ImplWitness or None, it will never
+// return a symbolic LookupImplWitness instruction.
+//
+// Generally prefer to call LookupImplWitness(). This method is used to look for
+// a final specialization in order to get concrete associated constants in
+// generic contexts.
+auto LookupFinalImplWitnessForSpecificInterface(
+    Context& context, SemIR::LocId loc_id,
+    SemIR::ConstantId query_self_const_id,
+    SemIR::SpecificInterface query_specific_interface) -> SemIR::InstId;
+
 }  // namespace Carbon::Check
 
 #endif  // CARBON_TOOLCHAIN_CHECK_IMPL_LOOKUP_H_

toolchain/check/member_access.cpp

@@ -176,6 +176,27 @@ static auto AccessMemberOfImplWitness(Context& context, SemIR::LocId loc_id,
                                                  .index = assoc_entity->index});
 }
 
+// For an impl lookup query with a single interface in it, we can convert the
+// result to a single witness InstId.
+//
+// This CHECKs that the result (and thus the query) was a single interface. This
+// generally only makes sense in member access, where the lookup query's
+// interface is found through name lookup, and we don't have an arbitrary
+// `FacetType`.
+static auto GetWitnessFromSingleImplLookupResult(
+    Context& context, SemIR::InstBlockIdOrError lookup_result)
+    -> SemIR::InstId {
+  auto witness_id = SemIR::InstId::None;
+  if (lookup_result.has_error_value()) {
+    witness_id = SemIR::ErrorInst::SingletonInstId;
+  } else {
+    auto witnesses = context.inst_blocks().Get(lookup_result.inst_block_id());
+    CARBON_CHECK(witnesses.size() == 1);
+    witness_id = witnesses[0];
+  }
+  return witness_id;
+}
+
 // Performs impl lookup for a member name expression. This finds the relevant
 // impl witness and extracts the corresponding impl member.
 static auto PerformImplLookup(
@@ -210,18 +231,8 @@ static auto PerformImplLookup(
     return SemIR::ErrorInst::SingletonInstId;
   }
 
-  // The query facet type given to `LookupImplWitness()` had only a single
-  // interface in it, so the returned witness set will have the same. Convert
-  // from the InstBlockId to the single ImplWitness instruction.
-  auto witness_id = SemIR::InstId::None;
-  if (lookup_result.has_error_value()) {
-    witness_id = SemIR::ErrorInst::SingletonInstId;
-  } else {
-    auto witnesses = context.inst_blocks().Get(lookup_result.inst_block_id());
-    CARBON_CHECK(witnesses.size() == 1);
-    witness_id = witnesses[0];
-  }
-
+  auto witness_id =
+      GetWitnessFromSingleImplLookupResult(context, lookup_result);
   return AccessMemberOfImplWitness(context, loc_id, self_type_id, witness_id,
                                    assoc_type.interface_specific_id, member_id);
 }
@@ -303,26 +314,59 @@ static auto LookupMemberNameInScope(Context& context, SemIR::LocId loc_id,
 
         auto assoc_interface = assoc_type->GetSpecificInterface();
 
-        // First look for `assoc_interface` in the type of the base. If it is
-        // found, get the witness that the interface is implemented from
-        // `base_id`.
-        auto identified_id = RequireIdentifiedFacetType(context, *facet_type);
-        const auto& identified =
-            context.identified_facet_types().Get(identified_id);
         // Witness that `T` implements the `assoc_interface`.
         SemIR::InstId witness_inst_id = SemIR::InstId::None;
-        for (auto [index, base_interface] :
-             llvm::enumerate(identified.required_interfaces())) {
-          // Get the witness that `T` implements `base_type_id`.
-          if (base_interface == assoc_interface) {
-            witness_inst_id = GetOrAddInst(
-                context, loc_id,
-                SemIR::FacetAccessWitness{
-                    .type_id = GetSingletonType(
-                        context, SemIR::WitnessType::SingletonInstId),
-                    .facet_value_inst_id = base_id,
-                    .index = SemIR::ElementIndex(index)});
-            break;
+
+        bool is_lookup_in_period_self = false;
+        if (auto name = context.insts().TryGetAs<SemIR::NameRef>(base_id)) {
+          if (name->name_id == SemIR::NameId::PeriodSelf) {
+            is_lookup_in_period_self = true;
+          }
+        }
+
+        // TODO: In `.Self` we want to find the witness through its FacetType,
+        // which is the code below this block. Instead of special-casing that
+        // here, we could have the impl lookup also include witnesses
+        // (FacetAccessWitness) from the FacetType? And even non-final results.
+        // Then we just call into lookup once here, for `.Self` or otherwise,
+        // and can drop the construction of FacetAccessWitness from this
+        // function, and resolve TODO below that for "associated entity not
+        // found in facet type".
+        if (!is_lookup_in_period_self) {
+          // For an associated constant value, we need to do impl lookup to try
+          // find a final impl declaration. If we find one, we can use the value
+          // assigned to the constant there, instead of its symbolic value.
+          auto assoc_entity = context.insts().GetAs<SemIR::AssociatedEntity>(
+              context.constant_values().GetConstantInstId(
+                  result.scope_result.target_inst_id()));
+          if (context.insts().Is<SemIR::AssociatedConstantDecl>(
+                  assoc_entity.decl_id)) {
+            witness_inst_id = LookupFinalImplWitnessForSpecificInterface(
+                context, loc_id, context.constant_values().Get(base_id),
+                assoc_interface);
+          }
+        }
+
+        if (!witness_inst_id.has_value()) {
+          // First look for `assoc_interface` in the type of the base. If it is
+          // found, get the witness that the interface is implemented from
+          // `base_id`.
+          auto identified_id = RequireIdentifiedFacetType(context, *facet_type);
+          const auto& identified =
+              context.identified_facet_types().Get(identified_id);
+          for (auto [index, base_interface] :
+               llvm::enumerate(identified.required_interfaces())) {
+            // Get the witness that `T` implements `base_type_id`.
+            if (base_interface == assoc_interface) {
+              witness_inst_id = GetOrAddInst(
+                  context, loc_id,
+                  SemIR::FacetAccessWitness{
+                      .type_id = GetSingletonType(
+                          context, SemIR::WitnessType::SingletonInstId),
+                      .facet_value_inst_id = base_id,
+                      .index = SemIR::ElementIndex(index)});
+              break;
+            }
           }
         }
         // TODO: If that fails, would need to do impl lookup to see if the facet

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

@@ -2,7 +2,7 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
-// INCLUDE-FILE: toolchain/testing/min_prelude/convert.carbon
+// INCLUDE-FILE: toolchain/testing/min_prelude/facet_types.carbon
 // EXTRA-ARGS: --no-dump-sem-ir --custom-core
 //
 // AUTOUPDATE
@@ -11,7 +11,7 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/lookup/min_prelude/find_in_final.carbon
 
-// --- fail_todo_keep_looking_for_final_impl.carbon
+// --- final_impl_precedence_over_facet.carbon
 library "[[@TEST_NAME]]";
 interface I {
   let T:! type;
@@ -20,18 +20,171 @@ interface I {
 final impl forall [U:! type] U as I where .T = () {}
 
 fn F(V:! I) -> V.T {
-  // TODO: Even though we have a witness that `V impls I` from the constraint
-  // on `I`, we should do an impl lookup to see if any effectively final impl
-  // applies when we find an unknown value in that witness. In this case, that
-  // lookup would find an impl with more specific values for associated
-  // constants that we should merge.
-
-  // CHECK:STDERR: fail_todo_keep_looking_for_final_impl.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `()` to `V.(I.T)` [ConversionFailure]
-  // CHECK:STDERR:   return ();
-  // CHECK:STDERR:   ^~~~~~~~~~
-  // CHECK:STDERR: fail_todo_keep_looking_for_final_impl.carbon:[[@LINE+4]]:3: note: type `()` does not implement interface `Core.ImplicitAs(V.(I.T))` [MissingImplInMemberAccessNote]
-  // CHECK:STDERR:   return ();
-  // CHECK:STDERR:   ^~~~~~~~~~
-  // CHECK:STDERR:
+  // Even though we have a witness that `V impls I` from the constraint on `I`,
+  // we should do an impl lookup to see if any effectively final impl applies
+  // when we find an unknown value in that witness. In this case, that lookup
+  // would find an impl with more specific values for associated constants that
+  // we should merge.
   return ();
 }
+
+// --- final_impl_precedence_over_facet_with_where.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let X:! type;
+  let Y:! type;
+}
+
+final impl forall [T:! type] T as Z where .X = () and .Y = () {}
+
+fn F(ZZ:! Z where .X = ()) {
+  // Z.Y is unspecified on `ZZ` so it's found on the final impl where it's known
+  // to be the concrete type (), which can then be used in this generic
+  // function.
+  let a: ZZ.Y = ();
+}
+
+// --- final_impl_precedence_over_facet_access_type_with_where.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let X:! type;
+  let Y:! type;
+}
+
+final impl forall [T:! type] T as Z where .X = () and .Y = () {}
+
+fn F[T:! Z where .X = ()](z: T) {
+  // z.Y is unspecified on `ZZ` so it's found on the final impl where it's known
+  // to be the concrete type (), which can then be used in this generic
+  // function.
+  let a: z.Y = ();
+}
+
+// --- fail_todo_final_impl_makes_compatible_facet_values.carbon
+library "[[@TEST_NAME]]";
+
+interface I {}
+interface J {}
+
+final impl forall [T:! J] T as I {}
+
+class C(T:! I) {}
+
+class D(T:! J) {
+  // Finds the final impl decl; The facet value of `T` in `C(T)` holds an
+  // `ImplWitness`.
+  var c: C(T)*;
+}
+
+fn F(T:! I & J) {
+  // `I` found in the facet type; The facet value of `T` in `C(T)` holds a `FacetAccessWitness`.
+  // TODO: It should use the `final impl` and thus hold the same `ImplWitness`
+  var x: C(T);
+  // `D.c` used the final impl decl to make a facet value with an `ImplWitness`
+  // for its `T` in `C(T)`.
+  var y: D(T);
+  // This converts a pointer to a facet value (should be `ImplWitness` but
+  // isn't) to a pointer to a facet value (with `ImplWitness`).
+  //
+  // TODO: It should type check when they are both the same generic type
+  // `C(T:! I)` defined by the `final impl` of `I`.
+  //
+  // CHECK:STDERR: fail_todo_final_impl_makes_compatible_facet_values.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `C(T as I)*` to `C(T as I)*` [ConversionFailure]
+  // CHECK:STDERR:   y.c = &x;
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR: fail_todo_final_impl_makes_compatible_facet_values.carbon:[[@LINE+4]]:3: note: type `C(T as I)*` does not implement interface `Core.ImplicitAs(C(T as I)*)` [MissingImplInMemberAccessNote]
+  // CHECK:STDERR:   y.c = &x;
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR:
+  y.c = &x;
+}
+
+// --- fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon
+library "[[@TEST_NAME]]";
+
+interface I { let X:! type; }
+interface J {}
+
+impl forall [T:! J] T as I where .X = () {}
+
+class C(T:! I) {
+  var b: T.X;
+}
+
+class D(T:! J) {
+  // Finds the impl decl; The facet value of `T` in `C(T)` holds a
+  // `LookupImplWitness`.
+  var c: C(T)*;
+}
+
+fn F(T:! I & J) -> T.(I.X) {
+  // `I` found in the facet type; The facet value of `T` in `C(T)` holds a
+  // `FacetAccessWitness`.
+  var x: C(T);
+  // `D.c` used the final impl decl to make a facet value with a
+  // `LookupImplWitness` for its `T` in `C(T)`.
+  var y: D(T);
+  // This converts a pointer to a facet value (with `FacetAccessWitness`) to a
+  // pointer to a facet value (with `LookupImplWitness`).
+  //
+  // TODO: It should type check as a `LookupImplWitness` will find the same
+  // witness that `FacetAccessWitness` is accessing from the caller.
+  //
+  // CHECK:STDERR: fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `C(T as I)*` to `C(T as I)*` [ConversionFailure]
+  // CHECK:STDERR:   y.c = &x;
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR: fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon:[[@LINE+4]]:3: note: type `C(T as I)*` does not implement interface `Core.ImplicitAs(C(T as I)*)` [MissingImplInMemberAccessNote]
+  // CHECK:STDERR:   y.c = &x;
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR:
+  y.c = &x;
+  // CHECK:STDERR: fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `(T as J as I).(I.X)` to `T.(TODO: element 0 in <witness for T, interface 0>)` [ConversionFailure]
+  // CHECK:STDERR:   return (*y.c).b;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
+  // CHECK:STDERR: fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon:[[@LINE+4]]:3: note: type `(T as J as I).(I.X)` does not implement interface `Core.ImplicitAs(T.(TODO: element 0 in <witness for T, interface 0>))` [MissingImplInMemberAccessNote]
+  // CHECK:STDERR:   return (*y.c).b;
+  // CHECK:STDERR:   ^~~~~~~~~~~~~~~~
+  // CHECK:STDERR:
+  return (*y.c).b;
+}
+
+class E1 {}
+impl E1 as J {}
+
+fn G1() {
+  // Doesn't see the `final impl` below. `E1.(I.X)` is `()`.
+  let a1: () = F(E1);
+}
+
+final impl forall [T:! J] T as I where .X = {} {}
+
+class E2 {}
+impl E2 as J {}
+
+fn G2() {
+  // TODO: Does see the `final impl` above. `E2.(I.X)` is `{}`.
+  // CHECK:STDERR: fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon:[[@LINE+7]]:16: error: cannot implicitly convert expression of type `()` to `{}` [ConversionFailure]
+  // CHECK:STDERR:   let a2: {} = F(E2);
+  // CHECK:STDERR:                ^~~~~
+  // CHECK:STDERR: fail_todo_convert_facet_value_of_facet_access_witness_to_facet_value_of_lookup_impl_witness.carbon:[[@LINE+4]]:16: note: type `()` does not implement interface `Core.ImplicitAs({})` [MissingImplInMemberAccessNote]
+  // CHECK:STDERR:   let a2: {} = F(E2);
+  // CHECK:STDERR:                ^~~~~
+  // CHECK:STDERR:
+  let a2: {} = F(E2);
+}
+
+// --- todo_fail_facet_value_rewrite_incompatible_with_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {
+  let X:! type;
+}
+
+final impl forall [T:! type] T as Z where .X = () {}
+
+// TODO: This should be diagnosed as there is a final impl defining `.X = ()`,
+// which makes the LHS of this rewrite constraint concrete. And since the RHS is
+// not the same (or convertible from), the rewrite is impossible.
+fn F(ZZ:! Z where .X = {.r: ()}) {}

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

@@ -237,6 +237,7 @@ fn F[D:! Y](d: D) {
   // The FacetValue deduced for the param of `C` will be a symbolic FacetValue
   // because we are in a generic where `D` is an unknown type, which will cause
   // the query and impl self type to be C(FacetValue) for a symbolic FacetValue.
+  //
   // CHECK:STDERR: fail_specialized_class_with_symbolic_facet_value_param.carbon:[[@LINE+7]]:23: error: cannot implicitly convert expression of type `()` to `(C(D) as Z).(Z.X)` [ConversionFailure]
   // CHECK:STDERR:   let a: C(D).(Z.X) = ();
   // CHECK:STDERR:                       ^~
@@ -426,41 +427,7 @@ fn F() {
   let x: {} = G(C);
 }
 
-// --- fail_todo_specialization_of_type_constant_in_generic_context_with_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface I {
-  let T:! type;
-  fn F[self: Self]() -> T;
-}
-
-impl forall [U:! type] U as I where .T = () {
-  fn F[self: Self]() -> () { return (); }
-}
-final impl forall [V:! type] V* as I where .T = V {
-  fn F[self: Self]() -> V { return *self; }
-}
-
-fn H[W:! type](v: W) -> W.(I.T) {
-  return v.(I.F)();
-}
-
-// The return of `H` is `(X*).(I.T)` which has a final impl making it `X`.
-// While this function is still a generic context, it should see the concrete
-// `X` type and the return of `H(p)` should convert (a no-op) to `X`.
-fn G[X:! type](p: X*) -> X {
-  // CHECK:STDERR: fail_todo_specialization_of_type_constant_in_generic_context_with_final_impl.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `(X* as I).(I.T)` to `X` [ConversionFailure]
-  // CHECK:STDERR:   return H(p);
-  // CHECK:STDERR:   ^~~~~~~~~~~~
-  // CHECK:STDERR: fail_todo_specialization_of_type_constant_in_generic_context_with_final_impl.carbon:[[@LINE+4]]:3: note: type `(X* as I).(I.T)` does not implement interface `Core.ImplicitAs(X)` [MissingImplInMemberAccessNote]
-  // CHECK:STDERR:   return H(p);
-  // CHECK:STDERR:   ^~~~~~~~~~~~
-  // CHECK:STDERR:
-  return H(p);
-}
-
 // --- type_structure_first_difference.carbon
-
 library "[[@TEST_NAME]]";
 
 interface Z(T:! type) {

toolchain/sem_ir/impl.h

@@ -39,6 +39,9 @@ struct ImplFields {
   // TODO: Handle control flow in the impl body, such as if-expressions.
   InstBlockId body_block_id = InstBlockId::None;
 
+  // Whether the impl declaration is marked `final`.
+  bool is_final;
+
   // The following members are set at the `}` of the impl definition.
   bool defined = false;
 };