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

@@ -183,14 +183,16 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id,
       .PopAndDiscardSoloNodeId<Parse::NodeKind::ClassIntroducer>();
 
   // 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)) {

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<SemIR::Inst> 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<Parse::NodeKind::FunctionIntroducer>();
 
   // 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<SemIR::InterfaceDecl>(
-                  scope_inst_id)) {
+              enclosing_scope_inst->TryAs<SemIR::InterfaceDecl>()) {
         lookup_result_id = BuildAssociatedEntity(
             context, interface_scope->interface_id, function_info.decl_id);
       }

toolchain/check/handle_interface.cpp

@@ -36,8 +36,10 @@ static auto BuildInterfaceDecl(Context& context,
       .PopAndDiscardSoloNodeId<Parse::NodeKind::InterfaceIntroducer>();
 
   // 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);
 

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;

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;

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<SemIR::ImplDecl>(inst_id)) {
+    if (inst->Is<SemIR::ImplDecl>()) {
       // TODO: Import the scope for an `impl` definition.
       return SemIR::NameScopeId::Invalid;
     }

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<SemIR::InterfaceDecl>()) {
+  if (inst->Is<SemIR::InterfaceDecl>()) {
     // Don't perform impl lookup if an associated entity is named as a member of
     // a facet type.
     return false;
   }
-  if (inst.Is<SemIR::Namespace>()) {
+  if (inst->Is<SemIR::Namespace>()) {
     // 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.

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<SemIR::Inst> {
-  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<SemIR::Namespace>()) {
-    // 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<SemIR::Inst> enclosing_scope_inst)
+    -> void {
+  if (enclosing_scope_inst) {
+    if (enclosing_scope_inst->Is<SemIR::Namespace>()) {
+      // 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<SemIR::ClassDecl>()) {
-    // Both `private` and `protected` allowed in a class definition.
-    return;
+    if (enclosing_scope_inst->Is<SemIR::ClassDecl>()) {
+      // 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<SemIR::Inst> 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<SemIR::ClassDecl>(target_id)) {
+  if (enclosing_scope_inst) {
+    if (auto class_decl = enclosing_scope_inst->TryAs<SemIR::ClassDecl>()) {
       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<SemIR::Inst> 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<SemIR::Namespace>(target_id)) {
-      ForbidModifiersOnDecl(context, KeywordModifierSet::Extern, decl_kind,
-                            " that is a member");
-    }
+  if (enclosing_scope_inst && !enclosing_scope_inst->Is<SemIR::Namespace>()) {
+    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<SemIR::InterfaceDecl>()) {
+auto RequireDefaultFinalOnlyInInterfaces(
+    Context& context, Lex::TokenKind decl_kind,
+    std::optional<SemIR::Inst> enclosing_scope_inst) -> void {
+  if (enclosing_scope_inst &&
+      enclosing_scope_inst->Is<SemIR::InterfaceDecl>()) {
     // Both `default` and `final` allowed in an interface definition.
     return;
   }

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<SemIR::Inst> 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<SemIR::Inst> 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<SemIR::Inst> 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<SemIR::Inst> enclosing_scope_inst) -> void;
 
 }  // namespace Carbon::Check
 

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_) {

toolchain/sem_ir/file.h

@@ -212,9 +212,6 @@ class File : public Printable<File> {
   // that are import-related.
   ValueStore<ImportIRInstId> 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<TypeBlockId> type_blocks_;
@@ -223,6 +220,9 @@ class File : public Printable<File> {
   // indices matching BuiltinKind ordering.
   InstStore insts_;
 
+  // Storage for name scopes.
+  NameScopeStore name_scopes_;
+
   // Constant values for instructions.
   ConstantValueStore constant_values_;
 

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<NameScope> {
 // 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<InstId, std::optional<Inst>> {
     if (!scope_id.is_valid()) {
-      return InstId::Invalid;
+      return {InstId::Invalid, std::nullopt};
+    }
+    auto inst_id = Get(scope_id).inst_id;
+    if (!inst_id.is_valid()) {
+      return {InstId::Invalid, std::nullopt};
     }
-    return Get(scope_id).inst_id;
+    return {inst_id, insts_->Get(inst_id)};
   }
 
   auto OutputYaml() const -> Yaml::OutputMapping {
@@ -118,6 +125,7 @@ class NameScopeStore {
   }
 
  private:
+  InstStore* insts_;
   ValueStore<NameScopeId> values_;
 };