Implement non-final impl overlap diagnostics (#5412)

When two non-final impls have the same type structure (neither is a
specialization of the other), it is an error unless they are within a
`match_first` block. For now, we don't have `match_first` implemented,
so it's always an error.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

toolchain/check/check_unit.cpp

@@ -23,6 +23,7 @@
 #include "toolchain/check/inst.h"
 #include "toolchain/check/node_id_traversal.h"
 #include "toolchain/check/type.h"
+#include "toolchain/check/type_structure.h"
 #include "toolchain/diagnostics/diagnostic.h"
 #include "toolchain/sem_ir/function.h"
 #include "toolchain/sem_ir/ids.h"
@@ -572,10 +573,81 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
   context_.inst_block_stack().PopAndDiscard();
 }
 
+auto CheckUnit::CheckOverlappingImpls() -> void {
+  // Collect all of the impls sorted into contiguous segments by their
+  // interface. We only need to compare impls within each such segment.
+  llvm::SmallVector<SemIR::Impl> impls_by_interface(
+      context_.impls().array_ref());
+  llvm::sort(
+      impls_by_interface, [](const SemIR::Impl& a, const SemIR::Impl& b) {
+        return a.interface.interface_id.index < b.interface.interface_id.index;
+      });
+
+  const auto* it = impls_by_interface.begin();
+  while (it != impls_by_interface.end()) {
+    const auto* segment_begin = it;
+    do {
+      ++it;
+    } while (it != impls_by_interface.end() &&
+             it->interface.interface_id ==
+                 segment_begin->interface.interface_id);
+    const auto* segment_end = it;
+
+    if (std::distance(segment_begin, segment_end) == 1) {
+      // Only 1 interface in the segment; nothing to overlap with.
+      continue;
+    }
+
+    CheckOverlappingImplsForInterface(
+        llvm::ArrayRef(segment_begin, segment_end));
+  }
+}
+
+auto CheckUnit::CheckOverlappingImplsForInterface(
+    llvm::ArrayRef<SemIR::Impl> impls) -> void {
+  for (auto [index_a, impl_a] : llvm::enumerate(impls)) {
+    if (impl_a.witness_id == SemIR::ErrorInst::InstId) {
+      continue;
+    }
+    auto impl_a_type_structure =
+        BuildTypeStructure(context_, impl_a.self_id, impl_a.interface);
+
+    for (const auto& impl_b : impls.drop_front(index_a + 1)) {
+      if (impl_b.witness_id == SemIR::ErrorInst::InstId) {
+        continue;
+      }
+
+      // The type structure each non-final `impl` must differ from all other
+      // non-final `impl` for the same interface visible from the file.
+      if (!impl_a.is_final && !impl_b.is_final) {
+        auto impl_b_type_structure =
+            BuildTypeStructure(context_, impl_b.self_id, impl_b.interface);
+        if (impl_a_type_structure == impl_b_type_structure) {
+          CARBON_DIAGNOSTIC(ImplFullyOverlapNonFinal, Error,
+                            "found non-final `impl` that fully overlaps "
+                            "previous non-final `impl`");
+          auto builder =
+              emitter_.Build(impl_b.latest_decl_id(), ImplFullyOverlapNonFinal);
+          CARBON_DIAGNOSTIC(ImplFullyOverlapNonFinalNote, Note,
+                            "fully overlaps `impl` here");
+          builder.Note(impl_a.latest_decl_id(), ImplFullyOverlapNonFinalNote);
+          builder.Emit();
+          break;
+        }
+      }
+    }
+
+    // TODO: The self + constraint of a `impl` must not match against (be
+    // fully subsumed by) any final `impl` visible from the file. Do a
+    // final-only query for all non-final impls?
+  }
+}
+
 auto CheckUnit::FinishRun() -> void {
   CheckRequiredDeclarations();
   CheckRequiredDefinitions();
   CheckPoisonedConcreteImplLookupQueries();
+  CheckOverlappingImpls();
 
   // Pop information for the file-level scope.
   context_.sem_ir().set_top_inst_block_id(context_.inst_block_stack().Pop());

toolchain/check/check_unit.h

@@ -169,6 +169,18 @@ class CheckUnit {
   // same witnesses.
   auto CheckPoisonedConcreteImplLookupQueries() -> void;
 
+  // Look for `impl` declarations that overlap in ways that are invalid.
+  //
+  // - The self + constraint of an `impl` must not match against (be fully
+  //   subsumed by) any final `impl` visible from the file.
+  // - The type structure each non-final `impl` must differ from every other
+  //   non-final `impl` for the same interface visible from the file.
+  auto CheckOverlappingImpls() -> void;
+  // Check for invalid overlap between impls, given the set of all impls for a
+  // single interface.
+  auto CheckOverlappingImplsForInterface(llvm::ArrayRef<SemIR::Impl> impls)
+      -> void;
+
   // Does work after processing the parse tree, such as finishing the IR and
   // checking for missing contents.
   auto FinishRun() -> void;

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

@@ -247,7 +247,7 @@ fn F() {
   ({} as Wraps(C)) as (Wraps(C) as ComparableTo(Wraps(D)));
 }
 
-// --- impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon
+// --- fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon
 library "[[@TEST_NAME]]";
 
 // Implement this for a type in one direction.
@@ -263,11 +263,28 @@ class D {}
 // - D is ComparableWith(C).
 impl C as ComparableTo(D) {}
 
+// TODO: These three `impl`s need to be placed in a match_first as they
+// overlap.
+
 // T is always ComparableWith(T).
 impl forall [T:! type] T as ComparableWith(T) {}
 // If U is ComparableTo(T), U is ComparableWith(T).
+// CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE-5]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl forall [T:! type] T as ComparableWith(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {}
 // If U is ComparableTo(T), T is ComparableWith(U).
+// CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl forall [T:! type, U:! ComparableTo(T)] T as ComparableWith(U) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_todo_impl_recurse_with_simpler_type_in_generic_param_bidirectional_no_cycle.carbon:[[@LINE-5]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl forall [T:! type, U:! ComparableTo(T)] U as ComparableWith(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! type, U:! ComparableTo(T)] T as ComparableWith(U) {}
 
 fn Compare[T:! type, U:! ComparableWith(T)](t: T, u: U) {}

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

@@ -214,7 +214,7 @@ final impl forall [T:! type] T as Z(T) {}
 
 final impl forall [T:! Y] T as Z(T) {}
 
-// --- todo_non_final_overlaps_non_final_impl.carbon
+// --- fail_non_final_overlaps_non_final_impl.carbon
 library "[[@TEST_NAME]]";
 
 interface Z(T:! type) {}
@@ -226,5 +226,11 @@ class C(T:! type) {}
 // No diagnosis here as the type structure is unique.
 impl forall [T:! type] T as Z(C(T)) {}
 
-// TODO: Diagnose overlap with first impl (same type structure).
+// CHECK:STDERR: fail_non_final_overlaps_non_final_impl.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_non_final_overlaps_non_final_impl.carbon:[[@LINE-9]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl forall [T:! Y] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! type] T as Z(T) {}

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

@@ -19,7 +19,6 @@ interface Y {}
 interface Z {}
 
 impl forall [T:! Y] T as X {}
-impl forall [T:! Z] T as X {}
 
 class C(T:! X) {}
 

toolchain/check/testdata/impl/min_prelude/generic_redeclaration.carbon

@@ -11,7 +11,7 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/min_prelude/generic_redeclaration.carbon
 
-// --- same_self_and_interface.carbon
+// --- fail_same_self_and_interface.carbon
 
 library "[[@TEST_NAME]]";
 
@@ -30,8 +30,29 @@ impl forall [T:! L] T as Interface;
 // These are different impls, so they are not redefinitions, even though the
 // self type and constraint type are the same.
 impl forall [T:! I] T as Interface {}
+// CHECK:STDERR: fail_same_self_and_interface.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl forall [T:! J] T as Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_same_self_and_interface.carbon:[[@LINE-4]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl forall [T:! I] T as Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! J] T as Interface {}
+// CHECK:STDERR: fail_same_self_and_interface.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl forall [T:! K] T as Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_same_self_and_interface.carbon:[[@LINE-4]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl forall [T:! J] T as Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! K] T as Interface {}
+// CHECK:STDERR: fail_same_self_and_interface.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl forall [T:! L] T as Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_same_self_and_interface.carbon:[[@LINE-4]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl forall [T:! K] T as Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
 impl forall [T:! L] T as Interface {}
 
 // --- fail_same_self_and_interface_redefined.carbon
@@ -51,20 +72,23 @@ impl forall [T:! I] T as J {}
 // CHECK:STDERR:
 impl forall [T:! I] T as J {}
 
-// --- same_type_different_spelling.carbon
+// --- fail_same_type_different_spelling.carbon
 
 library "[[@TEST_NAME]]";
 
 class C;
 interface I {}
 
-// We accept this because these two types are spelled differently, even though
-// they evaluate to the same type constant. Any use of this impl will be
-// ambiguous unless resolved by a `match_first` or similar.
-//
-// TODO: Produce a warning or maybe an error when this happens in a non-generic
-// impl.
+// Since the spelling is different, it's not caught as a redefinition, but it
+// is diagnosed as an overlapping impl without using a `match_first` block.
 impl C as I {}
+// CHECK:STDERR: fail_same_type_different_spelling.carbon:[[@LINE+7]]:1: error: found non-final `impl` that fully overlaps previous non-final `impl` [ImplFullyOverlapNonFinal]
+// CHECK:STDERR: impl (C, C).0 as I {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_same_type_different_spelling.carbon:[[@LINE-4]]:1: note: fully overlaps `impl` here [ImplFullyOverlapNonFinalNote]
+// CHECK:STDERR: impl C as I {}
+// CHECK:STDERR: ^~~~~~~~~~~~~
+// CHECK:STDERR:
 impl (C, C).0 as I {}
 
 // --- fail_redefinition_generic_regions.carbon
@@ -92,7 +116,7 @@ impl forall [T:! type] T as I {
   }
 }
 
-// CHECK:STDOUT: --- same_self_and_interface.carbon
+// CHECK:STDOUT: --- fail_same_self_and_interface.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Interface.type: type = facet_type <@Interface> [concrete]
@@ -205,32 +229,32 @@ impl forall [T:! type] T as I {
 // CHECK:STDOUT:   impl_decl @impl.793 [concrete] {
 // CHECK:STDOUT:     %T.patt: %pattern_type.28e = symbolic_binding_pattern T, 0 [concrete]
 // CHECK:STDOUT:   } {
-// CHECK:STDOUT:     %T.ref.loc19: %J.type = name_ref T, %T.loc19 [symbolic = %T.loc12_14.2 (constants.%T.ccd)]
-// CHECK:STDOUT:     %T.as_type.loc19: type = facet_access_type %T.ref.loc19 [symbolic = %T.as_type.loc12_21.2 (constants.%T.as_type.3df)]
-// CHECK:STDOUT:     %.loc19: type = converted %T.ref.loc19, %T.as_type.loc19 [symbolic = %T.as_type.loc12_21.2 (constants.%T.as_type.3df)]
-// CHECK:STDOUT:     %Interface.ref.loc19: type = name_ref Interface, file.%Interface.decl [concrete = constants.%Interface.type]
-// CHECK:STDOUT:     %J.ref.loc19: type = name_ref J, file.%J.decl [concrete = constants.%J.type]
-// CHECK:STDOUT:     %T.loc19: %J.type = bind_symbolic_name T, 0 [symbolic = %T.loc12_14.2 (constants.%T.ccd)]
+// CHECK:STDOUT:     %T.ref.loc26: %J.type = name_ref T, %T.loc26 [symbolic = %T.loc12_14.2 (constants.%T.ccd)]
+// CHECK:STDOUT:     %T.as_type.loc26: type = facet_access_type %T.ref.loc26 [symbolic = %T.as_type.loc12_21.2 (constants.%T.as_type.3df)]
+// CHECK:STDOUT:     %.loc26: type = converted %T.ref.loc26, %T.as_type.loc26 [symbolic = %T.as_type.loc12_21.2 (constants.%T.as_type.3df)]
+// CHECK:STDOUT:     %Interface.ref.loc26: type = name_ref Interface, file.%Interface.decl [concrete = constants.%Interface.type]
+// CHECK:STDOUT:     %J.ref.loc26: type = name_ref J, file.%J.decl [concrete = constants.%J.type]
+// CHECK:STDOUT:     %T.loc26: %J.type = bind_symbolic_name T, 0 [symbolic = %T.loc12_14.2 (constants.%T.ccd)]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   impl_decl @impl.c93 [concrete] {
 // CHECK:STDOUT:     %T.patt: %pattern_type.4a6 = symbolic_binding_pattern T, 0 [concrete]
 // CHECK:STDOUT:   } {
-// CHECK:STDOUT:     %T.ref.loc20: %K.type = name_ref T, %T.loc20 [symbolic = %T.loc13_14.2 (constants.%T.09f)]
-// CHECK:STDOUT:     %T.as_type.loc20: type = facet_access_type %T.ref.loc20 [symbolic = %T.as_type.loc13_21.2 (constants.%T.as_type.037)]
-// CHECK:STDOUT:     %.loc20: type = converted %T.ref.loc20, %T.as_type.loc20 [symbolic = %T.as_type.loc13_21.2 (constants.%T.as_type.037)]
-// CHECK:STDOUT:     %Interface.ref.loc20: type = name_ref Interface, file.%Interface.decl [concrete = constants.%Interface.type]
-// CHECK:STDOUT:     %K.ref.loc20: type = name_ref K, file.%K.decl [concrete = constants.%K.type]
-// CHECK:STDOUT:     %T.loc20: %K.type = bind_symbolic_name T, 0 [symbolic = %T.loc13_14.2 (constants.%T.09f)]
+// CHECK:STDOUT:     %T.ref.loc34: %K.type = name_ref T, %T.loc34 [symbolic = %T.loc13_14.2 (constants.%T.09f)]
+// CHECK:STDOUT:     %T.as_type.loc34: type = facet_access_type %T.ref.loc34 [symbolic = %T.as_type.loc13_21.2 (constants.%T.as_type.037)]
+// CHECK:STDOUT:     %.loc34: type = converted %T.ref.loc34, %T.as_type.loc34 [symbolic = %T.as_type.loc13_21.2 (constants.%T.as_type.037)]
+// CHECK:STDOUT:     %Interface.ref.loc34: type = name_ref Interface, file.%Interface.decl [concrete = constants.%Interface.type]
+// CHECK:STDOUT:     %K.ref.loc34: type = name_ref K, file.%K.decl [concrete = constants.%K.type]
+// CHECK:STDOUT:     %T.loc34: %K.type = bind_symbolic_name T, 0 [symbolic = %T.loc13_14.2 (constants.%T.09f)]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   impl_decl @impl.9e6 [concrete] {
 // CHECK:STDOUT:     %T.patt: %pattern_type.86b = symbolic_binding_pattern T, 0 [concrete]
 // CHECK:STDOUT:   } {
-// CHECK:STDOUT:     %T.ref.loc21: %L.type = name_ref T, %T.loc21 [symbolic = %T.loc14_14.2 (constants.%T.1d2)]
-// CHECK:STDOUT:     %T.as_type.loc21: type = facet_access_type %T.ref.loc21 [symbolic = %T.as_type.loc14_21.2 (constants.%T.as_type.0ed)]
-// CHECK:STDOUT:     %.loc21: type = converted %T.ref.loc21, %T.as_type.loc21 [symbolic = %T.as_type.loc14_21.2 (constants.%T.as_type.0ed)]
-// CHECK:STDOUT:     %Interface.ref.loc21: type = name_ref Interface, file.%Interface.decl [concrete = constants.%Interface.type]
-// CHECK:STDOUT:     %L.ref.loc21: type = name_ref L, file.%L.decl [concrete = constants.%L.type]
-// CHECK:STDOUT:     %T.loc21: %L.type = bind_symbolic_name T, 0 [symbolic = %T.loc14_14.2 (constants.%T.1d2)]
+// CHECK:STDOUT:     %T.ref.loc42: %L.type = name_ref T, %T.loc42 [symbolic = %T.loc14_14.2 (constants.%T.1d2)]
+// CHECK:STDOUT:     %T.as_type.loc42: type = facet_access_type %T.ref.loc42 [symbolic = %T.as_type.loc14_21.2 (constants.%T.as_type.0ed)]
+// CHECK:STDOUT:     %.loc42: type = converted %T.ref.loc42, %T.as_type.loc42 [symbolic = %T.as_type.loc14_21.2 (constants.%T.as_type.0ed)]
+// CHECK:STDOUT:     %Interface.ref.loc42: type = name_ref Interface, file.%Interface.decl [concrete = constants.%Interface.type]
+// CHECK:STDOUT:     %L.ref.loc42: type = name_ref L, file.%L.decl [concrete = constants.%L.type]
+// CHECK:STDOUT:     %T.loc42: %L.type = bind_symbolic_name T, 0 [symbolic = %T.loc14_14.2 (constants.%T.1d2)]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -454,17 +478,17 @@ impl forall [T:! type] T as I {
 // CHECK:STDOUT:   %T.as_type.loc15_21.2 => constants.%T.as_type
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: --- same_type_different_spelling.carbon
+// CHECK:STDOUT: --- fail_same_type_different_spelling.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %C: type = class_type @C [concrete]
 // CHECK:STDOUT:   %I.type: type = facet_type <@I> [concrete]
 // CHECK:STDOUT:   %Self: %I.type = bind_symbolic_name Self, 0 [symbolic]
-// CHECK:STDOUT:   %I.impl_witness.215123.1: <witness> = impl_witness file.%I.impl_witness_table.loc13 [concrete]
+// CHECK:STDOUT:   %I.impl_witness.215123.1: <witness> = impl_witness file.%I.impl_witness_table.loc9 [concrete]
 // CHECK:STDOUT:   %tuple.type: type = tuple_type (type, type) [concrete]
 // CHECK:STDOUT:   %int_0: Core.IntLiteral = int_value 0 [concrete]
 // CHECK:STDOUT:   %tuple: %tuple.type = tuple_value (%C, %C) [concrete]
-// CHECK:STDOUT:   %I.impl_witness.215123.2: <witness> = impl_witness file.%I.impl_witness_table.loc14 [concrete]
+// CHECK:STDOUT:   %I.impl_witness.215123.2: <witness> = impl_witness file.%I.impl_witness_table.loc17 [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {
@@ -486,20 +510,20 @@ impl forall [T:! type] T as I {
 // CHECK:STDOUT:     %C.ref: type = name_ref C, file.%C.decl [concrete = constants.%C]
 // CHECK:STDOUT:     %I.ref: type = name_ref I, file.%I.decl [concrete = constants.%I.type]
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %I.impl_witness_table.loc13 = impl_witness_table (), @impl.7704ae.1 [concrete]
-// CHECK:STDOUT:   %I.impl_witness.loc13: <witness> = impl_witness %I.impl_witness_table.loc13 [concrete = constants.%I.impl_witness.215123.1]
+// CHECK:STDOUT:   %I.impl_witness_table.loc9 = impl_witness_table (), @impl.7704ae.1 [concrete]
+// CHECK:STDOUT:   %I.impl_witness.loc9: <witness> = impl_witness %I.impl_witness_table.loc9 [concrete = constants.%I.impl_witness.215123.1]
 // CHECK:STDOUT:   impl_decl @impl.7704ae.2 [concrete] {} {
-// CHECK:STDOUT:     %C.ref.loc14_7: type = name_ref C, file.%C.decl [concrete = constants.%C]
-// CHECK:STDOUT:     %C.ref.loc14_10: type = name_ref C, file.%C.decl [concrete = constants.%C]
-// CHECK:STDOUT:     %.loc14_11.1: %tuple.type = tuple_literal (%C.ref.loc14_7, %C.ref.loc14_10)
+// CHECK:STDOUT:     %C.ref.loc17_7: type = name_ref C, file.%C.decl [concrete = constants.%C]
+// CHECK:STDOUT:     %C.ref.loc17_10: type = name_ref C, file.%C.decl [concrete = constants.%C]
+// CHECK:STDOUT:     %.loc17_11.1: %tuple.type = tuple_literal (%C.ref.loc17_7, %C.ref.loc17_10)
 // CHECK:STDOUT:     %int_0: Core.IntLiteral = int_value 0 [concrete = constants.%int_0]
-// CHECK:STDOUT:     %tuple: %tuple.type = tuple_value (%C.ref.loc14_7, %C.ref.loc14_10) [concrete = constants.%tuple]
-// CHECK:STDOUT:     %.loc14_11.2: %tuple.type = converted %.loc14_11.1, %tuple [concrete = constants.%tuple]
-// CHECK:STDOUT:     %tuple.elem0: type = tuple_access %.loc14_11.2, element0 [concrete = constants.%C]
+// CHECK:STDOUT:     %tuple: %tuple.type = tuple_value (%C.ref.loc17_7, %C.ref.loc17_10) [concrete = constants.%tuple]
+// CHECK:STDOUT:     %.loc17_11.2: %tuple.type = converted %.loc17_11.1, %tuple [concrete = constants.%tuple]
+// CHECK:STDOUT:     %tuple.elem0: type = tuple_access %.loc17_11.2, element0 [concrete = constants.%C]
 // CHECK:STDOUT:     %I.ref: type = name_ref I, file.%I.decl [concrete = constants.%I.type]
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %I.impl_witness_table.loc14 = impl_witness_table (), @impl.7704ae.2 [concrete]
-// CHECK:STDOUT:   %I.impl_witness.loc14: <witness> = impl_witness %I.impl_witness_table.loc14 [concrete = constants.%I.impl_witness.215123.2]
+// CHECK:STDOUT:   %I.impl_witness_table.loc17 = impl_witness_table (), @impl.7704ae.2 [concrete]
+// CHECK:STDOUT:   %I.impl_witness.loc17: <witness> = impl_witness %I.impl_witness_table.loc17 [concrete = constants.%I.impl_witness.215123.2]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: interface @I {
@@ -512,12 +536,12 @@ impl forall [T:! type] T as I {
 // CHECK:STDOUT:
 // CHECK:STDOUT: impl @impl.7704ae.1: %C.ref as %I.ref {
 // CHECK:STDOUT: !members:
-// CHECK:STDOUT:   witness = file.%I.impl_witness.loc13
+// CHECK:STDOUT:   witness = file.%I.impl_witness.loc9
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: impl @impl.7704ae.2: %tuple.elem0 as %I.ref {
 // CHECK:STDOUT: !members:
-// CHECK:STDOUT:   witness = file.%I.impl_witness.loc14
+// CHECK:STDOUT:   witness = file.%I.impl_witness.loc17
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @C;

toolchain/check/type_structure.cpp

@@ -4,6 +4,7 @@
 
 #include "toolchain/check/type_structure.h"
 
+#include <utility>
 #include <variant>
 
 #include "toolchain/base/kind_switch.h"
@@ -122,8 +123,9 @@ class TypeStructureBuilder {
            SemIR::SpecificInterface interface_constraint) -> TypeStructure {
     CARBON_CHECK(work_list_.empty());
 
-    symbolic_type_indices_.clear();
     structure_.clear();
+    symbolic_type_indices_.clear();
+    concrete_types_.clear();
 
     // The self type comes first in the type structure, so we push it last, as
     // the queue works from the back.
@@ -136,7 +138,8 @@ class TypeStructureBuilder {
     // TODO: This requires 4 SmallVector moves (two here and two in the
     // constructor). Find a way to reduce that.
     return TypeStructure(std::exchange(structure_, {}),
-                         std::exchange(symbolic_type_indices_, {}));
+                         std::exchange(symbolic_type_indices_, {}),
+                         std::exchange(concrete_types_, {}));
   }
 
  private:
@@ -146,7 +149,7 @@ class TypeStructureBuilder {
       work_list_.pop_back();
 
       if (std::holds_alternative<CloseType>(next)) {
-        AppendStructural(TypeStructure::Structural::ConcreteCloseParen);
+        AppendStructuralConcreteCloseParen();
         continue;
       }
 
@@ -154,9 +157,9 @@ class TypeStructureBuilder {
               std::get_if<SemIR::SpecificInterface>(&next)) {
         auto args = GetSpecificArgs(interface->specific_id);
         if (args.empty()) {
-          AppendStructural(TypeStructure::Structural::Concrete);
+          AppendStructuralConcrete(interface->interface_id);
         } else {
-          AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+          AppendStructuralConcreteOpenParen(interface->interface_id);
           Push(CloseType());
           PushArgs(args);
         }
@@ -164,7 +167,7 @@ class TypeStructureBuilder {
       }
 
       if (std::holds_alternative<SymbolicType>(next)) {
-        AppendStructural(TypeStructure::Structural::Symbolic);
+        AppendStructuralSymbolic();
         continue;
       }
 
@@ -174,12 +177,12 @@ class TypeStructureBuilder {
         // `{TypeWithPossibleNestedTypes, Concrete}`.
         // We might want a different bracket marker than ConcreteOpenParen for
         // this so that it can look different in the type structure when dumped.
-        AppendStructural(TypeStructure::Structural::Concrete);
+        AppendStructuralConcrete(SemIR::ErrorInst::TypeId);
         continue;
       }
 
-      SemIR::TypeId next_type_id = std::get<SemIR::TypeId>(next);
-      auto inst_id = context_->types().GetInstId(next_type_id);
+      SemIR::TypeId type_id = std::get<SemIR::TypeId>(next);
+      auto inst_id = context_->types().GetInstId(type_id);
       auto inst = context_->insts().Get(inst_id);
       CARBON_KIND_SWITCH(inst) {
           // ==== Symbolic types ====
@@ -213,10 +216,9 @@ class TypeStructureBuilder {
         case SemIR::NamespaceType::Kind:
         case SemIR::StringType::Kind:
         case SemIR::TypeType::Kind:
-        case SemIR::WitnessType::Kind: {
-          AppendStructural(TypeStructure::Structural::Concrete);
+        case SemIR::WitnessType::Kind:
+          AppendStructuralConcrete(type_id);
           break;
-        }
 
         case CARBON_KIND(SemIR::FacetType facet_type): {
           (void)facet_type;
@@ -229,14 +231,14 @@ class TypeStructureBuilder {
           //
           // The `FacetValue` may still be symbolic in generic code but its
           // type, the `FacetType` here, is concrete.
-          AppendStructural(TypeStructure::Structural::Concrete);
+          AppendStructuralConcrete(type_id);
           break;
         }
         case CARBON_KIND(SemIR::IntType int_type): {
-          if (context_->constant_values().Get(inst_id).is_concrete()) {
-            AppendStructural(TypeStructure::Structural::Concrete);
+          if (type_id.is_concrete()) {
+            AppendStructuralConcrete(type_id);
           } else {
-            AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+            AppendStructuralConcreteOpenParen(type_id);
             Push(CloseType());
             PushArgs({int_type.bit_width_id});
           }
@@ -246,7 +248,7 @@ class TypeStructureBuilder {
           // ==== Aggregate types ====
 
         case CARBON_KIND(SemIR::ArrayType array_type): {
-          AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+          AppendStructuralConcreteOpenParen(TypeStructure::ConcreteNoneType());
           Push(CloseType());
           PushInstId(array_type.element_type_inst_id);
           PushInstId(array_type.bound_id);
@@ -255,9 +257,9 @@ class TypeStructureBuilder {
         case CARBON_KIND(SemIR::ClassType class_type): {
           auto args = GetSpecificArgs(class_type.specific_id);
           if (args.empty()) {
-            AppendStructural(TypeStructure::Structural::Concrete);
+            AppendStructuralConcrete(class_type.class_id);
           } else {
-            AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+            AppendStructuralConcreteOpenParen(type_id);
             Push(CloseType());
             PushArgs(args);
           }
@@ -274,7 +276,7 @@ class TypeStructureBuilder {
           break;
         }
         case CARBON_KIND(SemIR::PointerType pointer_type): {
-          AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+          AppendStructuralConcreteOpenParen(TypeStructure::ConcreteNoneType());
           Push(CloseType());
           PushInstId(pointer_type.pointee_id);
           break;
@@ -283,9 +285,10 @@ class TypeStructureBuilder {
           auto inner_types =
               context_->inst_blocks().Get(tuple_type.type_elements_id);
           if (inner_types.empty()) {
-            AppendStructural(TypeStructure::Structural::Concrete);
+            AppendStructuralConcrete(type_id);
           } else {
-            AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+            AppendStructuralConcreteOpenParen(
+                TypeStructure::ConcreteNoneType());
             Push(CloseType());
             PushArgs(context_->inst_blocks().Get(tuple_type.type_elements_id));
           }
@@ -295,9 +298,9 @@ class TypeStructureBuilder {
           auto fields =
               context_->struct_type_fields().Get(struct_type.fields_id);
           if (fields.empty()) {
-            AppendStructural(TypeStructure::Structural::Concrete);
+            AppendStructuralConcrete(type_id);
           } else {
-            AppendStructural(TypeStructure::Structural::ConcreteOpenParen);
+            AppendStructuralConcreteOpenParen(type_id);
             Push(CloseType());
             for (const auto& field : llvm::reverse(fields)) {
               PushInstId(field.type_inst_id);
@@ -316,7 +319,10 @@ class TypeStructureBuilder {
   // A work item to mark a symbolic type.
   struct SymbolicType {};
   // A work item to mark a non-type value.
-  struct NonTypeValue {};
+  struct NonTypeValue {
+    // The type of the value.
+    SemIR::TypeId type_id;
+  };
 
   using WorkItem = std::variant<SemIR::TypeId, SymbolicType, NonTypeValue,
                                 SemIR::SpecificInterface, CloseType>;
@@ -387,7 +393,7 @@ class TypeStructureBuilder {
                type_id.has_value()) {
       Push(type_id);
     } else {
-      Push(NonTypeValue());
+      Push(NonTypeValue{.type_id = context_->insts().Get(inst_id).type_id()});
     }
   }
 
@@ -395,17 +401,32 @@ class TypeStructureBuilder {
   auto Push(WorkItem item) -> void { work_list_.push_back(item); }
 
   // Append a structural element to the TypeStructure being built.
-  auto AppendStructural(TypeStructure::Structural structural) -> void {
-    if (structural == TypeStructure::Structural::Symbolic) {
-      symbolic_type_indices_.push_back(structure_.size());
-    }
-    structure_.push_back(structural);
+  auto AppendStructuralConcrete(TypeStructure::ConcreteType type) -> void {
+    CARBON_CHECK(
+        !std::holds_alternative<TypeStructure::ConcreteNoneType>(type));
+    concrete_types_.push_back(type);
+    structure_.push_back(TypeStructure::Structural::Concrete);
+  }
+  auto AppendStructuralConcreteOpenParen(TypeStructure::ConcreteType type)
+      -> void {
+    concrete_types_.push_back(type);
+    structure_.push_back(TypeStructure::Structural::ConcreteOpenParen);
+  }
+  auto AppendStructuralConcreteCloseParen() -> void {
+    structure_.push_back(TypeStructure::Structural::ConcreteCloseParen);
+  }
+  auto AppendStructuralSymbolic() -> void {
+    symbolic_type_indices_.push_back(structure_.size());
+    structure_.push_back(TypeStructure::Structural::Symbolic);
   }
 
   Context* context_;
   llvm::SmallVector<WorkItem> work_list_;
-  llvm::SmallVector<int> symbolic_type_indices_;
+
+  // In-progress state for the equivalent `TypeStructure` fields.
   llvm::SmallVector<TypeStructure::Structural> structure_;
+  llvm::SmallVector<int> symbolic_type_indices_;
+  llvm::SmallVector<TypeStructure::ConcreteType> concrete_types_;
 };
 
 auto BuildTypeStructure(Context& context, SemIR::InstId self_inst_id,

toolchain/check/type_structure.h

@@ -45,6 +45,15 @@ class TypeStructure : public Printable<TypeStructure> {
         });
   }
 
+  // Equality of type structures. This compares that the structures are
+  // identical, which is a stronger requirement than that they are ordered the
+  // same.
+  friend auto operator==(const TypeStructure& lhs, const TypeStructure& rhs)
+      -> bool {
+    return lhs.structure_ == rhs.structure_ &&
+           lhs.concrete_types_ == rhs.concrete_types_;
+  }
+
   auto Print(llvm::raw_ostream& out) const -> void {
     out << "TypeStructure = ";
     for (auto s : structure_) {
@@ -68,25 +77,60 @@ class TypeStructure : public Printable<TypeStructure> {
  private:
   friend class TypeStructureBuilder;
 
+  // Elements of the type structure, indicating the presence of a concrete or
+  // symbolic element, and for aggregate concrete types (such as generic types),
+  // nesting for the types inside.
   enum class Structural : uint8_t {
+    // A concrete element in the type structure, such as `bool`.
     Concrete,
+
+    // A concrete element in the type structure that contains nested types
+    // within, such as `C(D)` for some classes C and D. It marks the start of
+    // the nested and is paired with a ConcreteCloseParen at the end of the
+    // nested types.
     ConcreteOpenParen,
+
+    // Closes a ConcreteOpenParen for a concrete type with nested types.
+    // Does not have its own concrete type.
     ConcreteCloseParen,
+
+    // A symbolic element in the type structure. When matching type structures,
+    // it represents a wildcard that matches against either a single `Concrete`
+    // or `Symbolic`, or everything from a `ConcreteOpenParen` to its paired
+    // `ConcreteCloseParen`.
     Symbolic,
   };
 
-  static constexpr int InfiniteDistance = -1;
+  // Indicates a concrete element in the type structure which does not add any
+  // type information of its own. See `ConcreteType`.
+  struct ConcreteNoneType {
+    friend auto operator==(ConcreteNoneType /*lhs*/, ConcreteNoneType /*rhs*/)
+        -> bool = default;
+  };
+  // The `concrete_types_` tracks the specific concrete type for each
+  // `Structural::Concrete` or `Structural::ConcreteOpenParen` in the type
+  // structure. But there are cases where the `ConcreteOpenParen` opens a scope
+  // for other concrete types but doesn't add any type data of its own, and
+  // `ConcreteNoneType` can appear there.
+  using ConcreteType = std::variant<ConcreteNoneType, SemIR::TypeId,
+                                    SemIR::ClassId, SemIR::InterfaceId>;
 
   TypeStructure(llvm::SmallVector<Structural> structure,
-                llvm::SmallVector<int> symbolic_type_indices)
+                llvm::SmallVector<int> symbolic_type_indices,
+                llvm::SmallVector<ConcreteType> concrete_types)
       : structure_(std::move(structure)),
-        symbolic_type_indices_(std::move(symbolic_type_indices)) {}
+        symbolic_type_indices_(std::move(symbolic_type_indices)),
+        concrete_types_(std::move(concrete_types)) {}
 
   // The structural position of concrete and symbolic values in the type.
   llvm::SmallVector<Structural> structure_;
 
   // Indices of the symbolic entries in structure_.
   llvm::SmallVector<int> symbolic_type_indices_;
+
+  // The related value for each `Concrete` and `ConcreteOpenParen` entry in
+  // the type `structure_`, in the same order. See `ConcreteType`.
+  llvm::SmallVector<ConcreteType> concrete_types_;
 };
 
 // Constructs the TypeStructure for a self type or facet value and an interface

toolchain/diagnostics/diagnostic_kind.def

@@ -313,13 +313,15 @@ CARBON_DIAGNOSTIC_KIND(ImplPreviousDefinition)
 CARBON_DIAGNOSTIC_KIND(ImplRedefinition)
 CARBON_DIAGNOSTIC_KIND(ImplOfNotOneInterface)
 CARBON_DIAGNOSTIC_KIND(ImplUnusedBinding)
+CARBON_DIAGNOSTIC_KIND(PoisonedImplLookupConcreteResult)
+CARBON_DIAGNOSTIC_KIND(PoisonedImplLookupConcreteResultNoteBadImpl)
+CARBON_DIAGNOSTIC_KIND(PoisonedImplLookupConcreteResultNotePreviousImpl)
+CARBON_DIAGNOSTIC_KIND(ImplFullyOverlapNonFinal)
+CARBON_DIAGNOSTIC_KIND(ImplFullyOverlapNonFinalNote)
 
 // Impl lookup.
 CARBON_DIAGNOSTIC_KIND(MissingImplInMemberAccess)
 CARBON_DIAGNOSTIC_KIND(MissingImplInMemberAccessNote)
-CARBON_DIAGNOSTIC_KIND(PoisonedImplLookupConcreteResult)
-CARBON_DIAGNOSTIC_KIND(PoisonedImplLookupConcreteResultNoteBadImpl)
-CARBON_DIAGNOSTIC_KIND(PoisonedImplLookupConcreteResultNotePreviousImpl)
 
 // Let declaration checking.
 CARBON_DIAGNOSTIC_KIND(ExpectedInitializerAfterLet)