From 3bd7252f29da1ff07f7387569fb90fbecad289f5 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 28 Jan 2025 14:49:03 -0800 Subject: [PATCH] Clean up obsolete import handling in class/function (#4857) Noticed because the `new_loc.inst_id` use would be invalid as-is --- toolchain/check/handle_class.cpp | 20 +++++++++----------- toolchain/check/handle_function.cpp | 12 ++++++------ 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp index 49509d707538..afb5acd4ccf7 100644 --- a/toolchain/check/handle_class.cpp +++ b/toolchain/check/handle_class.cpp @@ -14,6 +14,7 @@ #include "toolchain/check/merge.h" #include "toolchain/check/modifiers.h" #include "toolchain/check/name_component.h" +#include "toolchain/parse/node_ids.h" #include "toolchain/sem_ir/function.h" #include "toolchain/sem_ir/ids.h" #include "toolchain/sem_ir/inst.h" @@ -53,9 +54,8 @@ auto HandleParseNode(Context& context, Parse::ClassIntroducerId node_id) // // If merging is successful, returns true and may update the previous class. // Otherwise, returns false. Prints a diagnostic when appropriate. -static auto MergeClassRedecl(Context& context, SemIRLoc new_loc, - SemIR::Class& new_class, bool new_is_import, - bool new_is_definition, +static auto MergeClassRedecl(Context& context, Parse::AnyClassDeclId node_id, + SemIR::Class& new_class, bool new_is_definition, SemIR::ClassId prev_class_id, SemIR::ImportIRId prev_import_ir_id) -> bool { auto& prev_class = context.classes().Get(prev_class_id); @@ -69,7 +69,7 @@ static auto MergeClassRedecl(Context& context, SemIRLoc new_loc, DiagnoseIfInvalidRedecl( context, Lex::TokenKind::Class, prev_class.name_id, - RedeclInfo(new_class, new_loc, new_is_definition), + RedeclInfo(new_class, node_id, new_is_definition), RedeclInfo(prev_class, prev_loc, prev_class.has_definition_started()), prev_import_ir_id); @@ -87,12 +87,11 @@ static auto MergeClassRedecl(Context& context, SemIRLoc new_loc, prev_class.complete_type_witness_id = new_class.complete_type_witness_id; } - if ((prev_import_ir_id.has_value() && !new_is_import) || + if (prev_import_ir_id.has_value() || (prev_class.is_extern && !new_class.is_extern)) { prev_class.first_owning_decl_id = new_class.first_owning_decl_id; - ReplacePrevInstForMerge( - context, new_class.parent_scope_id, prev_class.name_id, - new_is_import ? new_loc.inst_id : new_class.first_owning_decl_id); + ReplacePrevInstForMerge(context, new_class.parent_scope_id, + prev_class.name_id, new_class.first_owning_decl_id); } return true; } @@ -165,9 +164,8 @@ static auto MergeOrAddName(Context& context, Parse::AnyClassDeclId node_id, // TODO: Fix `extern` logic. It doesn't work correctly, but doesn't seem worth // ripping out because existing code may incrementally help. - if (MergeClassRedecl(context, node_id, class_info, - /*new_is_import=*/false, is_definition, prev_class_id, - prev_import_ir_id)) { + if (MergeClassRedecl(context, node_id, class_info, is_definition, + prev_class_id, prev_import_ir_id)) { // When merging, use the existing entity rather than adding a new one. class_decl.class_id = prev_class_id; class_decl.type_id = prev.type_id(); diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index 5e8e9f73115d..2c170c9454d8 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -77,9 +77,10 @@ static auto DiagnoseModifiers(Context& context, DeclIntroducerState& introducer, // // If merging is successful, returns true and may update the previous function. // Otherwise, returns false. Prints a diagnostic when appropriate. -static auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc, +static auto MergeFunctionRedecl(Context& context, + Parse::AnyFunctionDeclId node_id, SemIR::Function& new_function, - bool new_is_import, bool new_is_definition, + bool new_is_definition, SemIR::FunctionId prev_function_id, SemIR::ImportIRId prev_import_ir_id) -> bool { auto& prev_function = context.functions().Get(prev_function_id); @@ -90,7 +91,7 @@ static auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc, DiagnoseIfInvalidRedecl( context, Lex::TokenKind::Fn, prev_function.name_id, - RedeclInfo(new_function, new_loc, new_is_definition), + RedeclInfo(new_function, node_id, new_is_definition), RedeclInfo(prev_function, prev_function.latest_decl_id(), prev_function.has_definition_started()), prev_import_ir_id); @@ -107,7 +108,7 @@ static auto MergeFunctionRedecl(Context& context, SemIRLoc new_loc, prev_function.MergeDefinition(new_function); prev_function.return_slot_pattern_id = new_function.return_slot_pattern_id; } - if ((prev_import_ir_id.has_value() && !new_is_import)) { + if (prev_import_ir_id.has_value()) { ReplacePrevInstForMerge(context, new_function.parent_scope_id, prev_function.name_id, new_function.first_owning_decl_id); @@ -162,8 +163,7 @@ static auto TryMergeRedecl(Context& context, Parse::AnyFunctionDeclId node_id, return; } - if (MergeFunctionRedecl(context, node_id, function_info, - /*new_is_import=*/false, is_definition, + if (MergeFunctionRedecl(context, node_id, function_info, is_definition, prev_function_id, prev_import_ir_id)) { // When merging, use the existing function rather than adding a new one. function_decl.function_id = prev_function_id;