Fix handling of interface redefinitions. (#4198)

The prior code crashed when trying to find the function's `Self`
parameter, I believe. `fail_redefine_with_dependents.carbon` handles
this case. It wasn't caught by the prior case because the `F` didn't
have any dependent parameters.

Note this also ran into a formatter crash, with invalid constants. I'm
fixing that here, but will also note it on #4145 (the crash in
FinishGenericDecl was muddled by a crash in Formatter code).

toolchain/check/handle_interface.cpp

@@ -29,7 +29,8 @@ auto HandleParseNode(Context& context, Parse::InterfaceIntroducerId node_id)
 }
 
 static auto BuildInterfaceDecl(Context& context,
-                               Parse::AnyInterfaceDeclId node_id)
+                               Parse::AnyInterfaceDeclId node_id,
+                               bool is_definition)
     -> std::tuple<SemIR::InterfaceId, SemIR::InstId> {
   auto name = PopNameComponent(context);
   auto name_context = context.decl_name_stack().FinishName(name);
@@ -58,21 +59,37 @@ static auto BuildInterfaceDecl(Context& context,
   if (existing_id.is_valid()) {
     if (auto existing_interface_decl =
             context.insts().Get(existing_id).TryAs<SemIR::InterfaceDecl>()) {
-      // TODO: Implement full redeclaration checking. See `MergeClassDecl`. For
-      // now we just check the generic parameters match.
+      auto existing_interface =
+          context.interfaces().Get(existing_interface_decl->interface_id);
       if (CheckRedeclParamsMatch(
               context,
               DeclParams(interface_decl_id, name.first_param_node_id,
                          name.last_param_node_id, name.implicit_params_id,
                          name.params_id),
-              DeclParams(context.interfaces().Get(
-                  existing_interface_decl->interface_id)))) {
-        // This is a redeclaration of an existing interface.
-        interface_decl.interface_id = existing_interface_decl->interface_id;
-        interface_decl.type_id = existing_interface_decl->type_id;
-        // TODO: If the new declaration is a definition, keep its parameter
-        // and implicit parameter lists rather than the ones from the
-        // previous declaration.
+              DeclParams(existing_interface))) {
+        // TODO: This should be refactored a little, particularly for
+        // prev_import_ir_id. See similar logic for classes and functions, which
+        // might also be refactored to merge.
+        CheckIsAllowedRedecl(context, Lex::TokenKind::Interface,
+                             existing_interface.name_id,
+                             {.loc = node_id,
+                              .is_definition = is_definition,
+                              .is_extern = false},
+                             {.loc = existing_interface.decl_id,
+                              .is_definition = existing_interface.is_defined(),
+                              .is_extern = false},
+                             /*prev_import_ir_id=*/SemIR::ImportIRId::Invalid);
+
+        // Can't merge interface definitions due to the generic requirements.
+        // TODO: Should this also be mirrored to classes/functions for generics?
+        if (!is_definition || !existing_interface.is_defined()) {
+          // This is a redeclaration of an existing interface.
+          interface_decl.interface_id = existing_interface_decl->interface_id;
+          interface_decl.type_id = existing_interface_decl->type_id;
+          // TODO: If the new declaration is a definition, keep its parameter
+          // and implicit parameter lists rather than the ones from the
+          // previous declaration.
+        }
       }
     } else {
       // This is a redeclaration of something other than a interface.
@@ -82,20 +99,13 @@ static auto BuildInterfaceDecl(Context& context,
 
   // Create a new interface if this isn't a valid redeclaration.
   if (!interface_decl.interface_id.is_valid()) {
-    auto generic_id = FinishGenericDecl(context, interface_decl_id);
     // TODO: If this is an invalid redeclaration of a non-interface entity or
     // there was an error in the qualifier, we will have lost track of the
     // interface name here. We should keep track of it even if the name is
     // invalid.
     SemIR::Interface interface_info = {
-        {.name_id = name_context.name_id_for_new_inst(),
-         .parent_scope_id = name_context.parent_scope_id_for_new_inst(),
-         .generic_id = generic_id,
-         .first_param_node_id = name.first_param_node_id,
-         .last_param_node_id = name.last_param_node_id,
-         .implicit_param_refs_id = name.implicit_params_id,
-         .param_refs_id = name.params_id,
-         .decl_id = interface_decl_id}};
+        name_context.MakeEntityWithParamsBase(interface_decl_id, name)};
+    interface_info.generic_id = FinishGenericDecl(context, interface_decl_id);
     interface_decl.interface_id = context.interfaces().Add(interface_info);
     if (interface_info.has_parameters()) {
       interface_decl.type_id =
@@ -114,32 +124,24 @@ static auto BuildInterfaceDecl(Context& context,
 }
 
 auto HandleParseNode(Context& context, Parse::InterfaceDeclId node_id) -> bool {
-  BuildInterfaceDecl(context, node_id);
+  BuildInterfaceDecl(context, node_id, /*is_definition=*/false);
   context.decl_name_stack().PopScope();
   return true;
 }
 
 auto HandleParseNode(Context& context,
                      Parse::InterfaceDefinitionStartId node_id) -> bool {
-  auto [interface_id, interface_decl_id] = BuildInterfaceDecl(context, node_id);
+  auto [interface_id, interface_decl_id] =
+      BuildInterfaceDecl(context, node_id, /*is_definition=*/true);
   auto& interface_info = context.interfaces().Get(interface_id);
 
   // Track that this declaration is the definition.
-  if (interface_info.is_defined()) {
-    CARBON_DIAGNOSTIC(InterfaceRedefinition, Error,
-                      "Redefinition of interface {0}.", SemIR::NameId);
-    CARBON_DIAGNOSTIC(InterfacePreviousDefinition, Note,
-                      "Previous definition was here.");
-    context.emitter()
-        .Build(node_id, InterfaceRedefinition, interface_info.name_id)
-        .Note(interface_info.definition_id, InterfacePreviousDefinition)
-        .Emit();
-  } else {
-    interface_info.definition_id = interface_decl_id;
-    interface_info.scope_id =
-        context.name_scopes().Add(interface_decl_id, SemIR::NameId::Invalid,
-                                  interface_info.parent_scope_id);
-  }
+  CARBON_CHECK(!interface_info.is_defined())
+      << "Can't merge with defined interfaces.";
+  interface_info.definition_id = interface_decl_id;
+  interface_info.scope_id =
+      context.name_scopes().Add(interface_decl_id, SemIR::NameId::Invalid,
+                                interface_info.parent_scope_id);
 
   auto self_specific_id =
       context.generics().GetSelfSpecific(interface_info.generic_id);

toolchain/check/testdata/interface/no_prelude/fail_duplicate.carbon

@@ -8,12 +8,16 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interface/no_prelude/fail_duplicate.carbon
 
+// --- fail_redefine_without_dependents.carbon
+
+library "redefine_without_dependents";
+
 interface Interface { }
 
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+7]]:1: ERROR: Redefinition of interface Interface.
+// CHECK:STDERR: fail_redefine_without_dependents.carbon:[[@LINE+7]]:1: ERROR: Redefinition of `interface Interface`.
 // CHECK:STDERR: interface Interface {
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE-5]]:1: Previous definition was here.
+// CHECK:STDERR: fail_redefine_without_dependents.carbon:[[@LINE-5]]:1: Previously defined here.
 // CHECK:STDERR: interface Interface { }
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
@@ -21,79 +25,199 @@ interface Interface {
   fn F();
 }
 
+// --- fail_redefine_with_dependents.carbon
+
+library "redefine_with_dependents";
+
+interface Interface {}
+
+// CHECK:STDERR: fail_redefine_with_dependents.carbon:[[@LINE+7]]:1: ERROR: Redefinition of `interface Interface`.
+// CHECK:STDERR: interface Interface {
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_redefine_with_dependents.carbon:[[@LINE-5]]:1: Previously defined here.
+// CHECK:STDERR: interface Interface {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+interface Interface {
+  fn F[self: Self]();
+}
+
+// --- fail_name_conflict_with_fn.carbon
+
+library "name_conflict";
+
 fn Function();
 
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+7]]:1: ERROR: Duplicate name being declared in the same scope.
+// CHECK:STDERR: fail_name_conflict_with_fn.carbon:[[@LINE+7]]:1: ERROR: Duplicate name being declared in the same scope.
 // CHECK:STDERR: interface Function;
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE-5]]:1: Name is previously declared here.
+// CHECK:STDERR: fail_name_conflict_with_fn.carbon:[[@LINE-5]]:1: Name is previously declared here.
 // CHECK:STDERR: fn Function();
 // CHECK:STDERR: ^~~~~~~~~~~~~~
 // CHECK:STDERR:
 interface Function;
 
+// --- fail_name_conflict_with_class.carbon
+
 class Class;
 
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope.
+// CHECK:STDERR: fail_name_conflict_with_class.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope.
 // CHECK:STDERR: interface Class { }
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE-5]]:1: Name is previously declared here.
+// CHECK:STDERR: fail_name_conflict_with_class.carbon:[[@LINE-5]]:1: Name is previously declared here.
 // CHECK:STDERR: class Class;
 // CHECK:STDERR: ^~~~~~~~~~~~
 interface Class { }
 
-// CHECK:STDOUT: --- fail_duplicate.carbon
+// CHECK:STDOUT: --- fail_redefine_without_dependents.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %.1: type = interface_type @Interface [template]
 // CHECK:STDOUT:   %Self.1: %.1 = bind_symbolic_name Self 0 [symbolic]
+// CHECK:STDOUT:   %.2: type = interface_type @.1 [template]
+// CHECK:STDOUT:   %Self.2: %.2 = bind_symbolic_name Self 0 [symbolic]
 // CHECK:STDOUT:   %F.type: type = fn_type @F [template]
-// CHECK:STDOUT:   %.2: type = tuple_type () [template]
+// CHECK:STDOUT:   %.3: type = tuple_type () [template]
 // CHECK:STDOUT:   %F: %F.type = struct_value () [template]
-// CHECK:STDOUT:   %Function.type: type = fn_type @Function [template]
-// CHECK:STDOUT:   %Function: %Function.type = struct_value () [template]
-// CHECK:STDOUT:   %.3: type = interface_type @.1 [template]
-// CHECK:STDOUT:   %Class: type = class_type @Class [template]
-// CHECK:STDOUT:   %.4: type = interface_type @.2 [template]
-// CHECK:STDOUT:   %Self.2: %.4 = bind_symbolic_name Self 0 [symbolic]
+// CHECK:STDOUT:   %.4: type = assoc_entity_type %.2, %F.type [template]
+// CHECK:STDOUT:   %.5: %.4 = assoc_entity element0, @.1.%F.decl [template]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: file {
 // CHECK:STDOUT:   package: <namespace> = namespace [template] {
-// CHECK:STDOUT:     .Interface = %Interface.decl.loc11
-// CHECK:STDOUT:     .Function = %Function.decl
-// CHECK:STDOUT:     .Class = %Class.decl
+// CHECK:STDOUT:     .Interface = %Interface.decl
 // CHECK:STDOUT:   }
-// CHECK:STDOUT:   %Interface.decl.loc11: type = interface_decl @Interface [template = constants.%.1] {}
-// CHECK:STDOUT:   %Interface.decl.loc20: type = interface_decl @Interface [template = constants.%.1] {}
-// CHECK:STDOUT:   %Function.decl: %Function.type = fn_decl @Function [template = constants.%Function] {}
-// CHECK:STDOUT:   %.decl.loc33: type = interface_decl @.1 [template = constants.%.3] {}
-// CHECK:STDOUT:   %Class.decl: type = class_decl @Class [template = constants.%Class] {}
-// CHECK:STDOUT:   %.decl.loc43: type = interface_decl @.2 [template = constants.%.4] {}
+// CHECK:STDOUT:   %Interface.decl: type = interface_decl @Interface [template = constants.%.1] {}
+// CHECK:STDOUT:   %.decl: type = interface_decl @.1 [template = constants.%.2] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: interface @Interface {
-// CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [template = constants.%F] {}
+// CHECK:STDOUT:   %Self: %.1 = bind_symbolic_name Self 0 [symbolic = constants.%Self.1]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
-// CHECK:STDOUT:   .Self = <unexpected>.inst+3
-// CHECK:STDOUT:   .F = <error>
+// CHECK:STDOUT:   .Self = %Self
 // CHECK:STDOUT:   witness = ()
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: interface @.1;
+// CHECK:STDOUT: interface @.1 {
+// CHECK:STDOUT:   %Self: %.2 = bind_symbolic_name Self 0 [symbolic = constants.%Self.2]
+// CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [template = constants.%F] {}
+// CHECK:STDOUT:   %.loc14: %.4 = assoc_entity element0, %F.decl [template = constants.%.5]
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   .Self = %Self
+// CHECK:STDOUT:   .F = %.loc14
+// CHECK:STDOUT:   witness = (%F.decl)
+// CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: interface @.2 {
-// CHECK:STDOUT:   %Self: %.4 = bind_symbolic_name Self 0 [symbolic = constants.%Self.2]
+// CHECK:STDOUT: generic fn @F(@.1.%Self: %.2) {
+// CHECK:STDOUT:
+// CHECK:STDOUT:   fn();
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: specific @F(constants.%Self.2) {}
+// CHECK:STDOUT:
+// CHECK:STDOUT: --- fail_redefine_with_dependents.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %.1: type = interface_type @Interface [template]
+// CHECK:STDOUT:   %Self.1: %.1 = bind_symbolic_name Self 0 [symbolic]
+// CHECK:STDOUT:   %.2: type = interface_type @.1 [template]
+// CHECK:STDOUT:   %Self.2: %.2 = bind_symbolic_name Self 0 [symbolic]
+// CHECK:STDOUT:   %F.type: type = fn_type @F [template]
+// CHECK:STDOUT:   %.3: type = tuple_type () [template]
+// CHECK:STDOUT:   %F: %F.type = struct_value () [template]
+// CHECK:STDOUT:   %.4: type = assoc_entity_type %.2, %F.type [template]
+// CHECK:STDOUT:   %.5: %.4 = assoc_entity element0, @.1.%F.decl [template]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   package: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .Interface = %Interface.decl
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %Interface.decl: type = interface_decl @Interface [template = constants.%.1] {}
+// CHECK:STDOUT:   %.decl: type = interface_decl @.1 [template = constants.%.2] {}
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: interface @Interface {
+// CHECK:STDOUT:   %Self: %.1 = bind_symbolic_name Self 0 [symbolic = constants.%Self.1]
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   .Self = %Self
 // CHECK:STDOUT:   witness = ()
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: class @Class;
+// CHECK:STDOUT: interface @.1 {
+// CHECK:STDOUT:   %Self: %.2 = bind_symbolic_name Self 0 [symbolic = constants.%Self.2]
+// CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [template = constants.%F] {
+// CHECK:STDOUT:     %Self.ref: %.2 = name_ref Self, %Self [symbolic = @F.%Self (constants.%Self.2)]
+// CHECK:STDOUT:     %.loc14_14.1: type = facet_type_access %Self.ref [symbolic = @F.%Self (constants.%Self.2)]
+// CHECK:STDOUT:     %.loc14_14.2: type = converted %Self.ref, %.loc14_14.1 [symbolic = @F.%Self (constants.%Self.2)]
+// CHECK:STDOUT:     %self.loc14_8.1: @F.%Self (%Self.2) = param self
+// CHECK:STDOUT:     %self.loc14_8.2: @F.%Self (%Self.2) = bind_name self, %self.loc14_8.1
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %.loc14_21: %.4 = assoc_entity element0, %F.decl [template = constants.%.5]
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   .Self = %Self
+// CHECK:STDOUT:   .F = %.loc14_21
+// CHECK:STDOUT:   witness = (%F.decl)
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: generic fn @F(@.1.%Self: %.2) {
+// CHECK:STDOUT:   %Self: %.2 = bind_symbolic_name Self 0 [symbolic = %Self (constants.%Self.2)]
+// CHECK:STDOUT:
+// CHECK:STDOUT:   fn[@.1.%self.loc14_8.2: @F.%Self (%Self.2)]();
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: specific @F(constants.%Self.2) {
+// CHECK:STDOUT:   %Self => constants.%Self.2
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: --- fail_name_conflict_with_fn.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %Function.type: type = fn_type @Function [template]
+// CHECK:STDOUT:   %.1: type = tuple_type () [template]
+// CHECK:STDOUT:   %Function: %Function.type = struct_value () [template]
+// CHECK:STDOUT:   %.2: type = interface_type @.1 [template]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   package: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .Function = %Function.decl
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %Function.decl: %Function.type = fn_decl @Function [template = constants.%Function] {}
+// CHECK:STDOUT:   %.decl: type = interface_decl @.1 [template = constants.%.2] {}
+// CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: fn @F();
+// CHECK:STDOUT: interface @.1;
 // CHECK:STDOUT:
 // CHECK:STDOUT: fn @Function();
 // CHECK:STDOUT:
+// CHECK:STDOUT: --- fail_name_conflict_with_class.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %Class: type = class_type @Class [template]
+// CHECK:STDOUT:   %.1: type = interface_type @.1 [template]
+// CHECK:STDOUT:   %Self: %.1 = bind_symbolic_name Self 0 [symbolic]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   package: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .Class = %Class.decl
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %Class.decl: type = class_decl @Class [template = constants.%Class] {}
+// CHECK:STDOUT:   %.decl: type = interface_decl @.1 [template = constants.%.1] {}
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: interface @.1 {
+// CHECK:STDOUT:   %Self: %.1 = bind_symbolic_name Self 0 [symbolic = constants.%Self]
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   .Self = %Self
+// CHECK:STDOUT:   witness = ()
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: class @Class;
+// CHECK:STDOUT:

toolchain/diagnostics/diagnostic_kind.def

@@ -216,8 +216,6 @@ CARBON_DIAGNOSTIC_KIND(ExportRedundant)
 CARBON_DIAGNOSTIC_KIND(ExportPrevious)
 
 // Interface checking.
-CARBON_DIAGNOSTIC_KIND(InterfacePreviousDefinition)
-CARBON_DIAGNOSTIC_KIND(InterfaceRedefinition)
 CARBON_DIAGNOSTIC_KIND(InterfaceForwardDeclaredHere)
 CARBON_DIAGNOSTIC_KIND(InterfaceUndefinedWithinDefinition)
 

toolchain/sem_ir/formatter.cpp

@@ -552,7 +552,8 @@ class FormatterImpl {
     out_ << InstT::Kind.ir_name();
     pending_constant_value_ = sem_ir_.constant_values().Get(inst_id);
     pending_constant_value_is_self_ =
-        sem_ir_.constant_values().GetInstId(pending_constant_value_) == inst_id;
+        sem_ir_.constant_values().GetInstIdIfValid(pending_constant_value_) ==
+        inst_id;
     FormatInstRHS(inst);
     FormatPendingConstantValue(AddSpace::Before);
     out_ << "\n";