When diagnosing name used before declared, set the location of the usage (#4860)

Done by adding a poisoning location for each poisoned name.
Part of #4622.

Author: bricknerb

toolchain/check/context.cpp

@@ -223,15 +223,15 @@ auto Context::DiagnoseDuplicateName(SemIRLoc dup_def, SemIRLoc prev_def)
       .Emit();
 }
 
-auto Context::DiagnosePoisonedName(SemIR::InstId loc) -> void {
-  // TODO: Improve the diagnostic to replace NodeId::None with the location
-  // where the name was poisoned. See discussion in
-  // https://github.com/carbon-language/carbon-lang/pull/4654#discussion_r1876607172
+auto Context::DiagnosePoisonedName(SemIR::LocId poisoning_loc_id,
+                                   SemIR::InstId decl_inst_id) -> void {
+  CARBON_CHECK(poisoning_loc_id.has_value(),
+               "Trying to diagnose poisoned name with no poisoning location");
   CARBON_DIAGNOSTIC(NameUseBeforeDecl, Error,
                     "name used before it was declared");
   CARBON_DIAGNOSTIC(NameUseBeforeDeclNote, Note, "declared here");
-  emitter_->Build(SemIR::LocId::None, NameUseBeforeDecl)
-      .Note(loc, NameUseBeforeDeclNote)
+  emitter_->Build(poisoning_loc_id, NameUseBeforeDecl)
+      .Note(decl_inst_id, NameUseBeforeDeclNote)
       .Emit();
 }
 
@@ -421,13 +421,14 @@ auto Context::LookupUnqualifiedName(Parse::NodeId node_id,
           .scope_result = SemIR::ScopeLookupResult::MakeError()};
 }
 
-auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
+auto Context::LookupNameInExactScope(SemIR::LocId loc_id, SemIR::NameId name_id,
                                      SemIR::NameScopeId scope_id,
                                      SemIR::NameScope& scope,
                                      bool is_being_declared)
     -> SemIR::ScopeLookupResult {
-  if (auto entry_id = is_being_declared ? scope.Lookup(name_id)
-                                        : scope.LookupOrPoison(name_id)) {
+  if (auto entry_id = is_being_declared
+                          ? scope.Lookup(name_id)
+                          : scope.LookupOrPoison(loc_id, name_id)) {
     auto lookup_result = scope.GetEntry(*entry_id).result;
     if (!lookup_result.is_poisoned()) {
       LoadImportRef(*this, lookup_result.target_inst_id());
@@ -438,7 +439,7 @@ auto Context::LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
   if (!scope.import_ir_scopes().empty()) {
     // TODO: Enforce other access modifiers for imports.
     return SemIR::ScopeLookupResult::MakeWrappedLookupResult(
-        ImportNameFromOtherPackage(*this, loc, scope_id,
+        ImportNameFromOtherPackage(*this, loc_id, scope_id,
                                    scope.import_ir_scopes(), name_id),
         SemIR::AccessKind::Public);
   }

toolchain/check/context.h

@@ -215,9 +215,7 @@ class Context {
 
   // Performs name lookup in a specified scope for a name appearing in a
   // declaration. If scope_id is `None`, performs lookup into the lexical scope
-  // specified by scope_index instead. If found, returns the referenced
-  // `InstId` and false. If poisoned, returns `InstId::None` and true.
-  // TODO: For poisoned names, return the poisoning `InstId`.
+  // specified by scope_index instead.
   auto LookupNameInDecl(SemIR::LocId loc_id, SemIR::NameId name_id,
                         SemIR::NameScopeId scope_id, ScopeIndex scope_index)
       -> SemIR::ScopeLookupResult;
@@ -235,7 +233,7 @@ class Context {
   // poisoned name will be treated as if it is not declared. Otherwise, this is
   // a lookup for a name being declared, so the name will not be poisoned, but
   // poison will be returned if it's already been looked up.
-  auto LookupNameInExactScope(SemIRLoc loc, SemIR::NameId name_id,
+  auto LookupNameInExactScope(SemIR::LocId loc_id, SemIR::NameId name_id,
                               SemIR::NameScopeId scope_id,
                               SemIR::NameScope& scope,
                               bool is_being_declared = false)
@@ -265,8 +263,9 @@ class Context {
   // Prints a diagnostic for a duplicate name.
   auto DiagnoseDuplicateName(SemIRLoc dup_def, SemIRLoc prev_def) -> void;
 
-  // Prints a diagnostic for a poisoned name.
-  auto DiagnosePoisonedName(SemIR::InstId loc) -> void;
+  // Prints a diagnostic for a poisoned name when it's later declared.
+  auto DiagnosePoisonedName(SemIR::LocId poisoning_loc_id,
+                            SemIR::InstId decl_inst_id) -> void;
 
   // Prints a diagnostic for a missing name.
   auto DiagnoseNameNotFound(SemIRLoc loc, SemIR::NameId name_id) -> void;

toolchain/check/decl_name_stack.cpp

@@ -175,7 +175,7 @@ auto DeclNameStack::AddNameOrDiagnose(NameContext name_context,
                                       SemIR::InstId target_id,
                                       SemIR::AccessKind access_kind) -> void {
   if (name_context.state == DeclNameStack::NameContext::State::Poisoned) {
-    context_->DiagnosePoisonedName(target_id);
+    context_->DiagnosePoisonedName(name_context.poisoning_loc_id, target_id);
   } else if (auto id = name_context.prev_inst_id(); id.has_value()) {
     context_->DiagnoseDuplicateName(target_id, id);
   } else {
@@ -188,7 +188,8 @@ auto DeclNameStack::LookupOrAddName(NameContext name_context,
                                     SemIR::AccessKind access_kind)
     -> SemIR::ScopeLookupResult {
   if (name_context.state == NameContext::State::Poisoned) {
-    return SemIR::ScopeLookupResult::MakePoisoned();
+    return SemIR::ScopeLookupResult::MakePoisoned(
+        name_context.poisoning_loc_id);
   }
   if (auto id = name_context.prev_inst_id(); id.has_value()) {
     return SemIR::ScopeLookupResult::MakeFound(id, access_kind);
@@ -267,6 +268,7 @@ auto DeclNameStack::ApplyAndLookupName(NameContext& name_context,
       name_context.initial_scope_index);
   if (lookup_result.is_poisoned()) {
     name_context.unresolved_name_id = name_id;
+    name_context.poisoning_loc_id = lookup_result.poisoning_loc_id();
     name_context.state = NameContext::State::Poisoned;
   } else if (!lookup_result.is_found()) {
     // Invalid indicates an unresolved name. Store it and return.

toolchain/check/decl_name_stack.h

@@ -156,6 +156,10 @@ class DeclNameStack {
       // The ID of an unresolved identifier.
       SemIR::NameId unresolved_name_id = SemIR::NameId::None;
     };
+
+    // When `state` is `Poisoned` (name is unresolved due to name poisoning),
+    // the poisoning location.
+    SemIR::LocId poisoning_loc_id = SemIR::LocId::None;
   };
 
   // Information about a declaration name that has been temporarily removed from

toolchain/check/handle_class.cpp

@@ -109,7 +109,8 @@ static auto MergeOrAddName(Context& context, Parse::AnyClassDeclId node_id,
                                                 access_kind);
   if (lookup_result.is_poisoned()) {
     // This is a declaration of a poisoned name.
-    context.DiagnosePoisonedName(class_decl_id);
+    context.DiagnosePoisonedName(lookup_result.poisoning_loc_id(),
+                                 class_decl_id);
     return;
   }
 

toolchain/check/handle_function.cpp

@@ -255,7 +255,8 @@ static auto BuildFunctionDecl(Context& context,
   }
 
   if (name_context.state == DeclNameStack::NameContext::State::Poisoned) {
-    context.DiagnosePoisonedName(function_info.latest_decl_id());
+    context.DiagnosePoisonedName(name_context.poisoning_loc_id,
+                                 function_info.latest_decl_id());
   } else {
     TryMergeRedecl(context, node_id, name_context.prev_inst_id(), function_decl,
                    function_info, is_definition);

toolchain/check/handle_interface.cpp

@@ -65,7 +65,8 @@ static auto BuildInterfaceDecl(Context& context,
           introducer.modifier_set.GetAccessKind());
   if (lookup_result.is_poisoned()) {
     // This is a declaration of a poisoned name.
-    context.DiagnosePoisonedName(interface_decl_id);
+    context.DiagnosePoisonedName(lookup_result.poisoning_loc_id(),
+                                 interface_decl_id);
   } else if (lookup_result.is_found()) {
     SemIR::InstId existing_id = lookup_result.target_inst_id();
     if (auto existing_interface_decl =

toolchain/check/handle_namespace.cpp

@@ -46,7 +46,8 @@ auto HandleParseNode(Context& context, Parse::NamespaceId node_id) -> bool {
       context.decl_name_stack().LookupOrAddName(name_context, namespace_id,
                                                 SemIR::AccessKind::Public);
   if (lookup_result.is_poisoned()) {
-    context.DiagnosePoisonedName(namespace_id);
+    context.DiagnosePoisonedName(lookup_result.poisoning_loc_id(),
+                                 namespace_id);
   } else if (lookup_result.is_found()) {
     SemIR::InstId existing_inst_id = lookup_result.target_inst_id();
     if (auto existing =

toolchain/check/impl.cpp

@@ -376,7 +376,8 @@ auto FinishImplWitness(Context& context, SemIR::Impl& impl) -> void {
         }
         auto& fn = context.functions().Get(fn_type->function_id);
         auto lookup_result = context.LookupNameInExactScope(
-            decl_id, fn.name_id, impl.scope_id, impl_scope);
+            context.insts().GetLocId(decl_id), fn.name_id, impl.scope_id,
+            impl_scope);
         if (lookup_result.is_found()) {
           used_decl_ids.push_back(lookup_result.target_inst_id());
           witness_block[index] = CheckAssociatedFunctionImplementation(