From d0fb4b5815b4e618c31c0b4d0878a2074b472982 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 18 Jan 2024 15:23:31 -0800 Subject: [PATCH] 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 | 12 ++++++------ toolchain/check/context.h | 5 ++--- toolchain/check/decl_name_stack.cpp | 5 ++--- toolchain/check/handle_class.cpp | 4 ++-- toolchain/check/handle_function.cpp | 3 +-- toolchain/check/handle_interface.cpp | 2 +- toolchain/check/handle_let.cpp | 3 +-- .../check/testdata/interface/fail_duplicate.carbon | 8 ++++---- .../testdata/namespace/fail_import_of_repeat.carbon | 4 ++-- 9 files changed, 21 insertions(+), 25 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index a03880ca338f..2bf0e5fd94ab 100644 --- a/toolchain/check/context.cpp +++ b/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); } } diff --git a/toolchain/check/context.h b/toolchain/check/context.h index b98bdfacf172..c3a9f407343a 100644 --- a/toolchain/check/context.h +++ b/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. diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index 74639bda3124..d9b64d8cc011 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/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); } } diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index 30c385a3d48f..50529482da2c 100644 --- a/toolchain/check/handle_class.cpp +++ b/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(); diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 530fdf50d320..0fab886d87fb 100644 --- a/toolchain/check/handle_function.cpp +++ b/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()) { 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 " diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp index 6f3a6df33c7b..0272574f26fe 100644 --- a/toolchain/check/handle_interface.cpp +++ b/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); } } diff --git a/toolchain/check/handle_let.cpp b/toolchain/check/handle_let.cpp index 93d55deb6045..5eba08dd2632 100644 --- a/toolchain/check/handle_let.cpp +++ b/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; } diff --git a/toolchain/check/testdata/interface/fail_duplicate.carbon b/toolchain/check/testdata/interface/fail_duplicate.carbon index fa9f26e2862b..3d581b7c70ef 100644 --- a/toolchain/check/testdata/interface/fail_duplicate.carbon +++ b/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: ^~~~~~~~~~~~ diff --git a/toolchain/check/testdata/namespace/fail_import_of_repeat.carbon b/toolchain/check/testdata/namespace/fail_import_of_repeat.carbon index c8bf91812146..227379ec1206 100644 --- a/toolchain/check/testdata/namespace/fail_import_of_repeat.carbon +++ b/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: ^~~~~~~~~~~~~