From 0feb757de01ea9757c071ac5121c3929969a6ccc Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Mon, 12 Aug 2024 12:50:15 -0700 Subject: [PATCH] Add fields for extern to EntityWithParamsBase (#4206) This adds fields to `EntityWithParamsBase` to reflect the intention with `extern library` design. I'm renaming `decl_id` because it shouldn't be expected to be assigned anymore. import_ref.cpp I'm deliberately keeping on `first_owning_decl_id` (which will break when importing `extern library` declarations). Most other cases are for diagnostics, and I'm using `latest_decl_id` to try and get the closest declaration to the error. Note I'm partly splitting out this PR to show the test effect, which apparently we don't test related cases. --- toolchain/check/context.cpp | 4 +-- toolchain/check/convert.cpp | 10 +++--- toolchain/check/decl_name_stack.h | 9 +++-- toolchain/check/function.cpp | 10 +++--- toolchain/check/global_init.cpp | 6 ++-- toolchain/check/handle_class.cpp | 6 ++-- toolchain/check/handle_function.cpp | 13 ++++---- toolchain/check/handle_interface.cpp | 6 ++-- toolchain/check/impl.cpp | 3 +- toolchain/check/import_ref.cpp | 38 +++++++++++++--------- toolchain/check/merge.h | 2 +- toolchain/check/return.cpp | 2 +- toolchain/sem_ir/entity_with_params_base.h | 23 ++++++++++--- toolchain/sem_ir/function.h | 2 -- 14 files changed, 80 insertions(+), 54 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 2beb3936a7c9f..15b82d9bb57c7 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -198,7 +198,7 @@ auto Context::NoteIncompleteClass(SemIR::ClassId class_id, } else { CARBON_DIAGNOSTIC(ClassForwardDeclaredHere, Note, "Class was forward declared here."); - builder.Note(class_info.decl_id, ClassForwardDeclaredHere); + builder.Note(class_info.latest_decl_id(), ClassForwardDeclaredHere); } } @@ -214,7 +214,7 @@ auto Context::NoteUndefinedInterface(SemIR::InterfaceId interface_id, } else { CARBON_DIAGNOSTIC(InterfaceForwardDeclaredHere, Note, "Interface was forward declared here."); - builder.Note(interface_info.decl_id, InterfaceForwardDeclaredHere); + builder.Note(interface_info.latest_decl_id(), InterfaceForwardDeclaredHere); } } diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index 307f81dc06c05..baa0d3a02801e 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -1193,7 +1193,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, context.emitter() .Build(call_loc_id, CallArgCountMismatch, arg_refs.size(), param_refs.size()) - .Note(callee.decl_id, InCallToFunction) + .Note(callee.latest_decl_id(), InCallToFunction) .Emit(); return SemIR::InstBlockId::Invalid; } @@ -1210,9 +1210,9 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, auto [param_id, param] = SemIR::Function::GetParamFromParamRefId( context.sem_ir(), implicit_param_id); if (param.name_id == SemIR::NameId::SelfValue) { - auto converted_self_id = - ConvertSelf(context, call_loc_id, callee.decl_id, callee_specific_id, - addr_pattern, param_id, param, self_id); + auto converted_self_id = ConvertSelf( + context, call_loc_id, callee.latest_decl_id(), callee_specific_id, + addr_pattern, param_id, param, self_id); if (converted_self_id == SemIR::InstId::BuiltinError) { return SemIR::InstBlockId::Invalid; } @@ -1230,7 +1230,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id, CARBON_DIAGNOSTIC( InCallToFunctionParam, Note, "Initializing parameter {0} of function declared here.", int); - builder.Note(callee.decl_id, InCallToFunctionParam, + builder.Note(callee.latest_decl_id(), InCallToFunctionParam, diag_param_index + 1); }); diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h index f5e1152cb960b..f0d87c280e733 100644 --- a/toolchain/check/decl_name_stack.h +++ b/toolchain/check/decl_name_stack.h @@ -90,8 +90,8 @@ class DeclNameStack { // Combines name information to produce a base struct for entity // construction. - auto MakeEntityWithParamsBase(SemIR::InstId decl_id, - const NameComponent& name) + auto MakeEntityWithParamsBase(const NameComponent& name, + SemIR::InstId decl_id, bool is_extern) -> SemIR::EntityWithParamsBase { return { .name_id = name_id_for_new_inst(), @@ -101,7 +101,10 @@ class DeclNameStack { .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 = decl_id, + .is_extern = is_extern, + .extern_library_id = StringLiteralValueId::Invalid, + .non_owning_decl_id = SemIR::InstId::Invalid, + .first_owning_decl_id = decl_id, }; } diff --git a/toolchain/check/function.cpp b/toolchain/check/function.cpp index 6aaca392b189d..2e09ed2c10b93 100644 --- a/toolchain/check/function.cpp +++ b/toolchain/check/function.cpp @@ -41,21 +41,21 @@ auto CheckFunctionTypeMatches(Context& context, "Function redeclaration differs because no return type is provided."); auto diag = new_return_type_id.is_valid() - ? context.emitter().Build(new_function.decl_id, + ? context.emitter().Build(new_function.latest_decl_id(), FunctionRedeclReturnTypeDiffers, new_return_type_id) - : context.emitter().Build(new_function.decl_id, + : context.emitter().Build(new_function.latest_decl_id(), FunctionRedeclReturnTypeDiffersNoReturn); if (prev_return_type_id.is_valid()) { CARBON_DIAGNOSTIC(FunctionRedeclReturnTypePrevious, Note, "Previously declared with return type `{0}`.", SemIR::TypeId); - diag.Note(prev_function.decl_id, FunctionRedeclReturnTypePrevious, - prev_return_type_id); + diag.Note(prev_function.latest_decl_id(), + FunctionRedeclReturnTypePrevious, prev_return_type_id); } else { CARBON_DIAGNOSTIC(FunctionRedeclReturnTypePreviousNoReturn, Note, "Previously declared with no return type."); - diag.Note(prev_function.decl_id, + diag.Note(prev_function.latest_decl_id(), FunctionRedeclReturnTypePreviousNoReturn); } diag.Emit(); diff --git a/toolchain/check/global_init.cpp b/toolchain/check/global_init.cpp index ce13d390b4ad6..e08a10f2499e0 100644 --- a/toolchain/check/global_init.cpp +++ b/toolchain/check/global_init.cpp @@ -42,9 +42,11 @@ auto GlobalInit::Finalize() -> void { .last_param_node_id = Parse::NodeId::Invalid, .implicit_param_refs_id = SemIR::InstBlockId::Invalid, .param_refs_id = SemIR::InstBlockId::Empty, - .decl_id = SemIR::InstId::Invalid}, - {.return_storage_id = SemIR::InstId::Invalid, .is_extern = false, + .extern_library_id = StringLiteralValueId::Invalid, + .non_owning_decl_id = SemIR::InstId::Invalid, + .first_owning_decl_id = SemIR::InstId::Invalid}, + {.return_storage_id = SemIR::InstId::Invalid, .body_block_ids = {SemIR::InstBlockId::GlobalInit}}}); } diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index cfc2bbf7bcf94..1cbe5870a8f13 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -102,10 +102,10 @@ static auto MergeClassRedecl(Context& context, SemIRLoc new_loc, if ((prev_import_ir_id.is_valid() && !new_is_import) || (prev_is_extern && !new_is_extern)) { - prev_class.decl_id = new_class.decl_id; + prev_class.first_owning_decl_id = new_class.first_owning_decl_id; ReplacePrevInstForMerge( context, prev_class.parent_scope_id, prev_class.name_id, - new_is_import ? new_loc.inst_id : new_class.decl_id); + new_is_import ? new_loc.inst_id : new_class.first_owning_decl_id); } return true; } @@ -219,7 +219,7 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id, // TODO: Store state regarding is_extern. SemIR::Class class_info = { - name_context.MakeEntityWithParamsBase(class_decl_id, name), + name_context.MakeEntityWithParamsBase(name, class_decl_id, is_extern), {// `.self_type_id` depends on the ClassType, so is set below. .self_type_id = SemIR::TypeId::Invalid, .inheritance_kind = inheritance_kind}}; diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index c6479ee29a3ab..9801cf457ce12 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -100,9 +100,10 @@ static auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc, if ((prev_import_ir_id.is_valid() && !new_is_import) || (prev_function.is_extern && !new_function.is_extern)) { prev_function.is_extern = new_function.is_extern; - prev_function.decl_id = new_function.decl_id; + prev_function.first_owning_decl_id = new_function.first_owning_decl_id; ReplacePrevInstForMerge(context, prev_function.parent_scope_id, - prev_function.name_id, new_function.decl_id); + prev_function.name_id, + new_function.first_owning_decl_id); } return true; } @@ -151,7 +152,7 @@ static auto TryMergeRedecl(Context& context, Parse::AnyFunctionDeclId node_id, } if (!prev_function_id.is_valid()) { - context.DiagnoseDuplicateName(function_info.decl_id, prev_id); + context.DiagnoseDuplicateName(function_info.latest_decl_id(), prev_id); return; } @@ -216,8 +217,8 @@ static auto BuildFunctionDecl(Context& context, // Build the function entity. This will be merged into an existing function if // there is one, or otherwise added to the function store. auto function_info = SemIR::Function{ - {name_context.MakeEntityWithParamsBase(decl_id, name)}, - {.return_storage_id = return_storage_id, .is_extern = is_extern}}; + {name_context.MakeEntityWithParamsBase(name, decl_id, is_extern)}, + {.return_storage_id = return_storage_id}}; if (is_definition) { function_info.definition_id = decl_id; } @@ -244,7 +245,7 @@ static auto BuildFunctionDecl(Context& context, if (!name_context.prev_inst_id().is_valid()) { // At interface scope, a function declaration introduces an associated // function. - auto lookup_result_id = function_info.decl_id; + auto lookup_result_id = decl_id; if (parent_scope_inst && !name_context.has_qualifiers) { if (auto interface_scope = parent_scope_inst->TryAs()) { diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index c4f56149a013a..b98b51375e23e 100644 --- a/toolchain/check/handle_interface.cpp +++ b/toolchain/check/handle_interface.cpp @@ -75,7 +75,7 @@ static auto BuildInterfaceDecl(Context& context, {.loc = node_id, .is_definition = is_definition, .is_extern = false}, - {.loc = existing_interface.decl_id, + {.loc = existing_interface.latest_decl_id(), .is_definition = existing_interface.is_defined(), .is_extern = false}, /*prev_import_ir_id=*/SemIR::ImportIRId::Invalid); @@ -103,8 +103,8 @@ static auto BuildInterfaceDecl(Context& context, // 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_context.MakeEntityWithParamsBase(interface_decl_id, name)}; + SemIR::Interface interface_info = {name_context.MakeEntityWithParamsBase( + name, interface_decl_id, /*is_extern=*/false)}; interface_info.generic_id = FinishGenericDecl(context, interface_decl_id); interface_decl.interface_id = context.interfaces().Add(interface_info); if (interface_info.has_parameters()) { diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index 49ab0ecb66025..6393d654fe725 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -25,7 +25,8 @@ static auto NoteAssociatedFunction(Context& context, CARBON_DIAGNOSTIC(ImplAssociatedFunctionHere, Note, "Associated function {0} declared here.", SemIR::NameId); const auto& function = context.functions().Get(function_id); - builder.Note(function.decl_id, ImplAssociatedFunctionHere, function.name_id); + builder.Note(function.latest_decl_id(), ImplAssociatedFunctionHere, + function.name_id); } // Gets the self specific of a generic declaration that is an interface member, diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index fe91c4185ba12..d39978c8b0b57 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -838,6 +838,7 @@ class ImportRefResolver { // of the parameter lists that we know whether this interface is a generic // interface and can build the right constant value for it. // + // TODO: Support extern. // TODO: Add a better way to represent a generic prior to importing the // parameters. auto GetIncompleteLocalEntityBase( @@ -855,7 +856,10 @@ class ImportRefResolver { .param_refs_id = import_base.param_refs_id.is_valid() ? SemIR::InstBlockId::Empty : SemIR::InstBlockId::Invalid, - .decl_id = decl_id, + .is_extern = import_base.is_extern, + .extern_library_id = StringLiteralValueId::Invalid, + .non_owning_decl_id = SemIR::InstId::Invalid, + .first_owning_decl_id = decl_id, }; } @@ -1176,8 +1180,9 @@ class ImportRefResolver { SemIR::ClassDecl class_decl = {.type_id = SemIR::TypeId::TypeType, .class_id = SemIR::ClassId::Invalid, .decl_block_id = SemIR::InstBlockId::Empty}; - auto class_decl_id = context_.AddPlaceholderInstInNoBlock( - SemIR::LocIdAndInst(AddImportIRInst(import_class.decl_id), class_decl)); + auto class_decl_id = + context_.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst( + AddImportIRInst(import_class.latest_decl_id()), class_decl)); // Regardless of whether ClassDecl is a complete type, we first need an // incomplete type so that any references have something to point at. class_decl.class_id = context_.classes().Add( @@ -1200,13 +1205,14 @@ class ImportRefResolver { SemIR::Class& new_class, SemIR::ConstantId object_repr_const_id, SemIR::InstId base_id) -> void { - new_class.definition_id = new_class.decl_id; + new_class.definition_id = new_class.first_owning_decl_id; new_class.object_repr_id = context_.GetTypeIdForTypeConstant(object_repr_const_id); new_class.scope_id = context_.name_scopes().Add( - new_class.decl_id, SemIR::NameId::Invalid, new_class.parent_scope_id); + new_class.first_owning_decl_id, SemIR::NameId::Invalid, + new_class.parent_scope_id); auto& new_scope = context_.name_scopes().Get(new_class.scope_id); const auto& import_scope = import_ir_.name_scopes().Get(import_class.scope_id); @@ -1301,8 +1307,8 @@ class ImportRefResolver { auto TryResolveTypedInst(SemIR::ClassType inst) -> ResolveResult { CARBON_CHECK(inst.type_id == SemIR::TypeId::TypeType); - auto class_const_id = - GetLocalConstantId(import_ir_.classes().Get(inst.class_id).decl_id); + auto class_const_id = GetLocalConstantId( + import_ir_.classes().Get(inst.class_id).first_owning_decl_id); auto specific_data = GetLocalSpecificData(inst.specific_id); if (HasNewWork()) { return Retry(); @@ -1373,7 +1379,6 @@ class ImportRefResolver { function_decl.function_id = context_.functions().Add( {GetIncompleteLocalEntityBase(function_decl_id, import_function), {.return_storage_id = SemIR::InstId::Invalid, - .is_extern = import_function.is_extern, .builtin_function_kind = import_function.builtin_function_kind}}); function_decl.type_id = @@ -1457,7 +1462,7 @@ class ImportRefResolver { } if (import_function.definition_id.is_valid()) { - new_function.definition_id = new_function.decl_id; + new_function.definition_id = new_function.first_owning_decl_id; } return ResolveAsConstant(function_const_id); @@ -1466,7 +1471,7 @@ class ImportRefResolver { auto TryResolveTypedInst(SemIR::FunctionType inst) -> ResolveResult { CARBON_CHECK(inst.type_id == SemIR::TypeId::TypeType); auto fn_val_id = GetLocalConstantInstId( - import_ir_.functions().Get(inst.function_id).decl_id); + import_ir_.functions().Get(inst.function_id).first_owning_decl_id); auto specific_data = GetLocalSpecificData(inst.specific_id); if (HasNewWork()) { return Retry(); @@ -1483,8 +1488,8 @@ class ImportRefResolver { auto TryResolveTypedInst(SemIR::GenericClassType inst) -> ResolveResult { CARBON_CHECK(inst.type_id == SemIR::TypeId::TypeType); - auto class_val_id = - GetLocalConstantInstId(import_ir_.classes().Get(inst.class_id).decl_id); + auto class_val_id = GetLocalConstantInstId( + import_ir_.classes().Get(inst.class_id).first_owning_decl_id); if (HasNewWork()) { return Retry(); } @@ -1498,7 +1503,7 @@ class ImportRefResolver { auto TryResolveTypedInst(SemIR::GenericInterfaceType inst) -> ResolveResult { CARBON_CHECK(inst.type_id == SemIR::TypeId::TypeType); auto interface_val_id = GetLocalConstantInstId( - import_ir_.interfaces().Get(inst.interface_id).decl_id); + import_ir_.interfaces().Get(inst.interface_id).first_owning_decl_id); if (HasNewWork()) { return Retry(); } @@ -1537,7 +1542,8 @@ class ImportRefResolver { .decl_block_id = SemIR::InstBlockId::Empty}; auto interface_decl_id = context_.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst( - AddImportIRInst(import_interface.decl_id), interface_decl)); + AddImportIRInst(import_interface.first_owning_decl_id), + interface_decl)); // Start with an incomplete interface. interface_decl.interface_id = context_.interfaces().Add( @@ -1561,7 +1567,7 @@ class ImportRefResolver { SemIR::Interface& new_interface, SemIR::InstId self_param_id) -> void { new_interface.scope_id = context_.name_scopes().Add( - new_interface.decl_id, SemIR::NameId::Invalid, + new_interface.first_owning_decl_id, SemIR::NameId::Invalid, new_interface.parent_scope_id); auto& new_scope = context_.name_scopes().Get(new_interface.scope_id); const auto& import_scope = @@ -1647,7 +1653,7 @@ class ImportRefResolver { auto TryResolveTypedInst(SemIR::InterfaceType inst) -> ResolveResult { CARBON_CHECK(inst.type_id == SemIR::TypeId::TypeType); auto interface_const_id = GetLocalConstantId( - import_ir_.interfaces().Get(inst.interface_id).decl_id); + import_ir_.interfaces().Get(inst.interface_id).first_owning_decl_id); auto specific_data = GetLocalSpecificData(inst.specific_id); if (HasNewWork()) { return Retry(); diff --git a/toolchain/check/merge.h b/toolchain/check/merge.h index 52b4d6a2656fd..88202d687ceb4 100644 --- a/toolchain/check/merge.h +++ b/toolchain/check/merge.h @@ -45,7 +45,7 @@ auto ReplacePrevInstForMerge(Context& context, SemIR::NameScopeId scope_id, // different kinds of entity such as classes and functions. struct DeclParams { explicit DeclParams(const SemIR::EntityWithParamsBase& base) - : loc(base.decl_id), + : loc(base.latest_decl_id()), first_param_node_id(base.first_param_node_id), last_param_node_id(base.last_param_node_id), implicit_param_refs_id(base.implicit_param_refs_id), diff --git a/toolchain/check/return.cpp b/toolchain/check/return.cpp index f02513711b5ec..b12adf40067bc 100644 --- a/toolchain/check/return.cpp +++ b/toolchain/check/return.cpp @@ -33,7 +33,7 @@ static auto NoteNoReturnTypeProvided(Context::DiagnosticBuilder& diag, const SemIR::Function& function) { CARBON_DIAGNOSTIC(ReturnTypeOmittedNote, Note, "There was no return type provided."); - diag.Note(function.decl_id, ReturnTypeOmittedNote); + diag.Note(function.latest_decl_id(), ReturnTypeOmittedNote); } // Produces a note describing the return type of the given function. diff --git a/toolchain/sem_ir/entity_with_params_base.h b/toolchain/sem_ir/entity_with_params_base.h index 7a6c27e4aeec7..751efb7b3b6ae 100644 --- a/toolchain/sem_ir/entity_with_params_base.h +++ b/toolchain/sem_ir/entity_with_params_base.h @@ -51,7 +51,13 @@ struct EntityWithParamsBase { // Returns the instruction for the latest declaration. auto latest_decl_id() const -> SemIR::InstId { - return definition_id.is_valid() ? definition_id : decl_id; + if (definition_id.is_valid()) { + return definition_id; + } + if (first_owning_decl_id.is_valid()) { + return first_owning_decl_id; + } + return non_owning_decl_id; } // Determines whether this entity has any parameter lists. @@ -77,12 +83,21 @@ struct EntityWithParamsBase { InstBlockId implicit_param_refs_id; // A block containing a single reference instruction per parameter. InstBlockId param_refs_id; - // The first declaration of the entity. This will be a Decl. - InstId decl_id = InstId::Invalid; + // True if declarations are `extern`. + bool is_extern; + // For an `extern library` declaration, the library name. + StringLiteralValueId extern_library_id; + // The non-owning declaration of the entity, if present. This will be a + // Decl. + InstId non_owning_decl_id; + // The first owning declaration of the entity, if present. This will be a + // Decl. It may either be a forward declaration, or the same as + // `definition_id`. + InstId first_owning_decl_id; // The following members are set at the `{` of the definition. - // The first declaration of the entity. This will be a Decl. + // The definition of the entity. This will be a Decl. InstId definition_id = InstId::Invalid; }; diff --git a/toolchain/sem_ir/function.h b/toolchain/sem_ir/function.h index cb8dfc6216727..43aaed9654195 100644 --- a/toolchain/sem_ir/function.h +++ b/toolchain/sem_ir/function.h @@ -22,8 +22,6 @@ struct FunctionFields { // function, depending on whether the return type needs a return slot, but is // always present if the function has a declared return type. InstId return_storage_id; - // Whether the declaration is extern. - bool is_extern; // The following member is set on the first call to the function, or at the // point where the function is defined.