From 3d13b8f71c574dc1e0a1ed78e481c6fb89524e14 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 7 Aug 2024 14:30:44 -0700 Subject: [PATCH] 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 | 76 +++---- .../no_prelude/fail_duplicate.carbon | 188 +++++++++++++++--- toolchain/diagnostics/diagnostic_kind.def | 2 - toolchain/sem_ir/formatter.cpp | 3 +- 4 files changed, 197 insertions(+), 72 deletions(-) diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index b3d2e855d2c6..c4f56149a013 100644 --- a/toolchain/check/handle_interface.cpp +++ b/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 { 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()) { - // 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); diff --git a/toolchain/check/testdata/interface/no_prelude/fail_duplicate.carbon b/toolchain/check/testdata/interface/no_prelude/fail_duplicate.carbon index 355d6fc31ae5..fb4f1a1b2339 100644 --- a/toolchain/check/testdata/interface/no_prelude/fail_duplicate.carbon +++ b/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,70 +25,194 @@ 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 [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 = .inst+3 -// CHECK:STDOUT: .F = +// CHECK:STDOUT: .Self = %Self // CHECK:STDOUT: witness = () // CHECK:STDOUT: } // CHECK:STDOUT: +// 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: 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 [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: 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 [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: interface @.1; // CHECK:STDOUT: -// CHECK:STDOUT: interface @.2 { -// CHECK:STDOUT: %Self: %.4 = bind_symbolic_name Self 0 [symbolic = constants.%Self.2] +// 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 [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 @@ -93,7 +221,3 @@ interface Class { } // CHECK:STDOUT: // CHECK:STDOUT: class @Class; // CHECK:STDOUT: -// CHECK:STDOUT: fn @F(); -// CHECK:STDOUT: -// CHECK:STDOUT: fn @Function(); -// CHECK:STDOUT: diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index af2d74ce4d9e..b88aece6d7a9 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/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) diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 3c0de28d2573..ac90c6503ae9 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/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";