From 74395ce6931b812a9c13bc0d1a97a97680362497 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 8 Jan 2025 09:36:14 +0100 Subject: [PATCH] Change name poisoning implementation to allow better diagnostics (#4764) Change the implementation to use an explicit `is_poisoned` bit instead of `InstId::PoisonedName` value. Zero behavior change. This would allow to more easily change the API to support accessing the poisoning declaration so we can have better name poisoning diagnosis. #4622 --- toolchain/check/context.cpp | 36 ++++++++++++++------------ toolchain/check/context.h | 29 +++++++++++++++++---- toolchain/check/decl_name_stack.cpp | 38 +++++++++++++--------------- toolchain/check/decl_name_stack.h | 9 ++++--- toolchain/check/handle_class.cpp | 10 ++++---- toolchain/check/handle_function.cpp | 16 ++++++------ toolchain/check/handle_interface.cpp | 16 +++++------- toolchain/check/handle_namespace.cpp | 15 ++++++----- toolchain/check/impl.cpp | 2 +- toolchain/check/import.cpp | 7 ++--- toolchain/check/import_ref.cpp | 5 +--- toolchain/check/member_access.cpp | 3 ++- toolchain/sem_ir/formatter.cpp | 4 +-- toolchain/sem_ir/ids.cpp | 2 -- toolchain/sem_ir/ids.h | 7 ----- toolchain/sem_ir/name_scope.cpp | 11 ++++---- toolchain/sem_ir/name_scope.h | 2 ++ toolchain/sem_ir/name_scope_test.cpp | 26 +++++++++++-------- 18 files changed, 127 insertions(+), 111 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 57e21c79f869..331b9b55d172 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -313,7 +313,8 @@ auto Context::AddNameToLookup(SemIR::NameId name_id, SemIR::InstId target_id) } auto Context::LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id, - SemIR::NameScopeId scope_id) -> SemIR::InstId { + SemIR::NameScopeId scope_id) + -> std::pair { if (!scope_id.is_valid()) { // Look for a name in the current scope only. There are two cases where the // name would be in an outer scope: @@ -338,7 +339,7 @@ auto Context::LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id, // In this case, we're not in the correct scope to define a member of // class A, so we should reject, and we achieve this by not finding the // name A from the outer scope. - return scope_stack().LookupInCurrentScope(name_id); + return {scope_stack().LookupInCurrentScope(name_id), false}; } else { // We do not look into `extend`ed scopes here. A qualified name in a // declaration must specify the exact scope in which the name was originally @@ -349,9 +350,9 @@ auto Context::LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id, // // // Error, no `F` in `B`. // fn B.F() {} - return LookupNameInExactScope(loc_id, name_id, scope_id, - name_scopes().Get(scope_id)) - .first; + auto result = LookupNameInExactScope(loc_id, name_id, scope_id, + name_scopes().Get(scope_id)); + return {result.inst_id, result.is_poisoned}; } } @@ -375,7 +376,7 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id, LookupScope{.name_scope_id = lookup_scope_id, .specific_id = specific_id}, /*required=*/false); - !non_lexical_result.inst_id.is_poisoned()) { + !non_lexical_result.is_poisoned) { if (non_lexical_result.inst_id.is_valid()) { // Poison the scopes for this name. for (const auto [scope_id, specific_id] : scopes_to_poison) { @@ -409,11 +410,13 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id, auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id, SemIR::NameScopeId scope_id, const SemIR::NameScope& scope) - -> std::pair { + -> LookupNameInExactScopeResult { 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 (!entry.is_poisoned) { + LoadImportRef(*this, entry.inst_id); + } + return {entry.inst_id, entry.access_kind, entry.is_poisoned}; } if (!scope.import_ir_scopes().empty()) { @@ -579,7 +582,7 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id, const auto& name_scope = name_scopes().Get(scope_id); has_error |= name_scope.has_error(); - auto [scope_result_id, access_kind] = + auto [scope_result_id, access_kind, is_poisoned] = LookupNameInExactScope(loc_id, name_id, scope_id, name_scope); auto is_access_prohibited = @@ -595,7 +598,7 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id, }); } - if (!scope_result_id.is_valid() || is_access_prohibited) { + if (!is_poisoned && (!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(); @@ -637,10 +640,10 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id, result.inst_id = scope_result_id; result.specific_id = specific_id; + result.is_poisoned = is_poisoned; } - if (required && - (!result.inst_id.is_valid() || result.inst_id.is_poisoned())) { + if (required && (!result.inst_id.is_valid() || result.is_poisoned)) { if (!has_error) { if (prohibited_accesses.empty()) { DiagnoseMemberNameNotFound(loc_id, name_id, lookup_scopes); @@ -659,7 +662,8 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id, } return {.specific_id = SemIR::SpecificId::Invalid, - .inst_id = SemIR::ErrorInst::SingletonInstId}; + .inst_id = SemIR::ErrorInst::SingletonInstId, + .is_poisoned = result.is_poisoned}; } return result; @@ -679,7 +683,7 @@ static auto GetCorePackage(Context& context, SemIRLoc loc, llvm::StringRef name) auto core_name_id = SemIR::NameId::ForIdentifier(core_ident_id); // Look up `package.Core`. - auto [core_inst_id, _] = context.LookupNameInExactScope( + auto [core_inst_id, _, is_poisoned] = context.LookupNameInExactScope( loc, core_name_id, SemIR::NameScopeId::Package, context.name_scopes().Get(SemIR::NameScopeId::Package)); if (core_inst_id.is_valid()) { @@ -707,7 +711,7 @@ auto Context::LookupNameInCore(SemIRLoc loc, llvm::StringRef name) } auto name_id = SemIR::NameId::ForIdentifier(identifiers().Add(name)); - auto [inst_id, _] = LookupNameInExactScope( + auto [inst_id, _, is_poisoned] = LookupNameInExactScope( loc, name_id, core_package_id, name_scopes().Get(core_package_id)); if (!inst_id.is_valid()) { CARBON_DIAGNOSTIC( diff --git a/toolchain/check/context.h b/toolchain/check/context.h index 80105e309a3f..0412b1a5c53b 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -44,7 +44,11 @@ struct LookupResult { // was not found in a specific. SemIR::SpecificId specific_id; // The declaration that was found by name lookup. + // Invalid for poisoned items. + // TODO: Make this point to the poisoning declaration. SemIR::InstId inst_id; + // Whether the lookup found a poisoned name. + bool is_poisoned = false; }; // Information about an access. @@ -68,6 +72,17 @@ class Context { using BuildDiagnosticFn = llvm::function_refContext::DiagnosticBuilder>; + struct LookupNameInExactScopeResult { + // The matching entity if found, or invalid if poisoned or not found. + SemIR::InstId inst_id; + + // The access level required to use inst_id, if it's valid. + SemIR::AccessKind access_kind; + + // Whether a poisoned entry was found. + bool is_poisoned = false; + }; + // Stores references for work. explicit Context(DiagnosticEmitter* emitter, llvm::function_ref @@ -208,10 +223,13 @@ class Context { auto AddNameToLookup(SemIR::NameId name_id, SemIR::InstId target_id) -> void; // Performs name lookup in a specified scope for a name appearing in a - // declaration, returning the referenced instruction. If scope_id is invalid, - // uses the current contextual scope. + // declaration. If scope_id is invalid, uses the current contextual scope. If + // found, returns the referenced instruction and false. If poisoned, returns + // an invalid instruction and true. + // TODO: For poisoned names, return the poisoning instruction. auto LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id, - SemIR::NameScopeId scope_id) -> SemIR::InstId; + SemIR::NameScopeId scope_id) + -> std::pair; // Performs an unqualified name lookup, returning the referenced instruction. auto LookupUnqualifiedName(Parse::NodeId node_id, SemIR::NameId name_id, @@ -219,11 +237,12 @@ class Context { // Performs a name lookup in a specified scope, returning the referenced // instruction. Does not look into extended scopes. Returns an invalid - // instruction if the name is not found. + // instruction if the name is poisoned or not found. + // TODO: Return the poisoning instruction if poisoned. auto LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id, SemIR::NameScopeId scope_id, const SemIR::NameScope& scope) - -> std::pair; + -> LookupNameInExactScopeResult; // Appends the lookup scopes corresponding to `base_const_id` to `*scopes`. // Returns `false` if not a scope. On invalid scopes, prints a diagnostic, but diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index ca0407c84fa7..6aaa5f58297a 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/toolchain/check/decl_name_stack.cpp @@ -34,7 +34,7 @@ auto DeclNameStack::NameContext::prev_inst_id() -> SemIR::InstId { return SemIR::InstId::Invalid; case NameContext::State::Poisoned: - return SemIR::InstId::PoisonedName; + CARBON_FATAL("Poisoned state should not call prev_inst_id()"); case NameContext::State::Finished: CARBON_FATAL("Finished state should only be used internally"); @@ -173,12 +173,10 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id, auto DeclNameStack::AddNameOrDiagnose(NameContext name_context, SemIR::InstId target_id, SemIR::AccessKind access_kind) -> void { - if (auto id = name_context.prev_inst_id(); id.is_valid()) { - if (id.is_poisoned()) { - context_->DiagnosePoisonedName(target_id); - } else { - context_->DiagnoseDuplicateName(target_id, id); - } + if (name_context.state == DeclNameStack::NameContext::State::Poisoned) { + context_->DiagnosePoisonedName(target_id); + } else if (auto id = name_context.prev_inst_id(); id.is_valid()) { + context_->DiagnoseDuplicateName(target_id, id); } else { AddName(name_context, target_id, access_kind); } @@ -187,12 +185,15 @@ auto DeclNameStack::AddNameOrDiagnose(NameContext name_context, auto DeclNameStack::LookupOrAddName(NameContext name_context, SemIR::InstId target_id, SemIR::AccessKind access_kind) - -> SemIR::InstId { + -> std::pair { + if (name_context.state == NameContext::State::Poisoned) { + return {SemIR::InstId::Invalid, true}; + } if (auto id = name_context.prev_inst_id(); id.is_valid()) { - return id; + return {id, false}; } AddName(name_context, target_id, access_kind); - return SemIR::InstId::Invalid; + return {SemIR::InstId::Invalid, false}; } // Push a scope corresponding to a name qualifier. For example, for @@ -260,15 +261,15 @@ auto DeclNameStack::ApplyAndLookupName(NameContext& name_context, } // For identifier nodes, we need to perform a lookup on the identifier. - auto resolved_inst_id = context_->LookupNameInDecl( + auto [resolved_inst_id, is_poisoned] = context_->LookupNameInDecl( name_context.loc_id, name_id, name_context.parent_scope_id); - if (!resolved_inst_id.is_valid()) { + if (is_poisoned) { + name_context.unresolved_name_id = name_id; + name_context.state = NameContext::State::Poisoned; + } else if (!resolved_inst_id.is_valid()) { // Invalid indicates an unresolved name. Store it and return. name_context.unresolved_name_id = name_id; name_context.state = NameContext::State::Unresolved; - } else if (resolved_inst_id.is_poisoned()) { - name_context.unresolved_name_id = name_id; - name_context.state = NameContext::State::Poisoned; } else { // Store the resolved instruction and continue for the target scope // update. @@ -286,11 +287,6 @@ static auto CheckQualifierIsResolved( CARBON_FATAL("No qualifier to resolve"); case DeclNameStack::NameContext::State::Resolved: - if (name_context.resolved_inst_id.is_poisoned()) { - context.DiagnoseNameNotFound(name_context.loc_id, - name_context.unresolved_name_id); - return false; - } return true; case DeclNameStack::NameContext::State::Poisoned: @@ -382,7 +378,7 @@ auto DeclNameStack::ResolveAsScope(const NameContext& name_context, return InvalidResult; } - if (name_context.resolved_inst_id.is_poisoned()) { + if (name_context.state == NameContext::State::Poisoned) { return InvalidResult; } diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h index d985e90fde47..ebfa984447f3 100644 --- a/toolchain/check/decl_name_stack.h +++ b/toolchain/check/decl_name_stack.h @@ -237,10 +237,13 @@ class DeclNameStack { auto AddNameOrDiagnose(NameContext name_context, SemIR::InstId target_id, SemIR::AccessKind access_kind) -> void; - // Adds a name to name lookup, or returns the existing instruction if this - // name has already been declared in this scope. + // Adds a name to name lookup if neither already declared nor poisoned in this + // scope. If declared, returns the existing instruction and false. If + // poisoned, returns an invalid instruction and true. + // TODO: Return the poisoning instruction if poisoned. auto LookupOrAddName(NameContext name_context, SemIR::InstId target_id, - SemIR::AccessKind access_kind) -> SemIR::InstId; + SemIR::AccessKind access_kind) + -> std::pair; private: // Returns a name context corresponding to an empty name. diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index 5fe5d116832f..36f0664cad70 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -107,15 +107,15 @@ static auto MergeOrAddName(Context& context, Parse::AnyClassDeclId node_id, SemIR::ClassDecl& class_decl, SemIR::Class& class_info, bool is_definition, SemIR::AccessKind access_kind) -> void { - auto prev_id = context.decl_name_stack().LookupOrAddName( + auto [prev_id, is_poisoned] = context.decl_name_stack().LookupOrAddName( name_context, class_decl_id, access_kind); - if (!prev_id.is_valid()) { + if (is_poisoned) { + // This is a declaration of a poisoned name. + context.DiagnosePoisonedName(class_decl_id); return; } - if (prev_id.is_poisoned()) { - // This is a declaration of a poisoned name. - context.DiagnosePoisonedName(class_decl_id); + if (!prev_id.is_valid()) { return; } diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 8a6707fc1f77..694455859dbb 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -123,11 +123,6 @@ static auto TryMergeRedecl(Context& context, Parse::AnyFunctionDeclId node_id, return; } - if (prev_id.is_poisoned()) { - context.DiagnosePoisonedName(function_info.latest_decl_id()); - return; - } - auto prev_function_id = SemIR::FunctionId::Invalid; auto prev_import_ir_id = SemIR::ImportIRId::Invalid; CARBON_KIND_SWITCH(context.insts().Get(prev_id)) { @@ -253,8 +248,12 @@ static auto BuildFunctionDecl(Context& context, function_info.definition_id = decl_id; } - TryMergeRedecl(context, node_id, name_context.prev_inst_id(), function_decl, - function_info, is_definition); + if (name_context.state == DeclNameStack::NameContext::State::Poisoned) { + context.DiagnosePoisonedName(function_info.latest_decl_id()); + } else { + TryMergeRedecl(context, node_id, name_context.prev_inst_id(), function_decl, + function_info, is_definition); + } // Create a new function if this isn't a valid redeclaration. if (!function_decl.function_id.is_valid()) { @@ -285,7 +284,8 @@ static auto BuildFunctionDecl(Context& context, // Check if we need to add this to name lookup, now that the function decl is // done. - if (!name_context.prev_inst_id().is_valid()) { + if (name_context.state != DeclNameStack::NameContext::State::Poisoned && + !name_context.prev_inst_id().is_valid()) { // At interface scope, a function declaration introduces an associated // function. auto lookup_result_id = decl_id; diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index 9d06b0deff3f..71967aed041e 100644 --- a/toolchain/check/handle_interface.cpp +++ b/toolchain/check/handle_interface.cpp @@ -63,16 +63,14 @@ static auto BuildInterfaceDecl(Context& context, SemIR::LibraryNameId::Invalid)}; // Check whether this is a redeclaration. - auto existing_id = context.decl_name_stack().LookupOrAddName( + auto [existing_id, is_poisoned] = context.decl_name_stack().LookupOrAddName( name_context, interface_decl_id, introducer.modifier_set.GetAccessKind()); - if (existing_id.is_valid()) { - if (existing_id.is_poisoned()) { - // This is a declaration of a poisoned name. - context.DiagnosePoisonedName(interface_decl_id); - } else if (auto existing_interface_decl = - context.insts() - .Get(existing_id) - .TryAs()) { + if (is_poisoned) { + // This is a declaration of a poisoned name. + context.DiagnosePoisonedName(interface_decl_id); + } else if (existing_id.is_valid()) { + if (auto existing_interface_decl = + context.insts().Get(existing_id).TryAs()) { auto existing_interface = context.interfaces().Get(existing_interface_decl->interface_id); if (CheckRedeclParamsMatch( diff --git a/toolchain/check/handle_namespace.cpp b/toolchain/check/handle_namespace.cpp index e4f027440466..52edf82c20f5 100644 --- a/toolchain/check/handle_namespace.cpp +++ b/toolchain/check/handle_namespace.cpp @@ -40,13 +40,14 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool { auto namespace_id = context.AddPlaceholderInst(SemIR::LocIdAndInst(node_id, namespace_inst)); - auto existing_inst_id = context.decl_name_stack().LookupOrAddName( - name_context, namespace_id, SemIR::AccessKind::Public); - if (existing_inst_id.is_valid()) { - if (existing_inst_id.is_poisoned()) { - context.DiagnosePoisonedName(namespace_id); - } else if (auto existing = context.insts().TryGetAs( - existing_inst_id)) { + auto [existing_inst_id, is_poisoned] = + context.decl_name_stack().LookupOrAddName(name_context, namespace_id, + SemIR::AccessKind::Public); + if (is_poisoned) { + context.DiagnosePoisonedName(namespace_id); + } else if (existing_inst_id.is_valid()) { + if (auto existing = + context.insts().TryGetAs(existing_inst_id)) { // If there's a name conflict with a namespace, "merge" by using the // previous declaration. Otherwise, diagnose the issue. diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index a35ad5f33de0..1a714a73b3c8 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -276,7 +276,7 @@ auto FinishImplWitness(Context& context, SemIR::Impl& impl) -> void { auto type_inst = context.types().GetAsInst(struct_value.type_id); auto fn_type = type_inst.As(); auto& fn = context.functions().Get(fn_type.function_id); - auto [impl_decl_id, _] = context.LookupNameInExactScope( + auto [impl_decl_id, _, is_poisoned] = context.LookupNameInExactScope( decl_id, fn.name_id, impl.scope_id, impl_scope); if (impl_decl_id.is_valid()) { used_decl_ids.push_back(impl_decl_id); diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index ac05a052bc1a..d3e29f3af194 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -109,8 +109,9 @@ static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, // This InstId is temporary and would be overridden if used. SemIR::InstId::Invalid, SemIR::AccessKind::Public); if (!inserted) { - auto prev_inst_id = parent_scope->GetEntry(entry_id).inst_id; - CARBON_CHECK(!prev_inst_id.is_poisoned()); + const auto& prev_entry = parent_scope->GetEntry(entry_id); + CARBON_CHECK(!prev_entry.is_poisoned); + auto prev_inst_id = prev_entry.inst_id; if (auto namespace_inst = context.insts().TryGetAs(prev_inst_id)) { if (diagnose_duplicate_namespace) { @@ -334,7 +335,7 @@ static auto ImportScopeFromApiFile(Context& context, auto& impl_scope = context.name_scopes().Get(impl_scope_id); for (const auto& api_entry : api_scope.entries()) { - if (api_entry.inst_id.is_poisoned()) { + if (api_entry.is_poisoned) { continue; } auto impl_name_id = diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index f8fc7d01358c..2989376e594b 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -1207,7 +1207,7 @@ static auto AddNameScopeImportRefs(ImportContext& context, const SemIR::NameScope& import_scope, SemIR::NameScope& new_scope) -> void { for (auto entry : import_scope.entries()) { - if (entry.inst_id.is_poisoned()) { + if (entry.is_poisoned) { continue; } auto ref_id = AddImportRef(context, entry.inst_id); @@ -2944,9 +2944,6 @@ static auto GetInstForLoad(Context& context, } auto LoadImportRef(Context& context, SemIR::InstId inst_id) -> void { - if (inst_id.is_poisoned()) { - return; - } auto inst = context.insts().TryGetAs(inst_id); if (!inst) { return; diff --git a/toolchain/check/member_access.cpp b/toolchain/check/member_access.cpp index 0bc26f7ef9e9..aee62581d6ed 100644 --- a/toolchain/check/member_access.cpp +++ b/toolchain/check/member_access.cpp @@ -55,8 +55,9 @@ static auto IsInstanceMethod(const SemIR::File& sem_ir, static auto GetHighestAllowedAccess(Context& context, SemIR::LocId loc_id, SemIR::ConstantId name_scope_const_id) -> SemIR::AccessKind { - auto [_, self_type_inst_id] = context.LookupUnqualifiedName( + auto [_, self_type_inst_id, is_poisoned] = context.LookupUnqualifiedName( loc_id.node_id(), SemIR::NameId::SelfType, /*required=*/false); + CARBON_CHECK(!is_poisoned); if (!self_type_inst_id.is_valid()) { return SemIR::AccessKind::Public; } diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 743ffb8fce47..51bd28954214 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -630,8 +630,8 @@ class FormatterImpl { out_ << label; } - for (auto [name_id, inst_id, access_kind] : scope.entries()) { - if (inst_id.is_poisoned()) { + for (auto [name_id, inst_id, access_kind, is_poisoned] : scope.entries()) { + if (is_poisoned) { // TODO: Add poisoned names. continue; } diff --git a/toolchain/sem_ir/ids.cpp b/toolchain/sem_ir/ids.cpp index 696d7cdf280c..b32b403a2a8a 100644 --- a/toolchain/sem_ir/ids.cpp +++ b/toolchain/sem_ir/ids.cpp @@ -12,8 +12,6 @@ namespace Carbon::SemIR { auto InstId::Print(llvm::raw_ostream& out) const -> void { if (IsSingletonInstId(*this)) { out << Label << "(" << SingletonInstKinds[index] << ")"; - } else if (is_poisoned()) { - out << ""; } else { IdBase::Print(out); } diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index b843a01c672c..02362160f2d4 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -40,19 +40,12 @@ struct InstId : public IdBase { // An explicitly invalid ID. static const InstId Invalid; - // Represents that the name in this scope was poisoned by using it without - // qualifications. - static const InstId PoisonedName; - using IdBase::IdBase; - constexpr auto is_poisoned() const -> bool { return *this == PoisonedName; } - auto Print(llvm::raw_ostream& out) const -> void; }; constexpr InstId InstId::Invalid = InstId(InvalidIndex); -constexpr InstId InstId::PoisonedName = InstId(InvalidIndex - 1); // An ID of an instruction that is referenced absolutely by another instruction. // This should only be used as the type of a field within a typed instruction diff --git a/toolchain/sem_ir/name_scope.cpp b/toolchain/sem_ir/name_scope.cpp index 811aa72d7f9f..2eab30092580 100644 --- a/toolchain/sem_ir/name_scope.cpp +++ b/toolchain/sem_ir/name_scope.cpp @@ -22,7 +22,7 @@ auto NameScope::Print(llvm::raw_ostream& out) const -> void { out << ", names: {"; llvm::ListSeparator sep; for (auto entry : names_) { - if (entry.inst_id.is_poisoned()) { + if (entry.is_poisoned) { continue; } out << sep << entry.name_id << ": " << entry.inst_id; @@ -33,7 +33,7 @@ auto NameScope::Print(llvm::raw_ostream& out) const -> void { } auto NameScope::AddRequired(Entry name_entry) -> void { - CARBON_CHECK(!name_entry.inst_id.is_poisoned(), + CARBON_CHECK(!name_entry.is_poisoned, "Cannot add a poisoned name: {0}. Use AddPoison()", name_entry.name_id); auto add_name = [&] { @@ -49,8 +49,6 @@ auto NameScope::AddRequired(Entry name_entry) -> void { auto NameScope::LookupOrAdd(SemIR::NameId name_id, InstId inst_id, AccessKind access_kind) -> std::pair { - CARBON_CHECK(!inst_id.is_poisoned(), - "Cannot add a poisoned name: {0}. Use AddPoison()", name_id); auto insert_result = name_map_.Insert(name_id, EntryId(names_.size())); if (!insert_result.is_inserted()) { return {false, EntryId(insert_result.value())}; @@ -66,8 +64,9 @@ auto NameScope::AddPoison(NameId name_id) -> void { CARBON_CHECK(insert_result.is_inserted(), "Trying to poison an existing name: {0}", name_id); names_.push_back({.name_id = name_id, - .inst_id = InstId::PoisonedName, - .access_kind = AccessKind::Public}); + .inst_id = InstId::Invalid, + .access_kind = AccessKind::Public, + .is_poisoned = true}); } auto NameScopeStore::GetInstIfValid(NameScopeId scope_id) const diff --git a/toolchain/sem_ir/name_scope.h b/toolchain/sem_ir/name_scope.h index 860666183e6d..8b96a8125545 100644 --- a/toolchain/sem_ir/name_scope.h +++ b/toolchain/sem_ir/name_scope.h @@ -24,7 +24,9 @@ class NameScope : public Printable { NameId name_id; InstId inst_id; AccessKind access_kind; + bool is_poisoned = false; }; + static_assert(sizeof(Entry) == 12); struct EntryId : public IdBase { static constexpr llvm::StringLiteral Label = "name_scope_entry"; diff --git a/toolchain/sem_ir/name_scope_test.cpp b/toolchain/sem_ir/name_scope_test.cpp index 1b246d967303..6d21d0c7691f 100644 --- a/toolchain/sem_ir/name_scope_test.cpp +++ b/toolchain/sem_ir/name_scope_test.cpp @@ -159,28 +159,32 @@ TEST(NameScope, Poison) { EXPECT_THAT(name_scope.entries(), ElementsAre(NameScopeEntryEquals( NameScope::Entry({.name_id = poison1, - .inst_id = InstId::PoisonedName, - .access_kind = AccessKind::Public})))); + .inst_id = InstId::Invalid, + .access_kind = AccessKind::Public, + .is_poisoned = true})))); NameId poison2(++id); name_scope.AddPoison(poison2); EXPECT_THAT(name_scope.entries(), ElementsAre(NameScopeEntryEquals(NameScope::Entry( {.name_id = poison1, - .inst_id = InstId::PoisonedName, - .access_kind = AccessKind::Public})), + .inst_id = InstId::Invalid, + .access_kind = AccessKind::Public, + .is_poisoned = true})), NameScopeEntryEquals(NameScope::Entry( {.name_id = poison2, - .inst_id = InstId::PoisonedName, - .access_kind = AccessKind::Public})))); + .inst_id = InstId::Invalid, + .access_kind = AccessKind::Public, + .is_poisoned = true})))); auto lookup = name_scope.Lookup(poison1); ASSERT_NE(lookup, std::nullopt); - EXPECT_THAT(name_scope.GetEntry(*lookup), - NameScopeEntryEquals( - NameScope::Entry({.name_id = poison1, - .inst_id = InstId::PoisonedName, - .access_kind = AccessKind::Public}))); + EXPECT_THAT( + name_scope.GetEntry(*lookup), + NameScopeEntryEquals(NameScope::Entry({.name_id = poison1, + .inst_id = InstId::Invalid, + .access_kind = AccessKind::Public, + .is_poisoned = true}))); } TEST(NameScope, ExtendedScopes) {