From 6c458ffe7e2fa0aa862da44b97bd1220e3c40604 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 27 Mar 2024 15:22:15 -0700 Subject: [PATCH] Add import context for locations. (#3807) As discussed around #3792, identify the import a diagnostic message came from prior to the diagnostic message itself. This occurs during location translation so that the logic can be central. I'd considered associating the parse node with ImportRef instructions, but I realized about halfway through that because I need to store the ImportDirectiveId on the ImportIR for cross-package imports, it's there for use in location translation without extra work. That saves a fair amount of stringing it through declarations, as well as an oddity where ImportRef instructions would have a node that didn't really represent them. --- toolchain/check/check.cpp | 31 ++++++---- toolchain/check/context.cpp | 10 ++-- toolchain/check/import.cpp | 26 ++++---- toolchain/check/import.h | 8 ++- toolchain/check/import_ref.cpp | 5 +- .../class/cross_package_import.carbon | 60 +++++++++++-------- .../testdata/class/fail_import_misuses.carbon | 5 +- .../fail_todo_import_forward_decl.carbon | 5 +- .../fail_todo_import_forward_decl.carbon | 7 ++- ...l_conflict_imported_namespace_first.carbon | 7 ++- ..._conflict_imported_namespace_second.carbon | 9 ++- ...conflict_in_imports_namespace_first.carbon | 18 ++++-- ...onflict_in_imports_namespace_second.carbon | 18 ++++-- .../packages/cross_package_import.carbon | 54 +++++++++++------ .../fail_conflict_no_namespaces.carbon | 18 ++++-- toolchain/diagnostics/diagnostic_converter.h | 11 +++- toolchain/diagnostics/diagnostic_emitter.h | 30 ++++++++-- .../diagnostics/diagnostic_emitter_test.cpp | 3 +- toolchain/diagnostics/diagnostic_kind.def | 3 + toolchain/diagnostics/null_diagnostics.h | 6 +- .../sorting_diagnostic_consumer_test.cpp | 2 +- toolchain/lex/test_helpers.h | 3 +- toolchain/lex/tokenized_buffer.cpp | 7 ++- toolchain/lex/tokenized_buffer.h | 6 +- .../parse/tree_node_diagnostic_converter.h | 9 +-- toolchain/sem_ir/file.cpp | 15 +++-- toolchain/sem_ir/file.h | 11 ++++ toolchain/sem_ir/ids.h | 3 +- toolchain/source/source_buffer.cpp | 3 +- 29 files changed, 263 insertions(+), 130 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index ce5577cd9707..f1835605fe97 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -36,10 +36,11 @@ class SemIRDiagnosticConverter : public DiagnosticConverter { const SemIR::File* sem_ir) : node_converters_(node_converters), sem_ir_(sem_ir) {} - auto ConvertLocation(SemIRLocation loc) const -> DiagnosticLocation override { + auto ConvertLocation(SemIRLocation loc, ContextFnT context_fn) const + -> DiagnosticLocation override { // Parse nodes always refer to the current IR. if (!loc.is_inst_id) { - return ConvertLocationInFile(sem_ir_, loc.node_location); + return ConvertLocationInFile(sem_ir_, loc.node_location, context_fn); } const auto* cursor_ir = sem_ir_; @@ -48,14 +49,19 @@ class SemIRDiagnosticConverter : public DiagnosticConverter { // If the parse node is valid, use it for the location. if (auto node_id = cursor_ir->insts().GetNodeId(cursor_inst_id); node_id.is_valid()) { - return ConvertLocationInFile(cursor_ir, node_id); + return ConvertLocationInFile(cursor_ir, node_id, context_fn); } // If the parse node was invalid, recurse through import references when // possible. if (auto import_ref = cursor_ir->insts().TryGetAs( cursor_inst_id)) { - cursor_ir = cursor_ir->import_irs().Get(import_ref->ir_id); + const auto& import_ir = cursor_ir->import_irs().Get(import_ref->ir_id); + auto context_loc = + ConvertLocationInFile(cursor_ir, import_ir.node_id, context_fn); + CARBON_DIAGNOSTIC(InImport, Note, "In import."); + context_fn(context_loc, InImport); + cursor_ir = import_ir.sem_ir; cursor_inst_id = import_ref->inst_id; continue; } @@ -71,7 +77,8 @@ class SemIRDiagnosticConverter : public DiagnosticConverter { } // Invalid parse node but not an import; just nothing to point at. - return ConvertLocationInFile(cursor_ir, Parse::NodeId::Invalid); + return ConvertLocationInFile(cursor_ir, Parse::NodeId::Invalid, + context_fn); } } @@ -91,11 +98,12 @@ class SemIRDiagnosticConverter : public DiagnosticConverter { private: auto ConvertLocationInFile(const SemIR::File* sem_ir, - Parse::NodeLocation node_location) const + Parse::NodeLocation node_location, + ContextFnT context_fn) const -> DiagnosticLocation { auto it = node_converters_->find(sem_ir); CARBON_CHECK(it != node_converters_->end()); - return it->second->ConvertLocation(node_location); + return it->second->ConvertLocation(node_location, context_fn); } const llvm::DenseMap* @@ -191,7 +199,7 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info) for (const auto& import : self_import->second.imports) { const auto& import_sem_ir = **import.unit_info->unit->sem_ir; ImportLibraryFromCurrentPackage(context, namespace_type_id, - import_sem_ir); + import.names.node_id, import_sem_ir); error_in_import |= import_sem_ir.name_scopes() .Get(SemIR::NameScopeId::Package) .has_error; @@ -216,13 +224,14 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info) continue; } - llvm::SmallVector sem_irs; + llvm::SmallVector import_irs; for (auto import : package_imports.imports) { - sem_irs.push_back(&**import.unit_info->unit->sem_ir); + import_irs.push_back({.node_id = import.names.node_id, + .sem_ir = &**import.unit_info->unit->sem_ir}); } ImportLibrariesFromOtherPackage(context, namespace_type_id, package_imports.node_id, package_id, - sem_irs, package_imports.has_load_error); + import_irs, package_imports.has_load_error); } CARBON_CHECK(context.import_irs().size() == num_irs) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 8a055777d7c7..bdb3a61c2542 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -137,7 +137,7 @@ auto Context::ReplaceInstBeforeConstantUse( auto Context::AddImportRef(SemIR::ImportIRId ir_id, SemIR::InstId inst_id) -> SemIR::InstId { auto import_ref_id = - AddPlaceholderInstInNoBlock(SemIR::ImportRefUnused{ir_id, inst_id}); + AddPlaceholderInstInNoBlock({SemIR::ImportRefUnused{ir_id, inst_id}}); // We can't insert this instruction into whatever block we happen to be in, // because this function is typically called by name lookup in the middle of @@ -303,7 +303,8 @@ static auto LookupInImportIRScopes(Context& context, SemIRLocation loc, // Determine the NameId in the import IR. SemIR::NameId import_name_id = name_id; if (identifier_id.is_valid()) { - auto import_identifier_id = import_ir->identifiers().Lookup(identifier); + auto import_identifier_id = + import_ir.sem_ir->identifiers().Lookup(identifier); if (!import_identifier_id.is_valid()) { // Name doesn't exist in the import IR. continue; @@ -312,7 +313,8 @@ static auto LookupInImportIRScopes(Context& context, SemIRLocation loc, } // Look up the name in the import scope. - const auto& import_scope = import_ir->name_scopes().Get(import_scope_id); + const auto& import_scope = + import_ir.sem_ir->name_scopes().Get(import_scope_id); auto it = import_scope.names.find(import_name_id); if (it == import_scope.names.end()) { // Name doesn't exist in the import scope. @@ -742,7 +744,7 @@ class TypeCompleter { auto BuildImportRefUsedValueRepr(SemIR::TypeId type_id, SemIR::ImportRefUsed import_ref) const -> SemIR::ValueRepr { - const auto& import_ir = context_.import_irs().Get(import_ref.ir_id); + const auto& import_ir = context_.import_irs().Get(import_ref.ir_id).sem_ir; auto import_inst = import_ir->insts().Get(import_ref.inst_id); CARBON_CHECK(import_inst.kind() != SemIR::InstKind::ImportRefUsed) << "If ImportRefUsed can point at another, this would be recursive."; diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 1425e640436e..68ae266d76df 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -130,12 +130,11 @@ static auto CacheCopiedNamespace( // name conflicts, but that won't change the result because namespaces supersede // other names in conflicts. static auto CopySingleNameScopeFromImportIR( - Context& context, + Context& context, SemIR::TypeId namespace_type_id, llvm::DenseMap& copied_namespaces, SemIR::ImportIRId ir_id, SemIR::InstId import_inst_id, SemIR::NameScopeId import_scope_id, SemIR::NameScopeId enclosing_scope_id, - SemIR::NameId name_id, SemIR::TypeId namespace_type_id) - -> SemIR::NameScopeId { + SemIR::NameId name_id) -> SemIR::NameScopeId { // Produce the namespace for the entry. auto make_import_id = [&]() { return context.AddInst(SemIR::ImportRefUsed{.type_id = namespace_type_id, @@ -199,8 +198,8 @@ static auto CopyEnclosingNameScopesFromImportIR( auto name_id = CopyNameFromImportIR(context, import_sem_ir, import_scope.name_id); scope_cursor = CopySingleNameScopeFromImportIR( - context, copied_namespaces, ir_id, import_scope.inst_id, - import_scope_id, scope_cursor, name_id, namespace_type_id); + context, namespace_type_id, copied_namespaces, ir_id, + import_scope.inst_id, import_scope_id, scope_cursor, name_id); } return scope_cursor; @@ -208,8 +207,10 @@ static auto CopyEnclosingNameScopesFromImportIR( auto ImportLibraryFromCurrentPackage(Context& context, SemIR::TypeId namespace_type_id, + Parse::ImportDirectiveId node_id, const SemIR::File& import_sem_ir) -> void { - auto ir_id = context.import_irs().Add(&import_sem_ir); + auto ir_id = + context.import_irs().Add({.node_id = node_id, .sem_ir = &import_sem_ir}); context.import_ir_constant_values()[ir_id.index].Set( SemIR::InstId::PackageNamespace, context.constant_values().Get(SemIR::InstId::PackageNamespace)); @@ -236,9 +237,8 @@ auto ImportLibraryFromCurrentPackage(Context& context, // Namespaces are always imported because they're essential for // qualifiers, and the type is simple. CopySingleNameScopeFromImportIR( - context, copied_namespaces, ir_id, import_inst_id, - import_namespace_inst->name_scope_id, enclosing_scope_id, name_id, - namespace_type_id); + context, namespace_type_id, copied_namespaces, ir_id, import_inst_id, + import_namespace_inst->name_scope_id, enclosing_scope_id, name_id); } else { // Leave a placeholder that the inst comes from the other IR. auto target_id = context.AddImportRef(ir_id, import_inst_id); @@ -260,9 +260,9 @@ auto ImportLibrariesFromOtherPackage(Context& context, SemIR::TypeId namespace_type_id, Parse::ImportDirectiveId node_id, IdentifierId package_id, - llvm::ArrayRef sem_irs, + llvm::ArrayRef import_irs, bool has_load_error) -> void { - CARBON_CHECK(has_load_error || !sem_irs.empty()) + CARBON_CHECK(has_load_error || !import_irs.empty()) << "There should be either a load error or at least one IR."; auto name_id = SemIR::NameId::ForIdentifier(package_id); @@ -273,8 +273,8 @@ auto ImportLibrariesFromOtherPackage(Context& context, auto& scope = context.name_scopes().Get(namespace_scope_id); scope.is_closed_import = !is_duplicate; - for (const auto* sem_ir : sem_irs) { - auto ir_id = context.import_irs().Add(sem_ir); + for (auto import_ir : import_irs) { + auto ir_id = context.import_irs().Add(import_ir); scope.import_ir_scopes.push_back({ir_id, SemIR::NameScopeId::Package}); context.import_ir_constant_values()[ir_id.index].Set( SemIR::InstId::PackageNamespace, namespace_const_id); diff --git a/toolchain/check/import.h b/toolchain/check/import.h index 5f4849d92650..e680a3f31545 100644 --- a/toolchain/check/import.h +++ b/toolchain/check/import.h @@ -6,6 +6,7 @@ #define CARBON_TOOLCHAIN_CHECK_IMPORT_H_ #include "toolchain/check/context.h" +#include "toolchain/parse/node_ids.h" #include "toolchain/sem_ir/file.h" namespace Carbon::Check { @@ -15,6 +16,7 @@ namespace Carbon::Check { // though they are several layers deep. auto ImportLibraryFromCurrentPackage(Context& context, SemIR::TypeId namespace_type_id, + Parse::ImportDirectiveId node_id, const SemIR::File& import_sem_ir) -> void; // Adds another package's imports to name lookup, with all libraries together. @@ -22,13 +24,13 @@ auto ImportLibraryFromCurrentPackage(Context& context, // will resolve, and will provide a name scope that can be used for further // qualified name lookups. // -// sem_irs will all be non-null, and may be empty. has_load_error is used to -// indicate if any library in the package failed to import correctly. +// import_irs may be empty. has_load_error is used to indicate if any library in +// the package failed to import correctly. auto ImportLibrariesFromOtherPackage(Context& context, SemIR::TypeId namespace_type_id, Parse::ImportDirectiveId node_id, IdentifierId package_id, - llvm::ArrayRef sem_irs, + llvm::ArrayRef import_irs, bool has_load_error) -> void; } // namespace Carbon::Check diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index e77c4757f179..492bdff7f526 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -57,7 +57,7 @@ class ImportRefResolver { explicit ImportRefResolver(Context& context, SemIR::ImportIRId import_ir_id) : context_(context), import_ir_id_(import_ir_id), - import_ir_(*context_.import_irs().Get(import_ir_id)), + import_ir_(*context_.import_irs().Get(import_ir_id).sem_ir), import_ir_constant_values_( context_.import_ir_constant_values()[import_ir_id.index]) {} @@ -912,7 +912,8 @@ auto TryResolveImportRefUnused(Context& context, SemIR::InstId inst_id) return; } - const SemIR::File& import_ir = *context.import_irs().Get(import_ref->ir_id); + const SemIR::File& import_ir = + *context.import_irs().Get(import_ref->ir_id).sem_ir; auto import_inst = import_ir.insts().Get(import_ref->inst_id); ImportRefResolver resolver(context, import_ref->ir_id); diff --git a/toolchain/check/testdata/class/cross_package_import.carbon b/toolchain/check/testdata/class/cross_package_import.carbon index b8844eaaf66a..b185436797e1 100644 --- a/toolchain/check/testdata/class/cross_package_import.carbon +++ b/toolchain/check/testdata/class/cross_package_import.carbon @@ -45,17 +45,11 @@ library "extern" api; import Other library "extern"; -// CHECK:STDERR: fail_extern.carbon:[[@LINE+11]]:8: ERROR: Variable has incomplete type `C`. +// CHECK:STDERR: fail_extern.carbon:[[@LINE+5]]:8: ERROR: Variable has incomplete type `C`. // CHECK:STDERR: var c: Other.C = {}; // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: fail_extern.carbon: Class was forward declared here. // CHECK:STDERR: -// CHECK:STDERR: other_extern.carbon:5:1: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: class C; -// CHECK:STDERR: ^~~~~~~~ -// CHECK:STDERR: other_define.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: class C {} -// CHECK:STDERR: ^~~~~~~~~ var c: Other.C = {}; // --- fail_merge_define_extern.carbon @@ -63,18 +57,24 @@ var c: Other.C = {}; library "fail_merge_define_extern" api; import Other library "define"; -import Other library "extern"; - -// CHECK:STDERR: fail_merge_define_extern.carbon:[[@LINE+10]]:8: In name lookup for `C`. -// CHECK:STDERR: var c: Other.C = {}; -// CHECK:STDERR: ^~~~~~~ -// CHECK:STDERR: -// CHECK:STDERR: other_conflict.carbon:4:1: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: fn C() {} +// CHECK:STDERR: fail_merge_define_extern.carbon:[[@LINE+12]]: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: 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`. +// CHECK:STDERR: var c: Other.C = {}; +// CHECK:STDERR: ^~~~~~~ +// CHECK:STDERR: var c: Other.C = {}; // --- fail_conflict.carbon @@ -82,6 +82,18 @@ var c: Other.C = {}; library "conflict" api; import Other library "define"; +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+12]]:1: In import. +// CHECK:STDERR: import Other library "conflict"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: other_conflict.carbon:4:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fn C() {} +// CHECK:STDERR: ^~~~~~~~ +// CHECK:STDERR: fail_conflict.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 "conflict"; // CHECK:STDERR: fail_conflict.carbon:[[@LINE+3]]:8: In name lookup for `C`. @@ -201,7 +213,7 @@ var c: Other.C = {}; // CHECK:STDOUT: // CHECK:STDOUT: fn @__global_init() { // CHECK:STDOUT: !entry: -// CHECK:STDOUT: %.loc17: {} = struct_literal () +// CHECK:STDOUT: %.loc11: {} = struct_literal () // CHECK:STDOUT: assign file.%c.var, // CHECK:STDOUT: return // CHECK:STDOUT: } @@ -243,10 +255,10 @@ var c: Other.C = {}; // CHECK:STDOUT: // CHECK:STDOUT: fn @__global_init() { // CHECK:STDOUT: !entry: -// 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: %.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: return // CHECK:STDOUT: } // CHECK:STDOUT: @@ -285,10 +297,10 @@ var c: Other.C = {}; // CHECK:STDOUT: // CHECK:STDOUT: fn @__global_init() { // CHECK:STDOUT: !entry: -// CHECK:STDOUT: %.loc10_19.1: {} = struct_literal () -// CHECK:STDOUT: %.loc10_19.2: init C = class_init (), file.%c.var [template = constants.%.4] -// CHECK:STDOUT: %.loc10_19.3: init C = converted %.loc10_19.1, %.loc10_19.2 [template = constants.%.4] -// CHECK:STDOUT: assign file.%c.var, %.loc10_19.3 +// CHECK:STDOUT: %.loc22_19.1: {} = struct_literal () +// CHECK:STDOUT: %.loc22_19.2: init C = class_init (), file.%c.var [template = constants.%.4] +// CHECK:STDOUT: %.loc22_19.3: init C = converted %.loc22_19.1, %.loc22_19.2 [template = constants.%.4] +// CHECK:STDOUT: assign file.%c.var, %.loc22_19.3 // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/check/testdata/class/fail_import_misuses.carbon b/toolchain/check/testdata/class/fail_import_misuses.carbon index 62dee713bced..2edb61eec4eb 100644 --- a/toolchain/check/testdata/class/fail_import_misuses.carbon +++ b/toolchain/check/testdata/class/fail_import_misuses.carbon @@ -19,9 +19,12 @@ library "b" api; import library "a"; -// CHECK:STDERR: fail_b.carbon:[[@LINE+7]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_b.carbon:[[@LINE+10]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: class Empty { // CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: fail_b.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ // CHECK:STDERR: a.carbon:4:1: Name is previously declared here. // CHECK:STDERR: class Empty { // CHECK:STDERR: ^~~~~~~~~~~~~ diff --git a/toolchain/check/testdata/class/fail_todo_import_forward_decl.carbon b/toolchain/check/testdata/class/fail_todo_import_forward_decl.carbon index 005a5863393c..f33152fdcc7f 100644 --- a/toolchain/check/testdata/class/fail_todo_import_forward_decl.carbon +++ b/toolchain/check/testdata/class/fail_todo_import_forward_decl.carbon @@ -17,9 +17,12 @@ library "b" api; import library "a"; // TODO: This should probably have a valid form. -// CHECK:STDERR: fail_b.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_b.carbon:[[@LINE+9]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: class ForwardDecl { // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~ +// CHECK:STDERR: fail_b.carbon:[[@LINE-6]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ // CHECK:STDERR: a.carbon:4:1: Name is previously declared here. // CHECK:STDERR: class ForwardDecl; // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~ diff --git a/toolchain/check/testdata/function/definition/fail_todo_import_forward_decl.carbon b/toolchain/check/testdata/function/definition/fail_todo_import_forward_decl.carbon index 1d89ea3f3a03..06008179cc44 100644 --- a/toolchain/check/testdata/function/definition/fail_todo_import_forward_decl.carbon +++ b/toolchain/check/testdata/function/definition/fail_todo_import_forward_decl.carbon @@ -18,9 +18,12 @@ import library "a"; // TODO: This should be an error until we can be clear whether `a` or `b` should // own the definition, as there might be a separate definition in `a`'s impl. -// CHECK:STDERR: fail_b.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_b.carbon:[[@LINE+9]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: fn Foo() {} // CHECK:STDERR: ^~~~~~~~~~ +// CHECK:STDERR: fail_b.carbon:[[@LINE-7]]:1: In import. +// CHECK:STDERR: import library "a"; +// CHECK:STDERR: ^~~~~~ // CHECK:STDERR: a.carbon:4:1: Name is previously declared here. // CHECK:STDERR: fn Foo(); // CHECK:STDERR: ^~~~~~~~~ @@ -44,7 +47,7 @@ fn Foo() {} // CHECK:STDOUT: .Foo = %import_ref // CHECK:STDOUT: } // CHECK:STDOUT: %import_ref: = import_ref ir1, inst+1, used [template = imports.%Foo] -// CHECK:STDOUT: %.loc14: = fn_decl @.1 [template] {} +// CHECK:STDOUT: %.loc17: = fn_decl @.1 [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: fn @Foo(); diff --git a/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_first.carbon b/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_first.carbon index 9b91dd6ebbdf..86a481fa02ef 100644 --- a/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_first.carbon +++ b/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_first.carbon @@ -16,9 +16,12 @@ package Example api; import library "namespace"; -// CHECK:STDERR: fail_conflict.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+9]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: fn NS(); // CHECK:STDERR: ^~~~~~~~ +// CHECK:STDERR: fail_conflict.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import library "namespace"; +// CHECK:STDERR: ^~~~~~ // CHECK:STDERR: namespace.carbon:4:1: Name is previously declared here. // CHECK:STDERR: namespace NS; // CHECK:STDERR: ^~~~~~~~~~~~~ @@ -45,7 +48,7 @@ fn NS.Foo(); // CHECK:STDOUT: %NS: = namespace %import_ref, [template] { // CHECK:STDOUT: .Foo = %Foo // CHECK:STDOUT: } -// CHECK:STDOUT: %.loc12: = fn_decl @.1 [template] {} +// CHECK:STDOUT: %.loc15: = fn_decl @.1 [template] {} // CHECK:STDOUT: %Foo: = fn_decl @Foo [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon b/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon index 879b9e1d0dec..69a34bc18c04 100644 --- a/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon +++ b/toolchain/check/testdata/namespace/fail_conflict_imported_namespace_second.carbon @@ -16,9 +16,12 @@ package Example api; import library "fn"; -// CHECK:STDERR: fail_conflict.carbon:[[@LINE+7]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+10]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: namespace NS; // CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: fail_conflict.carbon:[[@LINE-5]]:1: In import. +// CHECK:STDERR: import library "fn"; +// CHECK:STDERR: ^~~~~~ // CHECK:STDERR: fn.carbon:4:1: Name is previously declared here. // CHECK:STDERR: fn NS(); // CHECK:STDERR: ^~~~~~~~ @@ -51,8 +54,8 @@ fn NS.Foo(); // CHECK:STDOUT: .NS = %import_ref // CHECK:STDOUT: } // CHECK:STDOUT: %import_ref: = import_ref ir1, inst+1, used [template = imports.%NS] -// CHECK:STDOUT: %.loc13: = namespace [template] {} -// CHECK:STDOUT: %.loc21: = fn_decl @.1 [template] {} +// CHECK:STDOUT: %.loc16: = namespace [template] {} +// CHECK:STDOUT: %.loc24: = fn_decl @.1 [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: fn @NS(); diff --git a/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_first.carbon b/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_first.carbon index d520e5be005d..9913595042dc 100644 --- a/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_first.carbon +++ b/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_first.carbon @@ -15,12 +15,6 @@ fn NS.Foo() {} package Example library "fn" api; -// CHECK:STDERR: fn.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: fn NS() {} -// CHECK:STDERR: ^~~~~~~~~ -// CHECK:STDERR: namespace.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: namespace NS; -// CHECK:STDERR: ^~~~~~~~~~~~~ fn NS() {} // --- fail_conflict.carbon @@ -28,6 +22,18 @@ fn NS() {} package Example library "namespace_then_fn" api; import library "namespace"; +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+12]]:1: In import. +// CHECK:STDERR: import library "fn"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: fn.carbon:4:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fn NS() {} +// CHECK:STDERR: ^~~~~~~~~ +// CHECK:STDERR: fail_conflict.carbon:[[@LINE-7]]:1: In import. +// CHECK:STDERR: import library "namespace"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: namespace.carbon:4:1: Name is previously declared here. +// CHECK:STDERR: namespace NS; +// CHECK:STDERR: ^~~~~~~~~~~~~ import library "fn"; fn NS.Bar() {} diff --git a/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_second.carbon b/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_second.carbon index c43c641f1cef..0b013c38dcdb 100644 --- a/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_second.carbon +++ b/toolchain/check/testdata/namespace/fail_conflict_in_imports_namespace_second.carbon @@ -14,12 +14,6 @@ fn NS() {} package Example library "namespace" api; -// CHECK:STDERR: namespace.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: namespace NS; -// CHECK:STDERR: ^~~~~~~~~~~~~ -// CHECK:STDERR: fn.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: fn NS() {} -// CHECK:STDERR: ^~~~~~~~~ namespace NS; fn NS.Foo() {} @@ -28,6 +22,18 @@ fn NS.Foo() {} package Example library "fn_then_namespace" api; import library "fn"; +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+12]]:1: In import. +// CHECK:STDERR: import library "namespace"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: namespace.carbon:4:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: namespace NS; +// CHECK:STDERR: ^~~~~~~~~~~~~ +// CHECK:STDERR: fail_conflict.carbon:[[@LINE-7]]:1: In import. +// CHECK:STDERR: import library "fn"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: fn.carbon:4:1: Name is previously declared here. +// CHECK:STDERR: fn NS() {} +// CHECK:STDERR: ^~~~~~~~~ import library "namespace"; fn NS.Bar() {} diff --git a/toolchain/check/testdata/packages/cross_package_import.carbon b/toolchain/check/testdata/packages/cross_package_import.carbon index f36bda608968..1d9e28be561a 100644 --- a/toolchain/check/testdata/packages/cross_package_import.carbon +++ b/toolchain/check/testdata/packages/cross_package_import.carbon @@ -19,12 +19,6 @@ fn F() {} package Other library "fn_extern" api; // TODO: Mark extern -// CHECK:STDERR: other_fn_extern.carbon:[[@LINE+6]]:1: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: fn F(); -// CHECK:STDERR: ^~~~~~~ -// CHECK:STDERR: other_fn.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: fn F() {} -// CHECK:STDERR: ^~~~~~~~ fn F(); // --- other_fn_conflict.carbon @@ -66,19 +60,25 @@ fn Run() { 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:5:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: 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+10]]:3: In name lookup for `F`. + // CHECK:STDERR: fail_main_use_other_extern.carbon:[[@LINE+4]]:3: In name lookup for `F`. // CHECK:STDERR: Other.F(); // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: - // CHECK:STDERR: other_fn_conflict.carbon:4:1: ERROR: Duplicate name being declared in the same scope. - // CHECK:STDERR: fn F(x: i32) {} - // CHECK:STDERR: ^~~~~~~~~~~~~~ - // CHECK:STDERR: other_fn.carbon:4:1: Name is previously declared here. - // CHECK:STDERR: fn F() {} - // CHECK:STDERR: ^~~~~~~~ Other.F(); } @@ -94,6 +94,18 @@ import Other library "fn_conflict"; library "use_other_ambiguous" api; 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: 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: fn F() {} +// CHECK:STDERR: ^~~~~~~~ import Other library "fn_conflict"; fn Run() { @@ -109,18 +121,24 @@ fn Run() { library "namespace_conflict" api; import library "other_ns"; -// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE+7]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE+10]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: import Other library "fn"; // CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE-4]]:1: In import. +// CHECK:STDERR: import library "other_ns"; +// CHECK:STDERR: ^~~~~~ // CHECK:STDERR: main_other_ns.carbon:4:1: Name is previously declared here. // CHECK:STDERR: namespace Other; // CHECK:STDERR: ^~~~~~~~~~~~~~~~ // CHECK:STDERR: import Other library "fn"; -// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE+7]]:1: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE+10]]:1: ERROR: Duplicate name being declared in the same scope. // CHECK:STDERR: fn Other.F() {} // CHECK:STDERR: ^~~~~~~~~~~~~~ +// CHECK:STDERR: fail_main_namespace_conflict.carbon:[[@LINE-5]]: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: ^~~~~~~~ @@ -277,7 +295,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: %.loc18: init () = call %F.ref() +// CHECK:STDOUT: %.loc24: init () = call %F.ref() // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: @@ -315,7 +333,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: %.loc12: init () = call %F.ref() +// CHECK:STDOUT: %.loc24: init () = call %F.ref() // CHECK:STDOUT: return // CHECK:STDOUT: } // CHECK:STDOUT: @@ -332,7 +350,7 @@ fn Other.G() {} // CHECK:STDOUT: %import_ref.1: = import_ref ir1, inst+1, used // CHECK:STDOUT: %Other: = namespace %import_ref.1, [template] {} // CHECK:STDOUT: %import_ref.2: = import_ref ir2, inst+1, used [template = imports.%F] -// CHECK:STDOUT: %.loc21: = fn_decl @.1 [template] {} +// CHECK:STDOUT: %.loc27: = fn_decl @.1 [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: fn @F(); diff --git a/toolchain/check/testdata/packages/fail_conflict_no_namespaces.carbon b/toolchain/check/testdata/packages/fail_conflict_no_namespaces.carbon index feb2b8aec7b2..ad3927d9f082 100644 --- a/toolchain/check/testdata/packages/fail_conflict_no_namespaces.carbon +++ b/toolchain/check/testdata/packages/fail_conflict_no_namespaces.carbon @@ -14,12 +14,6 @@ fn Foo(); package Example library "var" api; -// CHECK:STDERR: var.carbon:[[@LINE+6]]:5: ERROR: Duplicate name being declared in the same scope. -// CHECK:STDERR: var Foo: i32; -// CHECK:STDERR: ^~~ -// CHECK:STDERR: fn.carbon:4:1: Name is previously declared here. -// CHECK:STDERR: fn Foo(); -// CHECK:STDERR: ^~~~~~~~~ var Foo: i32; // --- fail_conflict.carbon @@ -27,6 +21,18 @@ var Foo: i32; package Example library "conflict" api; import library "fn"; +// CHECK:STDERR: fail_conflict.carbon:[[@LINE+12]]:1: In import. +// CHECK:STDERR: import library "var"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: var.carbon:4:5: ERROR: Duplicate name being declared in the same scope. +// CHECK:STDERR: var Foo: i32; +// CHECK:STDERR: ^~~ +// CHECK:STDERR: fail_conflict.carbon:[[@LINE-7]]:1: In import. +// CHECK:STDERR: import library "fn"; +// CHECK:STDERR: ^~~~~~ +// CHECK:STDERR: fn.carbon:4:1: Name is previously declared here. +// CHECK:STDERR: fn Foo(); +// CHECK:STDERR: ^~~~~~~~~ import library "var"; // CHECK:STDOUT: --- fn.carbon diff --git a/toolchain/diagnostics/diagnostic_converter.h b/toolchain/diagnostics/diagnostic_converter.h index ebc28e033e3f..e043ce56f9d3 100644 --- a/toolchain/diagnostics/diagnostic_converter.h +++ b/toolchain/diagnostics/diagnostic_converter.h @@ -15,9 +15,18 @@ namespace Carbon { template class DiagnosticConverter { public: + // Callback type used to report context messages from ConvertLocation. + // Note that the first parameter type is DiagnosticLocation rather than + // LocationT, because ConvertLocation must not recurse. + using ContextFnT = llvm::function_ref&)>; + virtual ~DiagnosticConverter() = default; - virtual auto ConvertLocation(LocationT loc) const -> DiagnosticLocation = 0; + // Converts a LocationT to a DiagnosticLocation. ConvertLocation may invoke + // context_fn to provide context messages. + virtual auto ConvertLocation(LocationT loc, ContextFnT context_fn) const + -> DiagnosticLocation = 0; // Converts arg types as needed. Not all uses require conversion, so the // default returns the argument unchanged. diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index c27ff91ad024..6ca20c3761b1 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -65,8 +65,7 @@ class DiagnosticEmitter { Internal::NoTypeDeduction... args) -> DiagnosticBuilder& { CARBON_CHECK(diagnostic_base.Level == DiagnosticLevel::Note) << static_cast(diagnostic_base.Level); - AddMessage(emitter_, location, diagnostic_base, - {emitter_->MakeAny(args)...}); + AddMessage(location, diagnostic_base, {emitter_->MakeAny(args)...}); return *this; } @@ -89,18 +88,39 @@ class DiagnosticEmitter { const Internal::DiagnosticBase& diagnostic_base, llvm::SmallVector args) : emitter_(emitter), diagnostic_({.level = diagnostic_base.Level}) { - AddMessage(emitter, location, diagnostic_base, std::move(args)); + AddMessage(location, diagnostic_base, std::move(args)); CARBON_CHECK(diagnostic_base.Level != DiagnosticLevel::Note); } + // Adds a message to the diagnostic, handling conversion of the location and + // arguments. template - auto AddMessage(DiagnosticEmitter* emitter, LocationT location, + auto AddMessage(LocationT location, const Internal::DiagnosticBase& diagnostic_base, llvm::SmallVector args) -> void { + AddMessageWithDiagnosticLocation( + emitter_->converter_->ConvertLocation( + location, + [&](DiagnosticLocation location, + const Internal::DiagnosticBase<>& diagnostic_base) { + AddMessageWithDiagnosticLocation(location, diagnostic_base, + args); + }), + diagnostic_base, args); + } + + // Adds a message to the diagnostic, handling conversion of the arguments. A + // DiagnosticLocation must be provided instead of a LocationT in order to + // avoid potential recursion. + template + auto AddMessageWithDiagnosticLocation( + DiagnosticLocation location, + const Internal::DiagnosticBase& diagnostic_base, + llvm::SmallVector args) { diagnostic_.messages.emplace_back(DiagnosticMessage{ .kind = diagnostic_base.Kind, .level = diagnostic_base.Level, - .location = emitter->converter_->ConvertLocation(location), + .location = location, .format = diagnostic_base.Format, .format_args = std::move(args), .format_fn = [](const DiagnosticMessage& message) -> std::string { diff --git a/toolchain/diagnostics/diagnostic_emitter_test.cpp b/toolchain/diagnostics/diagnostic_emitter_test.cpp index c689c62e157e..b68fdb0970fe 100644 --- a/toolchain/diagnostics/diagnostic_emitter_test.cpp +++ b/toolchain/diagnostics/diagnostic_emitter_test.cpp @@ -18,7 +18,8 @@ using ::Carbon::Testing::IsSingleDiagnostic; using testing::ElementsAre; struct FakeDiagnosticConverter : DiagnosticConverter { - auto ConvertLocation(int n) const -> DiagnosticLocation override { + auto ConvertLocation(int n, ContextFnT /*context_fn*/) const + -> DiagnosticLocation override { return {.line_number = 1, .column_number = n}; } }; diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 00823cb05966..728a77d7f8d4 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -127,6 +127,9 @@ CARBON_DIAGNOSTIC_KIND(ExpectedAliasInitializer) CARBON_DIAGNOSTIC_KIND(SemanticsTodo) +// Location context. +CARBON_DIAGNOSTIC_KIND(InImport) + // Package/import checking diagnostics. CARBON_DIAGNOSTIC_KIND(IncorrectExtension) CARBON_DIAGNOSTIC_KIND(IncorrectExtensionImplNote) diff --git a/toolchain/diagnostics/null_diagnostics.h b/toolchain/diagnostics/null_diagnostics.h index c36dcd7cf3bc..5e9c6f8e3f45 100644 --- a/toolchain/diagnostics/null_diagnostics.h +++ b/toolchain/diagnostics/null_diagnostics.h @@ -11,8 +11,10 @@ namespace Carbon { template inline auto NullDiagnosticConverter() -> DiagnosticConverter& { - struct Converter : DiagnosticConverter { - auto ConvertLocation(LocationT /*loc*/) const + struct Converter : public DiagnosticConverter { + auto ConvertLocation( + LocationT /*loc*/, + DiagnosticConverter::ContextFnT /*context_fn*/) const -> DiagnosticLocation override { return {}; } diff --git a/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp b/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp index ef237e3d4ba0..ba3337145c04 100644 --- a/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp +++ b/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp @@ -20,7 +20,7 @@ using ::testing::InSequence; CARBON_DIAGNOSTIC(TestDiagnostic, Error, "{0}", llvm::StringLiteral); struct FakeDiagnosticConverter : DiagnosticConverter { - auto ConvertLocation(DiagnosticLocation loc) const + auto ConvertLocation(DiagnosticLocation loc, ContextFnT /*context_fn*/) const -> DiagnosticLocation override { return loc; } diff --git a/toolchain/lex/test_helpers.h b/toolchain/lex/test_helpers.h index 7d5d6418e3e2..2510a90ad7bd 100644 --- a/toolchain/lex/test_helpers.h +++ b/toolchain/lex/test_helpers.h @@ -24,7 +24,8 @@ class SingleTokenDiagnosticConverter : public DiagnosticConverter { explicit SingleTokenDiagnosticConverter(llvm::StringRef token) : token_(token) {} - auto ConvertLocation(const char* pos) const -> DiagnosticLocation override { + auto ConvertLocation(const char* pos, ContextFnT /*context_fn*/) const + -> DiagnosticLocation override { CARBON_CHECK(StringRefContainsPointer(token_, pos)) << "invalid diagnostic location"; llvm::StringRef prefix = token_.take_front(pos - token_.begin()); diff --git a/toolchain/lex/tokenized_buffer.cpp b/toolchain/lex/tokenized_buffer.cpp index 2af12fb3478d..65984170d463 100644 --- a/toolchain/lex/tokenized_buffer.cpp +++ b/toolchain/lex/tokenized_buffer.cpp @@ -346,7 +346,7 @@ auto TokenIterator::Print(llvm::raw_ostream& output) const -> void { } auto TokenizedBuffer::SourceBufferDiagnosticConverter::ConvertLocation( - const char* loc) const -> DiagnosticLocation { + const char* loc, ContextFnT /*context_fn*/) const -> DiagnosticLocation { CARBON_CHECK(StringRefContainsPointer(buffer_->source_->text(), loc)) << "location not within buffer"; int64_t offset = loc - buffer_->source_->text().begin(); @@ -390,7 +390,8 @@ auto TokenizedBuffer::SourceBufferDiagnosticConverter::ConvertLocation( .column_number = column_number + 1}; } -auto TokenDiagnosticConverter::ConvertLocation(TokenIndex token) const +auto TokenDiagnosticConverter::ConvertLocation(TokenIndex token, + ContextFnT context_fn) const -> DiagnosticLocation { // Map the token location into a position within the source buffer. const auto& token_info = buffer_->GetTokenInfo(token); @@ -403,7 +404,7 @@ auto TokenDiagnosticConverter::ConvertLocation(TokenIndex token) const // is a recovery token that doesn't correspond to the original source? DiagnosticLocation loc = TokenizedBuffer::SourceBufferDiagnosticConverter(buffer_).ConvertLocation( - token_start); + token_start, context_fn); loc.length = buffer_->GetTokenText(token).size(); return loc; } diff --git a/toolchain/lex/tokenized_buffer.h b/toolchain/lex/tokenized_buffer.h index b5bafde87cd0..af30bc8d39f5 100644 --- a/toolchain/lex/tokenized_buffer.h +++ b/toolchain/lex/tokenized_buffer.h @@ -119,7 +119,8 @@ class TokenDiagnosticConverter : public DiagnosticConverter { : buffer_(buffer) {} // Map the given token into a diagnostic location. - auto ConvertLocation(TokenIndex token) const -> DiagnosticLocation override; + auto ConvertLocation(TokenIndex token, ContextFnT context_fn) const + -> DiagnosticLocation override; private: const TokenizedBuffer* buffer_; @@ -255,7 +256,8 @@ class TokenizedBuffer : public Printable { // Map the given position within the source buffer into a diagnostic // location. - auto ConvertLocation(const char* loc) const -> DiagnosticLocation override; + auto ConvertLocation(const char* loc, ContextFnT context_fn) const + -> DiagnosticLocation override; private: const TokenizedBuffer* buffer_; diff --git a/toolchain/parse/tree_node_diagnostic_converter.h b/toolchain/parse/tree_node_diagnostic_converter.h index a6b13019981e..546f3a4a487a 100644 --- a/toolchain/parse/tree_node_diagnostic_converter.h +++ b/toolchain/parse/tree_node_diagnostic_converter.h @@ -44,7 +44,7 @@ class NodeLocationConverter : public DiagnosticConverter { parse_tree_(parse_tree) {} // Map the given token into a diagnostic location. - auto ConvertLocation(NodeLocation node_location) const + auto ConvertLocation(NodeLocation node_location, ContextFnT context_fn) const -> DiagnosticLocation override { // Support the invalid token as a way to emit only the filename, when there // is no line association. @@ -54,7 +54,7 @@ class NodeLocationConverter : public DiagnosticConverter { if (node_location.token_only()) { return token_converter_.ConvertLocation( - parse_tree_->node_token(node_location.node_id())); + parse_tree_->node_token(node_location.node_id()), context_fn); } // Construct a location that encompasses all tokens that descend from this @@ -74,11 +74,12 @@ class NodeLocationConverter : public DiagnosticConverter { } } DiagnosticLocation start_loc = - token_converter_.ConvertLocation(start_token); + token_converter_.ConvertLocation(start_token, context_fn); if (start_token == end_token) { return start_loc; } - DiagnosticLocation end_loc = token_converter_.ConvertLocation(end_token); + DiagnosticLocation end_loc = + token_converter_.ConvertLocation(end_token, context_fn); // For multiline locations we simply return the rest of the line for now // since true multiline locations are not yet supported. if (start_loc.line_number != end_loc.line_number) { diff --git a/toolchain/sem_ir/file.cpp b/toolchain/sem_ir/file.cpp index 1b1ac0d78b37..549337aec3f5 100644 --- a/toolchain/sem_ir/file.cpp +++ b/toolchain/sem_ir/file.cpp @@ -9,6 +9,7 @@ #include "llvm/ADT/SmallVector.h" #include "toolchain/base/value_store.h" #include "toolchain/base/yaml.h" +#include "toolchain/parse/node_ids.h" #include "toolchain/sem_ir/builtin_kind.h" #include "toolchain/sem_ir/ids.h" #include "toolchain/sem_ir/inst.h" @@ -69,7 +70,8 @@ File::File(SharedValueStores& value_stores, std::string filename, inst_blocks_(allocator_), constants_(*this, allocator_) { CARBON_CHECK(builtins != nullptr); - auto builtins_id = import_irs_.Add(builtins); + auto builtins_id = + import_irs_.Add({.node_id = Parse::NodeId::Invalid, .sem_ir = builtins}); CARBON_CHECK(builtins_id == ImportIRId::Builtins) << "Builtins must be the first IR"; @@ -105,8 +107,8 @@ File::File(SharedValueStores& value_stores, std::string filename, for (auto [i, inst] : llvm::enumerate(builtins->insts_.array_ref())) { // We can reuse the type_id from the builtin IR's inst because they're // special-cased values. - insts_.AddInNoBlock({ImportRefUsed{ - inst.type_id(), ImportIRId::Builtins, SemIR::InstId(i)}}); + insts_.AddInNoBlock(ImportRefUsed{ + inst.type_id(), ImportIRId::Builtins, SemIR::InstId(i)}); } }) {} @@ -374,8 +376,9 @@ static auto StringifyTypeExprImpl(const SemIR::File& outer_sem_ir, } case ImportRefUsed::Kind: { auto import_ref = inst.As(); - steps.push_back({.sem_ir = *sem_ir.import_irs().Get(import_ref.ir_id), - .inst_id = import_ref.inst_id}); + steps.push_back( + {.sem_ir = *sem_ir.import_irs().Get(import_ref.ir_id).sem_ir, + .inst_id = import_ref.inst_id}); break; } case InterfaceType::Kind: { @@ -556,7 +559,7 @@ auto GetExprCategory(const File& file, InstId inst_id) -> ExprCategory { case ImportRefUsed::Kind: { auto import_ref = inst.As(); - ir = ir->import_irs().Get(import_ref.ir_id); + ir = ir->import_irs().Get(import_ref.ir_id).sem_ir; inst_id = import_ref.inst_id; continue; } diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index 21da089b019e..038d2d5abd2f 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -38,6 +38,17 @@ struct BindNameInfo : public Printable { NameScopeId enclosing_scope_id; }; +class File; + +struct ImportIR : public Printable { + auto Print(llvm::raw_ostream& out) const -> void { out << node_id; } + + // The node ID for the import. + Parse::ImportDirectiveId node_id; + // The imported IR. + const File* sem_ir; +}; + // Provides semantic analysis on a Parse::Tree. class File : public Printable { public: diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index 1079e5e8157e..9bb6577165bc 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -20,6 +20,7 @@ class Inst; struct BindNameInfo; struct Class; struct Function; +struct ImportIR; struct Interface; struct Impl; struct NameScope; @@ -248,7 +249,7 @@ constexpr ImplId ImplId::Invalid = ImplId(InvalidIndex); // The ID of an imported IR. struct ImportIRId : public IdBase, public Printable { - using ValueType = const File*; + using ValueType = ImportIR; static const ImportIRId Builtins; using IdBase::IdBase; diff --git a/toolchain/source/source_buffer.cpp b/toolchain/source/source_buffer.cpp index c2887663c697..09248e2be823 100644 --- a/toolchain/source/source_buffer.cpp +++ b/toolchain/source/source_buffer.cpp @@ -12,7 +12,8 @@ namespace Carbon { namespace { struct FilenameConverter : DiagnosticConverter { - auto ConvertLocation(llvm::StringRef filename) const + auto ConvertLocation(llvm::StringRef filename, + ContextFnT /*context_fn*/) const -> DiagnosticLocation override { return {.filename = filename}; }