Просмотр исходного кода

Add diagnostics for invalid impl declarations (#5420)

Outside of `match_first` this adds diagnostics for invalid non-final and
final `impl` declarations in line with those being proposed in
https://github.com/carbon-language/carbon-lang/pull/5337.

- Two non-final `impl`s with the exact same type structure is invalid.
- A `final impl` that matches the self/constraint of another `impl` as a
query would always be preferred, making the second one invalid.
- Two `final impl`s that overlap (have compatible type structures) in
different files is invalid.
- Two `final impl`s that overlap (have compatible type structures) in
the same file is invalid outside of `match_first`.
- A `final impl` in a different file from its root self type and
interface is invalid.

We add tests for all these scenarios as well as correct scenarios.

The "compatible" test for two type structures was being done
symmetrically, which is incorrect. We want it to test that a query type
structure is the same _or more specific_ in a compatible way with an
impl's type structure. This is corrected in the implementation, and the
diagnostics now have to test both directions to get the desired output,
as expected.

---------

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Dana Jansens 11 месяцев назад
Родитель
Сommit
950d83451a

+ 2 - 0
toolchain/check/BUILD

@@ -31,6 +31,7 @@ cc_library(
         "global_init.cpp",
         "impl.cpp",
         "impl_lookup.cpp",
+        "impl_validation.cpp",
         "import.cpp",
         "import_cpp.cpp",
         "import_ref.cpp",
@@ -74,6 +75,7 @@ cc_library(
         "global_init.h",
         "impl.h",
         "impl_lookup.h",
+        "impl_validation.h",
         "import.h",
         "import_cpp.h",
         "import_ref.h",

+ 3 - 149
toolchain/check/check_unit.cpp

@@ -18,6 +18,7 @@
 #include "toolchain/check/handle.h"
 #include "toolchain/check/impl.h"
 #include "toolchain/check/impl_lookup.h"
+#include "toolchain/check/impl_validation.h"
 #include "toolchain/check/import.h"
 #include "toolchain/check/import_cpp.h"
 #include "toolchain/check/import_ref.h"
@@ -572,160 +573,13 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
   context_.inst_block_stack().PopAndDiscard();
 }
 
-// Check for invalid overlap between impls, given the set of all impls for a
-// single interface.
-static auto CheckOverlappingImplsForInterface(
-    Context& context,
-    llvm::ArrayRef<std::pair<SemIR::ImplId, SemIR::SpecificInterface>>
-        impls_and_interface) -> void {
-  // Range over `SemIR::ImplId` only. It'd be nice to make this the function
-  // parameter but we don't have a concept to express that outside the (banned)
-  // std::ranges.
-  auto impl_ids = llvm::map_range(
-      impls_and_interface,
-      [=](std::pair<SemIR::ImplId, SemIR::SpecificInterface> pair) {
-        auto [impl_id, _] = pair;
-        return impl_id;
-      });
-
-  // Avoid holding a reference into the ImplStore, as the diagnostic checks
-  // below can invalidate the reference. We copy out the part of the `Impl` we
-  // need.
-  struct ImplInfo {
-    bool is_final;
-    SemIR::InstId witness_id;
-    SemIR::TypeInstId self_id;
-    SemIR::InstId latest_decl_id;
-    SemIR::SpecificInterface interface;
-  };
-  auto get_impl_info = [&](SemIR::ImplId impl_id) -> ImplInfo {
-    const auto& impl = context.impls().Get(impl_id);
-    return {.is_final = impl.is_final,
-            .witness_id = impl.witness_id,
-            .self_id = impl.self_id,
-            .latest_decl_id = impl.latest_decl_id(),
-            .interface = impl.interface};
-  };
-
-  // TODO: We should revisit this and look for a way to do these checks in less
-  // than quadratic time. From @zygoloid: Possibly by converting the set of
-  // impls into a decision tree.
-  for (auto [index_a, impl_a_id] : llvm::enumerate(impl_ids)) {
-    auto impl_a = get_impl_info(impl_a_id);
-    if (impl_a.witness_id == SemIR::ErrorInst::InstId) {
-      continue;
-    }
-    auto type_structure =
-        BuildTypeStructure(context, impl_a.self_id, impl_a.interface);
-
-    for (auto impl_b_id : llvm::drop_begin(impl_ids, index_a + 1)) {
-      auto impl_b = get_impl_info(impl_b_id);
-      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 type_structure2 =
-            BuildTypeStructure(context, impl_b.self_id, impl_b.interface);
-        if (type_structure == type_structure2) {
-          CARBON_DIAGNOSTIC(ImplNonFinalSameTypeStructure, Error,
-                            "found non-final `impl` with the same type "
-                            "structure as another non-final `impl`");
-          auto builder = context.emitter().Build(impl_b.latest_decl_id,
-                                                 ImplNonFinalSameTypeStructure);
-          CARBON_DIAGNOSTIC(ImplNonFinalSameTypeStructureNote, Note,
-                            "other `impl` here");
-          builder.Note(impl_a.latest_decl_id,
-                       ImplNonFinalSameTypeStructureNote);
-          builder.Emit();
-          break;
-        }
-      } else {
-        CARBON_CHECK(impl_a.is_final || impl_b.is_final);
-
-        auto diagnose = [&](ImplInfo& query_impl, SemIR::ImplId final_impl_id,
-                            const ImplInfo& final_impl) -> bool {
-          if (LookupMatchesImpl(
-                  context, SemIR::LocId(query_impl.latest_decl_id),
-                  context.constant_values().Get(query_impl.self_id),
-                  query_impl.interface, final_impl_id)) {
-            CARBON_DIAGNOSTIC(ImplFinalOverlapsNonFinal, Error,
-                              "`impl` will never be used");
-            auto builder = context.emitter().Build(query_impl.latest_decl_id,
-                                                   ImplFinalOverlapsNonFinal);
-            CARBON_DIAGNOSTIC(
-                ImplFinalOverlapsNonFinalNote, Note,
-                "`final impl` declared here would always be used instead");
-            builder.Note(final_impl.latest_decl_id,
-                         ImplFinalOverlapsNonFinalNote);
-            builder.Emit();
-            return true;
-          }
-          return false;
-        };
-
-        bool did_diagnose = false;
-        if (impl_a.is_final) {
-          did_diagnose = diagnose(impl_b, impl_a_id, impl_a);
-        }
-        if (impl_b.is_final && !did_diagnose) {
-          diagnose(impl_a, impl_b_id, impl_b);
-        }
-      }
-    }
-
-    // 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::CheckOverlappingImpls() -> void {
-  // Collect all of the impls sorted into contiguous segments by their
-  // interface. We only need to compare impls within each such segment.
-  //
-  // Don't hold Impl pointers here because the process of looking for
-  // diagnostics may cause imports and may invalidate pointers into the
-  // ImplStore.
-  llvm::SmallVector<std::pair<SemIR::ImplId, SemIR::SpecificInterface>>
-      impl_ids_by_interface(llvm::map_range(
-          context_.impls().enumerate(),
-          [](std::pair<SemIR::ImplId, const SemIR::Impl&> pair) {
-            return std::make_pair(pair.first, pair.second.interface);
-          }));
-  llvm::stable_sort(
-      impl_ids_by_interface,
-      [](std::pair<SemIR::ImplId, SemIR::SpecificInterface> a,
-         std::pair<SemIR::ImplId, const SemIR::SpecificInterface> b) {
-        return a.second.interface_id.index < b.second.interface_id.index;
-      });
-
-  const auto* it = impl_ids_by_interface.begin();
-  while (it != impl_ids_by_interface.end()) {
-    const auto* segment_begin = it;
-    do {
-      ++it;
-    } while (it != impl_ids_by_interface.end() &&
-             it->second.interface_id == segment_begin->second.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(
-        context_, llvm::ArrayRef(segment_begin, segment_end));
-  }
-}
+auto CheckUnit::CheckImpls() -> void { ValidateImplsInFile(context_); }
 
 auto CheckUnit::FinishRun() -> void {
   CheckRequiredDeclarations();
   CheckRequiredDefinitions();
   CheckPoisonedConcreteImplLookupQueries();
-  CheckOverlappingImpls();
+  CheckImpls();
 
   // Pop information for the file-level scope.
   context_.sem_ir().set_top_inst_block_id(context_.inst_block_stack().Pop());

+ 2 - 7
toolchain/check/check_unit.h

@@ -169,13 +169,8 @@ 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;
+  // Look for `impl` declarations that are invalid.
+  auto CheckImpls() -> void;
 
   // Does work after processing the parse tree, such as finishing the IR and
   // checking for missing contents.

+ 6 - 4
toolchain/check/impl_lookup.cpp

@@ -418,11 +418,11 @@ auto LookupImplWitness(Context& context, SemIR::LocId loc_id,
   for (auto import_ir : import_irs) {
     // TODO: Instead of importing all impls, only import ones that are in some
     // way connected to this query.
-    for (auto impl_index : llvm::seq(
-             context.import_irs().Get(import_ir).sem_ir->impls().size())) {
+    for (auto [import_impl_id, _] :
+         context.import_irs().Get(import_ir).sem_ir->impls().enumerate()) {
       // TODO: Track the relevant impls and only consider those ones and any
       // local impls, rather than looping over all impls below.
-      ImportImpl(context, import_ir, SemIR::ImplId(impl_index));
+      ImportImpl(context, import_ir, import_impl_id);
     }
   }
 
@@ -555,7 +555,9 @@ static auto CollectCandidateImplsForQuery(
     // TODO: We can skip the comparison here if the `impl_interface_const_id` is
     // not symbolic, since when the interface and specific ids match, and they
     // aren't symbolic, the structure will be identical.
-    if (!query_type_structure.IsCompatibleWith(type_structure)) {
+    if (!query_type_structure.CompareStructure(
+            TypeStructure::CompareTest::IsEqualToOrMoreSpecificThan,
+            type_structure)) {
       continue;
     }
 

+ 469 - 0
toolchain/check/impl_validation.cpp

@@ -0,0 +1,469 @@
+// 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 "toolchain/check/impl_validation.h"
+
+#include <utility>
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include "toolchain/base/kind_switch.h"
+#include "toolchain/check/diagnostic_helpers.h"
+#include "toolchain/check/impl_lookup.h"
+#include "toolchain/check/import_ref.h"
+#include "toolchain/check/type_structure.h"
+#include "toolchain/sem_ir/entity_with_params_base.h"
+#include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/type_iterator.h"
+
+namespace Carbon::Check {
+
+namespace {
+
+struct ImplAndInterface {
+  SemIR::ImplId impl_id;
+  SemIR::SpecificInterface interface;
+};
+
+// We avoid holding a reference into the ImplStore, as the act of verifying the
+// diagnostic checks can invalidate such a reference. We collect what we need to
+// know about the the `Impl` for validation into this struct.
+struct ImplInfo {
+  bool is_final;
+  SemIR::InstId witness_id;
+  SemIR::TypeInstId self_id;
+  SemIR::InstId latest_decl_id;
+  SemIR::SpecificInterface interface;
+  // Whether the `impl` decl was imported or from the local file.
+  bool is_local;
+  // If imported, the IR from which the `impl` decl was imported.
+  SemIR::ImportIRId ir_id;
+  TypeStructure type_structure;
+};
+
+}  // namespace
+
+static auto GetIRId(Context& context, SemIR::InstId owning_inst_id)
+    -> SemIR::ImportIRId {
+  if (!owning_inst_id.has_value()) {
+    return SemIR::ImportIRId::None;
+  }
+  return GetCanonicalImportIRInst(context, owning_inst_id).ir_id();
+}
+
+auto GetImplInfo(Context& context, SemIR::ImplId impl_id) -> ImplInfo {
+  const auto& impl = context.impls().Get(impl_id);
+  auto ir_id = GetIRId(context, impl.first_owning_decl_id);
+  return {.is_final = impl.is_final,
+          .witness_id = impl.witness_id,
+          .self_id = impl.self_id,
+          .latest_decl_id = impl.latest_decl_id(),
+          .interface = impl.interface,
+          .is_local = !ir_id.has_value(),
+          .ir_id = ir_id,
+          .type_structure =
+              BuildTypeStructure(context, impl.self_id, impl.interface)};
+}
+
+// A final impl must be in the same file as either its root self type or the
+// interface in its constraint.
+//
+// Returns true if an error was diagnosed.
+static auto DiagnoseFinalImplNotInSameFileAsRootSelfTypeOrInterface(
+    Context& context, const ImplInfo& impl, SemIR::ImportIRId interface_ir_id)
+    -> bool {
+  bool self_type_same_file = false;
+
+  auto type_iter = SemIR::TypeIterator(&context.sem_ir());
+  type_iter.Add(impl.self_id);
+  auto step = type_iter.Next();
+
+  using Step = SemIR::TypeIterator::Step;
+  CARBON_KIND_SWITCH(step.any) {
+    case CARBON_KIND(Step::ClassStart start): {
+      auto inst_id = context.classes().Get(start.class_id).first_owning_decl_id;
+      if (!GetIRId(context, inst_id).has_value()) {
+        self_type_same_file = true;
+      }
+      break;
+    }
+    case CARBON_KIND(Step::ClassStartOnly start): {
+      auto inst_id = context.classes().Get(start.class_id).first_owning_decl_id;
+      if (!GetIRId(context, inst_id).has_value()) {
+        self_type_same_file = true;
+      }
+      break;
+    }
+
+    case CARBON_KIND(Step::Done _):
+      CARBON_FATAL("self type is empty?");
+
+    default:
+      break;
+  }
+
+  bool interface_same_file = !interface_ir_id.has_value();
+
+  if (!self_type_same_file && !interface_same_file) {
+    CARBON_DIAGNOSTIC(FinalImplInvalidFile, Error,
+                      "`final impl` found in file that does not contain "
+                      "the root self type nor the interface definition");
+    context.emitter().Emit(impl.latest_decl_id, FinalImplInvalidFile);
+    return true;
+  }
+
+  return false;
+}
+
+// The type structure each non-final `impl` must differ from all other non-final
+// `impl` for the same interface visible from the file.
+//
+// Returns true if an error was diagnosed.
+static auto DiagnoseNonFinalImplsWithSameTypeStructure(Context& context,
+                                                       const ImplInfo& impl_a,
+                                                       const ImplInfo& impl_b)
+    -> bool {
+  if (impl_a.type_structure == impl_b.type_structure) {
+    CARBON_DIAGNOSTIC(ImplNonFinalSameTypeStructure, Error,
+                      "found non-final `impl` with the same type "
+                      "structure as another non-final `impl`");
+    auto builder = context.emitter().Build(impl_b.latest_decl_id,
+                                           ImplNonFinalSameTypeStructure);
+    CARBON_DIAGNOSTIC(ImplNonFinalSameTypeStructureNote, Note,
+                      "other `impl` here");
+    builder.Note(impl_a.latest_decl_id, ImplNonFinalSameTypeStructureNote);
+    builder.Emit();
+    return true;
+  }
+
+  return false;
+}
+
+// An impl's self type and constraint can not match (as a lookup query)
+// against any final impl, or it would never be used instead of that
+// final impl.
+//
+// Returns true if an error was diagnosed.
+static auto DiagnoseUnmatchableNonFinalImplWithFinalImpl(
+    Context& context, SemIR::ImplId impl_a_id, const ImplInfo& impl_a,
+    SemIR::ImplId impl_b_id, const ImplInfo& impl_b) -> bool {
+  auto diagnose_unmatchable_impl = [&](const ImplInfo& query_impl,
+                                       SemIR::ImplId final_impl_id,
+                                       const ImplInfo& final_impl) -> bool {
+    if (LookupMatchesImpl(context, SemIR::LocId(query_impl.latest_decl_id),
+                          context.constant_values().Get(query_impl.self_id),
+                          query_impl.interface, final_impl_id)) {
+      CARBON_DIAGNOSTIC(ImplFinalOverlapsNonFinal, Error,
+                        "`impl` will never be used");
+      auto builder = context.emitter().Build(query_impl.latest_decl_id,
+                                             ImplFinalOverlapsNonFinal);
+      CARBON_DIAGNOSTIC(
+          ImplFinalOverlapsNonFinalNote, Note,
+          "`final impl` declared here would always be used instead");
+      builder.Note(final_impl.latest_decl_id, ImplFinalOverlapsNonFinalNote);
+      builder.Emit();
+      return true;
+    }
+    return false;
+  };
+
+  CARBON_CHECK(impl_a.is_final || impl_b.is_final);
+
+  if (impl_b.is_final) {
+    return diagnose_unmatchable_impl(impl_a, impl_b_id, impl_b);
+  } else {
+    return diagnose_unmatchable_impl(impl_b, impl_a_id, impl_a);
+  }
+}
+
+// Final impls that overlap in their type structure must be in the
+// same file.
+//
+// Returns true if an error was diagnosed.
+static auto DiagnoseFinalImplsOverlapInDifferentFiles(Context& context,
+                                                      const ImplInfo& impl_a,
+                                                      const ImplInfo& impl_b)
+    -> bool {
+  if (impl_a.ir_id != impl_b.ir_id) {
+    CARBON_DIAGNOSTIC(
+        FinalImplOverlapsDifferentFile, Error,
+        "`final impl` overlaps with `final impl` from another file");
+    CARBON_DIAGNOSTIC(FinalImplOverlapsDifferentFileNote, Note,
+                      "imported `final impl` here");
+    if (impl_a.is_local) {
+      auto builder = context.emitter().Build(impl_a.latest_decl_id,
+                                             FinalImplOverlapsDifferentFile);
+      builder.Note(impl_b.latest_decl_id, FinalImplOverlapsDifferentFileNote);
+      builder.Emit();
+    } else {
+      auto builder = context.emitter().Build(impl_b.latest_decl_id,
+                                             FinalImplOverlapsDifferentFile);
+      builder.Note(impl_a.latest_decl_id, FinalImplOverlapsDifferentFileNote);
+      builder.Emit();
+    }
+    return true;
+  }
+
+  return false;
+}
+
+// Two final impls in the same file can not overlap in their type
+// structure if they are not in the same match_first block.
+//
+// TODO: Support for match_first needed here when they exist in the
+// toolchain.
+//
+// Returns true if an error was diagnosed.
+static auto DiagnoseFinalImplsOverlapOutsideMatchFirst(Context& context,
+                                                       const ImplInfo& impl_a,
+                                                       const ImplInfo& impl_b)
+    -> bool {
+  if (impl_a.is_local && impl_b.is_local) {
+    CARBON_DIAGNOSTIC(FinalImplOverlapsSameFile, Error,
+                      "`final impl` overlaps with `final impl` from same file "
+                      "outside a `match_first` block");
+    auto builder = context.emitter().Build(impl_b.latest_decl_id,
+                                           FinalImplOverlapsSameFile);
+    CARBON_DIAGNOSTIC(FinalImplOverlapsSameFileNote, Note,
+                      "other `final impl` here");
+    builder.Note(impl_a.latest_decl_id, FinalImplOverlapsSameFileNote);
+    builder.Emit();
+    return true;
+  }
+
+  return false;
+}
+
+static auto ValidateImplsForInterface(
+    Context& context, llvm::ArrayRef<ImplAndInterface> impls_and_interface)
+    -> void {
+  // Range over `SemIR::ImplId` only. Caller has ensured all of these are
+  // for the same interface.
+  auto impl_ids = llvm::map_range(impls_and_interface,
+                                  [=](ImplAndInterface impl_and_interface) {
+                                    return impl_and_interface.impl_id;
+                                  });
+
+  // All `impl`s we look at here have the same `InterfaceId` (though different
+  // `SpecificId`s in their `SpecificInterface`s). So we can grab the
+  // `ImportIRId` for the interface a single time up front.
+  auto interface_decl_id =
+      context.interfaces()
+          .Get(impls_and_interface[0].interface.interface_id)
+          .first_owning_decl_id;
+  auto interface_ir_id = GetIRId(context, interface_decl_id);
+
+  for (auto impl_id : impl_ids) {
+    auto impl = GetImplInfo(context, impl_id);
+    if (impl.is_final && impl.is_local) {
+      // =======================================================================
+      /// Rules for an individual final impl.
+      // =======================================================================
+      DiagnoseFinalImplNotInSameFileAsRootSelfTypeOrInterface(context, impl,
+                                                              interface_ir_id);
+    }
+  }
+
+  // TODO: We should revisit this and look for a way to do these checks in less
+  // than quadratic time. From @zygoloid: Possibly by converting the set of
+  // impls into a decision tree.
+  //
+  // For each impl, we compare it pair-wise which each impl found before it, so
+  // that diagnostics are attached to the later impl, as the earlier impl on its
+  // own does not generate a diagnostic.
+  size_t num_impl_ids = impls_and_interface.size();
+  for (auto [split_point, impl_b_id] :
+       llvm::drop_begin(llvm::enumerate(impl_ids))) {
+    auto impl_b = GetImplInfo(context, impl_b_id);
+
+    // Prevent diagnosing the same error multiple times for the same `impl_b`
+    // against different impls before it. But still ensure we do give one of
+    // each diagnostic when they are different errors.
+    bool did_diagnose_non_final_impls_with_same_type_structure = false;
+    bool did_diagnose_unmatchable_non_final_impl_with_final_impl = false;
+    bool did_diagnose_final_impls_overlap_in_different_files = false;
+    bool did_diagnose_final_impls_overlap_outside_match_first = false;
+
+    auto impls_before = llvm::drop_end(impl_ids, num_impl_ids - split_point);
+    for (auto impl_a_id : impls_before) {
+      auto impl_a = GetImplInfo(context, impl_a_id);
+
+      // Only enforce rules when at least one of the impls was written in this
+      // file.
+      if (!impl_a.is_local && !impl_b.is_local) {
+        continue;
+      }
+
+      if (!impl_a.is_final && !impl_b.is_final) {
+        // =====================================================================
+        // Rules between two non-final impls.
+        // =====================================================================
+        if (!did_diagnose_non_final_impls_with_same_type_structure) {
+          // Two impls in separate files will need to have some different
+          // concrete element in their type structure, as enforced by the orphan
+          // rule. So we don't need to check against non-local impls.
+          if (impl_a.is_local && impl_b.is_local) {
+            if (DiagnoseNonFinalImplsWithSameTypeStructure(context, impl_a,
+                                                           impl_b)) {
+              // The same final `impl_a` may overlap with multiple `impl_b`s,
+              // and we want to diagnose each `impl_b`.
+              did_diagnose_non_final_impls_with_same_type_structure = true;
+            }
+          }
+        }
+      } else if (!impl_a.is_final || !impl_b.is_final) {
+        // =====================================================================
+        // Rules between final impl and non-final impl.
+        // =====================================================================
+        if (!did_diagnose_unmatchable_non_final_impl_with_final_impl) {
+          if (DiagnoseUnmatchableNonFinalImplWithFinalImpl(
+                  context, impl_a_id, impl_a, impl_b_id, impl_b)) {
+            did_diagnose_unmatchable_non_final_impl_with_final_impl = true;
+          }
+        }
+      } else if (impl_a.type_structure.CompareStructure(
+                     TypeStructure::CompareTest::HasOverlap,
+                     impl_b.type_structure)) {
+        // =====================================================================
+        // Rules between two overlapping final impls.
+        // =====================================================================
+        CARBON_CHECK(impl_a.is_final && impl_b.is_final);
+        if (!did_diagnose_final_impls_overlap_in_different_files) {
+          if (DiagnoseFinalImplsOverlapInDifferentFiles(context, impl_a,
+                                                        impl_b)) {
+            did_diagnose_final_impls_overlap_in_different_files = true;
+          }
+        }
+        if (!did_diagnose_final_impls_overlap_outside_match_first) {
+          if (DiagnoseFinalImplsOverlapOutsideMatchFirst(context, impl_a,
+                                                         impl_b)) {
+            did_diagnose_final_impls_overlap_outside_match_first = true;
+          }
+        }
+      }
+    }
+  }
+}
+
+// For each `impl` seen in this file, ensure that we import every available
+// `final impl` for the same interface, so that we can to check for
+// diagnostics about the relationship between them and the `impl`s in this
+// file.
+static auto ImportFinalImplsWithImplInFile(Context& context) -> void {
+  struct InterfaceToImport {
+    SemIR::ImportIRId ir_id;
+    SemIR::InterfaceId interface_id;
+
+    constexpr auto operator==(const InterfaceToImport& rhs) const
+        -> bool = default;
+    constexpr auto operator<=>(const InterfaceToImport& rhs) const -> auto {
+      if (ir_id != rhs.ir_id) {
+        return ir_id.index <=> rhs.ir_id.index;
+      }
+      return interface_id.index <=> rhs.interface_id.index;
+    }
+  };
+
+  llvm::SmallVector<InterfaceToImport> interfaces_to_import;
+  for (const auto& impl : context.impls().array_ref()) {
+    if (impl.witness_id == SemIR::ErrorInst::InstId) {
+      continue;
+    }
+    auto impl_import_ir_id = GetIRId(context, impl.first_owning_decl_id);
+    if (impl_import_ir_id.has_value()) {
+      // Only import `impl`s of interfaces for which there is a local `impl` of
+      // that that interface.
+      continue;
+    }
+
+    auto interface_id = impl.interface.interface_id;
+    const auto& interface = context.interfaces().Get(interface_id);
+    auto import_ir_id = GetIRId(context, interface.first_owning_decl_id);
+    if (!import_ir_id.has_value()) {
+      continue;
+    }
+    interfaces_to_import.push_back(
+        {.ir_id = import_ir_id, .interface_id = interface_id});
+  }
+
+  llvm::sort(interfaces_to_import);
+  llvm::unique(interfaces_to_import);
+
+  struct ImplToImport {
+    SemIR::ImportIRId ir_id;
+    SemIR::ImplId import_impl_id;
+
+    constexpr auto operator==(const ImplToImport& rhs) const -> bool = default;
+    constexpr auto operator<=>(const ImplToImport& rhs) const -> auto {
+      if (ir_id != rhs.ir_id) {
+        return ir_id.index <=> rhs.ir_id.index;
+      }
+      return import_impl_id.index <=> rhs.import_impl_id.index;
+    }
+  };
+
+  llvm::SmallVector<ImplToImport> impls_to_import;
+  for (auto [ir_id, interface_id] : interfaces_to_import) {
+    const SemIR::File& sem_ir = *context.import_irs().Get(ir_id).sem_ir;
+    for (auto [impl_id, impl] : sem_ir.impls().enumerate()) {
+      if (impl.is_final && impl.interface.interface_id == interface_id) {
+        impls_to_import.push_back({.ir_id = ir_id, .import_impl_id = impl_id});
+      }
+    }
+  }
+
+  llvm::sort(impls_to_import);
+  llvm::unique(impls_to_import);
+
+  for (auto [ir_id, import_impl_id] : impls_to_import) {
+    ImportImpl(context, ir_id, import_impl_id);
+  }
+}
+
+auto ValidateImplsInFile(Context& context) -> void {
+  ImportFinalImplsWithImplInFile(context);
+
+  // Collect all of the impls sorted into contiguous segments by their
+  // interface. We only need to compare impls within each such segment. We don't
+  // keep impls with an Error in them, as they may be missing other values
+  // needed to check the diagnostics and they already have a diagnostic printed
+  // about them anyhow. We also verify the impl has an `InterfaceId` since it
+  // can be missing, in which case a diagnostic would have been generated
+  // already as well.
+  //
+  // Don't hold Impl pointers here because the process of looking for
+  // diagnostics may cause imports and may invalidate pointers into the
+  // ImplStore.
+  llvm::SmallVector<ImplAndInterface> impl_ids_by_interface(llvm::map_range(
+      llvm::make_filter_range(
+          context.impls().enumerate(),
+          [](std::pair<SemIR::ImplId, const SemIR::Impl&> pair) {
+            return pair.second.witness_id != SemIR::ErrorInst::InstId &&
+                   pair.second.interface.interface_id.has_value();
+          }),
+      [](std::pair<SemIR::ImplId, const SemIR::Impl&> pair) {
+        return ImplAndInterface{.impl_id = pair.first,
+                                .interface = pair.second.interface};
+      }));
+  llvm::stable_sort(impl_ids_by_interface, [](const ImplAndInterface& lhs,
+                                              const ImplAndInterface& rhs) {
+    return lhs.interface.interface_id.index < rhs.interface.interface_id.index;
+  });
+
+  const auto* it = impl_ids_by_interface.begin();
+  while (it != impl_ids_by_interface.end()) {
+    const auto* segment_begin = it;
+    auto begin_interface_id = segment_begin->interface.interface_id;
+    do {
+      ++it;
+    } while (it != impl_ids_by_interface.end() &&
+             it->interface.interface_id == begin_interface_id);
+    const auto* segment_end = it;
+
+    ValidateImplsForInterface(context, {segment_begin, segment_end});
+  }
+}
+
+}  // namespace Carbon::Check

+ 19 - 0
toolchain/check/impl_validation.h

@@ -0,0 +1,19 @@
+// 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
+
+#ifndef CARBON_TOOLCHAIN_CHECK_IMPL_VALIDATION_H_
+#define CARBON_TOOLCHAIN_CHECK_IMPL_VALIDATION_H_
+
+#include "toolchain/check/context.h"
+
+namespace Carbon::Check {
+
+// Called after typechecking the full file to diagnose any `impl` declarations
+// that are invalid because they are in wrong file or overlap with other `impl`
+// declarations incorrectly.
+auto ValidateImplsInFile(Context& context) -> void;
+
+}  // namespace Carbon::Check
+
+#endif  // CARBON_TOOLCHAIN_CHECK_IMPL_VALIDATION_H_

+ 0 - 70
toolchain/check/testdata/impl/lookup/min_prelude/final_placement.carbon

@@ -1,70 +0,0 @@
-// 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
-//
-// 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/final_placement.carbon
-// TIP: To dump output, run:
-// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/lookup/min_prelude/final_placement.carbon
-
-// --- interface_z.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {
-  let X:! type;
-}
-
-// Blanket final impl on Z is allowed in the same file.
-final impl forall [T:! type] T as Z(T) where .X = {.z_t: ()} {}
-
-// Blanket final impl on Z with a concrete parameter.
-final impl forall [T:! type] T as Z(()) where .X = {.z_tuple: ()} {}
-
-// --- todo_final_with_root_self_same_file.carbon
-library "[[@TEST_NAME]]";
-
-import library "interface_z";
-
-class C(T:! type) { adapt (); }
-
-// Can provide a specialized final blanket impl for a type defined in the same
-// file.
-final impl forall [T:! type] C(T) as Z(T) where .X = {.c: ()} {}
-
-fn F() {
-  // TODO: It's not yet decided which `final` specialization wins, but it must
-  // be one from the interface file.
-  //
-  // See https://github.com/carbon-language/carbon-lang/pull/5337.
-  let x: C(()).(Z(()).X) = {.c = ()};
-}
-
-// --- todo_fail_final_with_symbolic_self.carbon
-library "[[@TEST_NAME]]";
-
-import library "interface_z";
-
-class C;
-
-// TODO: Can not make a specialized final blanket impl over `Z` for all types
-// except in the same file as `Z`, even if a type from the current file appears
-// in a parameter of `Z`.
-final impl forall [T:! type] T as Z(C) where .X = () {}
-
-// --- type_d.carbon
-library "[[@TEST_NAME]]";
-
-class D {}
-
-// --- todo_fail_final_with_root_self_different_file.carbon
-library "[[@TEST_NAME]]";
-
-import library "type_d";
-import library "interface_z";
-
-// TODO: Can not write a final impl on `D` outside the file where `D` is defined.
-final impl D as Z({}) where .X = () {}

+ 3 - 3
toolchain/check/testdata/impl/lookup/min_prelude/impl_cycle.carbon

@@ -281,9 +281,9 @@ impl forall [T:! type, U:! ComparableTo(T)] U as 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` with the same type structure as another non-final `impl` [ImplNonFinalSameTypeStructure]
 // 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: other `impl` here [ImplNonFinalSameTypeStructureNote]
-// 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-14]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
+// CHECK:STDERR: impl forall [T:! type] T as ComparableWith(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
 impl forall [T:! type, U:! ComparableTo(T)] T as ComparableWith(U) {}
 

+ 392 - 0
toolchain/check/testdata/impl/lookup/min_prelude/impl_overlap.carbon

@@ -0,0 +1,392 @@
+// 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
+//
+// 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/impl_overlap.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/lookup/min_prelude/impl_overlap.carbon
+
+// ============================================================================
+// Setup files
+// ============================================================================
+
+// --- interface_z.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+// --- interface_z_with_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+final impl forall [T:! type] T as Z(T) {}
+
+// --- type_d.carbon
+library "[[@TEST_NAME]]";
+
+class D(T:! type) {}
+
+// ============================================================================
+// Test files
+// ============================================================================
+
+// --- final_impl_with_interface_generic_self.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {}
+
+final impl forall [T:! type] T as Z {}
+
+// --- final_impl_with_interface_concrete_self_same_file.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {}
+
+class C;
+
+final impl C as Z {}
+
+// --- final_impl_with_interface_concrete_self_from_other_file.carbon
+library "[[@TEST_NAME]]";
+
+import library "type_d";
+
+interface Z {}
+
+// The final impl is for a root self type from another file, but the interface
+// is in the same file, so this is valid.
+final impl D(()) as Z {}
+
+// --- final_impl_with_interface_symbolic_self_from_other_file.carbon
+library "[[@TEST_NAME]]";
+
+import library "type_d";
+
+interface Z {}
+
+// The root self type is from another file, but the interface is from this file.
+// This tests the self type being a symbolic type due to the type parameter T.
+final impl forall [T:! type] D(T) as Z {}
+
+// --- fail_multiple_finals_overlap_with_interface.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+final impl forall [T:! type] T as Z(T) {}
+
+// The final impl here overlaps with the final impl above, but is not in a
+// match_first.
+//
+// CHECK:STDERR: fail_multiple_finals_overlap_with_interface.carbon:[[@LINE+7]]:1: error: `final impl` overlaps with `final impl` from same file outside a `match_first` block [FinalImplOverlapsSameFile]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_multiple_finals_overlap_with_interface.carbon:[[@LINE-8]]:1: note: other `final impl` here [FinalImplOverlapsSameFileNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl forall [T:! type] T as Z(()) {}
+
+class C;
+
+// The final impl here overlaps with the first final impl above, but is not in
+// a match_first.
+//
+// CHECK:STDERR: fail_multiple_finals_overlap_with_interface.carbon:[[@LINE+7]]:1: error: `final impl` overlaps with `final impl` from same file outside a `match_first` block [FinalImplOverlapsSameFile]
+// CHECK:STDERR: final impl C as Z({}) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_multiple_finals_overlap_with_interface.carbon:[[@LINE-22]]:1: note: other `final impl` here [FinalImplOverlapsSameFileNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl C as Z({}) {}
+
+// --- fail_multiple_finals_overlap_with_self_type.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z";
+
+class C;
+
+final impl C as Z(()) {}
+
+// The final impl here overlaps with the first final impl above, but is not in
+// a match_first.
+//
+// CHECK:STDERR: fail_multiple_finals_overlap_with_self_type.carbon:[[@LINE+7]]:1: error: `final impl` overlaps with `final impl` from same file outside a `match_first` block [FinalImplOverlapsSameFile]
+// CHECK:STDERR: final impl forall [T:! type] C as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_multiple_finals_overlap_with_self_type.carbon:[[@LINE-8]]:1: note: other `final impl` here [FinalImplOverlapsSameFileNote]
+// CHECK:STDERR: final impl C as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl forall [T:! type] C as Z(T) {}
+
+// --- multiple_finals_with_nonoverlapping_with_interface.carbon
+library "[[@TEST_NAME]]";
+
+import library "type_d";
+
+interface Z {}
+
+// Two final impls for D as Z, but they are not overlapping so they are allowed
+// outside of match_first.
+final impl D(()) as Z {}
+final impl D({}) as Z {}
+
+// --- multiple_finals_with_nonoverlapping_with_self_type.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z";
+
+class C;
+
+// Two final impls for C as Z, but they are not overlapping so they are allowed
+// outside of match_first.
+final impl C as Z(()) {}
+final impl C as Z({}) {}
+
+// --- final_impl_with_root_self_type.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z";
+
+class C(T:! type);
+
+// Can provide a specialized final blanket impl for a type defined in the same
+// file.
+final impl forall [T:! type] C(T) as Z(T) {}
+
+// --- fail_final_impl_with_both_interface_and_self_but_different_files.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z_with_impl";
+
+class C;
+
+// Can't write a final impl in both the interface's file and the root self
+// type's file (when they are different files).
+//
+// CHECK:STDERR: fail_final_impl_with_both_interface_and_self_but_different_files.carbon:[[@LINE+8]]:1: error: `final impl` overlaps with `final impl` from another file [FinalImplOverlapsDifferentFile]
+// CHECK:STDERR: final impl C as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_impl_with_both_interface_and_self_but_different_files.carbon:[[@LINE-10]]:1: in import [InImport]
+// CHECK:STDERR: interface_z_with_impl.carbon:5:1: note: imported `final impl` here [FinalImplOverlapsDifferentFileNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl C as Z(()) {}
+
+// --- fail_final_overlaps_final_from_other_file.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z_with_impl";
+
+class C;
+
+// This final impl is overlapped by a final impl in the interface file, and you
+// can't write a final impl in two different files.
+//
+// CHECK:STDERR: fail_final_overlaps_final_from_other_file.carbon:[[@LINE+8]]:1: error: `final impl` overlaps with `final impl` from another file [FinalImplOverlapsDifferentFile]
+// CHECK:STDERR: final impl C as Z(C) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_overlaps_final_from_other_file.carbon:[[@LINE-10]]:1: in import [InImport]
+// CHECK:STDERR: interface_z_with_impl.carbon:5:1: note: imported `final impl` here [FinalImplOverlapsDifferentFileNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl C as Z(C) {}
+
+// --- fail_final_overlaps_non_final_from_other_file.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z_with_impl";
+
+class C;
+
+// This non-final impl is subsumed by a final impl in the interface file.
+//
+// CHECK:STDERR: fail_final_overlaps_non_final_from_other_file.carbon:[[@LINE+8]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
+// CHECK:STDERR: impl C as Z(C) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_overlaps_non_final_from_other_file.carbon:[[@LINE-9]]:1: in import [InImport]
+// CHECK:STDERR: interface_z_with_impl.carbon:5:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+impl C as Z(C) {}
+
+// --- fail_final_different_file_from_self_and_interface.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z";
+import library "type_d";
+
+class C;
+
+// Can't make a final impl that is not in the same file as the self type nor
+// the interface.
+//
+// CHECK:STDERR: fail_final_different_file_from_self_and_interface.carbon:[[@LINE+4]]:1: error: `final impl` found in file that does not contain the root self type nor the interface definition [FinalImplInvalidFile]
+// CHECK:STDERR: final impl D(C) as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl D(C) as Z(()) {}
+
+// --- fail_final_different_file_from_self_and_interface_with_generic_self.carbon
+library "[[@TEST_NAME]]";
+
+import library "interface_z";
+
+class C;
+
+// Can't make a final impl that is not in the same file as the self type nor
+// the interface.
+//
+// CHECK:STDERR: fail_final_different_file_from_self_and_interface_with_generic_self.carbon:[[@LINE+4]]:1: error: `final impl` found in file that does not contain the root self type nor the interface definition [FinalImplInvalidFile]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(C) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl forall [T:! type] T as Z(C) {}
+
+// --- fail_final_overlaps_earlier_non_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+// CHECK:STDERR: fail_final_overlaps_earlier_non_final_impl.carbon:[[@LINE+3]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
+// CHECK:STDERR: impl () as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~
+impl () as Z(()) {}
+
+// CHECK:STDERR: fail_final_overlaps_earlier_non_final_impl.carbon:[[@LINE+4]]:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl forall [T:! type] T as Z(T) {}
+
+// --- fail_final_overlaps_later_non_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+final impl forall [T:! type] T as Z(T) {}
+
+// CHECK:STDERR: fail_final_overlaps_later_non_final_impl.carbon:[[@LINE+7]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
+// CHECK:STDERR: impl () as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_overlaps_later_non_final_impl.carbon:[[@LINE-5]]:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+impl () as Z(()) {}
+
+// --- fail_final_overlaps_earlier_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+class C;
+
+final impl C as Z(C) {}
+
+// CHECK:STDERR: fail_final_overlaps_earlier_final_impl.carbon:[[@LINE+7]]:1: error: `final impl` overlaps with `final impl` from same file outside a `match_first` block [FinalImplOverlapsSameFile]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_overlaps_earlier_final_impl.carbon:[[@LINE-5]]:1: note: other `final impl` here [FinalImplOverlapsSameFileNote]
+// CHECK:STDERR: final impl C as Z(C) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl forall [T:! type] T as Z(T) {}
+
+// --- fail_final_overlaps_later_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+class C;
+
+final impl forall [T:! type] T as Z(T) {}
+
+// CHECK:STDERR: fail_final_overlaps_later_final_impl.carbon:[[@LINE+7]]:1: error: `final impl` overlaps with `final impl` from same file outside a `match_first` block [FinalImplOverlapsSameFile]
+// CHECK:STDERR: final impl C as Z(C) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_overlaps_later_final_impl.carbon:[[@LINE-5]]:1: note: other `final impl` here [FinalImplOverlapsSameFileNote]
+// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl C as Z(C) {}
+
+// --- fail_non_final_type_structure_matches_non_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+interface Y {}
+
+impl forall [T:! Y] T as Z(T) {}
+
+class C(T:! type);
+
+// No diagnosis here as the type structure is unique.
+impl forall [T:! type] T as Z(C(T)) {}
+
+// CHECK:STDERR: fail_non_final_type_structure_matches_non_final_impl.carbon:[[@LINE+7]]:1: error: found non-final `impl` with the same type structure as another non-final `impl` [ImplNonFinalSameTypeStructure]
+// CHECK:STDERR: impl forall [T:! type] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_non_final_type_structure_matches_non_final_impl.carbon:[[@LINE-10]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
+// CHECK:STDERR: impl forall [T:! Y] T as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+impl forall [T:! type] T as Z(T) {}
+
+// --- non_final_type_structure_same_shape_but_different_concrete_types.carbon
+library "[[@TEST_NAME]]";
+
+interface Z {}
+
+class C;
+class D;
+
+impl C as Z {}
+impl D as Z {}
+
+// --- partial_overlap_type_structure_of_non_final_impl.carbon
+library "[[@TEST_NAME]]";
+
+interface Z(T:! type) {}
+
+impl forall [T:! type] T as Z(T) {}
+
+class C;
+// Partially overlaps the blanket impl, which is fine.
+final impl C as Z(C) {}
+
+class D;
+// Partially overlaps the blanket impl, which is fine.
+impl D as Z(D) {}
+
+// --- fail_final_overlap_where_each_is_more_specific_than_the_other.carbon
+
+interface Z(T:! type) {}
+
+class C(T:! type) {}
+
+// This should be diagnosed as overlapping final impls outside a `match_first`.
+// The first impl is more specific in the interface constraint. The second is
+// more specific in the self type. They overlap for the query `C(()) as Z(())`
+// but neither impl is completely more specific than the other.
+final impl forall [T:! type] C(T) as Z(()) {}
+// CHECK:STDERR: fail_final_overlap_where_each_is_more_specific_than_the_other.carbon:[[@LINE+7]]:1: error: `final impl` overlaps with `final impl` from same file outside a `match_first` block [FinalImplOverlapsSameFile]
+// CHECK:STDERR: final impl forall [T:! type] C(()) as Z(T) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_final_overlap_where_each_is_more_specific_than_the_other.carbon:[[@LINE-4]]:1: note: other `final impl` here [FinalImplOverlapsSameFileNote]
+// CHECK:STDERR: final impl forall [T:! type] C(T) as Z(()) {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl forall [T:! type] C(()) as Z(T) {}

+ 0 - 148
toolchain/check/testdata/impl/lookup/min_prelude/specialization_poison.carbon

@@ -169,151 +169,3 @@ fn G[X:! type](p: X*) -> (X*).(I.T) {
 final impl forall [V:! type] V* as I where .T = V {
   fn F[self: Self]() -> V { return *self; }
 }
-
-// --- fail_final_overlaps_earlier_non_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {}
-interface Y {}
-
-// CHECK:STDERR: fail_final_overlaps_earlier_non_final_impl.carbon:[[@LINE+3]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
-// CHECK:STDERR: impl () as Z(()) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~
-impl () as Z(()) {}
-
-// CHECK:STDERR: fail_final_overlaps_earlier_non_final_impl.carbon:[[@LINE+4]]:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
-// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR:
-final impl forall [T:! type] T as Z(T) {}
-
-// --- fail_final_overlaps_later_non_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {}
-interface Y {}
-
-final impl forall [T:! type] T as Z(T) {}
-
-// CHECK:STDERR: fail_final_overlaps_later_non_final_impl.carbon:[[@LINE+7]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
-// CHECK:STDERR: impl () as Z(()) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_final_overlaps_later_non_final_impl.carbon:[[@LINE-5]]:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
-// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR:
-impl () as Z(()) {}
-
-// --- fail_final_overlaps_earlier_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {}
-interface Y {}
-
-class C;
-
-// CHECK:STDERR: fail_final_overlaps_earlier_final_impl.carbon:[[@LINE+3]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
-// CHECK:STDERR: final impl C as Z(C) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~
-final impl C as Z(C) {}
-
-// CHECK:STDERR: fail_final_overlaps_earlier_final_impl.carbon:[[@LINE+4]]:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
-// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR:
-final impl forall [T:! type] T as Z(T) {}
-
-// --- fail_final_overlaps_later_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {}
-interface Y {}
-
-class C;
-
-final impl forall [T:! type] T as Z(T) {}
-
-// CHECK:STDERR: fail_final_overlaps_later_final_impl.carbon:[[@LINE+7]]:1: error: `impl` will never be used [ImplFinalOverlapsNonFinal]
-// CHECK:STDERR: final impl C as Z(C) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_final_overlaps_later_final_impl.carbon:[[@LINE-5]]:1: note: `final impl` declared here would always be used instead [ImplFinalOverlapsNonFinalNote]
-// CHECK:STDERR: final impl forall [T:! type] T as Z(T) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR:
-final impl C as Z(C) {}
-
-// --- fail_non_final_overlaps_non_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {}
-interface Y {}
-
-impl forall [T:! Y] T as Z(T) {}
-
-class C(T:! type);
-
-// No diagnosis here as the type structure is unique.
-impl forall [T:! type] T as Z(C(T)) {}
-
-// CHECK:STDERR: fail_non_final_overlaps_non_final_impl.carbon:[[@LINE+7]]:1: error: found non-final `impl` with the same type structure as another non-final `impl` [ImplNonFinalSameTypeStructure]
-// CHECK:STDERR: impl forall [T:! type] T as Z(T) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_non_final_overlaps_non_final_impl.carbon:[[@LINE-10]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
-// CHECK:STDERR: impl forall [T:! Y] T as Z(T) {}
-// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR:
-impl forall [T:! type] T as Z(T) {}
-
-// --- non_final_concrete_overlaps_non_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z {}
-
-class C;
-class D;
-
-impl C as Z {}
-impl D as Z {}
-
-// --- final_partial_overlaps_non_final_impl.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type) {}
-interface Y {}
-
-class C;
-
-impl forall [T:! type] T as Z(T) {}
-
-// Partially overlaps the previous impl, which is fine.
-final impl C as Z(C) {}
-
-// --- todo_final_partial_overlaps_final_impl_in_self_type.carbon
-library "[[@TEST_NAME]]";
-
-interface Z {}
-interface Y {}
-
-class C(T:! type, U:! type);
-
-final impl forall [T:! type] C(T, ()) as Z {}
-
-// TODO: Partially overlaps the previous impl, which may or may not be fine outside of
-// a `match_first`, depending how
-// https://github.com/carbon-language/carbon-lang/pull/5337 turns out.
-final impl forall [U:! type] C((), U) as Z {}
-
-// --- todo_final_partial_overlaps_final_impl_in_interface.carbon
-library "[[@TEST_NAME]]";
-
-interface Z(T:! type, U:! type) {}
-interface Y {}
-
-class C;
-
-final impl forall [T:! type] C as Z(T, ()) {}
-
-// TODO: Partially overlaps the previous impl, which may or may not be fine outside of
-// a `match_first`, depending how
-// https://github.com/carbon-language/carbon-lang/pull/5337 turns out.
-final impl forall [U:! type] C as Z((), U) {}

+ 4 - 4
toolchain/check/testdata/impl/min_prelude/generic_redeclaration.carbon

@@ -41,16 +41,16 @@ impl forall [T:! J] T as Interface {}
 // CHECK:STDERR: fail_todo_same_self_and_interface.carbon:[[@LINE+7]]:1: error: found non-final `impl` with the same type structure as another non-final `impl` [ImplNonFinalSameTypeStructure]
 // CHECK:STDERR: impl forall [T:! K] T as Interface {}
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_todo_same_self_and_interface.carbon:[[@LINE-4]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
-// CHECK:STDERR: impl forall [T:! J] T as Interface {}
+// CHECK:STDERR: fail_todo_same_self_and_interface.carbon:[[@LINE-12]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
+// CHECK:STDERR: impl forall [T:! I] T as Interface {}
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
 impl forall [T:! K] T as Interface {}
 // CHECK:STDERR: fail_todo_same_self_and_interface.carbon:[[@LINE+7]]:1: error: found non-final `impl` with the same type structure as another non-final `impl` [ImplNonFinalSameTypeStructure]
 // CHECK:STDERR: impl forall [T:! L] T as Interface {}
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_todo_same_self_and_interface.carbon:[[@LINE-4]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
-// CHECK:STDERR: impl forall [T:! K] T as Interface {}
+// CHECK:STDERR: fail_todo_same_self_and_interface.carbon:[[@LINE-20]]:1: note: other `impl` here [ImplNonFinalSameTypeStructureNote]
+// CHECK:STDERR: impl forall [T:! I] T as Interface {}
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
 impl forall [T:! L] T as Interface {}

+ 29 - 12
toolchain/check/type_structure.cpp

@@ -15,7 +15,8 @@
 
 namespace Carbon::Check {
 
-auto TypeStructure::IsCompatibleWith(const TypeStructure& other) const -> bool {
+auto TypeStructure::CompareStructure(CompareTest test,
+                                     const TypeStructure& other) const -> bool {
   const auto& lhs = structure_;
   const auto& rhs = other.structure_;
 
@@ -63,17 +64,33 @@ auto TypeStructure::IsCompatibleWith(const TypeStructure& other) const -> bool {
     // From here we know one side is a Symbolic and the other is not. We can
     // match the Symbolic against either a single Concrete or a larger bracketed
     // set of Concrete structural elements.
-    //
-    // We move the symbolic to the RHS to make only one case to handle in the
-    // lambda.
-    if (*lhs_cursor == Structural::Symbolic) {
-      if (!ConsumeRhsSymbolic(rhs_cursor, rhs_concrete_cursor, lhs_cursor)) {
-        return false;
-      }
-    } else {
-      if (!ConsumeRhsSymbolic(lhs_cursor, lhs_concrete_cursor, rhs_cursor)) {
-        return false;
-      }
+    switch (test) {
+      case CompareTest::IsEqualToOrMoreSpecificThan:
+        // If the symbolic is on the LHS, then the RHS structure is more
+        // specific and we return false. If the symbolic is on the RHS, we
+        // consume it and match it against the structure on the LHS.
+        if (*lhs_cursor == Structural::Symbolic) {
+          return false;
+        }
+        if (!ConsumeRhsSymbolic(lhs_cursor, lhs_concrete_cursor, rhs_cursor)) {
+          return false;
+        }
+        break;
+      case CompareTest::HasOverlap:
+        // The symbolic can be on either side, and whichever side it is on, we
+        // consume it and match it against the structure on the other side.
+        if (*lhs_cursor == Structural::Symbolic) {
+          if (!ConsumeRhsSymbolic(rhs_cursor, rhs_concrete_cursor,
+                                  lhs_cursor)) {
+            return false;
+          }
+        } else {
+          if (!ConsumeRhsSymbolic(lhs_cursor, lhs_concrete_cursor,
+                                  rhs_cursor)) {
+            return false;
+          }
+        }
+        break;
     }
   }
 

+ 19 - 5
toolchain/check/type_structure.h

@@ -23,10 +23,24 @@ namespace Carbon::Check {
 // better, more specified, match.
 class TypeStructure : public Printable<TypeStructure> {
  public:
-  // Returns whether the type structure is compatible with `other`. If false,
-  // they can not possibly match with one being an `impl` for the other as a
-  // lookup query.
-  auto IsCompatibleWith(const TypeStructure& other) const -> bool;
+  enum class CompareTest {
+    // Test whether `this` has the same structure as `other`, or `this` is
+    // strictly more specific (has more concrete values) than `other` while
+    // maintaining a compatible structure.
+    //
+    // If false, they can not possibly match with `this` being a lookup query
+    // and `other` being an `impl`.
+    IsEqualToOrMoreSpecificThan,
+
+    // Tests whether there is a possible query that could match both `this` and
+    // `other`, in which case we say `this` has overlap with `other`.
+    HasOverlap,
+  };
+
+  // Compares the structure of `this` and `other`, and returns whether the
+  // structures match according to the specified test.
+  auto CompareStructure(CompareTest test, const TypeStructure& other) const
+      -> bool;
 
   // Ordering of type structures. A lower value is a better match.
   // TODO: switch to operator<=> once we can depend on
@@ -143,7 +157,7 @@ class TypeStructure : public Printable<TypeStructure> {
         symbolic_type_indices_(std::move(symbolic_type_indices)),
         concrete_types_(std::move(concrete_types)) {}
 
-  // A helper for IsCompatibleWith.
+  // A helper for CompareStructure.
   static auto ConsumeRhsSymbolic(
       llvm::SmallVector<Structural>::const_iterator& lhs_cursor,
       llvm::SmallVector<ConcreteType>::const_iterator& lhs_concrete_cursor,

+ 5 - 0
toolchain/diagnostics/diagnostic_kind.def

@@ -320,6 +320,11 @@ CARBON_DIAGNOSTIC_KIND(ImplFinalOverlapsNonFinal)
 CARBON_DIAGNOSTIC_KIND(ImplFinalOverlapsNonFinalNote)
 CARBON_DIAGNOSTIC_KIND(ImplNonFinalSameTypeStructure)
 CARBON_DIAGNOSTIC_KIND(ImplNonFinalSameTypeStructureNote)
+CARBON_DIAGNOSTIC_KIND(FinalImplInvalidFile)
+CARBON_DIAGNOSTIC_KIND(FinalImplOverlapsSameFile)
+CARBON_DIAGNOSTIC_KIND(FinalImplOverlapsSameFileNote)
+CARBON_DIAGNOSTIC_KIND(FinalImplOverlapsDifferentFile)
+CARBON_DIAGNOSTIC_KIND(FinalImplOverlapsDifferentFileNote)
 
 // Impl lookup.
 CARBON_DIAGNOSTIC_KIND(MissingImplInMemberAccess)