From 7e81c1710e75ef6baaeb95b544d0a3936b818f3e Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 31 May 2024 08:36:52 -0700 Subject: [PATCH] Refactor modifier fetching of enclosing scopes to avoid duplicate calls. (#4010) Right now, each sequential modifier verification tends to re-fetch the enclosing scope, doing equivalent verification. Change code to more explicitly do the fetch once, sharing the result, also making the enclosing scope available to the caller for other work. Note, the type store similarly carries an inst store pointer; that's what I'm basing having the name scope store's inst store pointer on. --- toolchain/check/handle_class.cpp | 6 +- toolchain/check/handle_function.cpp | 32 ++++---- toolchain/check/handle_interface.cpp | 4 +- toolchain/check/handle_let.cpp | 9 ++- toolchain/check/handle_variable.cpp | 4 +- toolchain/check/import_ref.cpp | 7 +- toolchain/check/member_access.cpp | 9 +-- toolchain/check/modifiers.cpp | 106 +++++++++++---------------- toolchain/check/modifiers.h | 35 +++++---- toolchain/sem_ir/file.cpp | 1 + toolchain/sem_ir/file.h | 6 +- toolchain/sem_ir/name_scope.h | 20 +++-- 12 files changed, 119 insertions(+), 120 deletions(-) diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index 74afd4d3ba52..62e573d69379 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -183,14 +183,16 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id, .PopAndDiscardSoloNodeId(); // Process modifiers. + auto [_, enclosing_scope_inst] = + context.name_scopes().GetInstIfValid(name_context.enclosing_scope_id); CheckAccessModifiersOnDecl(context, Lex::TokenKind::Class, - name_context.enclosing_scope_id); + enclosing_scope_inst); LimitModifiersOnDecl(context, KeywordModifierSet::Class | KeywordModifierSet::Access | KeywordModifierSet::Extern, Lex::TokenKind::Class); RestrictExternModifierOnDecl(context, Lex::TokenKind::Class, - name_context.enclosing_scope_id, is_definition); + enclosing_scope_inst, is_definition); auto modifiers = context.decl_state_stack().innermost().modifier_set; if (modifiers.HasAnyOf(KeywordModifierSet::Access)) { diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 1c8521397cab..cc621d65fe9a 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -45,19 +45,21 @@ auto HandleReturnType(Context& context, Parse::ReturnTypeId node_id) -> bool { } static auto DiagnoseModifiers(Context& context, bool is_definition, - SemIR::NameScopeId enclosing_scope_id) + SemIR::InstId enclosing_scope_inst_id, + std::optional enclosing_scope_inst) -> KeywordModifierSet { - const Lex::TokenKind decl_kind = Lex::TokenKind::Fn; - CheckAccessModifiersOnDecl(context, decl_kind, enclosing_scope_id); + CheckAccessModifiersOnDecl(context, Lex::TokenKind::Fn, enclosing_scope_inst); LimitModifiersOnDecl(context, KeywordModifierSet::Access | KeywordModifierSet::Extern | KeywordModifierSet::Method | KeywordModifierSet::Interface, - decl_kind); - RestrictExternModifierOnDecl(context, decl_kind, enclosing_scope_id, - is_definition); - CheckMethodModifiersOnFunction(context, enclosing_scope_id); - RequireDefaultFinalOnlyInInterfaces(context, decl_kind, enclosing_scope_id); + Lex::TokenKind::Fn); + RestrictExternModifierOnDecl(context, Lex::TokenKind::Fn, + enclosing_scope_inst, is_definition); + CheckMethodModifiersOnFunction(context, enclosing_scope_inst_id, + enclosing_scope_inst); + RequireDefaultFinalOnlyInInterfaces(context, Lex::TokenKind::Fn, + enclosing_scope_inst); return context.decl_state_stack().innermost().modifier_set; } @@ -222,8 +224,10 @@ static auto BuildFunctionDecl(Context& context, .PopAndDiscardSoloNodeId(); // Process modifiers. - auto modifiers = DiagnoseModifiers(context, is_definition, - name_context.enclosing_scope_id); + auto [enclosing_scope_inst_id, enclosing_scope_inst] = + context.name_scopes().GetInstIfValid(name_context.enclosing_scope_id); + auto modifiers = DiagnoseModifiers( + context, is_definition, enclosing_scope_inst_id, enclosing_scope_inst); if (modifiers.HasAnyOf(KeywordModifierSet::Access)) { context.TODO(context.decl_state_stack().innermost().modifier_node_id( ModifierOrder::Access), @@ -279,13 +283,9 @@ static auto BuildFunctionDecl(Context& context, // At interface scope, a function declaration introduces an associated // function. auto lookup_result_id = function_info.decl_id; - if (name_context.enclosing_scope_id_for_new_inst().is_valid() && - !name_context.has_qualifiers) { - auto scope_inst_id = context.name_scopes().GetInstIdIfValid( - name_context.enclosing_scope_id_for_new_inst()); + if (enclosing_scope_inst && !name_context.has_qualifiers) { if (auto interface_scope = - context.insts().TryGetAsIfValid( - scope_inst_id)) { + enclosing_scope_inst->TryAs()) { lookup_result_id = BuildAssociatedEntity( context, interface_scope->interface_id, function_info.decl_id); } diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index 39b780786b62..61aa1036ad5d 100644 --- a/toolchain/check/handle_interface.cpp +++ b/toolchain/check/handle_interface.cpp @@ -36,8 +36,10 @@ static auto BuildInterfaceDecl(Context& context, .PopAndDiscardSoloNodeId(); // Process modifiers. + auto [_, enclosing_scope_inst] = + context.name_scopes().GetInstIfValid(name_context.enclosing_scope_id); CheckAccessModifiersOnDecl(context, Lex::TokenKind::Interface, - name_context.enclosing_scope_id); + enclosing_scope_inst); LimitModifiersOnDecl(context, KeywordModifierSet::Access, Lex::TokenKind::Interface); diff --git a/toolchain/check/handle_let.cpp b/toolchain/check/handle_let.cpp index 83f47c0c266a..7c4355d7a2e6 100644 --- a/toolchain/check/handle_let.cpp +++ b/toolchain/check/handle_let.cpp @@ -77,10 +77,13 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId node_id) -> bool { // Process declaration modifiers. // TODO: For a qualified `let` declaration, this should use the target scope // of the name introduced in the declaration. See #2590. + auto [enclosing_scope_inst_id, enclosing_scope_inst] = + context.name_scopes().GetInstIfValid( + context.scope_stack().PeekNameScopeId()); CheckAccessModifiersOnDecl(context, Lex::TokenKind::Let, - context.scope_stack().PeekNameScopeId()); + enclosing_scope_inst); RequireDefaultFinalOnlyInInterfaces(context, Lex::TokenKind::Let, - context.scope_stack().PeekNameScopeId()); + enclosing_scope_inst); LimitModifiersOnDecl( context, KeywordModifierSet::Access | KeywordModifierSet::Interface, Lex::TokenKind::Let); @@ -136,7 +139,7 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId node_id) -> bool { // Add the name of the binding to the current scope. auto name_id = context.bind_names().Get(bind_name.bind_name_id).name_id; context.AddNameToLookup(name_id, pattern_id); - if (context.scope_stack().PeekNameScopeId() == SemIR::NameScopeId::Package) { + if (enclosing_scope_inst_id == SemIR::InstId::PackageNamespace) { context.AddExport(pattern_id); } return true; diff --git a/toolchain/check/handle_variable.cpp b/toolchain/check/handle_variable.cpp index f3a496696753..25188508b624 100644 --- a/toolchain/check/handle_variable.cpp +++ b/toolchain/check/handle_variable.cpp @@ -97,8 +97,10 @@ auto HandleVariableDecl(Context& context, Parse::VariableDeclId node_id) // Process declaration modifiers. // TODO: For a qualified `var` declaration, this should use the target scope // of the name introduced in the declaration. See #2590. + auto [_, enclosing_scope_inst] = context.name_scopes().GetInstIfValid( + context.scope_stack().PeekNameScopeId()); CheckAccessModifiersOnDecl(context, Lex::TokenKind::Var, - context.scope_stack().PeekNameScopeId()); + enclosing_scope_inst); LimitModifiersOnDecl(context, KeywordModifierSet::Access, Lex::TokenKind::Var); auto modifiers = context.decl_state_stack().innermost().modifier_set; diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 3ceedf6aeb9b..308408cb5b97 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -495,14 +495,15 @@ class ImportRefResolver { // unresolved constants to the work stack. auto GetLocalNameScopeId(SemIR::NameScopeId name_scope_id) -> SemIR::NameScopeId { - auto inst_id = import_ir_.name_scopes().GetInstIdIfValid(name_scope_id); - if (!inst_id.is_valid()) { + auto [inst_id, inst] = + import_ir_.name_scopes().GetInstIfValid(name_scope_id); + if (!inst) { // Map scopes that aren't associated with an instruction to invalid // scopes. For now, such scopes aren't used, and we don't have a good way // to remap them. return SemIR::NameScopeId::Invalid; } - if (import_ir_.insts().Is(inst_id)) { + if (inst->Is()) { // TODO: Import the scope for an `impl` definition. return SemIR::NameScopeId::Invalid; } diff --git a/toolchain/check/member_access.cpp b/toolchain/check/member_access.cpp index dec4f451fdae..02718a92f841 100644 --- a/toolchain/check/member_access.cpp +++ b/toolchain/check/member_access.cpp @@ -97,18 +97,17 @@ static auto IsInstanceMethod(const SemIR::File& sem_ir, // performed if we find an associated entity. static auto ScopeNeedsImplLookup(Context& context, SemIR::NameScopeId name_scope_id) -> bool { - auto inst_id = context.name_scopes().GetInstIdIfValid(name_scope_id); - if (!inst_id.is_valid()) { + auto [_, inst] = context.name_scopes().GetInstIfValid(name_scope_id); + if (!inst) { return false; } - auto inst = context.insts().Get(inst_id); - if (inst.Is()) { + if (inst->Is()) { // Don't perform impl lookup if an associated entity is named as a member of // a facet type. return false; } - if (inst.Is()) { + if (inst->Is()) { // Don't perform impl lookup if an associated entity is named as a namespace // member. // TODO: This case is not yet listed in the design. diff --git a/toolchain/check/modifiers.cpp b/toolchain/check/modifiers.cpp index 280d7e2b29bf..1af512b2b7ff 100644 --- a/toolchain/check/modifiers.cpp +++ b/toolchain/check/modifiers.cpp @@ -61,43 +61,25 @@ auto ForbidModifiersOnDecl(Context& context, KeywordModifierSet forbidden, s.modifier_set.Remove(forbidden); } -// Returns the instruction that owns the given scope, or Invalid if the scope is -// not associated with an instruction. -static auto GetScopeInstId(Context& context, SemIR::NameScopeId scope_id) - -> SemIR::InstId { - if (!scope_id.is_valid()) { - return SemIR::InstId::Invalid; - } - return context.name_scopes().Get(scope_id).inst_id; -} - -// Returns the instruction that owns the given scope, or Invalid if the scope is -// not associated with an instruction. -static auto GetScopeInst(Context& context, SemIR::NameScopeId scope_id) - -> std::optional { - auto inst_id = GetScopeInstId(context, scope_id); - if (!inst_id.is_valid()) { - return std::nullopt; - } - return context.insts().Get(inst_id); -} - auto CheckAccessModifiersOnDecl(Context& context, Lex::TokenKind decl_kind, - SemIR::NameScopeId enclosing_scope_id) -> void { - auto target = GetScopeInst(context, enclosing_scope_id); - if (target && target->Is()) { - // TODO: This assumes that namespaces can only be declared at file scope. If - // we add support for non-file-scope namespaces, we will need to check the - // parents of the target scope to determine whether we're at file scope. - ForbidModifiersOnDecl( - context, KeywordModifierSet::Protected, decl_kind, - " at file scope, `protected` is only allowed on class members"); - return; - } + std::optional enclosing_scope_inst) + -> void { + if (enclosing_scope_inst) { + if (enclosing_scope_inst->Is()) { + // TODO: This assumes that namespaces can only be declared at file scope. + // If we add support for non-file-scope namespaces, we will need to check + // the parents of the target scope to determine whether we're at file + // scope. + ForbidModifiersOnDecl( + context, KeywordModifierSet::Protected, decl_kind, + " at file scope, `protected` is only allowed on class members"); + return; + } - if (target && target->Is()) { - // Both `private` and `protected` allowed in a class definition. - return; + if (enclosing_scope_inst->Is()) { + // Both `private` and `protected` allowed in a class definition. + return; + } } // Otherwise neither `private` nor `protected` allowed. @@ -108,25 +90,25 @@ auto CheckAccessModifiersOnDecl(Context& context, Lex::TokenKind decl_kind, ", `private` is only allowed on class members and at file scope"); } -auto CheckMethodModifiersOnFunction(Context& context, - SemIR::NameScopeId enclosing_scope_id) - -> void { +auto CheckMethodModifiersOnFunction( + Context& context, SemIR::InstId enclosing_scope_inst_id, + std::optional enclosing_scope_inst) -> void { const Lex::TokenKind decl_kind = Lex::TokenKind::Fn; - auto target_id = GetScopeInstId(context, enclosing_scope_id); - if (target_id.is_valid()) { - if (auto class_decl = - context.insts().TryGetAs(target_id)) { + if (enclosing_scope_inst) { + if (auto class_decl = enclosing_scope_inst->TryAs()) { auto inheritance_kind = context.classes().Get(class_decl->class_id).inheritance_kind; if (inheritance_kind == SemIR::Class::Final) { - ForbidModifiersOnDecl(context, KeywordModifierSet::Virtual, decl_kind, - " in a non-abstract non-base `class` definition", - context.insts().GetLocId(target_id)); + ForbidModifiersOnDecl( + context, KeywordModifierSet::Virtual, decl_kind, + " in a non-abstract non-base `class` definition", + context.insts().GetLocId(enclosing_scope_inst_id)); } if (inheritance_kind != SemIR::Class::Abstract) { - ForbidModifiersOnDecl(context, KeywordModifierSet::Abstract, decl_kind, - " in a non-abstract `class` definition", - context.insts().GetLocId(target_id)); + ForbidModifiersOnDecl( + context, KeywordModifierSet::Abstract, decl_kind, + " in a non-abstract `class` definition", + context.insts().GetLocId(enclosing_scope_inst_id)); } return; } @@ -136,29 +118,25 @@ auto CheckMethodModifiersOnFunction(Context& context, " outside of a class"); } -auto RestrictExternModifierOnDecl(Context& context, Lex::TokenKind decl_kind, - SemIR::NameScopeId enclosing_scope_id, - bool is_definition) -> void { +auto RestrictExternModifierOnDecl( + Context& context, Lex::TokenKind decl_kind, + std::optional enclosing_scope_inst, bool is_definition) + -> void { if (is_definition) { ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind, " that provides a definition"); } - if (enclosing_scope_id.is_valid()) { - auto target_id = context.name_scopes().Get(enclosing_scope_id).inst_id; - if (target_id.is_valid() && - !context.insts().Is(target_id)) { - ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind, - " that is a member"); - } + if (enclosing_scope_inst && !enclosing_scope_inst->Is()) { + ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind, + " that is a member"); } } -auto RequireDefaultFinalOnlyInInterfaces(Context& context, - Lex::TokenKind decl_kind, - SemIR::NameScopeId enclosing_scope_id) - -> void { - auto target = GetScopeInst(context, enclosing_scope_id); - if (target && target->Is()) { +auto RequireDefaultFinalOnlyInInterfaces( + Context& context, Lex::TokenKind decl_kind, + std::optional enclosing_scope_inst) -> void { + if (enclosing_scope_inst && + enclosing_scope_inst->Is()) { // Both `default` and `final` allowed in an interface definition. return; } diff --git a/toolchain/check/modifiers.h b/toolchain/check/modifiers.h index 36f8144b9e07..7c9b8bbc3440 100644 --- a/toolchain/check/modifiers.h +++ b/toolchain/check/modifiers.h @@ -10,21 +10,22 @@ namespace Carbon::Check { // Reports a diagnostic if access control modifiers on this are not allowed for -// a declaration in `enclosing_scope_id`, and updates the declaration state in +// a declaration in `enclosing_scope_inst`, and updates the declaration state in // `context`. // -// `enclosing_scope_id` may be Invalid for a declaration in a block scope. +// `enclosing_scope_inst` may be nullopt for a declaration in a block scope. auto CheckAccessModifiersOnDecl(Context& context, Lex::TokenKind decl_kind, - SemIR::NameScopeId enclosing_scope_id) -> void; + std::optional enclosing_scope_inst) + -> void; // Reports a diagnostic if the method function modifiers `abstract`, `virtual`, // or `impl` are present but not permitted on a function declaration in -// `enclosing_scope_id`. +// `enclosing_scope_inst`. // -// `enclosing_scope_id` may be Invalid for a declaration in a block scope. -auto CheckMethodModifiersOnFunction(Context& context, - SemIR::NameScopeId enclosing_scope_id) - -> void; +// `enclosing_scope_inst` may be nullopt for a declaration in a block scope. +auto CheckMethodModifiersOnFunction( + Context& context, SemIR::InstId enclosing_scope_inst_id, + std::optional enclosing_scope_inst) -> void; // Like `LimitModifiersOnDecl`, except says which modifiers are forbidden, and a // `context_string` (and optional `context_loc_id`) specifying the context in @@ -48,19 +49,21 @@ inline auto LimitModifiersOnDecl(Context& context, KeywordModifierSet allowed, // declarations, diagnosing and removing it on: // - `extern` on a definition. // - `extern` on a scoped entity. -auto RestrictExternModifierOnDecl(Context& context, Lex::TokenKind decl_kind, - SemIR::NameScopeId enclosing_scope_id, - bool is_definition) -> void; +// +// `enclosing_scope_inst` may be nullopt for a declaration in a block scope. +auto RestrictExternModifierOnDecl( + Context& context, Lex::TokenKind decl_kind, + std::optional enclosing_scope_inst, bool is_definition) + -> void; // Report a diagonostic if `default` and `final` modifiers are used on // declarations where they are not allowed. Right now they are only allowed // inside interfaces. // -// `enclosing_scope_id` may be Invalid for a declaration in a block scope. -auto RequireDefaultFinalOnlyInInterfaces(Context& context, - Lex::TokenKind decl_kind, - SemIR::NameScopeId enclosing_scope_id) - -> void; +// `enclosing_scope_inst` may be nullopt for a declaration in a block scope. +auto RequireDefaultFinalOnlyInInterfaces( + Context& context, Lex::TokenKind decl_kind, + std::optional enclosing_scope_inst) -> void; } // namespace Carbon::Check diff --git a/toolchain/sem_ir/file.cpp b/toolchain/sem_ir/file.cpp index 8115033f0743..337245798fde 100644 --- a/toolchain/sem_ir/file.cpp +++ b/toolchain/sem_ir/file.cpp @@ -68,6 +68,7 @@ File::File(CheckIRId check_ir_id, SharedValueStores& value_stores, value_stores_(&value_stores), filename_(std::move(filename)), type_blocks_(allocator_), + name_scopes_(&insts_), constant_values_(ConstantId::NotConstant), inst_blocks_(allocator_), constants_(*this, allocator_) { diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index 2a57691f7ee5..f1bc96db2a7a 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -212,9 +212,6 @@ class File : public Printable { // that are import-related. ValueStore import_ir_insts_; - // Storage for name scopes. - NameScopeStore name_scopes_; - // Type blocks within the IR. These reference entries in types_. Storage for // the data is provided by allocator_. BlockValueStore type_blocks_; @@ -223,6 +220,9 @@ class File : public Printable { // indices matching BuiltinKind ordering. InstStore insts_; + // Storage for name scopes. + NameScopeStore name_scopes_; + // Constant values for instructions. ConstantValueStore constant_values_; diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index 0df120ff080e..267d55ef4a82 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -6,6 +6,7 @@ #define CARBON_TOOLCHAIN_SEM_IR_NAME_SCOPE_H_ #include "toolchain/sem_ir/ids.h" +#include "toolchain/sem_ir/inst.h" namespace Carbon::SemIR { @@ -87,6 +88,8 @@ struct NameScope : Printable { // Provides a ValueStore wrapper for an API specific to name scopes. class NameScopeStore { public: + explicit NameScopeStore(InstStore* insts) : insts_(insts) {} + // Adds a name scope, returning an ID to reference it. auto Add(InstId inst_id, NameId name_id, NameScopeId enclosing_scope_id) -> NameScopeId { @@ -103,14 +106,18 @@ class NameScopeStore { return values_.Get(scope_id); } - // Returns the instruction owning the requested name scope, or an invalid - // instruction if the scope is either invalid or has no associated - // instruction. - auto GetInstIdIfValid(NameScopeId scope_id) const -> InstId { + // Returns the instruction owning the requested name scope, or Invalid with + // nullopt if the scope is either invalid or has no associated instruction. + auto GetInstIfValid(NameScopeId scope_id) const + -> std::pair> { if (!scope_id.is_valid()) { - return InstId::Invalid; + return {InstId::Invalid, std::nullopt}; } - return Get(scope_id).inst_id; + auto inst_id = Get(scope_id).inst_id; + if (!inst_id.is_valid()) { + return {InstId::Invalid, std::nullopt}; + } + return {inst_id, insts_->Get(inst_id)}; } auto OutputYaml() const -> Yaml::OutputMapping { @@ -118,6 +125,7 @@ class NameScopeStore { } private: + InstStore* insts_; ValueStore values_; };