From da83b65aa28735fe57fed7c9620a5b366e7449e6 Mon Sep 17 00:00:00 2001
From: Dana Jansens <danakj@orodu.net>
Date: Mon, 14 Apr 2025 14:48:55 -0400
Subject: [PATCH] Concrete impl takes precedence over a facet value (#5305)

If a concrete impl is found via lookup, its associated constants should
be used over the constants found through a facet value.
---
 toolchain/check/impl_lookup.cpp               | 24 ++++++++++++-------
 .../lookup/min_prelude/find_in_final.carbon   | 18 +++-----------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/toolchain/check/impl_lookup.cpp b/toolchain/check/impl_lookup.cpp
index 835cbb6833dd..dadc463d71cb 100644
--- a/toolchain/check/impl_lookup.cpp
+++ b/toolchain/check/impl_lookup.cpp
@@ -441,15 +441,23 @@ auto LookupImplWitness(Context& context, SemIR::LocId loc_id,
   for (const auto& interface : interfaces) {
     // TODO: Since both `interfaces` and `query_self_const_id` are sorted lists,
     // 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 = GetOrAddLookupImplWitness(
-          context, loc_id, query_self_const_id, interface);
+
+    auto result_witness_id = GetOrAddLookupImplWitness(
+        context, loc_id, query_self_const_id, interface);
+
+    if (!result_witness_id.has_value() ||
+        !context.insts().Is<SemIR::ImplWitness>(result_witness_id)) {
+      // If the witness is not concrete (the result is not final), then we
+      // prefer the witness from the query's facet value (if it is one), which
+      // may include rewrite rules of associated constants that can be relied
+      // on.
+      auto facet_value_witness_id =
+          FindWitnessInFacet(context, loc_id, query_self_const_id, interface);
+      if (facet_value_witness_id.has_value()) {
+        result_witness_id = facet_value_witness_id;
+      }
     }
+
     if (result_witness_id.has_value()) {
       result_witness_ids.push_back(result_witness_id);
     } else {
diff --git a/toolchain/check/testdata/impl/lookup/min_prelude/find_in_final.carbon b/toolchain/check/testdata/impl/lookup/min_prelude/find_in_final.carbon
index 3433c76bb0f4..94281b0a4fac 100644
--- a/toolchain/check/testdata/impl/lookup/min_prelude/find_in_final.carbon
+++ b/toolchain/check/testdata/impl/lookup/min_prelude/find_in_final.carbon
@@ -62,7 +62,7 @@ fn F[T:! Z where .X = ()](z: T) {
   let a: z.Y = ();
 }
 
-// --- fail_todo_final_impl_makes_compatible_facet_values.carbon
+// --- final_impl_makes_compatible_facet_values.carbon
 library "[[@TEST_NAME]]";
 
 interface I {}
@@ -80,24 +80,12 @@ class D(T:! J) {
 
 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:
+  // This converts a pointer to a type with with facet value (an `ImplWitness`)
+  // to a pointer to a type with a facet value (the same `ImplWitness`).
   y.c = &x;
 }