[NFC] Convert NameScope from struct to class (#4623)

This is a preparation change for adding name poisoning support (#4622), which is expected to require more elaborate logic around NameScope since a name can be not defined yet, defined, or poisoned. The API separates looking up a name from getting the full entry since we have cases where the entries are invalidated between the time we're looking for the name and when we access (and sometimes modify) the entry. This change has the following benefits: * `names` and `name_map` are internal to `NameScope` and are guaranteed to match. * `extended_scopes` and `import_ir_scopes` can not be manipulated (only new scopes can be added). * `inst_id`, `name_id` and `parent_scope_id` are constants. * `has_error` can only be mutated from false to true. --------- Co-authored-by: jonmeow <[email protected]>
carbon-language · Dec 6, 2024 · daba2c7 · daba2c7
1 parent e7a86b0
commit daba2c7
Show file tree

Hide file tree

Showing 19 changed files with 525 additions and 139 deletions.
diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp
@@ -198,7 +198,7 @@ static auto ImportCurrentPackage(Context& context, UnitInfo& unit_info,
       unit_info.package_imports[import_map_lookup.value()];
 
   if (self_import.has_load_error) {
-    context.name_scopes().Get(SemIR::NameScopeId::Package).has_error = true;
+    context.name_scopes().Get(SemIR::NameScopeId::Package).set_has_error();
   }
 
   ImportLibrariesFromCurrentPackage(
@@ -208,7 +208,7 @@ static auto ImportCurrentPackage(Context& context, UnitInfo& unit_info,
 
   context.scope_stack().Push(
       package_inst_id, SemIR::NameScopeId::Package, SemIR::SpecificId::Invalid,
-      context.name_scopes().Get(SemIR::NameScopeId::Package).has_error);
+      context.name_scopes().Get(SemIR::NameScopeId::Package).has_error());
 }
 
 // Imports all other packages (excluding the current package).

diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp
@@ -359,16 +359,16 @@ auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
                                      SemIR::NameScopeId scope_id,
                                      const SemIR::NameScope& scope)
     -> std::pair<SemIR::InstId, SemIR::AccessKind> {
-  if (auto lookup = scope.name_map.Lookup(name_id)) {
-    auto entry = scope.names[lookup.value()];
+  if (auto entry_id = scope.Lookup(name_id)) {
+    auto entry = scope.GetEntry(*entry_id);
     LoadImportRef(*this, entry.inst_id);
     return {entry.inst_id, entry.access_kind};
   }
 
-  if (!scope.import_ir_scopes.empty()) {
+  if (!scope.import_ir_scopes().empty()) {
     // TODO: Enforce other access modifiers for imports.
     return {ImportNameFromOtherPackage(*this, loc, scope_id,
-                                       scope.import_ir_scopes, name_id),
+                                       scope.import_ir_scopes(), name_id),
             SemIR::AccessKind::Public};
   }
   return {SemIR::InstId::Invalid, SemIR::AccessKind::Public};
@@ -525,7 +525,7 @@ auto Context::LookupQualifiedName(SemIRLoc loc, SemIR::NameId name_id,
       continue;
     }
     const auto& name_scope = name_scopes().Get(scope_id);
-    has_error |= name_scope.has_error;
+    has_error |= name_scope.has_error();
 
     auto [scope_result_id, access_kind] =
         LookupNameInExactScope(loc, name_id, scope_id, name_scope);
@@ -546,7 +546,7 @@ auto Context::LookupQualifiedName(SemIRLoc loc, SemIR::NameId name_id,
     if (!scope_result_id.is_valid() || is_access_prohibited) {
       // If nothing is found in this scope or if we encountered an invalid
       // access, look in its extended scopes.
-      const auto& extended = name_scope.extended_scopes;
+      const auto& extended = name_scope.extended_scopes();
       scopes.reserve(scopes.size() + extended.size());
       for (auto extended_id : llvm::reverse(extended)) {
         // Substitute into the constant describing the extended scope to

diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp
@@ -135,7 +135,7 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id,
         auto& name_scope =
             context_->name_scopes().Get(name_context.parent_scope_id);
         if (name_context.has_qualifiers) {
-          auto inst = context_->insts().Get(name_scope.inst_id);
+          auto inst = context_->insts().Get(name_scope.inst_id());
           if (!inst.Is<SemIR::Namespace>()) {
             // TODO: Point at the declaration for the scoped entity.
             CARBON_DIAGNOSTIC(
@@ -231,7 +231,7 @@ auto DeclNameStack::ApplyNameQualifier(const NameComponent& name) -> void {
   if (scope_id.is_valid()) {
     PushNameQualifierScope(*context_, name_context.resolved_inst_id, scope_id,
                            specific_id,
-                           context_->name_scopes().Get(scope_id).has_error);
+                           context_->name_scopes().Get(scope_id).has_error());
     name_context.parent_scope_id = scope_id;
   } else {
     name_context.state = NameContext::State::Error;
@@ -416,12 +416,12 @@ auto DeclNameStack::ResolveAsScope(const NameContext& name_context,
                          SemIR::InstBlockId::Invalid))) {
         return InvalidResult;
       }
-      if (scope.is_closed_import) {
+      if (scope.is_closed_import()) {
         DiagnoseQualifiedDeclInImportedPackage(*context_, name_context.loc_id,
-                                               scope.inst_id);
+                                               scope.inst_id());
         // Only error once per package. Recover by allowing this package name to
         // be used as a name qualifier.
-        scope.is_closed_import = false;
+        scope.set_is_closed_import(false);
       }
       return {scope_id, SemIR::SpecificId::Invalid};
     }

diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp
@@ -410,7 +410,7 @@ auto HandleParseNode(Context& context, Parse::AdaptDeclId node_id) -> bool {
   // Extend the class scope with the adapted type's scope if requested.
   if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
     auto& class_scope = context.name_scopes().Get(class_info.scope_id);
-    class_scope.extended_scopes.push_back(adapted_inst_id);
+    class_scope.AddExtendedScope(adapted_inst_id);
   }
   return true;
 }
@@ -555,9 +555,9 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
   if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
     auto& class_scope = context.name_scopes().Get(class_info.scope_id);
     if (base_info.scope_id.is_valid()) {
-      class_scope.extended_scopes.push_back(base_info.inst_id);
+      class_scope.AddExtendedScope(base_info.inst_id);
     } else {
-      class_scope.has_error = true;
+      class_scope.set_has_error();
     }
   }
   return true;

diff --git a/toolchain/check/handle_export.cpp b/toolchain/check/handle_export.cpp
@@ -81,8 +81,8 @@ auto HandleParseNode(Context& context, Parse::ExportDeclId node_id) -> bool {
   // diagnostic and so that cross-package imports can find it easily.
   auto entity_name = context.entity_names().Get(import_ref->entity_name_id);
   auto& parent_scope = context.name_scopes().Get(entity_name.parent_scope_id);
-  auto lookup = parent_scope.name_map.Lookup(entity_name.name_id);
-  auto& scope_inst_id = parent_scope.names[lookup.value()].inst_id;
+  auto& scope_inst_id =
+      parent_scope.GetEntry(*parent_scope.Lookup(entity_name.name_id)).inst_id;
   CARBON_CHECK(scope_inst_id == inst_id);
   scope_inst_id = export_id;
 

diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp
@@ -76,10 +76,10 @@ static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
     return std::nullopt;
   }
   auto& scope = context.name_scopes().Get(scope_id);
-  if (!scope.inst_id.is_valid()) {
+  if (!scope.inst_id().is_valid()) {
     return std::nullopt;
   }
-  return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id);
+  return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
 }
 
 static auto GetDefaultSelfType(Context& context) -> SemIR::TypeId {
@@ -145,7 +145,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
     CARBON_DIAGNOSTIC(ExtendImplForall, Error,
                       "cannot `extend` a parameterized `impl`");
     context.emitter().Emit(extend_node, ExtendImplForall);
-    parent_scope.has_error = true;
+    parent_scope.set_has_error();
     return;
   }
 
@@ -158,7 +158,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
     // If the explicit self type is not the default, just bail out.
     if (self_type_id != GetDefaultSelfType(context)) {
       diag.Emit();
-      parent_scope.has_error = true;
+      parent_scope.set_has_error();
       return;
     }
 
@@ -176,18 +176,21 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node,
 
   if (!context.types().Is<SemIR::FacetType>(constraint_id)) {
     context.TODO(node_id, "extending non-facet-type constraint");
-    parent_scope.has_error = true;
+    parent_scope.set_has_error();
     return;
   }
-  parent_scope.has_error |= !context.TryToDefineType(constraint_id, [&] {
-    CARBON_DIAGNOSTIC(ExtendUndefinedInterface, Error,
-                      "`extend impl` requires a definition for facet type {0}",
-                      InstIdAsType);
-    return context.emitter().Build(node_id, ExtendUndefinedInterface,
-                                   constraint_inst_id);
-  });
-
-  parent_scope.extended_scopes.push_back(constraint_inst_id);
+  if (!context.TryToDefineType(constraint_id, [&] {
+        CARBON_DIAGNOSTIC(
+            ExtendUndefinedInterface, Error,
+            "`extend impl` requires a definition for facet type {0}",
+            InstIdAsType);
+        return context.emitter().Build(node_id, ExtendUndefinedInterface,
+                                       constraint_inst_id);
+      })) {
+    parent_scope.set_has_error();
+  };
+
+  parent_scope.AddExtendedScope(constraint_inst_id);
 }
 
 // Pops the parameters of an `impl`, forming a `NameComponent` with no

diff --git a/toolchain/check/handle_let_and_var.cpp b/toolchain/check/handle_let_and_var.cpp
@@ -75,7 +75,7 @@ static auto BuildAssociatedConstantDecl(Context& context,
                       "single `:!` binding");
     context.emitter().Emit(pattern.loc_id,
                            ExpectedSymbolicBindingInAssociatedConstant);
-    context.name_scopes().Get(interface_info.scope_id).has_error = true;
+    context.name_scopes().Get(interface_info.scope_id).set_has_error();
     return;
   }
 

diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp
@@ -50,14 +50,17 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
       // Point at the other namespace.
       namespace_inst.name_scope_id = existing->name_scope_id;
 
-      if (context.name_scopes().Get(existing->name_scope_id).is_closed_import) {
+      if (context.name_scopes()
+              .Get(existing->name_scope_id)
+              .is_closed_import()) {
         // The existing name is a package name, so this is a name conflict.
         context.DiagnoseDuplicateName(namespace_id, existing_inst_id);
 
         // Treat this as a local namespace name from now on to avoid further
         // diagnostics.
-        context.name_scopes().Get(existing->name_scope_id).is_closed_import =
-            false;
+        context.name_scopes()
+            .Get(existing->name_scope_id)
+            .set_is_closed_import(false);
       } else if (existing->import_id.is_valid() &&
                  !context.insts().GetLocId(existing_inst_id).is_valid()) {
         // When the name conflict is an imported namespace, fill the location ID