Change DiagnoseDuplicateName to expect an inst ID for the duplicate. (#3616)

This makes duplicate and previous definition handling match. While we
may want to make both point more fine-grained at the name, the necessary
logic seems likely to be equivalent.

Note, I'm looking at this mainly due to duplicate names in imports,
where it's especially helpful to take an instruction instead of a parse
node. We'll eventually want to handle parse nodes from other imports
better, and I think this is the way it would most likely work.

toolchain/check/context.cpp

@@ -137,13 +137,13 @@ auto Context::ReplaceInstBeforeConstantUse(
   constant_values().Set(inst_id, const_id);
 }
 
-auto Context::DiagnoseDuplicateName(Parse::NodeId parse_node,
+auto Context::DiagnoseDuplicateName(SemIR::InstId dup_def_id,
                                     SemIR::InstId prev_def_id) -> void {
   CARBON_DIAGNOSTIC(NameDeclDuplicate, Error,
                     "Duplicate name being declared in the same scope.");
   CARBON_DIAGNOSTIC(NameDeclPrevious, Note,
                     "Name is previously declared here.");
-  emitter_->Build(parse_node, NameDeclDuplicate)
+  emitter_->Build(dup_def_id, NameDeclDuplicate)
       .Note(prev_def_id, NameDeclPrevious)
       .Emit();
 }
@@ -195,7 +195,7 @@ auto Context::AddPackageImports(Parse::NodeId import_node,
 
   // Add the import to lookup. Should always succeed because imports will be
   // uniquely named.
-  AddNameToLookup(import_node, name_id, inst_id);
+  AddNameToLookup(name_id, inst_id);
   // Add a name for formatted output. This isn't used in name lookup in order
   // to reduce indirection, but it's separate from the Import because it
   // otherwise fits in an Inst.
@@ -206,8 +206,8 @@ auto Context::AddPackageImports(Parse::NodeId import_node,
                                         .value_id = inst_id}});
 }
 
-auto Context::AddNameToLookup(Parse::NodeId name_node, SemIR::NameId name_id,
-                              SemIR::InstId target_id) -> void {
+auto Context::AddNameToLookup(SemIR::NameId name_id, SemIR::InstId target_id)
+    -> void {
   if (current_scope().names.insert(name_id).second) {
     // TODO: Reject if we previously performed a failed lookup for this name in
     // this scope or a scope nested within it.
@@ -218,7 +218,7 @@ auto Context::AddNameToLookup(Parse::NodeId name_node, SemIR::NameId name_id,
     lexical_results.push_back(
         {.inst_id = target_id, .scope_index = current_scope_index()});
   } else {
-    DiagnoseDuplicateName(name_node,
+    DiagnoseDuplicateName(target_id,
                           lexical_lookup_.Get(name_id).back().inst_id);
   }
 }

toolchain/check/context.h

@@ -121,8 +121,7 @@ class Context {
                          bool has_load_error) -> void;
 
   // Adds a name to name lookup. Prints a diagnostic for name conflicts.
-  auto AddNameToLookup(Parse::NodeId name_node, SemIR::NameId name_id,
-                       SemIR::InstId target_id) -> void;
+  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,
@@ -147,7 +146,7 @@ class Context {
       -> SemIR::InstId;
 
   // Prints a diagnostic for a duplicate name.
-  auto DiagnoseDuplicateName(Parse::NodeId parse_node,
+  auto DiagnoseDuplicateName(SemIR::InstId dup_def_id,
                              SemIR::InstId prev_def_id) -> void;
 
   // Prints a diagnostic for a missing name.

toolchain/check/decl_name_stack.cpp

@@ -67,8 +67,7 @@ auto DeclNameStack::LookupOrAddName(NameContext name_context,
 
     case NameContext::State::Unresolved:
       if (!name_context.target_scope_id.is_valid()) {
-        context_->AddNameToLookup(name_context.parse_node,
-                                  name_context.unresolved_name_id, target_id);
+        context_->AddNameToLookup(name_context.unresolved_name_id, target_id);
       } else {
         auto& name_scope =
             context_->name_scopes().Get(name_context.target_scope_id);
@@ -103,7 +102,7 @@ auto DeclNameStack::AddNameToLookup(NameContext name_context,
                                     SemIR::InstId target_id) -> void {
   auto existing_inst_id = LookupOrAddName(name_context, target_id);
   if (existing_inst_id.is_valid()) {
-    context_->DiagnoseDuplicateName(name_context.parse_node, existing_inst_id);
+    context_->DiagnoseDuplicateName(target_id, existing_inst_id);
   }
 }
 

toolchain/check/handle_class.cpp

@@ -85,7 +85,7 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId parse_node)
       // declaration.
     } else {
       // This is a redeclaration of something other than a class.
-      context.DiagnoseDuplicateName(name_context.parse_node, existing_id);
+      context.DiagnoseDuplicateName(class_decl_id, existing_id);
     }
   }
 
@@ -149,7 +149,7 @@ auto HandleClassDefinitionStart(Context& context,
   context.PushScope(class_decl_id, class_info.scope_id);
 
   // Introduce `Self`.
-  context.AddNameToLookup(parse_node, SemIR::NameId::SelfType,
+  context.AddNameToLookup(SemIR::NameId::SelfType,
                           context.types().GetInstId(class_info.self_type_id));
 
   context.inst_block_stack().Push();

toolchain/check/handle_function.cpp

@@ -164,7 +164,7 @@ static auto BuildFunctionDecl(Context& context,
       }
     } else {
       // This is a redeclaration of something other than a function.
-      context.DiagnoseDuplicateName(name_context.parse_node, existing_id);
+      context.DiagnoseDuplicateName(function_decl_id, existing_id);
     }
   }
 
@@ -264,7 +264,6 @@ auto HandleFunctionDefinitionStart(Context& context,
 
     if (auto fn_param = param.TryAs<SemIR::AnyBindName>()) {
       context.AddNameToLookup(
-          context.insts().GetParseNode(param_id),
           context.bind_names().Get(fn_param->bind_name_id).name_id, param_id);
     } else {
       CARBON_FATAL() << "Unexpected kind of parameter in function definition "

toolchain/check/handle_interface.cpp

@@ -68,7 +68,7 @@ static auto BuildInterfaceDecl(Context& context,
       // declaration.
     } else {
       // This is a redeclaration of something other than a interface.
-      context.DiagnoseDuplicateName(name_context.parse_node, existing_id);
+      context.DiagnoseDuplicateName(interface_decl_id, existing_id);
     }
   }
 

toolchain/check/handle_let.cpp

@@ -66,8 +66,7 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId parse_node) -> bool {
 
   // Add the name of the binding to the current scope.
   auto name_id = context.bind_names().Get(bind_name.bind_name_id).name_id;
-  context.AddNameToLookup(context.insts().GetParseNode(pattern_id), name_id,
-                          pattern_id);
+  context.AddNameToLookup(name_id, pattern_id);
   return true;
 }
 

toolchain/check/testdata/interface/fail_duplicate.carbon

@@ -18,9 +18,9 @@ interface Interface {
 
 fn Function();
 
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+6]]:11: ERROR: Duplicate name being declared in the same scope.
+// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope.
 // CHECK:STDERR: interface Function;
-// CHECK:STDERR:           ^~~~~~~~
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR: fail_duplicate.carbon:[[@LINE-5]]:1: Name is previously declared here.
 // CHECK:STDERR: fn Function();
 // CHECK:STDERR: ^~~~~~~~~~~~~~
@@ -28,9 +28,9 @@ interface Function;
 
 class Class;
 
-// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+6]]:11: ERROR: Duplicate name being declared in the same scope.
+// CHECK:STDERR: fail_duplicate.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope.
 // CHECK:STDERR: interface Class { }
-// CHECK:STDERR:           ^~~~~
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~
 // CHECK:STDERR: fail_duplicate.carbon:[[@LINE-5]]:1: Name is previously declared here.
 // CHECK:STDERR: class Class;
 // CHECK:STDERR: ^~~~~~~~~~~~

toolchain/check/testdata/namespace/fail_import_of_repeat.carbon

@@ -9,9 +9,9 @@
 package Implicit api;
 
 namespace NS;
-// CHECK:STDERR: implicit.carbon:[[@LINE+6]]:11: ERROR: Duplicate name being declared in the same scope.
+// CHECK:STDERR: implicit.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope.
 // CHECK:STDERR: namespace NS;
-// CHECK:STDERR:           ^~
+// CHECK:STDERR: ^~~~~~~~~~~~~
 // CHECK:STDERR: implicit.carbon:[[@LINE-4]]:1: Name is previously declared here.
 // CHECK:STDERR: namespace NS;
 // CHECK:STDERR: ^~~~~~~~~~~~~