From e49844e7011f089350dbb141716818744b471498 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Tue, 3 Dec 2024 17:54:50 +0100 Subject: [PATCH 01/19] [NFC] Convert NameScope from struct to class This is a preparation change for adding name poisoning support, which is expected to require more elaborate logic around NameScope since a name can be not defined yet, defined, or poisoned. 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. --- toolchain/check/check.cpp | 4 +- toolchain/check/context.cpp | 12 +-- toolchain/check/decl_name_stack.cpp | 10 +- toolchain/check/handle_class.cpp | 6 +- toolchain/check/handle_export.cpp | 4 +- toolchain/check/handle_impl.cpp | 31 +++--- toolchain/check/handle_let_and_var.cpp | 2 +- toolchain/check/handle_namespace.cpp | 9 +- toolchain/check/import.cpp | 136 +++++++++++++------------ toolchain/check/import_ref.cpp | 8 +- toolchain/check/merge.cpp | 5 +- toolchain/lower/mangler.cpp | 6 +- toolchain/sem_ir/formatter.cpp | 12 +-- toolchain/sem_ir/ids.h | 2 +- toolchain/sem_ir/inst_namer.cpp | 3 +- toolchain/sem_ir/name_scope.h | 125 +++++++++++++++++++---- 16 files changed, 237 insertions(+), 138 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index 2767e681a80cc..22d1f2bc7d5da 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -193,7 +193,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( @@ -203,7 +203,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 index 0620507d4b7cf..7f881d1980f87 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -361,16 +361,16 @@ auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id, SemIR::NameScopeId scope_id, const SemIR::NameScope& scope) -> std::pair { - 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}; @@ -527,7 +527,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); @@ -548,7 +548,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 index 25abc2e08589b..c8b795e6243c8 100644 --- 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()) { // 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 index 0fabcbd3d72b5..9e9acb41aac23 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -409,7 +409,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; } @@ -554,9 +554,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 index 0203e94df33dd..ed532f3e1923b 100644 --- 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 index 9d8479126849d..815570327f1a6 100644 --- 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(scope.inst_id); + return context.insts().TryGetAs(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(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 (!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.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 index 7d830c1553381..4df6051a10b1c 100644 --- 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 index 8769c660ca949..560957b0e6cf9 100644 --- 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 diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index c41ff906c1809..1234b042583c0 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -26,6 +26,11 @@ static auto GetImportNameForEntity(const T& entity) -> std::pair { return {entity.name_id, entity.parent_scope_id}; } +template <> +auto GetImportNameForEntity(const SemIR::NameScope& entity) + -> std::pair { + return {entity.name_id(), entity.parent_scope_id()}; +} // Returns name information for the entity, corresponding to IDs in the import // IR rather than the current IR. @@ -98,11 +103,31 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, bool diagnose_duplicate_namespace, llvm::function_ref make_import_id) -> NamespaceResult { + std::optional namespace_inst; + auto make_namespace_id = [&]() { + auto import_id = make_import_id(); + CARBON_CHECK(import_id.is_valid()); + auto import_loc_id = context.insts().GetLocId(import_id); + + namespace_inst = SemIR::Namespace{namespace_type_id, + SemIR::NameScopeId::Invalid, import_id}; + auto namespace_inst_and_loc = + import_loc_id.is_import_ir_inst_id() + ? context.MakeImportedLocAndInst(import_loc_id.import_ir_inst_id(), + *namespace_inst) + // TODO: Check that this actually is an `AnyNamespaceId`. + : SemIR::LocIdAndInst( + Parse::AnyNamespaceId(import_loc_id.node_id()), + *namespace_inst); + return context.AddPlaceholderInstInNoBlock(namespace_inst_and_loc); + }; + auto* parent_scope = &context.name_scopes().Get(parent_scope_id); - auto insert_result = - parent_scope->name_map.Insert(name_id, parent_scope->names.size()); - if (!insert_result.is_inserted()) { - auto prev_inst_id = parent_scope->names[insert_result.value()].inst_id; + auto insert_result = parent_scope->LookupOrAdd(name_id, make_namespace_id, + SemIR::AccessKind::Public); + + if (!insert_result.first) { + auto prev_inst_id = parent_scope->GetEntry(insert_result.second).inst_id; if (auto namespace_inst = context.insts().TryGetAs(prev_inst_id)) { if (diagnose_duplicate_namespace) { @@ -114,25 +139,14 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, } } - auto import_id = make_import_id(); - CARBON_CHECK(import_id.is_valid()); - auto import_loc_id = context.insts().GetLocId(import_id); - - auto namespace_inst = SemIR::Namespace{ - namespace_type_id, SemIR::NameScopeId::Invalid, import_id}; - auto namespace_inst_and_loc = - import_loc_id.is_import_ir_inst_id() - ? context.MakeImportedLocAndInst(import_loc_id.import_ir_inst_id(), - namespace_inst) - // TODO: Check that this actually is an `AnyNamespaceId`. - : SemIR::LocIdAndInst(Parse::AnyNamespaceId(import_loc_id.node_id()), - namespace_inst); - auto namespace_id = - context.AddPlaceholderInstInNoBlock(namespace_inst_and_loc); + auto namespace_id = insert_result.first + ? parent_scope->GetEntry(insert_result.second).inst_id + : make_namespace_id(); + context.import_ref_ids().push_back(namespace_id); - namespace_inst.name_scope_id = + namespace_inst->name_scope_id = context.name_scopes().Add(namespace_id, name_id, parent_scope_id); - context.ReplaceInstBeforeConstantUse(namespace_id, namespace_inst); + context.ReplaceInstBeforeConstantUse(namespace_id, *namespace_inst); // Note we have to get the parent scope freshly, creating the imported // namespace may invalidate the pointer above. @@ -140,18 +154,13 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, // Diagnose if there's a name conflict, but still produce the namespace to // supersede the name conflict in order to avoid repeat diagnostics. - if (!insert_result.is_inserted()) { - auto& entry = parent_scope->names[insert_result.value()]; + if (!insert_result.first) { + auto& entry = parent_scope->GetEntry(insert_result.second); context.DiagnoseDuplicateName(namespace_id, entry.inst_id); entry.inst_id = namespace_id; entry.access_kind = SemIR::AccessKind::Public; - } else { - parent_scope->names.push_back({.name_id = name_id, - .inst_id = namespace_id, - .access_kind = SemIR::AccessKind::Public}); } - - return {namespace_inst.name_scope_id, namespace_id, false}; + return {namespace_inst->name_scope_id, namespace_id, false}; } // Adds a copied namespace to the cache. @@ -237,20 +246,20 @@ static auto CopyAncestorNameScopesFromImportIR( // The namespace hasn't been copied yet, so add it to our list. const auto& scope = import_sem_ir.name_scopes().Get(import_parent_scope_id); auto scope_inst = - import_sem_ir.insts().GetAs(scope.inst_id); + import_sem_ir.insts().GetAs(scope.inst_id()); new_namespaces.push_back(scope_inst.name_scope_id); - import_parent_scope_id = scope.parent_scope_id; + import_parent_scope_id = scope.parent_scope_id(); } // Add ancestor namespace names, starting with the outermost. for (auto import_scope_id : llvm::reverse(new_namespaces)) { auto import_scope = import_sem_ir.name_scopes().Get(import_scope_id); auto name_id = - CopyNameFromImportIR(context, import_sem_ir, import_scope.name_id); + CopyNameFromImportIR(context, import_sem_ir, import_scope.name_id()); scope_cursor = CopySingleNameScopeFromImportIR( context, namespace_type_id, &copied_namespaces, ir_id, - import_scope.inst_id, import_scope_id, scope_cursor, name_id) + import_scope.inst_id(), import_scope_id, scope_cursor, name_id) .name_scope_id; } @@ -265,25 +274,23 @@ static auto AddImportRefOrMerge(Context& context, SemIR::ImportIRId ir_id, SemIR::NameId name_id) -> void { // Leave a placeholder that the inst comes from the other IR. auto& parent_scope = context.name_scopes().Get(parent_scope_id); - auto insert = parent_scope.name_map.Insert(name_id, [&] { - auto entity_name_id = context.entity_names().Add( - {.name_id = name_id, - .parent_scope_id = parent_scope_id, - .bind_index = SemIR::CompileTimeBindIndex::Invalid}); - int index = parent_scope.names.size(); - parent_scope.names.push_back( - {.name_id = name_id, - .inst_id = - AddImportRef(context, {.ir_id = ir_id, .inst_id = import_inst_id}, - entity_name_id), - .access_kind = SemIR::AccessKind::Public}); - return index; - }); - if (insert.is_inserted()) { + auto insert = parent_scope.LookupOrAdd( + name_id, + [&] { + auto entity_name_id = context.entity_names().Add( + {.name_id = name_id, + .parent_scope_id = parent_scope_id, + .bind_index = SemIR::CompileTimeBindIndex::Invalid}); + return AddImportRef(context, + {.ir_id = ir_id, .inst_id = import_inst_id}, + entity_name_id); + }, + SemIR::AccessKind::Public); + if (insert.first) { return; } - auto inst_id = parent_scope.names[insert.value()].inst_id; + auto inst_id = parent_scope.GetEntry(insert.second).inst_id; auto prev_ir_inst = GetCanonicalImportIRInst(context, &context.sem_ir(), inst_id); VerifySameCanonicalImportIRInst(context, inst_id, prev_ir_inst, ir_id, @@ -334,7 +341,7 @@ static auto ImportScopeFromApiFile(Context& context, const auto& api_scope = api_sem_ir.name_scopes().Get(api_scope_id); auto& impl_scope = context.name_scopes().Get(impl_scope_id); - for (const auto& api_entry : api_scope.names) { + for (const auto& api_entry : api_scope.entries()) { auto impl_name_id = CopyNameFromImportIR(context, api_sem_ir, api_entry.name_id); if (auto ns = @@ -343,7 +350,7 @@ static auto ImportScopeFromApiFile(Context& context, // ImportLibrariesFromOtherPackage. if (api_scope_id == SemIR::NameScopeId::Package) { const auto& ns_scope = api_sem_ir.name_scopes().Get(ns->name_scope_id); - if (!ns_scope.import_ir_scopes.empty()) { + if (!ns_scope.import_ir_scopes().empty()) { continue; } } @@ -428,8 +435,8 @@ auto ImportLibrariesFromCurrentPackage( // file, it transitively affects the current file too. if (import_ir.sem_ir->name_scopes() .Get(SemIR::NameScopeId::Package) - .has_error) { - context.name_scopes().Get(SemIR::NameScopeId::Package).has_error = true; + .has_error()) { + context.name_scopes().Get(SemIR::NameScopeId::Package).set_has_error(); } } } @@ -451,15 +458,16 @@ auto ImportLibrariesFromOtherPackage(Context& context, auto namespace_const_id = context.constant_values().Get(result.inst_id); auto& scope = context.name_scopes().Get(result.name_scope_id); - scope.is_closed_import = !result.is_duplicate_of_namespace_in_current_package; + scope.set_is_closed_import( + !result.is_duplicate_of_namespace_in_current_package); for (auto import_ir : import_irs) { auto ir_id = AddImportIR(context, import_ir); - scope.import_ir_scopes.push_back({ir_id, SemIR::NameScopeId::Package}); + scope.AddImportIrScope({ir_id, SemIR::NameScopeId::Package}); context.import_ir_constant_values()[ir_id.index].Set( SemIR::InstId::PackageNamespace, namespace_const_id); } if (has_load_error) { - scope.has_error = has_load_error; + scope.set_has_error(); } } @@ -484,13 +492,15 @@ static auto LookupNameInImport(const SemIR::File& import_ir, // Look up the name in the import scope. const auto& import_scope = import_ir.name_scopes().Get(import_scope_id); - auto lookup = import_scope.name_map.Lookup(import_name_id); - if (!lookup) { + auto import_scope_entry_id = import_scope.Lookup(import_name_id); + if (!import_scope_entry_id) { // Name doesn't exist in the import scope. return nullptr; } - const auto& import_scope_entry = import_scope.names[lookup.value()]; + const auto& import_scope_entry = + import_scope.GetEntry(*import_scope_entry_id); + if (import_scope_entry.access_kind != SemIR::AccessKind::Public) { // Ignore cross-package non-public names. return nullptr; @@ -513,8 +523,9 @@ static auto AddNamespaceFromOtherPackage(Context& context, context, namespace_type_id, /*copied_namespaces=*/nullptr, import_ir_id, import_inst_id, import_ns.name_scope_id, parent_scope_id, name_id); auto& scope = context.name_scopes().Get(result.name_scope_id); - scope.is_closed_import = !result.is_duplicate_of_namespace_in_current_package; - scope.import_ir_scopes.push_back({import_ir_id, import_ns.name_scope_id}); + scope.set_is_closed_import( + !result.is_duplicate_of_namespace_in_current_package); + scope.AddImportIrScope({import_ir_id, import_ns.name_scope_id}); return result.inst_id; } @@ -585,8 +596,7 @@ auto ImportNameFromOtherPackage( if (auto import_ns = import_inst.TryAs()) { if (auto ns = context.insts().TryGetAs(result_id)) { auto& name_scope = context.name_scopes().Get(ns->name_scope_id); - name_scope.import_ir_scopes.push_back( - {import_ir_id, import_ns->name_scope_id}); + name_scope.AddImportIrScope({import_ir_id, import_ns->name_scope_id}); continue; } } diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index b374b160fd3b3..d8acc8ff02773 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -1194,14 +1194,14 @@ static auto GetIncompleteLocalEntityBase( static auto AddNameScopeImportRefs(ImportContext& context, const SemIR::NameScope& import_scope, SemIR::NameScope& new_scope) -> void { - for (auto entry : import_scope.names) { + for (auto entry : import_scope.entries()) { auto ref_id = AddImportRef(context, entry.inst_id); new_scope.AddRequired({.name_id = GetLocalNameId(context, entry.name_id), .inst_id = ref_id, .access_kind = entry.access_kind}); } - for (auto scope_inst_id : import_scope.extended_scopes) { - new_scope.extended_scopes.push_back(AddImportRef(context, scope_inst_id)); + for (auto scope_inst_id : import_scope.extended_scopes()) { + new_scope.AddExtendedScope(AddImportRef(context, scope_inst_id)); } } @@ -1993,7 +1993,7 @@ static auto AddInterfaceDefinition(ImportContext& context, context.local_context().inst_block_stack().Pop(); new_interface.self_param_id = self_param_id; - CARBON_CHECK(import_scope.extended_scopes.empty(), + CARBON_CHECK(import_scope.extended_scopes().empty(), "Interfaces don't currently have extended scopes to support."); } diff --git a/toolchain/check/merge.cpp b/toolchain/check/merge.cpp index 44e50bf89033b..4790803b37175 100644 --- a/toolchain/check/merge.cpp +++ b/toolchain/check/merge.cpp @@ -169,8 +169,9 @@ auto ReplacePrevInstForMerge(Context& context, SemIR::NameScopeId scope_id, SemIR::NameId name_id, SemIR::InstId new_inst_id) -> void { auto& scope = context.name_scopes().Get(scope_id); - if (auto lookup = scope.name_map.Lookup(name_id)) { - scope.names[lookup.value()].inst_id = new_inst_id; + auto entry_id = scope.Lookup(name_id); + if (entry_id) { + scope.GetEntry(*entry_id).inst_id = new_inst_id; } } diff --git a/toolchain/lower/mangler.cpp b/toolchain/lower/mangler.cpp index fb910e1355336..16ceb05d72574 100644 --- a/toolchain/lower/mangler.cpp +++ b/toolchain/lower/mangler.cpp @@ -39,7 +39,7 @@ auto Mangler::MangleInverseQualifiedNameScope(llvm::raw_ostream& os, continue; } const auto& name_scope = sem_ir().name_scopes().Get(name_scope_id); - CARBON_KIND_SWITCH(sem_ir().insts().Get(name_scope.inst_id)) { + CARBON_KIND_SWITCH(sem_ir().insts().Get(name_scope.inst_id())) { case CARBON_KIND(SemIR::ImplDecl impl_decl): { const auto& impl = sem_ir().impls().Get(impl_decl.impl_id); @@ -110,7 +110,7 @@ auto Mangler::MangleInverseQualifiedNameScope(llvm::raw_ostream& os, break; } case SemIR::Namespace::Kind: { - os << names().GetAsStringIfIdentifier(name_scope.name_id); + os << names().GetAsStringIfIdentifier(name_scope.name_id()); break; } default: @@ -119,7 +119,7 @@ auto Mangler::MangleInverseQualifiedNameScope(llvm::raw_ostream& os, } if (!name_scope.is_imported_package()) { names_to_render.push_back( - {.name_scope_id = name_scope.parent_scope_id, .prefix = '.'}); + {.name_scope_id = name_scope.parent_scope_id(), .prefix = '.'}); } } } diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index f5ec0b805d0bc..0bcfb8570e4a9 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -591,8 +591,8 @@ class FormatterImpl { auto FormatNameScope(NameScopeId id, llvm::StringRef label = "") -> void { const auto& scope = sem_ir_.name_scopes().Get(id); - if (scope.names.empty() && scope.extended_scopes.empty() && - scope.import_ir_scopes.empty() && !scope.has_error) { + if (scope.entries().empty() && scope.extended_scopes().empty() && + scope.import_ir_scopes().empty() && !scope.has_error()) { // Name scope is empty. return; } @@ -602,7 +602,7 @@ class FormatterImpl { out_ << label; } - for (auto [name_id, inst_id, access_kind] : scope.names) { + for (auto [name_id, inst_id, access_kind] : scope.entries()) { Indent(); out_ << "."; FormatName(name_id); @@ -621,7 +621,7 @@ class FormatterImpl { out_ << "\n"; } - for (auto extended_scope_id : scope.extended_scopes) { + for (auto extended_scope_id : scope.extended_scopes()) { Indent(); out_ << "extend "; FormatName(extended_scope_id); @@ -633,7 +633,7 @@ class FormatterImpl { // add or remove an unused prelude file, but is intended to still show the // existence of indirect imports. bool has_prelude_components = false; - for (auto [import_ir_id, unused] : scope.import_ir_scopes) { + for (auto [import_ir_id, unused] : scope.import_ir_scopes()) { auto label = GetImportIRLabel(import_ir_id); if (label.starts_with("Core//prelude/")) { if (has_prelude_components) { @@ -648,7 +648,7 @@ class FormatterImpl { out_ << "import " << label << "\n"; } - if (scope.has_error) { + if (scope.has_error()) { Indent(); out_ << "has_error\n"; } diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index c0e25d15ae2c1..6d7d431bfa0a8 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -18,6 +18,7 @@ namespace Carbon::SemIR { // Forward declare indexed types, for integration with ValueStore. class File; class Inst; +class NameScope; struct EntityName; struct Class; struct FacetTypeInfo; @@ -28,7 +29,6 @@ struct ImportIR; struct ImportIRInst; struct Impl; struct Interface; -struct NameScope; struct StructTypeField; struct TypeInfo; diff --git a/toolchain/sem_ir/inst_namer.cpp b/toolchain/sem_ir/inst_namer.cpp index cccf4686ef658..e5b8cfccd50ee 100644 --- a/toolchain/sem_ir/inst_namer.cpp +++ b/toolchain/sem_ir/inst_namer.cpp @@ -560,7 +560,8 @@ auto InstNamer::CollectNamesInBlock(ScopeId scope_id, } // The namespace is specified here due to the name conflict. case CARBON_KIND(SemIR::Namespace inst): { - add_inst_name_id(sem_ir_.name_scopes().Get(inst.name_scope_id).name_id); + add_inst_name_id( + sem_ir_.name_scopes().Get(inst.name_scope_id).name_id()); continue; } case OutParam::Kind: diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index fe985ab7c996a..baea84674cdaf 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -18,27 +18,42 @@ enum class AccessKind : int8_t { Private, }; -struct NameScope : Printable { +class NameScope : public Printable { + public: struct Entry { NameId name_id; InstId inst_id; AccessKind access_kind; }; + class EntryId { + private: + friend class NameScope; + + EntryId(int entry_index) : entry_index_(entry_index) {} + + int entry_index_; + }; + + NameScope(InstId inst_id, NameId name_id, NameScopeId parent_scope_id) + : inst_id_(inst_id), + name_id_(name_id), + parent_scope_id_(parent_scope_id) {} + auto Print(llvm::raw_ostream& out) const -> void { - out << "{inst: " << inst_id << ", parent_scope: " << parent_scope_id - << ", has_error: " << (has_error ? "true" : "false"); + out << "{inst: " << inst_id() << ", parent_scope: " << parent_scope_id() + << ", has_error: " << (has_error_ ? "true" : "false"); out << ", extended_scopes: ["; llvm::ListSeparator scope_sep; - for (auto id : extended_scopes) { + for (auto id : extended_scopes()) { out << scope_sep << id; } out << "]"; out << ", names: {"; llvm::ListSeparator sep; - for (auto entry : names) { + for (auto entry : entries()) { out << sep << entry.name_id << ": " << entry.inst_id; } out << "}"; @@ -46,23 +61,91 @@ struct NameScope : Printable { out << "}"; } + auto entries() const -> const llvm::SmallVector& { return names_; } + + auto GetEntry(EntryId entry_id) const -> const Entry& { + return names_[entry_id.entry_index_]; + } + + auto GetEntry(EntryId entry_id) -> Entry& { + return names_[entry_id.entry_index_]; + } + + auto Lookup(NameId name_id) const -> std::optional { + auto lookup = name_map_.Lookup(name_id); + if (!lookup) { + return std::nullopt; + } + return EntryId(lookup.value()); + } + // Adds a name to the scope that must not already exist. auto AddRequired(Entry name_entry) -> void { auto add_name = [&] { - int index = names.size(); - names.push_back(name_entry); + int index = names_.size(); + names_.push_back(name_entry); return index; }; - auto result = name_map.Insert(name_entry.name_id, add_name); + auto result = name_map_.Insert(name_entry.name_id, add_name); CARBON_CHECK(result.is_inserted(), "Failed to add required name: {0}", name_entry.name_id); } + auto LookupOrAdd(SemIR::NameId name_id, + llvm::function_ref make_inst_id, + AccessKind access_kind) -> std::pair { + auto insert_result = name_map_.Insert(name_id, names_.size()); + if (!insert_result.is_inserted()) { + return {false, EntryId(insert_result.value())}; + } + + names_.push_back({.name_id = name_id, + .inst_id = make_inst_id(), + .access_kind = access_kind}); + return {true, EntryId(names_.size() - 1)}; + } + + auto extended_scopes() const -> const llvm::SmallVector& { + return extended_scopes_; + } + + auto AddExtendedScope(SemIR::InstId extended_scope) -> void { + extended_scopes_.push_back(extended_scope); + } + + auto inst_id() const -> InstId { return inst_id_; } + + auto name_id() const -> NameId { return name_id_; } + + auto parent_scope_id() const -> NameScopeId { return parent_scope_id_; } + + auto has_error() const -> bool { return has_error_; } + + auto set_has_error() -> void { has_error_ = true; } + + auto is_closed_import() const -> bool { return is_closed_import_; } + + auto set_is_closed_import(bool is_closed_import) -> void { + is_closed_import_ = is_closed_import; + } + // Returns true if this name scope describes an imported package. auto is_imported_package() const -> bool { - return is_closed_import && parent_scope_id == NameScopeId::Package; + return is_closed_import() && parent_scope_id() == NameScopeId::Package; + } + + auto import_ir_scopes() const -> const + llvm::SmallVector, 0>& { + return import_ir_scopes_; } + auto AddImportIrScope( + const std::pair& import_ir_scope) + -> void { + return import_ir_scopes_.push_back(import_ir_scope); + } + + private: // Names in the scope. We store both an insertion-ordered vector for iterating // and a map from `NameId` to the index of that vector for name lookup. // @@ -72,8 +155,8 @@ struct NameScope : Printable { // vector so that these can pack densely. If this ends up both cold and memory // intensive, we can also switch the lookup to a set of indices into the // vector rather than a map from `NameId` to index. - llvm::SmallVector names; - Map name_map; + llvm::SmallVector names_; + Map name_map_; // Instructions returning values that are extended by this scope. // @@ -81,31 +164,31 @@ struct NameScope : Printable { // than a single extended scope. // TODO: Revisit this once we have more kinds of extended scope and data. // TODO: Consider using something like `TinyPtrVector` for this. - llvm::SmallVector extended_scopes; + llvm::SmallVector extended_scopes_; // The instruction which owns the scope. - InstId inst_id; + InstId inst_id_; // When the scope is a namespace, the name. Otherwise, invalid. - NameId name_id; + NameId name_id_; // The parent scope. - NameScopeId parent_scope_id; + NameScopeId parent_scope_id_; // Whether we have diagnosed an error in a construct that would have added // names to this scope. For example, this can happen if an `import` failed or // an `extend` declaration was ill-formed. If true, the `names` map is assumed // to be missing names as a result of the error, and no further errors are // produced for lookup failures in this scope. - bool has_error = false; + bool has_error_ = false; // True if this is a closed namespace created by importing a package. - bool is_closed_import = false; + bool is_closed_import_ = false; // Imported IR scopes that compose this namespace. This will be empty for // scopes that correspond to the current package. llvm::SmallVector, 0> - import_ir_scopes; + import_ir_scopes_; }; // Provides a ValueStore wrapper for an API specific to name scopes. @@ -116,9 +199,7 @@ class NameScopeStore { // Adds a name scope, returning an ID to reference it. auto Add(InstId inst_id, NameId name_id, NameScopeId parent_scope_id) -> NameScopeId { - return values_.Add({.inst_id = inst_id, - .name_id = name_id, - .parent_scope_id = parent_scope_id}); + return values_.Add(NameScope(inst_id, name_id, parent_scope_id)); } // Adds a name that is required to exist in a name scope, such as `Self`. @@ -145,7 +226,7 @@ class NameScopeStore { if (!scope_id.is_valid()) { return {InstId::Invalid, std::nullopt}; } - auto inst_id = Get(scope_id).inst_id; + auto inst_id = Get(scope_id).inst_id(); if (!inst_id.is_valid()) { return {InstId::Invalid, std::nullopt}; } From 64dbb92698b015ffcfeac31556b0abc2034ac7a9 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 09:25:09 +0100 Subject: [PATCH 02/19] Style fixes: * Make EntryId a struct that inherits from IdBase. * Return ArrayRef and not const vector &. * Rename AddImportIrScope() to AddImportIRScope() to be consistent with ImportIRId and SemIR. --- toolchain/check/import.cpp | 6 +++--- toolchain/sem_ir/name_scope.h | 28 +++++++++++----------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 1234b042583c0..1e8b124f4284d 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -462,7 +462,7 @@ auto ImportLibrariesFromOtherPackage(Context& context, !result.is_duplicate_of_namespace_in_current_package); for (auto import_ir : import_irs) { auto ir_id = AddImportIR(context, import_ir); - scope.AddImportIrScope({ir_id, SemIR::NameScopeId::Package}); + scope.AddImportIRScope({ir_id, SemIR::NameScopeId::Package}); context.import_ir_constant_values()[ir_id.index].Set( SemIR::InstId::PackageNamespace, namespace_const_id); } @@ -525,7 +525,7 @@ static auto AddNamespaceFromOtherPackage(Context& context, auto& scope = context.name_scopes().Get(result.name_scope_id); scope.set_is_closed_import( !result.is_duplicate_of_namespace_in_current_package); - scope.AddImportIrScope({import_ir_id, import_ns.name_scope_id}); + scope.AddImportIRScope({import_ir_id, import_ns.name_scope_id}); return result.inst_id; } @@ -596,7 +596,7 @@ auto ImportNameFromOtherPackage( if (auto import_ns = import_inst.TryAs()) { if (auto ns = context.insts().TryGetAs(result_id)) { auto& name_scope = context.name_scopes().Get(ns->name_scope_id); - name_scope.AddImportIrScope({import_ir_id, import_ns->name_scope_id}); + name_scope.AddImportIRScope({import_ir_id, import_ns->name_scope_id}); continue; } } diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index baea84674cdaf..f9ac1c47afde6 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -26,16 +26,12 @@ class NameScope : public Printable { AccessKind access_kind; }; - class EntryId { - private: - friend class NameScope; - - EntryId(int entry_index) : entry_index_(entry_index) {} - - int entry_index_; + struct EntryId : public IdBase { + using IdBase::IdBase; }; - NameScope(InstId inst_id, NameId name_id, NameScopeId parent_scope_id) + explicit NameScope(InstId inst_id, NameId name_id, + NameScopeId parent_scope_id) : inst_id_(inst_id), name_id_(name_id), parent_scope_id_(parent_scope_id) {} @@ -61,15 +57,13 @@ class NameScope : public Printable { out << "}"; } - auto entries() const -> const llvm::SmallVector& { return names_; } + auto entries() const -> llvm::ArrayRef { return names_; } auto GetEntry(EntryId entry_id) const -> const Entry& { - return names_[entry_id.entry_index_]; + return names_[entry_id.index]; } - auto GetEntry(EntryId entry_id) -> Entry& { - return names_[entry_id.entry_index_]; - } + auto GetEntry(EntryId entry_id) -> Entry& { return names_[entry_id.index]; } auto Lookup(NameId name_id) const -> std::optional { auto lookup = name_map_.Lookup(name_id); @@ -105,7 +99,7 @@ class NameScope : public Printable { return {true, EntryId(names_.size() - 1)}; } - auto extended_scopes() const -> const llvm::SmallVector& { + auto extended_scopes() const -> llvm::ArrayRef { return extended_scopes_; } @@ -134,12 +128,12 @@ class NameScope : public Printable { return is_closed_import() && parent_scope_id() == NameScopeId::Package; } - auto import_ir_scopes() const -> const - llvm::SmallVector, 0>& { + auto import_ir_scopes() const + -> llvm::ArrayRef> { return import_ir_scopes_; } - auto AddImportIrScope( + auto AddImportIRScope( const std::pair& import_ir_scope) -> void { return import_ir_scopes_.push_back(import_ir_scope); From a7df529074e5894f5b1cf1e4d3547c314ac2bea3 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 09:31:14 +0100 Subject: [PATCH 03/19] Move NameScope::AddRequired() and NameScope::LookupOrAdd() to a cpp file. --- toolchain/sem_ir/BUILD | 1 + toolchain/sem_ir/name_scope.cpp | 35 +++++++++++++++++++++++++++++++++ toolchain/sem_ir/name_scope.h | 24 ++-------------------- 3 files changed, 38 insertions(+), 22 deletions(-) create mode 100644 toolchain/sem_ir/name_scope.cpp diff --git a/toolchain/sem_ir/BUILD b/toolchain/sem_ir/BUILD index 6a6635dd35d46..0bc2f2dccd3a6 100644 --- a/toolchain/sem_ir/BUILD +++ b/toolchain/sem_ir/BUILD @@ -74,6 +74,7 @@ cc_library( "generic.cpp", "impl.cpp", "name.cpp", + "name_scope.cpp", "type.cpp", "type_info.cpp", ], diff --git a/toolchain/sem_ir/name_scope.cpp b/toolchain/sem_ir/name_scope.cpp new file mode 100644 index 0000000000000..b6443a2d7854b --- /dev/null +++ b/toolchain/sem_ir/name_scope.cpp @@ -0,0 +1,35 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "toolchain/sem_ir/name_scope.h" + +namespace Carbon::SemIR { + +auto NameScope::AddRequired(Entry name_entry) -> void { + auto add_name = [&] { + int index = names_.size(); + names_.push_back(name_entry); + return index; + }; + auto result = name_map_.Insert(name_entry.name_id, add_name); + CARBON_CHECK(result.is_inserted(), "Failed to add required name: {0}", + name_entry.name_id); +} + +auto NameScope::LookupOrAdd(SemIR::NameId name_id, + llvm::function_ref make_inst_id, + AccessKind access_kind) + -> std::pair { + auto insert_result = name_map_.Insert(name_id, names_.size()); + if (!insert_result.is_inserted()) { + return {false, EntryId(insert_result.value())}; + } + + names_.push_back({.name_id = name_id, + .inst_id = make_inst_id(), + .access_kind = access_kind}); + return {true, EntryId(names_.size() - 1)}; +} + +} // namespace Carbon::SemIR diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index f9ac1c47afde6..dd9ac398d31aa 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -73,31 +73,11 @@ class NameScope : public Printable { return EntryId(lookup.value()); } - // Adds a name to the scope that must not already exist. - auto AddRequired(Entry name_entry) -> void { - auto add_name = [&] { - int index = names_.size(); - names_.push_back(name_entry); - return index; - }; - auto result = name_map_.Insert(name_entry.name_id, add_name); - CARBON_CHECK(result.is_inserted(), "Failed to add required name: {0}", - name_entry.name_id); - } + auto AddRequired(Entry name_entry) -> void; auto LookupOrAdd(SemIR::NameId name_id, llvm::function_ref make_inst_id, - AccessKind access_kind) -> std::pair { - auto insert_result = name_map_.Insert(name_id, names_.size()); - if (!insert_result.is_inserted()) { - return {false, EntryId(insert_result.value())}; - } - - names_.push_back({.name_id = name_id, - .inst_id = make_inst_id(), - .access_kind = access_kind}); - return {true, EntryId(names_.size() - 1)}; - } + AccessKind access_kind) -> std::pair; auto extended_scopes() const -> llvm::ArrayRef { return extended_scopes_; From 841ef2bab78b89f84c3d8bac0605048da2bc2df9 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 09:44:31 +0100 Subject: [PATCH 04/19] Add and update comments in NameScope API. --- toolchain/sem_ir/name_scope.h | 44 +++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index dd9ac398d31aa..d51f898841ed4 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -57,14 +57,21 @@ class NameScope : public Printable { out << "}"; } + // Names in the scope. + // Entries could become invalidated if the scope object is invalidated or if a + // name is added. auto entries() const -> llvm::ArrayRef { return names_; } + // Get a specific Name entry based on an EntryId that return from a lookup. + // The Entry could become invalidated if the scope object is invalidated or if + // a name is added. auto GetEntry(EntryId entry_id) const -> const Entry& { return names_[entry_id.index]; } - auto GetEntry(EntryId entry_id) -> Entry& { return names_[entry_id.index]; } + // Searches for the given name and returns an EntryId if found or nullopt if + // not. auto Lookup(NameId name_id) const -> std::optional { auto lookup = name_map_.Lookup(name_id); if (!lookup) { @@ -73,12 +80,17 @@ class NameScope : public Printable { return EntryId(lookup.value()); } + // Adds a new name known to not exist. auto AddRequired(Entry name_entry) -> void; + // If the given name already exists, return true and an EntryId. + // If not, adds the name using the result of make_inst_id() and access_kind + // and return false and an EntryId. auto LookupOrAdd(SemIR::NameId name_id, llvm::function_ref make_inst_id, AccessKind access_kind) -> std::pair; + // Instructions returning values that are extended by this scope. auto extended_scopes() const -> llvm::ArrayRef { return extended_scopes_; } @@ -87,16 +99,27 @@ class NameScope : public Printable { extended_scopes_.push_back(extended_scope); } + // The instruction which owns the scope. auto inst_id() const -> InstId { return inst_id_; } + // When the scope is a namespace, the name. Otherwise, invalid. auto name_id() const -> NameId { return name_id_; } + // The parent scope. auto parent_scope_id() const -> NameScopeId { return parent_scope_id_; } + // Whether we have diagnosed an error in a construct that would have added + // names to this scope. For example, this can happen if an `import` failed or + // an `extend` declaration was ill-formed. If true, names are assumed to be + // missing as a result of the error, and no further errors are produced for + // lookup failures in this scope. auto has_error() const -> bool { return has_error_; } + // Mark that we have diagnosed an error in a construct that would have added + // names to this scope. auto set_has_error() -> void { has_error_ = true; } + // True if this is a closed namespace created by importing a package. auto is_closed_import() const -> bool { return is_closed_import_; } auto set_is_closed_import(bool is_closed_import) -> void { @@ -108,6 +131,8 @@ class NameScope : public Printable { return is_closed_import() && parent_scope_id() == NameScopeId::Package; } + // Imported IR scopes that compose this namespace. This will be empty for + // scopes that correspond to the current package. auto import_ir_scopes() const -> llvm::ArrayRef> { return import_ir_scopes_; @@ -120,7 +145,7 @@ class NameScope : public Printable { } private: - // Names in the scope. We store both an insertion-ordered vector for iterating + // We store both an insertion-ordered vector for iterating // and a map from `NameId` to the index of that vector for name lookup. // // Optimization notes: this is somewhat memory inefficient. If this ends up @@ -132,35 +157,20 @@ class NameScope : public Printable { llvm::SmallVector names_; Map name_map_; - // Instructions returning values that are extended by this scope. - // // Small vector size is set to 1: we expect that there will rarely be more // than a single extended scope. // TODO: Revisit this once we have more kinds of extended scope and data. // TODO: Consider using something like `TinyPtrVector` for this. llvm::SmallVector extended_scopes_; - // The instruction which owns the scope. InstId inst_id_; - - // When the scope is a namespace, the name. Otherwise, invalid. NameId name_id_; - - // The parent scope. NameScopeId parent_scope_id_; - // Whether we have diagnosed an error in a construct that would have added - // names to this scope. For example, this can happen if an `import` failed or - // an `extend` declaration was ill-formed. If true, the `names` map is assumed - // to be missing names as a result of the error, and no further errors are - // produced for lookup failures in this scope. bool has_error_ = false; - // True if this is a closed namespace created by importing a package. bool is_closed_import_ = false; - // Imported IR scopes that compose this namespace. This will be empty for - // scopes that correspond to the current package. llvm::SmallVector, 0> import_ir_scopes_; }; From e87e7c9252a1cf4e0314c569fb864d952c7f969c Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 12:06:05 +0100 Subject: [PATCH 05/19] NameScope unit tests --- toolchain/sem_ir/BUILD | 11 ++ toolchain/sem_ir/name_scope_test.cpp | 264 +++++++++++++++++++++++++++ 2 files changed, 275 insertions(+) create mode 100644 toolchain/sem_ir/name_scope_test.cpp diff --git a/toolchain/sem_ir/BUILD b/toolchain/sem_ir/BUILD index 0bc2f2dccd3a6..62e2fb2a82ca5 100644 --- a/toolchain/sem_ir/BUILD +++ b/toolchain/sem_ir/BUILD @@ -181,6 +181,17 @@ cc_library( ], ) +cc_test( + name = "name_scope_test", + size = "small", + srcs = ["name_scope_test.cpp"], + deps = [ + ":file", + "//testing/base:gtest_main", + "@googletest//:gtest", + ], +) + cc_test( name = "typed_insts_test", size = "small", diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp new file mode 100644 index 0000000000000..775a1ffa07c07 --- /dev/null +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -0,0 +1,264 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "toolchain/sem_ir/name_scope.h" + +#include +#include + +namespace Carbon::SemIR { +namespace { + +using testing::ElementsAre; +using testing::Pair; + +TEST(NameScope, Empty) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id(id++); + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_THAT(name_scope.entries(), ElementsAre()); + EXPECT_THAT(name_scope.extended_scopes(), ElementsAre()); + EXPECT_EQ(name_scope.inst_id(), scope_inst_id); + EXPECT_EQ(name_scope.name_id(), scope_name_id); + EXPECT_EQ(name_scope.parent_scope_id(), parent_scope_id); + EXPECT_FALSE(name_scope.has_error()); + EXPECT_FALSE(name_scope.is_closed_import()); + EXPECT_FALSE(name_scope.is_imported_package()); + EXPECT_THAT(name_scope.import_ir_scopes(), ElementsAre()); +} + +TEST(NameScope, Lookup) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id(id++); + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + NameScope::Entry entry1{.name_id = NameId(id++), + .inst_id = InstId(id++), + .access_kind = AccessKind::Public}; + name_scope.AddRequired(entry1); + + NameScope::Entry entry2{.name_id = NameId(id++), + .inst_id = InstId(id++), + .access_kind = AccessKind::Protected}; + name_scope.AddRequired(entry2); + + NameScope::Entry entry3{.name_id = NameId(id++), + .inst_id = InstId(id++), + .access_kind = AccessKind::Private}; + name_scope.AddRequired(entry3); + + auto lookup = name_scope.Lookup(entry1.name_id); + ASSERT_NE(lookup, std::nullopt); + EXPECT_EQ(StructReflection::AsTuple( + static_cast(name_scope).GetEntry(*lookup)), + StructReflection::AsTuple(entry1)); + EXPECT_EQ(StructReflection::AsTuple( + static_cast(name_scope).GetEntry(*lookup)), + StructReflection::AsTuple(entry1)); + + lookup = name_scope.Lookup(entry2.name_id); + ASSERT_NE(lookup, std::nullopt); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(*lookup)), + StructReflection::AsTuple(entry2)); + + lookup = name_scope.Lookup(entry3.name_id); + ASSERT_NE(lookup, std::nullopt); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(*lookup)), + StructReflection::AsTuple(entry3)); + + NameId unknown_name_id(id++); + lookup = name_scope.Lookup(unknown_name_id); + EXPECT_EQ(lookup, std::nullopt); +} + +TEST(NameScope, LookupOrAdd) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id(id++); + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + NameScope::Entry entry1{.name_id = NameId(id++), + .inst_id = InstId(id++), + .access_kind = AccessKind::Public}; + auto lookup = name_scope.LookupOrAdd( + entry1.name_id, [&]() { return entry1.inst_id; }, entry1.access_kind); + EXPECT_TRUE(lookup.first); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), + StructReflection::AsTuple(entry1)); + + NameScope::Entry entry2{.name_id = NameId(id++), + .inst_id = InstId(id++), + .access_kind = AccessKind::Protected}; + lookup = name_scope.LookupOrAdd( + entry2.name_id, [&]() { return entry2.inst_id; }, entry2.access_kind); + EXPECT_TRUE(lookup.first); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), + StructReflection::AsTuple(entry2)); + + NameScope::Entry entry3{.name_id = NameId(id++), + .inst_id = InstId(id++), + .access_kind = AccessKind::Private}; + lookup = name_scope.LookupOrAdd( + entry3.name_id, [&]() { return entry3.inst_id; }, entry3.access_kind); + EXPECT_TRUE(lookup.first); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), + StructReflection::AsTuple(entry3)); + + lookup = name_scope.LookupOrAdd( + entry1.name_id, + [&]() { + ADD_FAILURE() << "Unexpected call"; + return entry1.inst_id; + }, + entry1.access_kind); + EXPECT_FALSE(lookup.first); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), + StructReflection::AsTuple(entry1)); + + lookup = name_scope.LookupOrAdd( + entry2.name_id, + [&]() { + ADD_FAILURE() << "Unexpected call"; + return entry2.inst_id; + }, + entry2.access_kind); + EXPECT_FALSE(lookup.first); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), + StructReflection::AsTuple(entry2)); + + lookup = name_scope.LookupOrAdd( + entry3.name_id, + [&]() { + ADD_FAILURE() << "Unexpected call"; + return entry3.inst_id; + }, + entry3.access_kind); + EXPECT_FALSE(lookup.first); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), + StructReflection::AsTuple(entry3)); +} + +TEST(NameScope, ExtendedScopes) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id = NameScopeId::Package; + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_THAT(name_scope.extended_scopes(), ElementsAre()); + + InstId extended_scope1(id++); + name_scope.AddExtendedScope(extended_scope1); + EXPECT_THAT(name_scope.extended_scopes(), ElementsAre(extended_scope1)); + + InstId extended_scope2(id++); + name_scope.AddExtendedScope(extended_scope2); + EXPECT_THAT(name_scope.extended_scopes(), + ElementsAre(extended_scope1, extended_scope2)); +} + +TEST(NameScope, HasError) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id(id++); + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_FALSE(name_scope.has_error()); + + name_scope.set_has_error(); + EXPECT_TRUE(name_scope.has_error()); + + name_scope.set_has_error(); + EXPECT_TRUE(name_scope.has_error()); +} + +TEST(NameScope, IsClosedImport) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id(id++); + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_FALSE(name_scope.is_closed_import()); + + name_scope.set_is_closed_import(true); + EXPECT_TRUE(name_scope.is_closed_import()); + + name_scope.set_is_closed_import(false); + EXPECT_FALSE(name_scope.is_closed_import()); +} + +TEST(NameScope, IsImportedPackageParentNonPackageScope) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id(id++); + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_FALSE(name_scope.is_imported_package()); + + name_scope.set_is_closed_import(true); + EXPECT_FALSE(name_scope.is_imported_package()); + + name_scope.set_is_closed_import(false); + EXPECT_FALSE(name_scope.is_imported_package()); +} + +TEST(NameScope, IsImportedPackageParentPackageScope) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id = NameScopeId::Package; + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_FALSE(name_scope.is_imported_package()); + + name_scope.set_is_closed_import(true); + EXPECT_TRUE(name_scope.is_imported_package()); + + name_scope.set_is_closed_import(false); + EXPECT_FALSE(name_scope.is_imported_package()); +} + +TEST(NameScope, ImportIRScopes) { + int id = 1; + + InstId scope_inst_id(id++); + NameId scope_name_id(id++); + NameScopeId parent_scope_id = NameScopeId::Package; + NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); + + EXPECT_THAT(name_scope.import_ir_scopes(), ElementsAre()); + + ImportIRId import_ir_id1(id++); + NameScopeId import_name_scope_id1(id++); + name_scope.AddImportIRScope({import_ir_id1, import_name_scope_id1}); + EXPECT_THAT(name_scope.import_ir_scopes(), + ElementsAre(Pair(import_ir_id1, import_name_scope_id1))); + + ImportIRId import_ir_id2(id++); + NameScopeId import_name_scope_id2(id++); + name_scope.AddImportIRScope({import_ir_id2, import_name_scope_id2}); + EXPECT_THAT(name_scope.import_ir_scopes(), + ElementsAre(Pair(import_ir_id1, import_name_scope_id1), + Pair(import_ir_id2, import_name_scope_id2))); +} + +} // namespace +} // namespace Carbon::SemIR From d8c69545642008967a139791f6c525a30e07a694 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 16:27:37 +0100 Subject: [PATCH 06/19] Change AddNamespace() to be more similar to the code that used NameScope as a struct to make it simpler --- toolchain/check/import.cpp | 58 ++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 1e8b124f4284d..03cd19c4ff8a2 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -103,29 +103,14 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, bool diagnose_duplicate_namespace, llvm::function_ref make_import_id) -> NamespaceResult { - std::optional namespace_inst; - auto make_namespace_id = [&]() { - auto import_id = make_import_id(); - CARBON_CHECK(import_id.is_valid()); - auto import_loc_id = context.insts().GetLocId(import_id); - - namespace_inst = SemIR::Namespace{namespace_type_id, - SemIR::NameScopeId::Invalid, import_id}; - auto namespace_inst_and_loc = - import_loc_id.is_import_ir_inst_id() - ? context.MakeImportedLocAndInst(import_loc_id.import_ir_inst_id(), - *namespace_inst) - // TODO: Check that this actually is an `AnyNamespaceId`. - : SemIR::LocIdAndInst( - Parse::AnyNamespaceId(import_loc_id.node_id()), - *namespace_inst); - return context.AddPlaceholderInstInNoBlock(namespace_inst_and_loc); - }; - auto* parent_scope = &context.name_scopes().Get(parent_scope_id); - auto insert_result = parent_scope->LookupOrAdd(name_id, make_namespace_id, - SemIR::AccessKind::Public); - + auto insert_result = parent_scope->LookupOrAdd( + name_id, + []() { + // This is temporary and would be overridden. + return SemIR::InstId::Invalid; + }, + SemIR::AccessKind::Public); if (!insert_result.first) { auto prev_inst_id = parent_scope->GetEntry(insert_result.second).inst_id; if (auto namespace_inst = @@ -139,14 +124,25 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, } } - auto namespace_id = insert_result.first - ? parent_scope->GetEntry(insert_result.second).inst_id - : make_namespace_id(); - + auto import_id = make_import_id(); + CARBON_CHECK(import_id.is_valid()); + auto import_loc_id = context.insts().GetLocId(import_id); + + auto namespace_inst = SemIR::Namespace{ + namespace_type_id, SemIR::NameScopeId::Invalid, import_id}; + auto namespace_inst_and_loc = + import_loc_id.is_import_ir_inst_id() + ? context.MakeImportedLocAndInst(import_loc_id.import_ir_inst_id(), + namespace_inst) + // TODO: Check that this actually is an `AnyNamespaceId`. + : SemIR::LocIdAndInst(Parse::AnyNamespaceId(import_loc_id.node_id()), + namespace_inst); + auto namespace_id = + context.AddPlaceholderInstInNoBlock(namespace_inst_and_loc); context.import_ref_ids().push_back(namespace_id); - namespace_inst->name_scope_id = + namespace_inst.name_scope_id = context.name_scopes().Add(namespace_id, name_id, parent_scope_id); - context.ReplaceInstBeforeConstantUse(namespace_id, *namespace_inst); + context.ReplaceInstBeforeConstantUse(namespace_id, namespace_inst); // Note we have to get the parent scope freshly, creating the imported // namespace may invalidate the pointer above. @@ -154,13 +150,13 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, // Diagnose if there's a name conflict, but still produce the namespace to // supersede the name conflict in order to avoid repeat diagnostics. + auto& entry = parent_scope->GetEntry(insert_result.second); if (!insert_result.first) { - auto& entry = parent_scope->GetEntry(insert_result.second); context.DiagnoseDuplicateName(namespace_id, entry.inst_id); - entry.inst_id = namespace_id; entry.access_kind = SemIR::AccessKind::Public; } - return {namespace_inst->name_scope_id, namespace_id, false}; + entry.inst_id = namespace_id; + return {namespace_inst.name_scope_id, namespace_id, false}; } // Adds a copied namespace to the cache. From ac9b3f7f30dae785ca6b9ae2f52082c119446499 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 16:40:51 +0100 Subject: [PATCH 07/19] Simplify NameScope::LookupOrAdd() by passing InstId instead of a function that generates it --- toolchain/check/import.cpp | 25 +++++++----------- toolchain/sem_ir/name_scope.cpp | 8 +++--- toolchain/sem_ir/name_scope.h | 7 +++-- toolchain/sem_ir/name_scope_test.cpp | 39 +++++++++------------------- 4 files changed, 28 insertions(+), 51 deletions(-) diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 03cd19c4ff8a2..bf695053a0e28 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -106,11 +106,8 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, auto* parent_scope = &context.name_scopes().Get(parent_scope_id); auto insert_result = parent_scope->LookupOrAdd( name_id, - []() { - // This is temporary and would be overridden. - return SemIR::InstId::Invalid; - }, - SemIR::AccessKind::Public); + // This InstId is temporary and would be overridden if used. + SemIR::InstId::Invalid, SemIR::AccessKind::Public); if (!insert_result.first) { auto prev_inst_id = parent_scope->GetEntry(insert_result.second).inst_id; if (auto namespace_inst = @@ -272,17 +269,15 @@ static auto AddImportRefOrMerge(Context& context, SemIR::ImportIRId ir_id, auto& parent_scope = context.name_scopes().Get(parent_scope_id); auto insert = parent_scope.LookupOrAdd( name_id, - [&] { - auto entity_name_id = context.entity_names().Add( - {.name_id = name_id, - .parent_scope_id = parent_scope_id, - .bind_index = SemIR::CompileTimeBindIndex::Invalid}); - return AddImportRef(context, - {.ir_id = ir_id, .inst_id = import_inst_id}, - entity_name_id); - }, - SemIR::AccessKind::Public); + // This InstId is temporary and would be overridden if used. + SemIR::InstId::Invalid, SemIR::AccessKind::Public); if (insert.first) { + auto entity_name_id = context.entity_names().Add( + {.name_id = name_id, + .parent_scope_id = parent_scope_id, + .bind_index = SemIR::CompileTimeBindIndex::Invalid}); + parent_scope.GetEntry(insert.second).inst_id = AddImportRef( + context, {.ir_id = ir_id, .inst_id = import_inst_id}, entity_name_id); return; } diff --git a/toolchain/sem_ir/name_scope.cpp b/toolchain/sem_ir/name_scope.cpp index b6443a2d7854b..f42b3fac9f0a7 100644 --- a/toolchain/sem_ir/name_scope.cpp +++ b/toolchain/sem_ir/name_scope.cpp @@ -17,8 +17,7 @@ auto NameScope::AddRequired(Entry name_entry) -> void { name_entry.name_id); } -auto NameScope::LookupOrAdd(SemIR::NameId name_id, - llvm::function_ref make_inst_id, +auto NameScope::LookupOrAdd(SemIR::NameId name_id, InstId inst_id, AccessKind access_kind) -> std::pair { auto insert_result = name_map_.Insert(name_id, names_.size()); @@ -26,9 +25,8 @@ auto NameScope::LookupOrAdd(SemIR::NameId name_id, return {false, EntryId(insert_result.value())}; } - names_.push_back({.name_id = name_id, - .inst_id = make_inst_id(), - .access_kind = access_kind}); + names_.push_back( + {.name_id = name_id, .inst_id = inst_id, .access_kind = access_kind}); return {true, EntryId(names_.size() - 1)}; } diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index d51f898841ed4..e9667be3fbedb 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -84,10 +84,9 @@ class NameScope : public Printable { auto AddRequired(Entry name_entry) -> void; // If the given name already exists, return true and an EntryId. - // If not, adds the name using the result of make_inst_id() and access_kind - // and return false and an EntryId. - auto LookupOrAdd(SemIR::NameId name_id, - llvm::function_ref make_inst_id, + // If not, adds the name using inst_id and access_kind and returns false and + // an EntryId. + auto LookupOrAdd(SemIR::NameId name_id, InstId inst_id, AccessKind access_kind) -> std::pair; // Instructions returning values that are extended by this scope. diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index 775a1ffa07c07..62743972204e8 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -90,8 +90,8 @@ TEST(NameScope, LookupOrAdd) { NameScope::Entry entry1{.name_id = NameId(id++), .inst_id = InstId(id++), .access_kind = AccessKind::Public}; - auto lookup = name_scope.LookupOrAdd( - entry1.name_id, [&]() { return entry1.inst_id; }, entry1.access_kind); + auto lookup = name_scope.LookupOrAdd(entry1.name_id, entry1.inst_id, + entry1.access_kind); EXPECT_TRUE(lookup.first); EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), StructReflection::AsTuple(entry1)); @@ -99,8 +99,8 @@ TEST(NameScope, LookupOrAdd) { NameScope::Entry entry2{.name_id = NameId(id++), .inst_id = InstId(id++), .access_kind = AccessKind::Protected}; - lookup = name_scope.LookupOrAdd( - entry2.name_id, [&]() { return entry2.inst_id; }, entry2.access_kind); + lookup = name_scope.LookupOrAdd(entry2.name_id, entry2.inst_id, + entry2.access_kind); EXPECT_TRUE(lookup.first); EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), StructReflection::AsTuple(entry2)); @@ -108,41 +108,26 @@ TEST(NameScope, LookupOrAdd) { NameScope::Entry entry3{.name_id = NameId(id++), .inst_id = InstId(id++), .access_kind = AccessKind::Private}; - lookup = name_scope.LookupOrAdd( - entry3.name_id, [&]() { return entry3.inst_id; }, entry3.access_kind); + lookup = name_scope.LookupOrAdd(entry3.name_id, entry3.inst_id, + entry3.access_kind); EXPECT_TRUE(lookup.first); EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), StructReflection::AsTuple(entry3)); - lookup = name_scope.LookupOrAdd( - entry1.name_id, - [&]() { - ADD_FAILURE() << "Unexpected call"; - return entry1.inst_id; - }, - entry1.access_kind); + lookup = name_scope.LookupOrAdd(entry1.name_id, entry1.inst_id, + entry1.access_kind); EXPECT_FALSE(lookup.first); EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), StructReflection::AsTuple(entry1)); - lookup = name_scope.LookupOrAdd( - entry2.name_id, - [&]() { - ADD_FAILURE() << "Unexpected call"; - return entry2.inst_id; - }, - entry2.access_kind); + lookup = name_scope.LookupOrAdd(entry2.name_id, entry2.inst_id, + entry2.access_kind); EXPECT_FALSE(lookup.first); EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), StructReflection::AsTuple(entry2)); - lookup = name_scope.LookupOrAdd( - entry3.name_id, - [&]() { - ADD_FAILURE() << "Unexpected call"; - return entry3.inst_id; - }, - entry3.access_kind); + lookup = name_scope.LookupOrAdd(entry3.name_id, entry3.inst_id, + entry3.access_kind); EXPECT_FALSE(lookup.first); EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), StructReflection::AsTuple(entry3)); From a9c5543ea2b45fe71267668839395096195af9f0 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 16:50:11 +0100 Subject: [PATCH 08/19] Use structured binding for the return value of NameScope::LookupOrAdd() --- toolchain/check/import.cpp | 18 +++---- toolchain/sem_ir/name_scope_test.cpp | 78 ++++++++++++++++------------ 2 files changed, 54 insertions(+), 42 deletions(-) diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index bf695053a0e28..be6a7eb2eb9ee 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -104,12 +104,12 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, llvm::function_ref make_import_id) -> NamespaceResult { auto* parent_scope = &context.name_scopes().Get(parent_scope_id); - auto insert_result = parent_scope->LookupOrAdd( + auto [inserted, entry_id] = parent_scope->LookupOrAdd( name_id, // This InstId is temporary and would be overridden if used. SemIR::InstId::Invalid, SemIR::AccessKind::Public); - if (!insert_result.first) { - auto prev_inst_id = parent_scope->GetEntry(insert_result.second).inst_id; + if (!inserted) { + auto prev_inst_id = parent_scope->GetEntry(entry_id).inst_id; if (auto namespace_inst = context.insts().TryGetAs(prev_inst_id)) { if (diagnose_duplicate_namespace) { @@ -147,8 +147,8 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, // Diagnose if there's a name conflict, but still produce the namespace to // supersede the name conflict in order to avoid repeat diagnostics. - auto& entry = parent_scope->GetEntry(insert_result.second); - if (!insert_result.first) { + auto& entry = parent_scope->GetEntry(entry_id); + if (!inserted) { context.DiagnoseDuplicateName(namespace_id, entry.inst_id); entry.access_kind = SemIR::AccessKind::Public; } @@ -267,21 +267,21 @@ static auto AddImportRefOrMerge(Context& context, SemIR::ImportIRId ir_id, SemIR::NameId name_id) -> void { // Leave a placeholder that the inst comes from the other IR. auto& parent_scope = context.name_scopes().Get(parent_scope_id); - auto insert = parent_scope.LookupOrAdd( + auto [inserted, entry_id] = parent_scope.LookupOrAdd( name_id, // This InstId is temporary and would be overridden if used. SemIR::InstId::Invalid, SemIR::AccessKind::Public); - if (insert.first) { + if (inserted) { auto entity_name_id = context.entity_names().Add( {.name_id = name_id, .parent_scope_id = parent_scope_id, .bind_index = SemIR::CompileTimeBindIndex::Invalid}); - parent_scope.GetEntry(insert.second).inst_id = AddImportRef( + parent_scope.GetEntry(entry_id).inst_id = AddImportRef( context, {.ir_id = ir_id, .inst_id = import_inst_id}, entity_name_id); return; } - auto inst_id = parent_scope.GetEntry(insert.second).inst_id; + auto inst_id = parent_scope.GetEntry(entry_id).inst_id; auto prev_ir_inst = GetCanonicalImportIRInst(context, &context.sem_ir(), inst_id); VerifySameCanonicalImportIRInst(context, inst_id, prev_ir_inst, ir_id, diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index 62743972204e8..27cd40ea96620 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -90,47 +90,59 @@ TEST(NameScope, LookupOrAdd) { NameScope::Entry entry1{.name_id = NameId(id++), .inst_id = InstId(id++), .access_kind = AccessKind::Public}; - auto lookup = name_scope.LookupOrAdd(entry1.name_id, entry1.inst_id, - entry1.access_kind); - EXPECT_TRUE(lookup.first); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), - StructReflection::AsTuple(entry1)); + { + auto [added, entry_id] = name_scope.LookupOrAdd( + entry1.name_id, entry1.inst_id, entry1.access_kind); + EXPECT_TRUE(added); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), + StructReflection::AsTuple(entry1)); + } NameScope::Entry entry2{.name_id = NameId(id++), .inst_id = InstId(id++), .access_kind = AccessKind::Protected}; - lookup = name_scope.LookupOrAdd(entry2.name_id, entry2.inst_id, - entry2.access_kind); - EXPECT_TRUE(lookup.first); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), - StructReflection::AsTuple(entry2)); + { + auto [added, entry_id] = name_scope.LookupOrAdd( + entry2.name_id, entry2.inst_id, entry2.access_kind); + EXPECT_TRUE(added); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), + StructReflection::AsTuple(entry2)); + } NameScope::Entry entry3{.name_id = NameId(id++), .inst_id = InstId(id++), .access_kind = AccessKind::Private}; - lookup = name_scope.LookupOrAdd(entry3.name_id, entry3.inst_id, - entry3.access_kind); - EXPECT_TRUE(lookup.first); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), - StructReflection::AsTuple(entry3)); - - lookup = name_scope.LookupOrAdd(entry1.name_id, entry1.inst_id, - entry1.access_kind); - EXPECT_FALSE(lookup.first); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), - StructReflection::AsTuple(entry1)); - - lookup = name_scope.LookupOrAdd(entry2.name_id, entry2.inst_id, - entry2.access_kind); - EXPECT_FALSE(lookup.first); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), - StructReflection::AsTuple(entry2)); - - lookup = name_scope.LookupOrAdd(entry3.name_id, entry3.inst_id, - entry3.access_kind); - EXPECT_FALSE(lookup.first); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(lookup.second)), - StructReflection::AsTuple(entry3)); + { + auto [added, entry_id] = name_scope.LookupOrAdd( + entry3.name_id, entry3.inst_id, entry3.access_kind); + EXPECT_TRUE(added); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), + StructReflection::AsTuple(entry3)); + } + + { + auto [added, entry_id] = name_scope.LookupOrAdd( + entry1.name_id, entry1.inst_id, entry1.access_kind); + EXPECT_FALSE(added); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), + StructReflection::AsTuple(entry1)); + } + + { + auto [added, entry_id] = name_scope.LookupOrAdd( + entry2.name_id, entry2.inst_id, entry2.access_kind); + EXPECT_FALSE(added); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), + StructReflection::AsTuple(entry2)); + } + + { + auto [added, entry_id] = name_scope.LookupOrAdd( + entry3.name_id, entry3.inst_id, entry3.access_kind); + EXPECT_FALSE(added); + EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), + StructReflection::AsTuple(entry3)); + } } TEST(NameScope, ExtendedScopes) { From f62a938df18b04bbf1013c8fb808ef21321b7a8c Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 4 Dec 2024 16:58:57 +0100 Subject: [PATCH 09/19] Deduplicate calling NameScope::GetEntry() in AddImportRefOrMerge() --- toolchain/check/import.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index be6a7eb2eb9ee..a13f314a625c6 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -271,17 +271,18 @@ static auto AddImportRefOrMerge(Context& context, SemIR::ImportIRId ir_id, name_id, // This InstId is temporary and would be overridden if used. SemIR::InstId::Invalid, SemIR::AccessKind::Public); + auto& entry = parent_scope.GetEntry(entry_id); if (inserted) { auto entity_name_id = context.entity_names().Add( {.name_id = name_id, .parent_scope_id = parent_scope_id, .bind_index = SemIR::CompileTimeBindIndex::Invalid}); - parent_scope.GetEntry(entry_id).inst_id = AddImportRef( + entry.inst_id = AddImportRef( context, {.ir_id = ir_id, .inst_id = import_inst_id}, entity_name_id); return; } - auto inst_id = parent_scope.GetEntry(entry_id).inst_id; + auto inst_id = entry.inst_id; auto prev_ir_inst = GetCanonicalImportIRInst(context, &context.sem_ir(), inst_id); VerifySameCanonicalImportIRInst(context, inst_id, prev_ir_inst, ir_id, From 35592db86715d11805760bf685f09c4b7bcd64e5 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 08:57:16 +0100 Subject: [PATCH 10/19] Use data members directly in NameScope::Print() --- toolchain/sem_ir/name_scope.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index e9667be3fbedb..bad992cf0c379 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -37,19 +37,19 @@ class NameScope : public Printable { parent_scope_id_(parent_scope_id) {} auto Print(llvm::raw_ostream& out) const -> void { - out << "{inst: " << inst_id() << ", parent_scope: " << parent_scope_id() + out << "{inst: " << inst_id_ << ", parent_scope: " << parent_scope_id_ << ", has_error: " << (has_error_ ? "true" : "false"); out << ", extended_scopes: ["; llvm::ListSeparator scope_sep; - for (auto id : extended_scopes()) { + for (auto id : extended_scopes_) { out << scope_sep << id; } out << "]"; out << ", names: {"; llvm::ListSeparator sep; - for (auto entry : entries()) { + for (auto entry : names_) { out << sep << entry.name_id << ": " << entry.inst_id; } out << "}"; From 0fa5621f29e7a02dc447f19d966b18913bf50593 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 08:58:46 +0100 Subject: [PATCH 11/19] Use preincrmenet in name_scope_test.cpp instead of postincrement. --- toolchain/sem_ir/name_scope_test.cpp | 104 +++++++++++++-------------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index 27cd40ea96620..58bf1d6b4f117 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -14,11 +14,11 @@ using testing::ElementsAre; using testing::Pair; TEST(NameScope, Empty) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); - NameScopeId parent_scope_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); + NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); EXPECT_THAT(name_scope.entries(), ElementsAre()); @@ -33,25 +33,25 @@ TEST(NameScope, Empty) { } TEST(NameScope, Lookup) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); - NameScopeId parent_scope_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); + NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); - NameScope::Entry entry1{.name_id = NameId(id++), - .inst_id = InstId(id++), + NameScope::Entry entry1{.name_id = NameId(++id), + .inst_id = InstId(++id), .access_kind = AccessKind::Public}; name_scope.AddRequired(entry1); - NameScope::Entry entry2{.name_id = NameId(id++), - .inst_id = InstId(id++), + NameScope::Entry entry2{.name_id = NameId(++id), + .inst_id = InstId(++id), .access_kind = AccessKind::Protected}; name_scope.AddRequired(entry2); - NameScope::Entry entry3{.name_id = NameId(id++), - .inst_id = InstId(id++), + NameScope::Entry entry3{.name_id = NameId(++id), + .inst_id = InstId(++id), .access_kind = AccessKind::Private}; name_scope.AddRequired(entry3); @@ -74,21 +74,21 @@ TEST(NameScope, Lookup) { EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(*lookup)), StructReflection::AsTuple(entry3)); - NameId unknown_name_id(id++); + NameId unknown_name_id(++id); lookup = name_scope.Lookup(unknown_name_id); EXPECT_EQ(lookup, std::nullopt); } TEST(NameScope, LookupOrAdd) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); - NameScopeId parent_scope_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); + NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); - NameScope::Entry entry1{.name_id = NameId(id++), - .inst_id = InstId(id++), + NameScope::Entry entry1{.name_id = NameId(++id), + .inst_id = InstId(++id), .access_kind = AccessKind::Public}; { auto [added, entry_id] = name_scope.LookupOrAdd( @@ -98,8 +98,8 @@ TEST(NameScope, LookupOrAdd) { StructReflection::AsTuple(entry1)); } - NameScope::Entry entry2{.name_id = NameId(id++), - .inst_id = InstId(id++), + NameScope::Entry entry2{.name_id = NameId(++id), + .inst_id = InstId(++id), .access_kind = AccessKind::Protected}; { auto [added, entry_id] = name_scope.LookupOrAdd( @@ -109,8 +109,8 @@ TEST(NameScope, LookupOrAdd) { StructReflection::AsTuple(entry2)); } - NameScope::Entry entry3{.name_id = NameId(id++), - .inst_id = InstId(id++), + NameScope::Entry entry3{.name_id = NameId(++id), + .inst_id = InstId(++id), .access_kind = AccessKind::Private}; { auto [added, entry_id] = name_scope.LookupOrAdd( @@ -146,31 +146,31 @@ TEST(NameScope, LookupOrAdd) { } TEST(NameScope, ExtendedScopes) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); NameScopeId parent_scope_id = NameScopeId::Package; NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); EXPECT_THAT(name_scope.extended_scopes(), ElementsAre()); - InstId extended_scope1(id++); + InstId extended_scope1(++id); name_scope.AddExtendedScope(extended_scope1); EXPECT_THAT(name_scope.extended_scopes(), ElementsAre(extended_scope1)); - InstId extended_scope2(id++); + InstId extended_scope2(++id); name_scope.AddExtendedScope(extended_scope2); EXPECT_THAT(name_scope.extended_scopes(), ElementsAre(extended_scope1, extended_scope2)); } TEST(NameScope, HasError) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); - NameScopeId parent_scope_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); + NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); EXPECT_FALSE(name_scope.has_error()); @@ -183,11 +183,11 @@ TEST(NameScope, HasError) { } TEST(NameScope, IsClosedImport) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); - NameScopeId parent_scope_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); + NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); EXPECT_FALSE(name_scope.is_closed_import()); @@ -200,11 +200,11 @@ TEST(NameScope, IsClosedImport) { } TEST(NameScope, IsImportedPackageParentNonPackageScope) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); - NameScopeId parent_scope_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); + NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); EXPECT_FALSE(name_scope.is_imported_package()); @@ -217,10 +217,10 @@ TEST(NameScope, IsImportedPackageParentNonPackageScope) { } TEST(NameScope, IsImportedPackageParentPackageScope) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); NameScopeId parent_scope_id = NameScopeId::Package; NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); @@ -234,23 +234,23 @@ TEST(NameScope, IsImportedPackageParentPackageScope) { } TEST(NameScope, ImportIRScopes) { - int id = 1; + int id = 0; - InstId scope_inst_id(id++); - NameId scope_name_id(id++); + InstId scope_inst_id(++id); + NameId scope_name_id(++id); NameScopeId parent_scope_id = NameScopeId::Package; NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); EXPECT_THAT(name_scope.import_ir_scopes(), ElementsAre()); - ImportIRId import_ir_id1(id++); - NameScopeId import_name_scope_id1(id++); + ImportIRId import_ir_id1(++id); + NameScopeId import_name_scope_id1(++id); name_scope.AddImportIRScope({import_ir_id1, import_name_scope_id1}); EXPECT_THAT(name_scope.import_ir_scopes(), ElementsAre(Pair(import_ir_id1, import_name_scope_id1))); - ImportIRId import_ir_id2(id++); - NameScopeId import_name_scope_id2(id++); + ImportIRId import_ir_id2(++id); + NameScopeId import_name_scope_id2(++id); name_scope.AddImportIRScope({import_ir_id2, import_name_scope_id2}); EXPECT_THAT(name_scope.import_ir_scopes(), ElementsAre(Pair(import_ir_id1, import_name_scope_id1), From f9e9509fcea83886f78817b890e3f074f55f7b55 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:00:12 +0100 Subject: [PATCH 12/19] Use full qualification for using statements in name_scope_test.cpp --- toolchain/sem_ir/name_scope_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index 58bf1d6b4f117..f2fd952432d6b 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -10,8 +10,8 @@ namespace Carbon::SemIR { namespace { -using testing::ElementsAre; -using testing::Pair; +using ::testing::ElementsAre; +using ::testing::Pair; TEST(NameScope, Empty) { int id = 0; From 77b946630569cfb9abf5558e5e87d20dd288677b Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:18:02 +0100 Subject: [PATCH 13/19] Define and use a dedicate NameScope::Entry gmock matcher --- toolchain/sem_ir/name_scope_test.cpp | 44 ++++++++++++++-------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index f2fd952432d6b..bd5d05c70c21e 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -11,8 +11,18 @@ namespace Carbon::SemIR { namespace { using ::testing::ElementsAre; +using ::testing::Field; using ::testing::Pair; +MATCHER_P(NameScopeEntryEquals, entry, "") { + return ExplainMatchResult( + AllOf(Field("name_id", &NameScope::Entry::name_id, entry.name_id), + Field("inst_id", &NameScope::Entry::inst_id, entry.inst_id), + Field("access_kind", &NameScope::Entry::access_kind, + entry.access_kind)), + arg, result_listener); +} + TEST(NameScope, Empty) { int id = 0; @@ -57,22 +67,18 @@ TEST(NameScope, Lookup) { auto lookup = name_scope.Lookup(entry1.name_id); ASSERT_NE(lookup, std::nullopt); - EXPECT_EQ(StructReflection::AsTuple( - static_cast(name_scope).GetEntry(*lookup)), - StructReflection::AsTuple(entry1)); - EXPECT_EQ(StructReflection::AsTuple( - static_cast(name_scope).GetEntry(*lookup)), - StructReflection::AsTuple(entry1)); + EXPECT_THAT(static_cast(name_scope).GetEntry(*lookup), + NameScopeEntryEquals(entry1)); + EXPECT_THAT(static_cast(name_scope).GetEntry(*lookup), + NameScopeEntryEquals(entry1)); lookup = name_scope.Lookup(entry2.name_id); ASSERT_NE(lookup, std::nullopt); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(*lookup)), - StructReflection::AsTuple(entry2)); + EXPECT_THAT(name_scope.GetEntry(*lookup), NameScopeEntryEquals(entry2)); lookup = name_scope.Lookup(entry3.name_id); ASSERT_NE(lookup, std::nullopt); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(*lookup)), - StructReflection::AsTuple(entry3)); + EXPECT_THAT(name_scope.GetEntry(*lookup), NameScopeEntryEquals(entry3)); NameId unknown_name_id(++id); lookup = name_scope.Lookup(unknown_name_id); @@ -94,8 +100,7 @@ TEST(NameScope, LookupOrAdd) { auto [added, entry_id] = name_scope.LookupOrAdd( entry1.name_id, entry1.inst_id, entry1.access_kind); EXPECT_TRUE(added); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), - StructReflection::AsTuple(entry1)); + EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry1)); } NameScope::Entry entry2{.name_id = NameId(++id), @@ -105,8 +110,7 @@ TEST(NameScope, LookupOrAdd) { auto [added, entry_id] = name_scope.LookupOrAdd( entry2.name_id, entry2.inst_id, entry2.access_kind); EXPECT_TRUE(added); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), - StructReflection::AsTuple(entry2)); + EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry2)); } NameScope::Entry entry3{.name_id = NameId(++id), @@ -116,32 +120,28 @@ TEST(NameScope, LookupOrAdd) { auto [added, entry_id] = name_scope.LookupOrAdd( entry3.name_id, entry3.inst_id, entry3.access_kind); EXPECT_TRUE(added); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), - StructReflection::AsTuple(entry3)); + EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry3)); } { auto [added, entry_id] = name_scope.LookupOrAdd( entry1.name_id, entry1.inst_id, entry1.access_kind); EXPECT_FALSE(added); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), - StructReflection::AsTuple(entry1)); + EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry1)); } { auto [added, entry_id] = name_scope.LookupOrAdd( entry2.name_id, entry2.inst_id, entry2.access_kind); EXPECT_FALSE(added); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), - StructReflection::AsTuple(entry2)); + EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry2)); } { auto [added, entry_id] = name_scope.LookupOrAdd( entry3.name_id, entry3.inst_id, entry3.access_kind); EXPECT_FALSE(added); - EXPECT_EQ(StructReflection::AsTuple(name_scope.GetEntry(entry_id)), - StructReflection::AsTuple(entry3)); + EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry3)); } } From ca252bd911bdc4a36012443bee9414ea28169b8e Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:25:01 +0100 Subject: [PATCH 14/19] Keep EntryId as the index instead of int --- toolchain/sem_ir/name_scope.cpp | 4 ++-- toolchain/sem_ir/name_scope.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/toolchain/sem_ir/name_scope.cpp b/toolchain/sem_ir/name_scope.cpp index f42b3fac9f0a7..23ca668e4199a 100644 --- a/toolchain/sem_ir/name_scope.cpp +++ b/toolchain/sem_ir/name_scope.cpp @@ -8,7 +8,7 @@ namespace Carbon::SemIR { auto NameScope::AddRequired(Entry name_entry) -> void { auto add_name = [&] { - int index = names_.size(); + EntryId index(names_.size()); names_.push_back(name_entry); return index; }; @@ -20,7 +20,7 @@ auto NameScope::AddRequired(Entry name_entry) -> void { auto NameScope::LookupOrAdd(SemIR::NameId name_id, InstId inst_id, AccessKind access_kind) -> std::pair { - auto insert_result = name_map_.Insert(name_id, names_.size()); + auto insert_result = name_map_.Insert(name_id, EntryId(names_.size())); if (!insert_result.is_inserted()) { return {false, EntryId(insert_result.value())}; } diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index bad992cf0c379..1db9911cd3714 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -77,7 +77,7 @@ class NameScope : public Printable { if (!lookup) { return std::nullopt; } - return EntryId(lookup.value()); + return lookup.value(); } // Adds a new name known to not exist. @@ -154,7 +154,7 @@ class NameScope : public Printable { // intensive, we can also switch the lookup to a set of indices into the // vector rather than a map from `NameId` to index. llvm::SmallVector names_; - Map name_map_; + Map name_map_; // Small vector size is set to 1: we expect that there will rarely be more // than a single extended scope. From 6fd0dd1c0ec0a2401ce54124260e5259e8eec13d Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:28:33 +0100 Subject: [PATCH 15/19] Move NameScope::Print() implementation to cpp file --- toolchain/sem_ir/name_scope.cpp | 21 +++++++++++++++++++++ toolchain/sem_ir/name_scope.h | 21 +-------------------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/toolchain/sem_ir/name_scope.cpp b/toolchain/sem_ir/name_scope.cpp index 23ca668e4199a..d87e9d2e96626 100644 --- a/toolchain/sem_ir/name_scope.cpp +++ b/toolchain/sem_ir/name_scope.cpp @@ -6,6 +6,27 @@ namespace Carbon::SemIR { +auto NameScope::Print(llvm::raw_ostream& out) const -> void { + out << "{inst: " << inst_id_ << ", parent_scope: " << parent_scope_id_ + << ", has_error: " << (has_error_ ? "true" : "false"); + + out << ", extended_scopes: ["; + llvm::ListSeparator scope_sep; + for (auto id : extended_scopes_) { + out << scope_sep << id; + } + out << "]"; + + out << ", names: {"; + llvm::ListSeparator sep; + for (auto entry : names_) { + out << sep << entry.name_id << ": " << entry.inst_id; + } + out << "}"; + + out << "}"; +} + auto NameScope::AddRequired(Entry name_entry) -> void { auto add_name = [&] { EntryId index(names_.size()); diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index 1db9911cd3714..3f13616d0d1da 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -36,26 +36,7 @@ class NameScope : public Printable { name_id_(name_id), parent_scope_id_(parent_scope_id) {} - auto Print(llvm::raw_ostream& out) const -> void { - out << "{inst: " << inst_id_ << ", parent_scope: " << parent_scope_id_ - << ", has_error: " << (has_error_ ? "true" : "false"); - - out << ", extended_scopes: ["; - llvm::ListSeparator scope_sep; - for (auto id : extended_scopes_) { - out << scope_sep << id; - } - out << "]"; - - out << ", names: {"; - llvm::ListSeparator sep; - for (auto entry : names_) { - out << sep << entry.name_id << ": " << entry.inst_id; - } - out << "}"; - - out << "}"; - } + auto Print(llvm::raw_ostream& out) const -> void; // Names in the scope. // Entries could become invalidated if the scope object is invalidated or if a From 2a1b87289aff723eacb4dfd4f754bf141fecdf3c Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:43:01 +0100 Subject: [PATCH 16/19] Move NameScope comments from trivial accessors to data members --- toolchain/sem_ir/name_scope.h | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index 3f13616d0d1da..34373a4e245c9 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -38,9 +38,6 @@ class NameScope : public Printable { auto Print(llvm::raw_ostream& out) const -> void; - // Names in the scope. - // Entries could become invalidated if the scope object is invalidated or if a - // name is added. auto entries() const -> llvm::ArrayRef { return names_; } // Get a specific Name entry based on an EntryId that return from a lookup. @@ -70,7 +67,6 @@ class NameScope : public Printable { auto LookupOrAdd(SemIR::NameId name_id, InstId inst_id, AccessKind access_kind) -> std::pair; - // Instructions returning values that are extended by this scope. auto extended_scopes() const -> llvm::ArrayRef { return extended_scopes_; } @@ -79,27 +75,18 @@ class NameScope : public Printable { extended_scopes_.push_back(extended_scope); } - // The instruction which owns the scope. auto inst_id() const -> InstId { return inst_id_; } - // When the scope is a namespace, the name. Otherwise, invalid. auto name_id() const -> NameId { return name_id_; } - // The parent scope. auto parent_scope_id() const -> NameScopeId { return parent_scope_id_; } - // Whether we have diagnosed an error in a construct that would have added - // names to this scope. For example, this can happen if an `import` failed or - // an `extend` declaration was ill-formed. If true, names are assumed to be - // missing as a result of the error, and no further errors are produced for - // lookup failures in this scope. auto has_error() const -> bool { return has_error_; } // Mark that we have diagnosed an error in a construct that would have added // names to this scope. auto set_has_error() -> void { has_error_ = true; } - // True if this is a closed namespace created by importing a package. auto is_closed_import() const -> bool { return is_closed_import_; } auto set_is_closed_import(bool is_closed_import) -> void { @@ -111,8 +98,6 @@ class NameScope : public Printable { return is_closed_import() && parent_scope_id() == NameScopeId::Package; } - // Imported IR scopes that compose this namespace. This will be empty for - // scopes that correspond to the current package. auto import_ir_scopes() const -> llvm::ArrayRef> { return import_ir_scopes_; @@ -125,6 +110,10 @@ class NameScope : public Printable { } private: + // Names in the scope. + // Entries could become invalidated if the scope object is invalidated or if a + // name is added. + // // We store both an insertion-ordered vector for iterating // and a map from `NameId` to the index of that vector for name lookup. // @@ -137,20 +126,35 @@ class NameScope : public Printable { llvm::SmallVector names_; Map name_map_; + // Instructions returning values that are extended by this scope. + // // Small vector size is set to 1: we expect that there will rarely be more // than a single extended scope. // TODO: Revisit this once we have more kinds of extended scope and data. // TODO: Consider using something like `TinyPtrVector` for this. llvm::SmallVector extended_scopes_; + // The instruction which owns the scope. InstId inst_id_; + + // When the scope is a namespace, the name. Otherwise, invalid. NameId name_id_; + + // The parent scope. NameScopeId parent_scope_id_; + // Whether we have diagnosed an error in a construct that would have added + // names to this scope. For example, this can happen if an `import` failed or + // an `extend` declaration was ill-formed. If true, names are assumed to be + // missing as a result of the error, and no further errors are produced for + // lookup failures in this scope. bool has_error_ = false; + // True if this is a closed namespace created by importing a package. bool is_closed_import_ = false; + // Imported IR scopes that compose this namespace. This will be empty for + // scopes that correspond to the current package. llvm::SmallVector, 0> import_ir_scopes_; }; From 98a2c5ef76065cfe86f375d7b6096a8b8fb79625 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:49:45 +0100 Subject: [PATCH 17/19] Bring back the logic to call TryToDefineType() regardless of whether we encounter errors before --- toolchain/check/handle_impl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 815570327f1a6..dd3e17586d699 100644 --- a/toolchain/check/handle_impl.cpp +++ b/toolchain/check/handle_impl.cpp @@ -179,7 +179,7 @@ static auto ExtendImpl(Context& context, Parse::NodeId extend_node, parent_scope.set_has_error(); return; } - if (!parent_scope.has_error() && !context.TryToDefineType(constraint_id, [&] { + if (!context.TryToDefineType(constraint_id, [&] { CARBON_DIAGNOSTIC( ExtendUndefinedInterface, Error, "`extend impl` requires a definition for facet type {0}", From cea32c497da0ebaa7a3cbe0919f4e0a0389a1e2b Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Thu, 5 Dec 2024 09:54:25 +0100 Subject: [PATCH 18/19] Use assignment operator for initialization --- toolchain/sem_ir/name_scope_test.cpp | 36 ++++++++++++++-------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index bd5d05c70c21e..c34aa1d8f010e 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -50,19 +50,19 @@ TEST(NameScope, Lookup) { NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); - NameScope::Entry entry1{.name_id = NameId(++id), - .inst_id = InstId(++id), - .access_kind = AccessKind::Public}; + NameScope::Entry entry1 = {.name_id = NameId(++id), + .inst_id = InstId(++id), + .access_kind = AccessKind::Public}; name_scope.AddRequired(entry1); - NameScope::Entry entry2{.name_id = NameId(++id), - .inst_id = InstId(++id), - .access_kind = AccessKind::Protected}; + NameScope::Entry entry2 = {.name_id = NameId(++id), + .inst_id = InstId(++id), + .access_kind = AccessKind::Protected}; name_scope.AddRequired(entry2); - NameScope::Entry entry3{.name_id = NameId(++id), - .inst_id = InstId(++id), - .access_kind = AccessKind::Private}; + NameScope::Entry entry3 = {.name_id = NameId(++id), + .inst_id = InstId(++id), + .access_kind = AccessKind::Private}; name_scope.AddRequired(entry3); auto lookup = name_scope.Lookup(entry1.name_id); @@ -93,9 +93,9 @@ TEST(NameScope, LookupOrAdd) { NameScopeId parent_scope_id(++id); NameScope name_scope(scope_inst_id, scope_name_id, parent_scope_id); - NameScope::Entry entry1{.name_id = NameId(++id), - .inst_id = InstId(++id), - .access_kind = AccessKind::Public}; + NameScope::Entry entry1 = {.name_id = NameId(++id), + .inst_id = InstId(++id), + .access_kind = AccessKind::Public}; { auto [added, entry_id] = name_scope.LookupOrAdd( entry1.name_id, entry1.inst_id, entry1.access_kind); @@ -103,9 +103,9 @@ TEST(NameScope, LookupOrAdd) { EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry1)); } - NameScope::Entry entry2{.name_id = NameId(++id), - .inst_id = InstId(++id), - .access_kind = AccessKind::Protected}; + NameScope::Entry entry2 = {.name_id = NameId(++id), + .inst_id = InstId(++id), + .access_kind = AccessKind::Protected}; { auto [added, entry_id] = name_scope.LookupOrAdd( entry2.name_id, entry2.inst_id, entry2.access_kind); @@ -113,9 +113,9 @@ TEST(NameScope, LookupOrAdd) { EXPECT_THAT(name_scope.GetEntry(entry_id), NameScopeEntryEquals(entry2)); } - NameScope::Entry entry3{.name_id = NameId(++id), - .inst_id = InstId(++id), - .access_kind = AccessKind::Private}; + NameScope::Entry entry3 = {.name_id = NameId(++id), + .inst_id = InstId(++id), + .access_kind = AccessKind::Private}; { auto [added, entry_id] = name_scope.LookupOrAdd( entry3.name_id, entry3.inst_id, entry3.access_kind); From 7e61dfe4a33205618fe43f7c12de4314dfce8cbf Mon Sep 17 00:00:00 2001 From: jonmeow Date: Fri, 6 Dec 2024 07:30:34 -0800 Subject: [PATCH 19/19] Update IdBase use --- toolchain/sem_ir/name_scope.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index 34373a4e245c9..e7b38278220bb 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -26,7 +26,8 @@ class NameScope : public Printable { AccessKind access_kind; }; - struct EntryId : public IdBase { + struct EntryId : public IdBase { + static constexpr llvm::StringLiteral Label = "name_scope_entry"; using IdBase::IdBase; };