Stop merging invalid impl redefinitions (#4798)

Fixes a crash, see the new regression test in toolchain/check/testdata/impl/no_prelude/generic_redeclaration.carbon. Stopping merging seems like the most straightforward way to prevent references to generic regions with the incorrect block.
carbon-language · Jan 14, 2025 · 9dc450e · 9dc450e
1 parent a3e66d6
commit 9dc450e
Show file tree

Hide file tree

Showing 4 changed files with 341 additions and 65 deletions.
diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp
@@ -295,6 +295,22 @@ static auto IsValidImplRedecl(Context& context, SemIR::Impl& new_impl,
     return false;
   }
 
+  if (prev_impl.has_definition_started()) {
+    // Impls aren't merged in order to avoid generic region lookup into a
+    // mismatching table.
+    CARBON_DIAGNOSTIC(ImplRedefinition, Error,
+                      "redefinition of `impl {0} as {1}`", InstIdAsRawType,
+                      InstIdAsRawType);
+    CARBON_DIAGNOSTIC(ImplPreviousDefinition, Note,
+                      "previous definition was here");
+    context.emitter()
+        .Build(new_impl.latest_decl_id(), ImplRedefinition, new_impl.self_id,
+               new_impl.constraint_id)
+        .Note(prev_impl.definition_id, ImplPreviousDefinition)
+        .Emit();
+    return false;
+  }
+
   // TODO: Only allow redeclaration in a match_first/impl_priority block.
 
   // TODO: Merge information from the new declaration into the old one as
@@ -427,23 +443,11 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
       BuildImplDecl(context, node_id, /*is_definition=*/true);
   auto& impl_info = context.impls().Get(impl_id);
 
-  if (impl_info.has_definition_started()) {
-    CARBON_DIAGNOSTIC(ImplRedefinition, Error,
-                      "redefinition of `impl {0} as {1}`", InstIdAsRawType,
-                      InstIdAsRawType);
-    CARBON_DIAGNOSTIC(ImplPreviousDefinition, Note,
-                      "previous definition was here");
-    context.emitter()
-        .Build(node_id, ImplRedefinition, impl_info.self_id,
-               impl_info.constraint_id)
-        .Note(impl_info.definition_id, ImplPreviousDefinition)
-        .Emit();
-  } else {
-    impl_info.definition_id = impl_decl_id;
-    impl_info.scope_id = context.name_scopes().Add(
-        impl_decl_id, SemIR::NameId::Invalid,
-        context.decl_name_stack().PeekParentScopeId());
-  }
+  CARBON_CHECK(!impl_info.has_definition_started());
+  impl_info.definition_id = impl_decl_id;
+  impl_info.scope_id =
+      context.name_scopes().Add(impl_decl_id, SemIR::NameId::Invalid,
+                                context.decl_name_stack().PeekParentScopeId());
 
   context.scope_stack().Push(
       impl_decl_id, impl_info.scope_id,

diff --git a/toolchain/check/testdata/impl/fail_redefinition.carbon b/toolchain/check/testdata/impl/fail_redefinition.carbon
@@ -45,17 +45,18 @@ impl i32 as I {}
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Core.import = import Core
 // CHECK:STDOUT:   %I.decl: type = interface_decl @I [template = constants.%I.type] {} {}
-// CHECK:STDOUT:   impl_decl @impl [template] {} {
-// CHECK:STDOUT:     %int_32.loc13: Core.IntLiteral = int_value 32 [template = constants.%int_32]
-// CHECK:STDOUT:     %i32.loc13: type = class_type @Int, @Int(constants.%int_32) [template = constants.%i32]
-// CHECK:STDOUT:     %I.ref.loc13: type = name_ref I, file.%I.decl [template = constants.%I.type]
+// CHECK:STDOUT:   impl_decl @impl.1 [template] {} {
+// CHECK:STDOUT:     %int_32: Core.IntLiteral = int_value 32 [template = constants.%int_32]
+// CHECK:STDOUT:     %i32: type = class_type @Int, @Int(constants.%int_32) [template = constants.%i32]
+// CHECK:STDOUT:     %I.ref: type = name_ref I, file.%I.decl [template = constants.%I.type]
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness () [template = constants.%impl_witness]
-// CHECK:STDOUT:   impl_decl @impl [template] {} {
-// CHECK:STDOUT:     %int_32.loc21: Core.IntLiteral = int_value 32 [template = constants.%int_32]
-// CHECK:STDOUT:     %i32.loc21: type = class_type @Int, @Int(constants.%int_32) [template = constants.%i32]
-// CHECK:STDOUT:     %I.ref.loc21: type = name_ref I, file.%I.decl [template = constants.%I.type]
+// CHECK:STDOUT:   %impl_witness.loc13: <witness> = impl_witness () [template = constants.%impl_witness]
+// CHECK:STDOUT:   impl_decl @impl.2 [template] {} {
+// CHECK:STDOUT:     %int_32: Core.IntLiteral = int_value 32 [template = constants.%int_32]
+// CHECK:STDOUT:     %i32: type = class_type @Int, @Int(constants.%int_32) [template = constants.%i32]
+// CHECK:STDOUT:     %I.ref: type = name_ref I, file.%I.decl [template = constants.%I.type]
 // CHECK:STDOUT:   }
+// CHECK:STDOUT:   %impl_witness.loc21: <witness> = impl_witness () [template = constants.%impl_witness]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: interface @I {
@@ -66,8 +67,13 @@ impl i32 as I {}
 // CHECK:STDOUT:   witness = ()
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: impl @impl: %i32.loc13 as %I.ref.loc13 {
+// CHECK:STDOUT: impl @impl.1: %i32 as %I.ref {
 // CHECK:STDOUT: !members:
-// CHECK:STDOUT:   witness = file.%impl_witness
+// CHECK:STDOUT:   witness = file.%impl_witness.loc13
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: impl @impl.2: %i32 as %I.ref {
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   witness = file.%impl_witness.loc21
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
diff --git a/toolchain/check/testdata/impl/no_prelude/fail_alias.carbon b/toolchain/check/testdata/impl/no_prelude/fail_alias.carbon
@@ -48,15 +48,16 @@ impl AC as AI {}
 // CHECK:STDOUT:   %AI: type = bind_alias AI, %I.decl [template = constants.%I.type]
 // CHECK:STDOUT:   %C.ref: type = name_ref C, %C.decl [template = constants.%C]
 // CHECK:STDOUT:   %AC: type = bind_alias AC, %C.decl [template = constants.%C]
-// CHECK:STDOUT:   impl_decl @impl [template] {} {
-// CHECK:STDOUT:     %AC.ref.loc17: type = name_ref AC, file.%AC [template = constants.%C]
-// CHECK:STDOUT:     %AI.ref.loc17: type = name_ref AI, file.%AI [template = constants.%I.type]
+// CHECK:STDOUT:   impl_decl @impl.1 [template] {} {
+// CHECK:STDOUT:     %AC.ref: type = name_ref AC, file.%AC [template = constants.%C]
+// CHECK:STDOUT:     %AI.ref: type = name_ref AI, file.%AI [template = constants.%I.type]
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %impl_witness: <witness> = impl_witness () [template = constants.%impl_witness]
-// CHECK:STDOUT:   impl_decl @impl [template] {} {
-// CHECK:STDOUT:     %AC.ref.loc25: type = name_ref AC, file.%AC [template = constants.%C]
-// CHECK:STDOUT:     %AI.ref.loc25: type = name_ref AI, file.%AI [template = constants.%I.type]
+// CHECK:STDOUT:   %impl_witness.loc17: <witness> = impl_witness () [template = constants.%impl_witness]
+// CHECK:STDOUT:   impl_decl @impl.2 [template] {} {
+// CHECK:STDOUT:     %AC.ref: type = name_ref AC, file.%AC [template = constants.%C]
+// CHECK:STDOUT:     %AI.ref: type = name_ref AI, file.%AI [template = constants.%I.type]
 // CHECK:STDOUT:   }
+// CHECK:STDOUT:   %impl_witness.loc25: <witness> = impl_witness () [template = constants.%impl_witness]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: interface @I {
@@ -67,9 +68,14 @@ impl AC as AI {}
 // CHECK:STDOUT:   witness = ()
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: impl @impl: %AC.ref.loc17 as %AI.ref.loc17 {
+// CHECK:STDOUT: impl @impl.1: %AC.ref as %AI.ref {
 // CHECK:STDOUT: !members:
-// CHECK:STDOUT:   witness = file.%impl_witness
+// CHECK:STDOUT:   witness = file.%impl_witness.loc17
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: impl @impl.2: %AC.ref as %AI.ref {
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   witness = file.%impl_witness.loc25
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @C {