From 456d1652585c337c6b239ea4fcbfad0179052c1b Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 17 Nov 2023 12:10:56 -0800 Subject: [PATCH] Diagnose repeat Main//default files as a redundant api file. (#3408) --- toolchain/check/check.cpp | 64 +++++++++++-------- .../check/testdata/basics/multifile.carbon | 4 ++ .../multifile_raw_and_textual_ir.carbon | 4 ++ .../testdata/basics/multifile_raw_ir.carbon | 4 ++ .../packages/fail_duplicate_api.carbon | 60 +++++++++++++++++ .../packages/fail_import_invalid.carbon | 11 ++-- toolchain/diagnostics/diagnostic_kind.def | 1 + .../parse/tree_node_location_translator.h | 11 +++- 8 files changed, 126 insertions(+), 33 deletions(-) create mode 100644 toolchain/check/testdata/packages/fail_duplicate_api.carbon diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index 47ff599b0790..e1eeae934d1b 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -8,6 +8,7 @@ #include "toolchain/base/pretty_stack_trace_function.h" #include "toolchain/base/value_store.h" #include "toolchain/check/context.h" +#include "toolchain/diagnostics/diagnostic_emitter.h" #include "toolchain/parse/tree.h" #include "toolchain/parse/tree_node_location_translator.h" #include "toolchain/sem_ir/file.h" @@ -17,7 +18,7 @@ namespace Carbon::Check { struct UnitInfo { explicit UnitInfo(Unit& unit) : unit(&unit), - translator(unit.tokens, unit.parse_tree), + translator(unit.tokens, unit.tokens->filename(), unit.parse_tree), err_tracker(*unit.consumer), emitter(translator, err_tracker) {} @@ -258,33 +259,46 @@ auto CheckParseTrees(const SemIR::File& builtin_ir, // put .impl.carbon on almost all tests (which are in `Main//default`). We // should probably get leads direction on filenames before enforcing. const auto& packaging = unit_info.unit->parse_tree->packaging_directive(); - if (packaging) { - auto import_key = - GetImportKey(unit_info, IdentifierId::Invalid, packaging->names); - // Diagnose explicit `Main` uses before they become marked as possible - // APIs. - if (import_key.first == ExplicitMainName) { - CARBON_DIAGNOSTIC(ExplicitMainPackage, Error, - "`Main//default` must omit `package` directive."); - CARBON_DIAGNOSTIC( - ExplicitMainLibrary, Error, - "Use `library` directive in `Main` package libraries."); - unit_info.emitter.Emit(packaging->names.node, - import_key.second.empty() ? ExplicitMainPackage - : ExplicitMainLibrary); - continue; - } + // An import key formed from the `package` or `library` directive. Or, for + // Main//default, a placeholder key. + auto import_key = packaging ? GetImportKey(unit_info, IdentifierId::Invalid, + packaging->names) + // Construct a boring key for Main//default. + : ImportKey{"", ""}; - if (packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl) { - continue; - } + // Diagnose explicit `Main` uses before they become marked as possible + // APIs. + if (import_key.first == ExplicitMainName) { + CARBON_DIAGNOSTIC(ExplicitMainPackage, Error, + "`Main//default` must omit `package` directive."); + CARBON_DIAGNOSTIC(ExplicitMainLibrary, Error, + "Use `library` directive in `Main` package libraries."); + unit_info.emitter.Emit(packaging->names.node, import_key.second.empty() + ? ExplicitMainPackage + : ExplicitMainLibrary); + continue; + } - auto [entry, success] = api_map.insert({import_key, &unit_info}); - if (!success) { - // TODO: Cross-reference the source, deal with library, etc. + if (packaging && packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl) { + continue; + } + + auto [entry, success] = api_map.insert({import_key, &unit_info}); + if (!success) { + llvm::StringRef prev_filename = entry->second->unit->tokens->filename(); + if (packaging) { CARBON_DIAGNOSTIC(DuplicateLibraryApi, Error, - "Library's API declared in more than one file."); - unit_info.emitter.Emit(packaging->names.node, DuplicateLibraryApi); + "Library's API previously provided by `{0}`.", + llvm::StringRef); + unit_info.emitter.Emit(packaging->names.node, DuplicateLibraryApi, + prev_filename); + } else { + CARBON_DIAGNOSTIC(DuplicateMainApi, Error, + "Main//default previously provided by `{0}`.", + llvm::StringRef); + // Use the invalid node because there's no node to associate with. + unit_info.emitter.Emit(Parse::Node::Invalid, DuplicateMainApi, + prev_filename); } } } diff --git a/toolchain/check/testdata/basics/multifile.carbon b/toolchain/check/testdata/basics/multifile.carbon index a5b282aa00bb..aa07d1cdad5d 100644 --- a/toolchain/check/testdata/basics/multifile.carbon +++ b/toolchain/check/testdata/basics/multifile.carbon @@ -5,9 +5,13 @@ // AUTOUPDATE // --- a.carbon +package A api; + fn A() {} // --- b.carbon +package B api; + fn B() {} // CHECK:STDOUT: file "a.carbon" { diff --git a/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon b/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon index 159dc3c22674..fb9b588cdb38 100644 --- a/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon +++ b/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon @@ -9,9 +9,13 @@ // AUTOUPDATE // --- a.carbon +package A api; + fn A() {} // --- b.carbon +package B api; + fn B() {} // CHECK:STDOUT: --- diff --git a/toolchain/check/testdata/basics/multifile_raw_ir.carbon b/toolchain/check/testdata/basics/multifile_raw_ir.carbon index f87a00c1abe1..f041c4f02f1f 100644 --- a/toolchain/check/testdata/basics/multifile_raw_ir.carbon +++ b/toolchain/check/testdata/basics/multifile_raw_ir.carbon @@ -9,9 +9,13 @@ // AUTOUPDATE // --- a.carbon +package A api; + fn A() {} // --- b.carbon +package B api; + fn B() {} // CHECK:STDOUT: --- diff --git a/toolchain/check/testdata/packages/fail_duplicate_api.carbon b/toolchain/check/testdata/packages/fail_duplicate_api.carbon new file mode 100644 index 000000000000..7bbb27c88cd2 --- /dev/null +++ b/toolchain/check/testdata/packages/fail_duplicate_api.carbon @@ -0,0 +1,60 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE +// CHECK:STDERR: main2.carbon: ERROR: Main//default previously provided by `main1.carbon`. + +// --- main1.carbon + +// --- main2.carbon + +// --- main_lib1.carbon + +library "lib" api; + +// --- main_lib2.carbon + +// CHECK:STDERR: main_lib2.carbon:[[@LINE+3]]:1: ERROR: Library's API previously provided by `main_lib1.carbon`. +// CHECK:STDERR: library "lib" api; +// CHECK:STDERR: ^ +library "lib" api; + +// --- package1.carbon + +package Package api; + +// --- package2.carbon + +// CHECK:STDERR: package2.carbon:[[@LINE+3]]:1: ERROR: Library's API previously provided by `package1.carbon`. +// CHECK:STDERR: package Package api; +// CHECK:STDERR: ^ +package Package api; + +// --- package_lib1.carbon + +package Package library "lib" api; + +// --- package_lib2.carbon + +// CHECK:STDERR: package_lib2.carbon:[[@LINE+3]]:1: ERROR: Library's API previously provided by `package_lib1.carbon`. +// CHECK:STDERR: package Package library "lib" api; +// CHECK:STDERR: ^ +package Package library "lib" api; + +// CHECK:STDOUT: file "main1.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "main2.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "main_lib1.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "main_lib2.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "package1.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "package2.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "package_lib1.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "package_lib2.carbon" { +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/fail_import_invalid.carbon b/toolchain/check/testdata/packages/fail_import_invalid.carbon index 58ebbaacbb2f..bebf2790fac0 100644 --- a/toolchain/check/testdata/packages/fail_import_invalid.carbon +++ b/toolchain/check/testdata/packages/fail_import_invalid.carbon @@ -11,9 +11,7 @@ // CHECK:STDERR: ^ import Main; -// --- main_lib.carbon - -// CHECK:STDERR: main_lib.carbon:[[@LINE+3]]:1: ERROR: Imports from the current package must omit the package name. +// CHECK:STDERR: main.carbon:[[@LINE+3]]:1: ERROR: Imports from the current package must omit the package name. // CHECK:STDERR: import Main library "lib"; // CHECK:STDERR: ^ import Main library "lib"; @@ -77,16 +75,15 @@ package Implicit library "lib" impl; import Implicit library "lib"; // --- not_found.carbon +package NotFound api; // CHECK:STDERR: not_found.carbon:[[@LINE+3]]:1: ERROR: Imported API not found. -// CHECK:STDERR: import NotFound; +// CHECK:STDERR: import ImportNotFound; // CHECK:STDERR: ^ -import NotFound; +import ImportNotFound; // CHECK:STDOUT: file "main.carbon" { // CHECK:STDOUT: } -// CHECK:STDOUT: file "main_lib.carbon" { -// CHECK:STDOUT: } // CHECK:STDOUT: file "not_main.carbon" { // CHECK:STDOUT: } // CHECK:STDOUT: file "this.carbon" { diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 280712c32344..156b5e008804 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -118,6 +118,7 @@ CARBON_DIAGNOSTIC_KIND(SemanticsTodo) // Package/import checking diagnostics. CARBON_DIAGNOSTIC_KIND(DuplicateLibraryApi) +CARBON_DIAGNOSTIC_KIND(DuplicateMainApi) CARBON_DIAGNOSTIC_KIND(LibraryApiNotFound) CARBON_DIAGNOSTIC_KIND(ImportNotFound) CARBON_DIAGNOSTIC_KIND(ImportCycleDetected) diff --git a/toolchain/parse/tree_node_location_translator.h b/toolchain/parse/tree_node_location_translator.h index 84d22e13da7a..0ad0eab5c038 100644 --- a/toolchain/parse/tree_node_location_translator.h +++ b/toolchain/parse/tree_node_location_translator.h @@ -12,16 +12,25 @@ namespace Carbon::Parse { class NodeLocationTranslator : public DiagnosticLocationTranslator { public: explicit NodeLocationTranslator(const Lex::TokenizedBuffer* tokens, + llvm::StringRef filename, const Tree* parse_tree) - : token_translator_(tokens), parse_tree_(parse_tree) {} + : token_translator_(tokens), + filename_(filename), + parse_tree_(parse_tree) {} // Map the given token into a diagnostic location. auto GetLocation(Node node) -> DiagnosticLocation override { + // Support the invalid token as a way to emit only the filename, when there + // is no line association. + if (!node.is_valid()) { + return {.file_name = filename_}; + } return token_translator_.GetLocation(parse_tree_->node_token(node)); } private: Lex::TokenLocationTranslator token_translator_; + llvm::StringRef filename_; const Tree* parse_tree_; };