From da86a1bbb956a69c62f4741123ca8de597659bb0 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 2 Apr 2024 04:21:47 -0700 Subject: [PATCH] Start attempting ambiguous import merges. (#3827) Just connecting this together. Note the diagnostics still need to be cleaned up/improved, although this is correctly handling some cases. This exposed an incorrect parameter passed in DiagnosticEmitter, which is fixed here -- context_fn shouldn't include args. Note this was caught by the CHECK, I'm just expanding on the message since I used the detail to help figure out the bug. --- toolchain/check/context.cpp | 6 ++--- .../class/cross_package_import.carbon | 18 +++++-------- .../packages/cross_package_import.carbon | 26 ++++--------------- toolchain/diagnostics/diagnostic_emitter.h | 11 +++++--- 4 files changed, 21 insertions(+), 40 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 4b6b69872216..8c40b4c19e9f 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -14,6 +14,7 @@ #include "toolchain/check/eval.h" #include "toolchain/check/import_ref.h" #include "toolchain/check/inst_block_stack.h" +#include "toolchain/check/merge.h" #include "toolchain/diagnostics/diagnostic_emitter.h" #include "toolchain/lex/tokenized_buffer.h" #include "toolchain/parse/node_ids.h" @@ -337,11 +338,10 @@ static auto LookupInImportIRScopes(Context& context, SemIRLoc loc, continue; } auto import_inst_id = context.AddImportRef(import_ir_id, it->second); - TryResolveImportRefUnused(context, import_inst_id); if (result_id.is_valid()) { - // TODO: Add generalized merge functionality (merge_decls.h?). - context.DiagnoseDuplicateName(import_inst_id, result_id); + MergeImportRef(context, import_inst_id, result_id); } else { + TryResolveImportRefUnused(context, import_inst_id); result_id = import_inst_id; } } diff --git a/toolchain/check/testdata/class/cross_package_import.carbon b/toolchain/check/testdata/class/cross_package_import.carbon index 247547f96dea..0188167589ee 100644 --- a/toolchain/check/testdata/class/cross_package_import.carbon +++ b/toolchain/check/testdata/class/cross_package_import.carbon @@ -62,18 +62,12 @@ var c: Other.C = {}; library "fail_merge_define_extern" api; import Other library "define"; -// CHECK:STDERR: fail_merge_define_extern.carbon:[[@LINE+12]]:1: In import. +// CHECK:STDERR: fail_merge_define_extern.carbon:[[@LINE+6]]:1: In import. // CHECK:STDERR: import Other library "extern"; // CHECK:STDERR: ^~~~~~ -// CHECK:STDERR: other_extern.carbon:5:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: other_extern.carbon:5:1: ERROR: Semantics TODO: `Merging not yet supported.`. // CHECK:STDERR: class C; // CHECK:STDERR: ^~~~~~~~ -// CHECK:STDERR: fail_merge_define_extern.carbon:[[@LINE-7]]:1: In import. -// CHECK:STDERR: import Other library "define"; -// CHECK:STDERR: ^~~~~~ -// CHECK:STDERR: other_define.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: class C {} -// CHECK:STDERR: ^~~~~~~~~ import Other library "extern"; // CHECK:STDERR: fail_merge_define_extern.carbon:[[@LINE+4]]:8: In name lookup for `C`. @@ -260,10 +254,10 @@ var c: Other.C = {}; // CHECK:STDOUT: // CHECK:STDOUT: fn @__global_init() { // CHECK:STDOUT: !entry: -// CHECK:STDOUT: %.loc23_19.1: {} = struct_literal () -// CHECK:STDOUT: %.loc23_19.2: init C = class_init (), file.%c.var [template = constants.%.4] -// CHECK:STDOUT: %.loc23_19.3: init C = converted %.loc23_19.1, %.loc23_19.2 [template = constants.%.4] -// CHECK:STDOUT: assign file.%c.var, %.loc23_19.3 +// CHECK:STDOUT: %.loc17_19.1: {} = struct_literal () +// CHECK:STDOUT: %.loc17_19.2: init C = class_init (), file.%c.var [template = constants.%.4] +// CHECK:STDOUT: %.loc17_19.3: init C = converted %.loc17_19.1, %.loc17_19.2 [template = constants.%.4] +// CHECK:STDOUT: assign file.%c.var, %.loc17_19.3 // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/check/testdata/packages/cross_package_import.carbon b/toolchain/check/testdata/packages/cross_package_import.carbon index 5ee5e12e2836..823e7cfab0a2 100644 --- a/toolchain/check/testdata/packages/cross_package_import.carbon +++ b/toolchain/check/testdata/packages/cross_package_import.carbon @@ -54,30 +54,14 @@ fn Run() { Other.F2(); } -// --- fail_main_use_other_extern.carbon +// --- main_use_other_extern.carbon library "use_other_extern" api; import Other library "fn"; -// CHECK:STDERR: fail_main_use_other_extern.carbon:[[@LINE+12]]:1: In import. -// CHECK:STDERR: import Other library "fn_extern"; -// CHECK:STDERR: ^~~~~~ -// CHECK:STDERR: other_fn_extern.carbon:4:1: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: extern fn F(); -// CHECK:STDERR: ^~~~~~~~~~~~~~ -// CHECK:STDERR: fail_main_use_other_extern.carbon:[[@LINE-7]]:1: In import. -// CHECK:STDERR: import Other library "fn"; -// CHECK:STDERR: ^~~~~~ -// CHECK:STDERR: other_fn.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: fn F() {} -// CHECK:STDERR: ^~~~~~~~ import Other library "fn_extern"; fn Run() { - // CHECK:STDERR: fail_main_use_other_extern.carbon:[[@LINE+4]]:3: In name lookup for `F`. - // CHECK:STDERR: Other.F(); - // CHECK:STDERR: ^~~~~~~ - // CHECK:STDERR: Other.F(); } @@ -96,13 +80,13 @@ import Other library "fn"; // CHECK:STDERR: fail_main_use_other_ambiguous.carbon:[[@LINE+12]]:1: In import. // CHECK:STDERR: import Other library "fn_conflict"; // CHECK:STDERR: ^~~~~~ -// CHECK:STDERR: other_fn_conflict.carbon:4:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: other_fn_conflict.carbon:4:1: ERROR: Function redeclaration differs because of parameter count of 1. // CHECK:STDERR: fn F(x: i32) {} // CHECK:STDERR: ^~~~~~~~~~~~~~ // CHECK:STDERR: fail_main_use_other_ambiguous.carbon:[[@LINE-7]]:1: In import. // CHECK:STDERR: import Other library "fn"; // CHECK:STDERR: ^~~~~~ -// CHECK:STDERR: other_fn.carbon:4:1: Name is previously declared here. +// CHECK:STDERR: other_fn.carbon:4:1: Previously declared with parameter count of 0. // CHECK:STDERR: fn F() {} // CHECK:STDERR: ^~~~~~~~ import Other library "fn_conflict"; @@ -273,7 +257,7 @@ fn Other.G() {} // CHECK:STDOUT: // CHECK:STDOUT: fn @F2(); // CHECK:STDOUT: -// CHECK:STDOUT: --- fail_main_use_other_extern.carbon +// CHECK:STDOUT: --- main_use_other_extern.carbon // CHECK:STDOUT: // CHECK:STDOUT: constants { // CHECK:STDOUT: %.1: type = tuple_type () [template] @@ -294,7 +278,7 @@ fn Other.G() {} // CHECK:STDOUT: !entry: // CHECK:STDOUT: %Other.ref: = name_ref Other, file.%Other [template = file.%Other] // CHECK:STDOUT: %F.ref: = name_ref F, file.%import_ref.1 [template = imports.%F.1] -// CHECK:STDOUT: %.loc24: init () = call %F.ref() +// CHECK:STDOUT: %.loc8: init () = call %F.ref() // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index cb0481393f72..6cfaf6e147be 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -101,9 +101,10 @@ class DiagnosticEmitter { AddMessageWithDiagnosticLoc( emitter_->converter_->ConvertLoc( loc, - [&](DiagnosticLoc loc, - const Internal::DiagnosticBase<>& diagnostic_base) { - AddMessageWithDiagnosticLoc(loc, diagnostic_base, args); + [&](DiagnosticLoc context_loc, + const Internal::DiagnosticBase<>& context_diagnostic_base) { + AddMessageWithDiagnosticLoc(context_loc, + context_diagnostic_base, {}); }), diagnostic_base, args); } @@ -136,7 +137,9 @@ class DiagnosticEmitter { static auto FormatFn(const DiagnosticMessage& message, std::index_sequence /*indices*/) -> std::string { static_assert(sizeof...(Args) == sizeof...(N), "Invalid template args"); - CARBON_CHECK(message.format_args.size() == sizeof...(Args)); + CARBON_CHECK(message.format_args.size() == sizeof...(Args)) + << "Argument count mismatch on " << message.kind << ": " + << message.format_args.size() << " != " << sizeof...(Args); return llvm::formatv( message.format.data(), llvm::any_cast<