Wrap lookup result in a new ScopeLookupResult (#4831)

Benefits:
* Provide a proper API for accessing lookup information.
* Make assumptions on whether the result is poisoned or not and how we
can use `InstId` explicit.
* Allow safely reusing the `InstId` value for pointing to the poisoning
entity for poisoned results (in a future PR).
* Consolidate `LookupNameInExactScopeResult`, `std::pair<SemIR::InstId,
bool>` and part of `LookupResult`.
Part of #4622.

Author: bricknerb

toolchain/check/context.cpp

@@ -317,7 +317,7 @@ 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,
                                ScopeIndex scope_index)
-    -> std::pair<SemIR::InstId, bool> {
+    -> SemIR::ScopeLookupResult {
   if (!scope_id.has_value()) {
     // Look for a name in the specified scope or a scope nested within it only.
     // There are two cases where the name would be in an outer scope:
@@ -353,8 +353,9 @@ auto Context::LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id,
     //
     //    In this case, the class C is not a redeclaration of its parameter, but
     //    we find the parameter in order to diagnose a redeclaration error.
-    return {scope_stack().LookupInLexicalScopesWithin(name_id, scope_index),
-            false};
+    return SemIR::ScopeLookupResult::MakeWrappedLookupResult(
+        scope_stack().LookupInLexicalScopesWithin(name_id, scope_index),
+        SemIR::AccessKind::Public);
   } 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
@@ -365,10 +366,9 @@ auto Context::LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id,
     //
     //    // Error, no `F` in `B`.
     //    fn B.F() {}
-    auto result = LookupNameInExactScope(loc_id, name_id, scope_id,
-                                         name_scopes().Get(scope_id),
-                                         /*is_being_declared=*/true);
-    return {result.inst_id, result.is_poisoned};
+    return LookupNameInExactScope(loc_id, name_id, scope_id,
+                                  name_scopes().Get(scope_id),
+                                  /*is_being_declared=*/true);
   }
 }
 
@@ -390,7 +390,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.has_value()) {
+        non_lexical_result.scope_result.is_found()) {
       return non_lexical_result;
     }
   }
@@ -400,14 +400,16 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id,
                       "`{0}` used before initialization", SemIR::NameId);
     emitter_->Emit(node_id, UsedBeforeInitialization, name_id);
     return {.specific_id = SemIR::SpecificId::None,
-            .inst_id = SemIR::ErrorInst::SingletonInstId};
+            .scope_result = SemIR::ScopeLookupResult::MakeError()};
   }
 
   if (lexical_result.has_value()) {
     // A lexical scope never needs an associated specific. If there's a
     // lexically enclosing generic, then it also encloses the point of use of
     // the name.
-    return {.specific_id = SemIR::SpecificId::None, .inst_id = lexical_result};
+    return {.specific_id = SemIR::SpecificId::None,
+            .scope_result = SemIR::ScopeLookupResult::MakeFound(
+                lexical_result, SemIR::AccessKind::Public)};
   }
 
   // We didn't find anything at all.
@@ -416,32 +418,32 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id,
   }
 
   return {.specific_id = SemIR::SpecificId::None,
-          .inst_id = SemIR::ErrorInst::SingletonInstId};
+          .scope_result = SemIR::ScopeLookupResult::MakeError()};
 }
 
 auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
                                      SemIR::NameScopeId scope_id,
                                      SemIR::NameScope& scope,
                                      bool is_being_declared)
-    -> LookupNameInExactScopeResult {
+    -> SemIR::ScopeLookupResult {
   if (auto entry_id = is_being_declared ? scope.Lookup(name_id)
                                         : scope.LookupOrPoison(name_id)) {
-    auto entry = scope.GetEntry(*entry_id);
-    if (!entry.is_poisoned) {
-      LoadImportRef(*this, entry.inst_id);
-    } else if (is_being_declared) {
-      entry.inst_id = SemIR::InstId::None;
+    auto lookup_result = scope.GetEntry(*entry_id).result;
+    if (!lookup_result.is_poisoned()) {
+      LoadImportRef(*this, lookup_result.target_inst_id());
+      return lookup_result;
     }
-    return {entry.inst_id, entry.access_kind, entry.is_poisoned};
+    return SemIR::ScopeLookupResult::MakePoisoned();
   }
 
   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),
-            SemIR::AccessKind::Public};
+    return SemIR::ScopeLookupResult::MakeWrappedLookupResult(
+        ImportNameFromOtherPackage(*this, loc, scope_id,
+                                   scope.import_ir_scopes(), name_id),
+        SemIR::AccessKind::Public);
   }
-  return {SemIR::InstId::None, SemIR::AccessKind::Public};
+  return SemIR::ScopeLookupResult::MakeNotFound();
 }
 
 // Prints diagnostics on invalid qualified name access.
@@ -583,8 +585,9 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
   // TODO: Support reporting of multiple prohibited access.
   llvm::SmallVector<ProhibitedAccessInfo> prohibited_accesses;
 
-  LookupResult result = {.specific_id = SemIR::SpecificId::None,
-                         .inst_id = SemIR::InstId::None};
+  LookupResult result = {
+      .specific_id = SemIR::SpecificId::None,
+      .scope_result = SemIR::ScopeLookupResult::MakeNotFound()};
   bool has_error = false;
   bool is_parent_access = false;
 
@@ -598,8 +601,9 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
     auto& name_scope = name_scopes().Get(scope_id);
     has_error |= name_scope.has_error();
 
-    auto [scope_result_id, access_kind, is_poisoned] =
+    const SemIR::ScopeLookupResult scope_result =
         LookupNameInExactScope(loc_id, name_id, scope_id, name_scope);
+    SemIR::AccessKind access_kind = scope_result.access_kind();
 
     auto is_access_prohibited =
         IsAccessProhibited(access_info, access_kind, is_parent_access);
@@ -608,13 +612,13 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
     // multiple prohibited accesses if we can't find a suitable lookup.
     if (is_access_prohibited) {
       prohibited_accesses.push_back({
-          .scope_result_id = scope_result_id,
+          .scope_result_id = scope_result.target_inst_id(),
           .access_kind = access_kind,
           .is_parent_access = is_parent_access,
       });
     }
 
-    if (!scope_result_id.has_value() || is_access_prohibited) {
+    if (!scope_result.is_found() || 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();
@@ -643,23 +647,22 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
     }
 
     // If this is our second lookup result, diagnose an ambiguity.
-    if (result.inst_id.has_value()) {
+    if (result.scope_result.is_found()) {
       CARBON_DIAGNOSTIC(
           NameAmbiguousDueToExtend, Error,
           "ambiguous use of name `{0}` found in multiple extended scopes",
           SemIR::NameId);
       emitter_->Emit(loc_id, NameAmbiguousDueToExtend, name_id);
       // TODO: Add notes pointing to the scopes.
       return {.specific_id = SemIR::SpecificId::None,
-              .inst_id = SemIR::ErrorInst::SingletonInstId};
+              .scope_result = SemIR::ScopeLookupResult::MakeError()};
     }
 
-    result.inst_id = scope_result_id;
+    result.scope_result = scope_result;
     result.specific_id = specific_id;
-    result.is_poisoned = is_poisoned;
   }
 
-  if (required && !result.inst_id.has_value()) {
+  if (required && !result.scope_result.is_found()) {
     if (!has_error) {
       if (prohibited_accesses.empty()) {
         DiagnoseMemberNameNotFound(loc_id, name_id, lookup_scopes);
@@ -677,9 +680,9 @@ auto Context::LookupQualifiedName(SemIR::LocId loc_id, SemIR::NameId name_id,
       }
     }
 
+    CARBON_CHECK(!result.scope_result.is_poisoned());
     return {.specific_id = SemIR::SpecificId::None,
-            .inst_id = SemIR::ErrorInst::SingletonInstId,
-            .is_poisoned = result.is_poisoned};
+            .scope_result = SemIR::ScopeLookupResult::MakeError()};
   }
 
   return result;
@@ -699,13 +702,13 @@ 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, _, is_poisoned] = context.LookupNameInExactScope(
+  auto core_scope_result = context.LookupNameInExactScope(
       loc, core_name_id, SemIR::NameScopeId::Package,
       context.name_scopes().Get(SemIR::NameScopeId::Package));
-  if (core_inst_id.has_value()) {
+  if (core_scope_result.is_found()) {
     // We expect it to be a namespace.
-    if (auto namespace_inst =
-            context.insts().TryGetAs<SemIR::Namespace>(core_inst_id)) {
+    if (auto namespace_inst = context.insts().TryGetAs<SemIR::Namespace>(
+            core_scope_result.target_inst_id())) {
       // TODO: Decide whether to allow the case where `Core` is not a package.
       return namespace_inst->name_scope_id;
     }
@@ -727,9 +730,9 @@ auto Context::LookupNameInCore(SemIRLoc loc, llvm::StringRef name)
   }
 
   auto name_id = SemIR::NameId::ForIdentifier(identifiers().Add(name));
-  auto [inst_id, _, is_poisoned] = LookupNameInExactScope(
+  auto scope_result = LookupNameInExactScope(
       loc, name_id, core_package_id, name_scopes().Get(core_package_id));
-  if (!inst_id.has_value()) {
+  if (!scope_result.is_found()) {
     CARBON_DIAGNOSTIC(
         CoreNameNotFound, Error,
         "name `Core.{0}` implicitly referenced here, but not found",
@@ -739,7 +742,7 @@ auto Context::LookupNameInCore(SemIRLoc loc, llvm::StringRef name)
   }
 
   // Look through import_refs and aliases.
-  return constant_values().GetConstantInstId(inst_id);
+  return constant_values().GetConstantInstId(scope_result.target_inst_id());
 }
 
 template <typename BranchNode, typename... Args>

toolchain/check/context.h

@@ -45,11 +45,9 @@ struct LookupResult {
   // The specific in which the lookup result was found. `None` if the result
   // was not found in a specific.
   SemIR::SpecificId specific_id;
-  // The declaration that was found by name lookup. `None` 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;
+
+  // The result from the lookup in the scope.
+  SemIR::ScopeLookupResult scope_result;
 };
 
 // Information about an access.
@@ -73,17 +71,6 @@ class Context {
   using BuildDiagnosticFn =
       llvm::function_ref<auto()->Context::DiagnosticBuilder>;
 
-  struct LookupNameInExactScopeResult {
-    // The matching entity if found, or `None` if poisoned or not found.
-    SemIR::InstId inst_id;
-
-    // The access level required to use inst_id when it's not `None`.
-    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<const Parse::TreeAndSubtrees&()>
@@ -233,7 +220,7 @@ class Context {
   // TODO: For poisoned names, return the poisoning `InstId`.
   auto LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id,
                         SemIR::NameScopeId scope_id, ScopeIndex scope_index)
-      -> std::pair<SemIR::InstId, bool>;
+      -> SemIR::ScopeLookupResult;
 
   // Performs an unqualified name lookup, returning the referenced `InstId`.
   auto LookupUnqualifiedName(Parse::NodeId node_id, SemIR::NameId name_id,
@@ -252,7 +239,7 @@ class Context {
                               SemIR::NameScopeId scope_id,
                               SemIR::NameScope& scope,
                               bool is_being_declared = false)
-      -> LookupNameInExactScopeResult;
+      -> SemIR::ScopeLookupResult;
 
   // Appends the lookup scopes corresponding to `base_const_id` to `*scopes`.
   // Returns `false` if not a scope. On invalid scopes, prints a diagnostic, but

toolchain/check/decl_name_stack.cpp

@@ -160,8 +160,8 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id,
         }
 
         name_scope.AddRequired({.name_id = name_context.unresolved_name_id,
-                                .inst_id = target_id,
-                                .access_kind = access_kind});
+                                .result = SemIR::ScopeLookupResult::MakeFound(
+                                    target_id, access_kind)});
       }
       break;
 
@@ -262,20 +262,20 @@ auto DeclNameStack::ApplyAndLookupName(NameContext& name_context,
   }
 
   // For identifier nodes, we need to perform a lookup on the identifier.
-  auto [resolved_inst_id, is_poisoned] = context_->LookupNameInDecl(
+  auto lookup_result = context_->LookupNameInDecl(
       name_context.loc_id, name_id, name_context.parent_scope_id,
       name_context.initial_scope_index);
-  if (is_poisoned) {
+  if (lookup_result.is_poisoned()) {
     name_context.unresolved_name_id = name_id;
     name_context.state = NameContext::State::Poisoned;
-  } else if (!resolved_inst_id.has_value()) {
+  } else if (!lookup_result.is_found()) {
     // Invalid indicates an unresolved name. Store it and return.
     name_context.unresolved_name_id = name_id;
     name_context.state = NameContext::State::Unresolved;
   } else {
     // Store the resolved instruction and continue for the target scope
     // update.
-    name_context.resolved_inst_id = resolved_inst_id;
+    name_context.resolved_inst_id = lookup_result.target_inst_id();
     name_context.state = NameContext::State::Resolved;
   }
 }

toolchain/check/handle_export.cpp

@@ -78,10 +78,11 @@ 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& 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;
+  auto& scope_result =
+      parent_scope.GetEntry(*parent_scope.Lookup(entity_name.name_id)).result;
+  CARBON_CHECK(scope_result.target_inst_id() == inst_id);
+  scope_result = SemIR::ScopeLookupResult::MakeFound(
+      export_id, scope_result.access_kind());
 
   return true;
 }

toolchain/check/handle_name.cpp

@@ -103,24 +103,24 @@ static auto GetIdentifierAsName(Context& context, Parse::NodeId node_id)
 static auto HandleNameAsExpr(Context& context, Parse::NodeId node_id,
                              SemIR::NameId name_id) -> SemIR::InstId {
   auto result = context.LookupUnqualifiedName(node_id, name_id);
-  auto value = context.insts().Get(result.inst_id);
+  SemIR::InstId inst_id = result.scope_result.target_inst_id();
+  auto value = context.insts().Get(inst_id);
   auto type_id = SemIR::GetTypeInSpecific(context.sem_ir(), result.specific_id,
                                           value.type_id());
   CARBON_CHECK(type_id.has_value(), "Missing type for {0}", value);
 
   // If the named entity has a constant value that depends on its specific,
   // store the specific too.
   if (result.specific_id.has_value() &&
-      context.constant_values().Get(result.inst_id).is_symbolic()) {
-    result.inst_id = context.AddInst<SemIR::SpecificConstant>(
+      context.constant_values().Get(inst_id).is_symbolic()) {
+    inst_id = context.AddInst<SemIR::SpecificConstant>(
         node_id, {.type_id = type_id,
-                  .inst_id = result.inst_id,
+                  .inst_id = inst_id,
                   .specific_id = result.specific_id});
   }
 
   return context.AddInst<SemIR::NameRef>(
-      node_id,
-      {.type_id = type_id, .name_id = name_id, .value_id = result.inst_id});
+      node_id, {.type_id = type_id, .name_id = name_id, .value_id = inst_id});
 }
 
 static auto HandleIdentifierName(Context& context,

toolchain/check/impl.cpp

@@ -416,12 +416,13 @@ auto FinishImplWitness(Context& context, SemIR::Impl& impl) -> void {
           CARBON_FATAL("Unexpected type: {0}", type_inst);
         }
         auto& fn = context.functions().Get(fn_type->function_id);
-        auto [impl_decl_id, _, is_poisoned] = context.LookupNameInExactScope(
+        auto lookup_result = context.LookupNameInExactScope(
             decl_id, fn.name_id, impl.scope_id, impl_scope);
-        if (impl_decl_id.has_value()) {
-          used_decl_ids.push_back(impl_decl_id);
+        if (lookup_result.is_found()) {
+          used_decl_ids.push_back(lookup_result.target_inst_id());
           witness_block[index] = CheckAssociatedFunctionImplementation(
-              context, *fn_type, impl_decl_id, self_type_id, impl.witness_id);
+              context, *fn_type, lookup_result.target_inst_id(), self_type_id,
+              impl.witness_id);
         } else {
           CARBON_DIAGNOSTIC(
               ImplMissingFunction, Error,