From d024403dc42e27ab36be0f406189f4a6a80ef8f1 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Tue, 14 Nov 2023 14:24:03 -0800 Subject: [PATCH] Refactor checking flow to allow for ordering based on import/package. (#3379) As I was working on this, I noticed `import` and `library` syntax needs to be fixed for how it imports the current package, and for `Main` libraries. This mostly reflects the current state in its testing. Otherwise, this should handle most of the errors I could think of: dependency cycles, redundant imports, etc. It does not actually deal with the nuances of cross-IR references. --------- Co-authored-by: Richard Smith --- toolchain/check/check.cpp | 258 +++++++++++++++++- toolchain/check/check.h | 22 +- toolchain/check/handle_import_and_package.cpp | 43 ++- .../testdata/packages/explicit_imports.carbon | 35 +++ .../packages/fail_api_not_found.carbon | 29 ++ .../check/testdata/packages/fail_cycle.carbon | 59 ++++ .../packages/fail_import_invalid.carbon | 111 ++++++++ .../packages/fail_import_repeat.carbon | 50 ++++ .../packages/fail_package_main.carbon | 48 ++++ .../testdata/packages/implicit_imports.carbon | 64 +++++ toolchain/diagnostics/diagnostic_kind.def | 13 + toolchain/driver/driver.cpp | 49 ++-- toolchain/parse/BUILD | 1 + toolchain/parse/context.h | 13 + toolchain/parse/handle_import_and_package.cpp | 39 ++- toolchain/parse/tree.h | 29 +- 16 files changed, 797 insertions(+), 66 deletions(-) create mode 100644 toolchain/check/testdata/packages/explicit_imports.carbon create mode 100644 toolchain/check/testdata/packages/fail_api_not_found.carbon create mode 100644 toolchain/check/testdata/packages/fail_cycle.carbon create mode 100644 toolchain/check/testdata/packages/fail_import_invalid.carbon create mode 100644 toolchain/check/testdata/packages/fail_import_repeat.carbon create mode 100644 toolchain/check/testdata/packages/fail_package_main.carbon create mode 100644 toolchain/check/testdata/packages/implicit_imports.carbon diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index 0337417406e5..51392d65458c 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -2,27 +2,58 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -#include "common/check.h" +#include "toolchain/check/check.h" +#include "common/check.h" #include "toolchain/base/pretty_stack_trace_function.h" +#include "toolchain/base/value_store.h" #include "toolchain/check/context.h" #include "toolchain/parse/tree_node_location_translator.h" #include "toolchain/sem_ir/file.h" namespace Carbon::Check { -auto CheckParseTree(SharedValueStores& value_stores, - const SemIR::File& builtin_ir, - const Lex::TokenizedBuffer& tokens, - const Parse::Tree& parse_tree, DiagnosticConsumer& consumer, - llvm::raw_ostream* vlog_stream) -> SemIR::File { - auto sem_ir = SemIR::File(value_stores, tokens.filename().str(), &builtin_ir); +struct UnitInfo { + explicit UnitInfo(Unit& unit) + : unit(&unit), + translator(unit.tokens, unit.parse_tree), + err_tracker(*unit.consumer), + emitter(translator, err_tracker) {} - Parse::NodeLocationTranslator translator(&tokens, &parse_tree); - ErrorTrackingDiagnosticConsumer err_tracker(consumer); - DiagnosticEmitter emitter(translator, err_tracker); + Unit* unit; - Check::Context context(tokens, emitter, parse_tree, sem_ir, vlog_stream); + // Emitter information. + Parse::NodeLocationTranslator translator; + ErrorTrackingDiagnosticConsumer err_tracker; + DiagnosticEmitter emitter; + + // A list of outgoing imports. + llvm::SmallVector> imports; + + // The remaining number of imports which must be checked before this unit can + // be processed. + int32_t imports_remaining = 0; + + // A list of incoming imports. This will be empty for `impl` files, because + // imports only touch `api` files. + llvm::SmallVector incoming_imports; +}; + +// Produces and checks the IR for the provided Parse::Tree. +// TODO: Both valid and invalid imports should be recorded on the SemIR. Invalid +// imports should suppress errors where it makes sense. +static auto CheckParseTree(const SemIR::File& builtin_ir, UnitInfo& unit_info, + llvm::raw_ostream* vlog_stream) -> void { + unit_info.unit->sem_ir->emplace(*unit_info.unit->value_stores, + unit_info.unit->tokens->filename().str(), + &builtin_ir); + + // For ease-of-access. + SemIR::File& sem_ir = **unit_info.unit->sem_ir; + const Parse::Tree& parse_tree = *unit_info.unit->parse_tree; + + Check::Context context(*unit_info.unit->tokens, unit_info.emitter, parse_tree, + sem_ir, vlog_stream); PrettyStackTraceFunction context_dumper( [&](llvm::raw_ostream& output) { context.PrintForStackDump(output); }); @@ -39,10 +70,10 @@ auto CheckParseTree(SharedValueStores& value_stores, #define CARBON_PARSE_NODE_KIND(Name) \ case Parse::NodeKind::Name: { \ if (!Check::Handle##Name(context, parse_node)) { \ - CARBON_CHECK(err_tracker.seen_error()) \ + CARBON_CHECK(unit_info.err_tracker.seen_error()) \ << "Handle" #Name " returned false without printing a diagnostic"; \ sem_ir.set_has_errors(true); \ - return sem_ir; \ + return; \ } \ break; \ } @@ -56,7 +87,7 @@ auto CheckParseTree(SharedValueStores& value_stores, context.VerifyOnFinish(); - sem_ir.set_has_errors(err_tracker.seen_error()); + sem_ir.set_has_errors(unit_info.err_tracker.seen_error()); #ifndef NDEBUG if (auto verify = sem_ir.Verify(); !verify.ok()) { @@ -64,8 +95,205 @@ auto CheckParseTree(SharedValueStores& value_stores, << "\n"; } #endif +} - return sem_ir; +// The package and library names, used as map keys. +using ImportKey = std::pair; + +// Returns a key form of the package object. +static auto GetImportKey(UnitInfo& unit_info, + Parse::Tree::PackagingNames package) -> ImportKey { + auto* stores = unit_info.unit->value_stores; + return {package.package_id.is_valid() + ? stores->identifiers().Get(package.package_id) + : "", + package.library_id.is_valid() + ? stores->string_literals().Get(package.library_id) + : ""}; +} + +static constexpr llvm::StringLiteral ExplicitMainName = "Main"; + +// Marks an import as required on both the source and target file. +// TODO: When importing without a package name is supported, check that it's +// used correctly. +static auto TrackImport( + llvm::DenseMap& api_map, + llvm::DenseMap* explicit_import_map, + UnitInfo& unit_info, Parse::Tree::PackagingNames import) -> void { + auto import_key = GetImportKey(unit_info, import); + + // Specialize the error for imports from `Main`. + if (import_key.first == ExplicitMainName) { + // Implicit imports will have already warned. + if (explicit_import_map) { + CARBON_DIAGNOSTIC(ImportMainPackage, Error, + "Cannot import `Main` from other packages."); + unit_info.emitter.Emit(import.node, ImportMainPackage); + } + return; + } + + if (explicit_import_map) { + // Check for redundant imports. + if (auto [insert_it, success] = + explicit_import_map->insert({import_key, import.node}); + !success) { + CARBON_DIAGNOSTIC(RepeatedImport, Error, + "Library imported more than once."); + CARBON_DIAGNOSTIC(FirstImported, Note, "First import here."); + unit_info.emitter.Build(import.node, RepeatedImport) + .Note(insert_it->second, FirstImported) + .Emit(); + return; + } + + // Check for explicit imports of the same library. The ID comparison is okay + // in this case because both come from the same file. + auto packaging = unit_info.unit->parse_tree->packaging_directive(); + if (packaging && import.package_id == packaging->names.package_id && + import.library_id == packaging->names.library_id) { + CARBON_DIAGNOSTIC(ExplicitImportApi, Error, + "Explicit import of `api` from `impl` file is " + "redundant with implicit import."); + CARBON_DIAGNOSTIC(ImportSelf, Error, "File cannot import itself."); + unit_info.emitter.Emit( + import.node, packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl + ? ExplicitImportApi + : ImportSelf); + return; + } + } + + if (auto api = api_map.find(import_key); api != api_map.end()) { + unit_info.imports.push_back({import.node, api->second}); + ++unit_info.imports_remaining; + api->second->incoming_imports.push_back(&unit_info); + } else { + CARBON_DIAGNOSTIC(LibraryApiNotFound, Error, + "Corresponding API not found."); + CARBON_DIAGNOSTIC(ImportNotFound, Error, "Imported API not found."); + unit_info.emitter.Emit( + import.node, explicit_import_map ? ImportNotFound : LibraryApiNotFound); + } +} + +auto CheckParseTrees(const SemIR::File& builtin_ir, + llvm::MutableArrayRef units, + llvm::raw_ostream* vlog_stream) -> void { + // Prepare diagnostic emitters in case we run into issues during package + // checking. + // + // UnitInfo is big due to its SmallVectors, so we default to 0 on the stack. + llvm::SmallVector unit_infos; + unit_infos.reserve(units.size()); + for (auto& unit : units) { + unit_infos.emplace_back(unit); + } + + // Create a map of APIs which might be imported. + llvm::DenseMap api_map; + for (auto& unit_info : unit_infos) { + const auto& packaging = unit_info.unit->parse_tree->packaging_directive(); + if (packaging) { + auto import_key = GetImportKey(unit_info, packaging->names); + // Catch explicit `Main` errors before they become marked as possible + // APIs. + if (import_key.first == ExplicitMainName) { + CARBON_DIAGNOSTIC( + ExplicitMainPackage, Error, + "Default `Main` library 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; + } + + if (packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl) { + continue; + } + + auto [entry, success] = api_map.insert({import_key, &unit_info}); + if (!success) { + // TODO: Cross-reference the source, deal with library, etc. + CARBON_DIAGNOSTIC(DuplicateLibraryApi, Error, + "Library's API declared in more than one file."); + unit_info.emitter.Emit(packaging->names.node, DuplicateLibraryApi); + } + } + } + + // Mark down imports for all files. + llvm::SmallVector ready_to_check; + ready_to_check.reserve(units.size()); + for (auto& unit_info : unit_infos) { + const auto& packaging = unit_info.unit->parse_tree->packaging_directive(); + if (packaging && packaging->api_or_impl == Parse::Tree::ApiOrImpl::Impl) { + // An `impl` has an implicit import of its `api`. + TrackImport(api_map, nullptr, unit_info, packaging->names); + } + + llvm::DenseMap explicit_import_map; + for (const auto& import : unit_info.unit->parse_tree->imports()) { + TrackImport(api_map, &explicit_import_map, unit_info, import); + } + + // If there were no imports, mark the file as ready to check for below. + if (unit_info.imports_remaining == 0) { + ready_to_check.push_back(&unit_info); + } + } + + // Check everything with no dependencies. Earlier entries with dependencies + // will be checked as soon as all their dependencies have been checked. + for (int check_index = 0; + check_index < static_cast(ready_to_check.size()); ++check_index) { + auto* unit_info = ready_to_check[check_index]; + CheckParseTree(builtin_ir, *unit_info, vlog_stream); + for (auto* incoming_import : unit_info->incoming_imports) { + --incoming_import->imports_remaining; + if (incoming_import->imports_remaining == 0) { + ready_to_check.push_back(incoming_import); + } + } + } + + // If there are still units with remaining imports, it means there's a + // dependency loop. + if (ready_to_check.size() < unit_infos.size()) { + // Go through units and mask out unevaluated imports. This breaks everything + // associated with a loop equivalently, whether it's part of it or depending + // on a part of it. + // TODO: Better identify cycles, maybe try to untangle them. + for (auto& unit_info : unit_infos) { + if (unit_info.imports_remaining > 0) { + for (auto* import_it = unit_info.imports.begin(); + import_it != unit_info.imports.end();) { + auto* import_unit = import_it->second->unit; + if (*import_unit->sem_ir) { + ++import_it; + } else { + CARBON_DIAGNOSTIC(ImportCycleDetected, Error, + "Import cannot be used due to a cycle. Cycle " + "must be fixed to import."); + unit_info.emitter.Emit(import_it->first, ImportCycleDetected); + import_it = unit_info.imports.erase(import_it); + } + } + } + } + + // Check the remaining file contents, which are probably broken due to + // incomplete imports. + for (auto& unit_info : unit_infos) { + if (unit_info.imports_remaining > 0) { + CheckParseTree(builtin_ir, unit_info, vlog_stream); + } + } + } } } // namespace Carbon::Check diff --git a/toolchain/check/check.h b/toolchain/check/check.h index 6d1290e83ab6..2a4215d1296b 100644 --- a/toolchain/check/check.h +++ b/toolchain/check/check.h @@ -20,13 +20,21 @@ inline auto MakeBuiltins(SharedValueStores& value_stores) -> SemIR::File { return SemIR::File(value_stores); } -// Produces and checks the IR for the provided Parse::Tree. -extern auto CheckParseTree(SharedValueStores& value_stores, - const SemIR::File& builtin_ir, - const Lex::TokenizedBuffer& tokens, - const Parse::Tree& parse_tree, - DiagnosticConsumer& consumer, - llvm::raw_ostream* vlog_stream) -> SemIR::File; +// Checking information that's tracked per file. +struct Unit { + SharedValueStores* value_stores; + const Lex::TokenizedBuffer* tokens; + const Parse::Tree* parse_tree; + DiagnosticConsumer* consumer; + // The generated IR. Unset on input, set on output. + std::optional* sem_ir; +}; + +// Checks a group of parse trees. This will use imports to decide the order of +// checking. +auto CheckParseTrees(const SemIR::File& builtin_ir, + llvm::MutableArrayRef units, + llvm::raw_ostream* vlog_stream) -> void; } // namespace Carbon::Check diff --git a/toolchain/check/handle_import_and_package.cpp b/toolchain/check/handle_import_and_package.cpp index 8f75770ce09f..ded009468445 100644 --- a/toolchain/check/handle_import_and_package.cpp +++ b/toolchain/check/handle_import_and_package.cpp @@ -6,32 +6,47 @@ namespace Carbon::Check { -auto HandleImportIntroducer(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandleImportIntroducer"); +// `import` and `package` are structured by parsing. As a consequence, no +// checking logic is needed here. + +auto HandleImportIntroducer(Context& /*context*/, Parse::Node /*parse_node*/) + -> bool { + return true; } -auto HandlePackageIntroducer(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandlePackageIntroducer"); +auto HandlePackageIntroducer(Context& /*context*/, Parse::Node /*parse_node*/) + -> bool { + return true; } -auto HandleLibrary(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandleLibrary"); +auto HandleLibrary(Context& context, Parse::Node /*parse_node*/) -> bool { + // Pop and discard the library name from the node stack. + context.node_stack().Pop(); + return true; } -auto HandlePackageApi(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandlePackageApi"); +auto HandlePackageApi(Context& /*context*/, Parse::Node /*parse_node*/) + -> bool { + return true; } -auto HandlePackageImpl(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandlePackageImpl"); +auto HandlePackageImpl(Context& /*context*/, Parse::Node /*parse_node*/) + -> bool { + return true; } -auto HandleImportDirective(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandleImportDirective"); +auto HandleImportDirective(Context& context, Parse::Node /*parse_node*/) + -> bool { + // Pop and discard the identifier from the node stack. + context.node_stack().Pop(); + return true; } -auto HandlePackageDirective(Context& context, Parse::Node parse_node) -> bool { - return context.TODO(parse_node, "HandlePackageDirective"); +auto HandlePackageDirective(Context& context, Parse::Node /*parse_node*/) + -> bool { + // Pop and discard the identifier from the node stack. + context.node_stack().Pop(); + return true; } } // namespace Carbon::Check diff --git a/toolchain/check/testdata/packages/explicit_imports.carbon b/toolchain/check/testdata/packages/explicit_imports.carbon new file mode 100644 index 000000000000..b1077e470e80 --- /dev/null +++ b/toolchain/check/testdata/packages/explicit_imports.carbon @@ -0,0 +1,35 @@ +// 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 + +// --- api.carbon + +package Api api; + +// --- api_lib.carbon + +package Api library "lib" api; + +// --- import_api.carbon + +import Api; +import Api library "lib"; + +// CHECK:STDOUT: file "api.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "api_lib.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "import_api.carbon" { +// CHECK:STDOUT: %.loc3: String = string_literal "lib" +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/fail_api_not_found.carbon b/toolchain/check/testdata/packages/fail_api_not_found.carbon new file mode 100644 index 000000000000..72b0dba05926 --- /dev/null +++ b/toolchain/check/testdata/packages/fail_api_not_found.carbon @@ -0,0 +1,29 @@ +// 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 + +// --- no_api.carbon + +// CHECK:STDERR: no_api.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found. +// CHECK:STDERR: package Foo impl; +// CHECK:STDERR: ^ +package Foo impl; + +// --- no_api_lib.carbon + +// CHECK:STDERR: no_api_lib.carbon:[[@LINE+3]]:1: ERROR: Corresponding API not found. +// CHECK:STDERR: package Foo library "Bar" impl; +// CHECK:STDERR: ^ +package Foo library "Bar" impl; + +// CHECK:STDOUT: file "no_api.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "no_api_lib.carbon" { +// CHECK:STDOUT: %.loc5: String = string_literal "Bar" +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/fail_cycle.carbon b/toolchain/check/testdata/packages/fail_cycle.carbon new file mode 100644 index 000000000000..e92350c5fe5f --- /dev/null +++ b/toolchain/check/testdata/packages/fail_cycle.carbon @@ -0,0 +1,59 @@ +// 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 + +// --- a.carbon + +package A api; + +// CHECK:STDERR: a.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import. +// CHECK:STDERR: import B; +// CHECK:STDERR: ^ +import B; + +// --- b.carbon + +package B api; + +// CHECK:STDERR: b.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import. +// CHECK:STDERR: import C; +// CHECK:STDERR: ^ +import C; + +// --- c.carbon + +package C api; + +// CHECK:STDERR: c.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import. +// CHECK:STDERR: import A; +// CHECK:STDERR: ^ +import A; + +// --- c_impl.carbon + +// CHECK:STDERR: c_impl.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import. +// CHECK:STDERR: package C impl; +// CHECK:STDERR: ^ +package C impl; + +// --- cycle_child.carbon + +package CycleChild api; + +// CHECK:STDERR: cycle_child.carbon:[[@LINE+3]]:1: ERROR: Import cannot be used due to a cycle. Cycle must be fixed to import. +// CHECK:STDERR: import B; +// CHECK:STDERR: ^ +import B; + +// CHECK:STDOUT: file "a.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "b.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "c.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "c_impl.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "cycle_child.carbon" { +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/fail_import_invalid.carbon b/toolchain/check/testdata/packages/fail_import_invalid.carbon new file mode 100644 index 000000000000..c8e847933f9c --- /dev/null +++ b/toolchain/check/testdata/packages/fail_import_invalid.carbon @@ -0,0 +1,111 @@ +// 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 + +// --- main.carbon + +// CHECK:STDERR: main.carbon:[[@LINE+3]]:1: ERROR: Cannot import `Main` from other packages. +// CHECK:STDERR: import Main; +// CHECK:STDERR: ^ +import Main; + +// --- main_lib.carbon + +// CHECK:STDERR: main_lib.carbon:[[@LINE+3]]:1: ERROR: Cannot import `Main` from other packages. +// CHECK:STDERR: import Main library "lib"; +// CHECK:STDERR: ^ +import Main library "lib"; + +// --- this.carbon + +package This api; + +// CHECK:STDERR: this.carbon:[[@LINE+3]]:1: ERROR: File cannot import itself. +// CHECK:STDERR: import This; +// CHECK:STDERR: ^ +import This; + +// --- this_lib.carbon + +package This library "lib" api; + +// CHECK:STDERR: this_lib.carbon:[[@LINE+3]]:1: ERROR: File cannot import itself. +// CHECK:STDERR: import This library "lib"; +// CHECK:STDERR: ^ +import This library "lib"; + +// --- implicit_api.carbon + +package Implicit api; + +// --- implicit_impl.carbon + +package Implicit impl; + +// CHECK:STDERR: implicit_impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import. +// CHECK:STDERR: import Implicit; +// CHECK:STDERR: ^ +import Implicit; + +// --- implicit_lib_api.carbon + +package Implicit library "lib" api; + +// --- implicit_lib_impl.carbon + +package Implicit library "lib" impl; + +// CHECK:STDERR: implicit_lib_impl.carbon:[[@LINE+3]]:1: ERROR: Explicit import of `api` from `impl` file is redundant with implicit import. +// CHECK:STDERR: import Implicit library "lib"; +// CHECK:STDERR: ^ +import Implicit library "lib"; + +// --- unknown.carbon + +// CHECK:STDERR: unknown.carbon:[[@LINE+3]]:1: ERROR: Imported API not found. +// CHECK:STDERR: import Unknown; +// CHECK:STDERR: ^ +import Unknown; + +// CHECK:STDOUT: file "main.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "main_lib.carbon" { +// CHECK:STDOUT: %.loc5: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: file "this.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "this_lib.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: %.loc7: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: file "implicit_api.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "implicit_impl.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "implicit_lib_api.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "implicit_lib_impl.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: %.loc7: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: file "unknown.carbon" { +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/fail_import_repeat.carbon b/toolchain/check/testdata/packages/fail_import_repeat.carbon new file mode 100644 index 000000000000..473a28206361 --- /dev/null +++ b/toolchain/check/testdata/packages/fail_import_repeat.carbon @@ -0,0 +1,50 @@ +// 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 + +// --- api.carbon + +package Api api; + +// --- api_lib.carbon + +package Api library "lib" api; + +// --- import_api.carbon + +import Api; +// CHECK:STDERR: import_api.carbon:[[@LINE+6]]:1: ERROR: Library imported more than once. +// CHECK:STDERR: import Api; +// CHECK:STDERR: ^ +// CHECK:STDERR: import_api.carbon:[[@LINE-4]]:1: First import here. +// CHECK:STDERR: import Api; +// CHECK:STDERR: ^ +import Api; +import Api library "lib"; +// CHECK:STDERR: import_api.carbon:[[@LINE+6]]:1: ERROR: Library imported more than once. +// CHECK:STDERR: import Api library "lib"; +// CHECK:STDERR: ^ +// CHECK:STDERR: import_api.carbon:[[@LINE-4]]:1: First import here. +// CHECK:STDERR: import Api library "lib"; +// CHECK:STDERR: ^ +import Api library "lib"; + +// CHECK:STDOUT: file "api.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "api_lib.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "import_api.carbon" { +// CHECK:STDOUT: %.loc10: String = string_literal "lib" +// CHECK:STDOUT: %.loc17: String = string_literal "lib" +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/fail_package_main.carbon b/toolchain/check/testdata/packages/fail_package_main.carbon new file mode 100644 index 000000000000..ee572cbe578f --- /dev/null +++ b/toolchain/check/testdata/packages/fail_package_main.carbon @@ -0,0 +1,48 @@ +// 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 + +// --- main.carbon + +// CHECK:STDERR: main.carbon:[[@LINE+3]]:1: ERROR: Default `Main` library must omit `package` directive. +// CHECK:STDERR: package Main api; +// CHECK:STDERR: ^ +package Main api; + +// --- main_impl.carbon + +// CHECK:STDERR: main_impl.carbon:[[@LINE+3]]:1: ERROR: Default `Main` library must omit `package` directive. +// CHECK:STDERR: package Main impl; +// CHECK:STDERR: ^ +package Main impl; + +// --- raw_main.carbon + +// `Main` isn't a keyword, so this fails the same way. +// CHECK:STDERR: raw_main.carbon:[[@LINE+3]]:1: ERROR: Default `Main` library must omit `package` directive. +// CHECK:STDERR: package r#Main api; +// CHECK:STDERR: ^ +package r#Main api; + +// --- main_lib.carbon + +// CHECK:STDERR: main_lib.carbon:[[@LINE+3]]:1: ERROR: Use `library` directive in `Main` package libraries. +// CHECK:STDERR: package Main library "lib" api; +// CHECK:STDERR: ^ +package Main library "lib" api; + +// CHECK:STDOUT: file "main.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "main_impl.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "raw_main.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "main_lib.carbon" { +// CHECK:STDOUT: %.loc5: String = string_literal "lib" +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/packages/implicit_imports.carbon b/toolchain/check/testdata/packages/implicit_imports.carbon new file mode 100644 index 000000000000..79c96e56e9ef --- /dev/null +++ b/toolchain/check/testdata/packages/implicit_imports.carbon @@ -0,0 +1,64 @@ +// 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 + +// --- api_only.carbon + +package ApiOnly api; + +// --- api_only_lib.carbon + +package ApiOnly library "lib" api; + +// --- with_impl_api.carbon + +package WithImpl api; + +// --- with_impl_impl.carbon + +package WithImpl impl; + +// --- with_impl_impl_extra.carbon + +// Multiple impls are allowed. +package WithImpl impl; + +// --- with_impl_lib_api.carbon + +package WithImpl library "lib" api; + +// --- with_impl_lib_impl.carbon + +package WithImpl library "lib" impl; + +// CHECK:STDOUT: file "api_only.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "api_only_lib.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: file "with_impl_api.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "with_impl_impl.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: file "with_impl_impl_extra.carbon" { +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "with_impl_lib_api.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: } +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %.1: type = ptr_type String +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: file "with_impl_lib_impl.carbon" { +// CHECK:STDOUT: %.loc2: String = string_literal "lib" +// CHECK:STDOUT: } diff --git a/toolchain/diagnostics/diagnostic_kind.def b/toolchain/diagnostics/diagnostic_kind.def index 27780527c63a..84d913055fec 100644 --- a/toolchain/diagnostics/diagnostic_kind.def +++ b/toolchain/diagnostics/diagnostic_kind.def @@ -114,6 +114,19 @@ CARBON_DIAGNOSTIC_KIND(ParamsRequiredByIntroducer) CARBON_DIAGNOSTIC_KIND(SemanticsTodo) +// Package/import checking diagnostics. +CARBON_DIAGNOSTIC_KIND(DuplicateLibraryApi) +CARBON_DIAGNOSTIC_KIND(LibraryApiNotFound) +CARBON_DIAGNOSTIC_KIND(ImportNotFound) +CARBON_DIAGNOSTIC_KIND(ImportCycleDetected) +CARBON_DIAGNOSTIC_KIND(ExplicitMainPackage) +CARBON_DIAGNOSTIC_KIND(ExplicitMainLibrary) +CARBON_DIAGNOSTIC_KIND(ImportMainPackage) +CARBON_DIAGNOSTIC_KIND(ImportSelf) +CARBON_DIAGNOSTIC_KIND(ExplicitImportApi) +CARBON_DIAGNOSTIC_KIND(RepeatedImport) +CARBON_DIAGNOSTIC_KIND(FirstImported) + // Function call checking. CARBON_DIAGNOSTIC_KIND(AddrSelfIsNonReference) CARBON_DIAGNOSTIC_KIND(CallArgCountMismatch) diff --git a/toolchain/driver/driver.cpp b/toolchain/driver/driver.cpp index 532e8581f8c6..60906f51f778 100644 --- a/toolchain/driver/driver.cpp +++ b/toolchain/driver/driver.cpp @@ -432,10 +432,6 @@ class Driver::CompilationUnit { // Parses tokens. Returns true on success. auto RunParse() -> bool { - // Can be called when the file fails to load, so ensure there's source. - if (!source_) { - return false; - } CARBON_CHECK(tokens_); LogCall("Parse::Tree::Parse", [&] { @@ -449,18 +445,19 @@ class Driver::CompilationUnit { return !parse_tree_->has_errors(); } - // Check the parse tree and produce SemIR. Returns true on success. - auto RunCheck(const SemIR::File& builtins) -> bool { - // Can be called when the file fails to load, so ensure there's source. - if (!source_) { - return false; - } + // Returns information needed to check this unit. + auto GetCheckUnit() -> Check::Unit { CARBON_CHECK(parse_tree_); + return {.value_stores = &value_stores_, + .tokens = &*tokens_, + .parse_tree = &*parse_tree_, + .consumer = consumer_, + .sem_ir = &sem_ir_}; + } - LogCall("Check::CheckParseTree", [&] { - sem_ir_ = Check::CheckParseTree(value_stores_, builtins, *tokens_, - *parse_tree_, *consumer_, vlog_stream_); - }); + // Runs post-check logic. Returns true if checking succeeded for the IR. + auto PostCheck() -> bool { + CARBON_CHECK(sem_ir_); // We've finished all steps that can produce diagnostics. Emit the // diagnostics now, so that the developer sees them sooner and doesn't need @@ -575,6 +572,8 @@ class Driver::CompilationUnit { value_stores_.OutputYaml(input_file_name_)); } + auto has_source() -> bool { return source_.has_value(); } + private: // Wraps a call with log statements to indicate start and end. auto LogCall(llvm::StringLiteral label, llvm::function_ref fn) @@ -641,10 +640,15 @@ auto Driver::Compile(const CompileOptions& options) -> bool { if (options.phase == CompileOptions::Phase::Lex) { return success_before_lower; } + // Parse and check phases examine `has_source` because they want to proceed if + // lex failed, but not if source doesn't exist. Later steps are skipped if + // anything failed, so don't need this. // Parse. for (auto& unit : units) { - success_before_lower &= unit->RunParse(); + if (unit->has_source()) { + success_before_lower &= unit->RunParse(); + } } if (options.phase == CompileOptions::Phase::Parse) { return success_before_lower; @@ -653,9 +657,20 @@ auto Driver::Compile(const CompileOptions& options) -> bool { // Check. SharedValueStores builtin_value_stores; auto builtins = Check::MakeBuiltins(builtin_value_stores); - // TODO: Organize units to compile in dependency order. + llvm::SmallVector check_units; for (auto& unit : units) { - success_before_lower &= unit->RunCheck(builtins); + if (unit->has_source()) { + check_units.push_back(unit->GetCheckUnit()); + } + } + CARBON_VLOG() << "*** Check::CheckParseTrees ***\n"; + Check::CheckParseTrees(builtins, llvm::MutableArrayRef(check_units), + vlog_stream_); + CARBON_VLOG() << "*** Check::CheckParseTrees done ***\n"; + for (auto& unit : units) { + if (unit->has_source()) { + success_before_lower &= unit->PostCheck(); + } } if (options.phase == CompileOptions::Phase::Check) { return success_before_lower; diff --git a/toolchain/parse/BUILD b/toolchain/parse/BUILD index e6e63054e04d..ada73b5ee55b 100644 --- a/toolchain/parse/BUILD +++ b/toolchain/parse/BUILD @@ -54,6 +54,7 @@ cc_library( "//common:ostream", "//common:vlog", "//toolchain/base:pretty_stack_trace_function", + "//toolchain/base:value_store", "//toolchain/diagnostics:diagnostic_emitter", "//toolchain/lex:token_kind", "//toolchain/lex:tokenized_buffer", diff --git a/toolchain/parse/context.h b/toolchain/parse/context.h index 425fe02271ba..6a1a8f4b8937 100644 --- a/toolchain/parse/context.h +++ b/toolchain/parse/context.h @@ -300,6 +300,19 @@ class Context { auto RecoverFromDeclError(StateStackEntry state, NodeKind parse_node_kind, bool skip_past_likely_end) -> void; + // Sets the package directive information. Called at most once. + auto set_packaging_directive(Tree::PackagingNames packaging_names, + Tree::ApiOrImpl api_or_impl) -> void { + CARBON_CHECK(!tree_->packaging_directive_); + tree_->packaging_directive_ = {.names = packaging_names, + .api_or_impl = api_or_impl}; + } + + // Adds an import. + auto AddImport(Tree::PackagingNames package) -> void { + tree_->imports_.push_back(package); + } + // Prints information for a stack dump. auto PrintForStackDump(llvm::raw_ostream& output) const -> void; diff --git a/toolchain/parse/handle_import_and_package.cpp b/toolchain/parse/handle_import_and_package.cpp index d9a7d97144e0..1345a8c9c28f 100644 --- a/toolchain/parse/handle_import_and_package.cpp +++ b/toolchain/parse/handle_import_and_package.cpp @@ -2,6 +2,7 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +#include "toolchain/base/value_store.h" #include "toolchain/lex/tokenized_buffer.h" #include "toolchain/parse/context.h" @@ -20,10 +21,13 @@ static auto ExitOnParseError(Context& context, Context::StateStackEntry state, // introducer is already added. static auto HandleImportAndPackage(Context& context, Context::StateStackEntry state, - NodeKind directive, bool expect_api_or_impl) + NodeKind directive, bool is_package) -> void { - if (!context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::Identifier, - NodeKind::Name)) { + Tree::PackagingNames names{.node = Node(state.subtree_start)}; + if (auto package_name_token = context.ConsumeIf(Lex::TokenKind::Identifier)) { + names.package_id = context.tokens().GetIdentifier(*package_name_token); + context.AddLeafNode(NodeKind::Name, *package_name_token); + } else { CARBON_DIAGNOSTIC(ExpectedIdentifierAfterKeyword, Error, "Expected identifier after `{0}`.", Lex::TokenKind); context.emitter().Emit(*context.position(), ExpectedIdentifierAfterKeyword, @@ -32,12 +36,14 @@ static auto HandleImportAndPackage(Context& context, return; } - bool library_parsed = false; if (auto library_token = context.ConsumeIf(Lex::TokenKind::Library)) { auto library_start = context.tree().size(); - if (!context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::StringLiteral, - NodeKind::Literal)) { + if (auto library_name_token = + context.ConsumeIf(Lex::TokenKind::StringLiteral)) { + names.library_id = context.tokens().GetStringLiteral(*library_name_token); + context.AddLeafNode(NodeKind::Literal, *library_name_token); + } else { CARBON_DIAGNOSTIC( ExpectedLibraryName, Error, "Expected a string literal to specify the library name."); @@ -48,11 +54,11 @@ static auto HandleImportAndPackage(Context& context, context.AddNode(NodeKind::Library, *library_token, library_start, /*has_error=*/false); - library_parsed = true; } - auto next_kind = context.tokens().GetKind(*(context.position())); - if (!library_parsed && next_kind == Lex::TokenKind::StringLiteral) { + auto next_kind = context.PositionKind(); + if (!names.library_id.is_valid() && + next_kind == Lex::TokenKind::StringLiteral) { // If we come acroess a string literal and we didn't parse `library // "..."` yet, then most probably the user forgot to add `library` // before the library name. @@ -63,14 +69,17 @@ static auto HandleImportAndPackage(Context& context, return; } - if (expect_api_or_impl) { + Tree::ApiOrImpl api_or_impl; + if (is_package) { switch (next_kind) { case Lex::TokenKind::Api: { context.AddLeafNode(NodeKind::PackageApi, context.Consume()); + api_or_impl = Tree::ApiOrImpl::Api; break; } case Lex::TokenKind::Impl: { context.AddLeafNode(NodeKind::PackageImpl, context.Consume()); + api_or_impl = Tree::ApiOrImpl::Impl; break; } default: { @@ -89,6 +98,12 @@ static auto HandleImportAndPackage(Context& context, return; } + if (is_package) { + context.set_packaging_directive(names, api_or_impl); + } else { + context.AddImport(names); + } + context.AddNode(directive, context.Consume(), state.subtree_start, state.has_error); } @@ -107,7 +122,7 @@ auto HandleImport(Context& context) -> void { case Context::PackagingState::InImports: HandleImportAndPackage(context, state, NodeKind::ImportDirective, - /*expect_api_or_impl=*/false); + /*is_package=*/false); break; case Context::PackagingState::AfterNonPackagingDecl: { @@ -157,7 +172,7 @@ auto HandlePackage(Context& context) -> void { // `package` is no longer allowed, but `import` may repeat. context.set_packaging_state(Context::PackagingState::InImports); HandleImportAndPackage(context, state, NodeKind::PackageDirective, - /*expect_api_or_impl=*/true); + /*is_package=*/true); } } // namespace Carbon::Parse diff --git a/toolchain/parse/tree.h b/toolchain/parse/tree.h index 803ba44a1908..f01ae5f98827 100644 --- a/toolchain/parse/tree.h +++ b/toolchain/parse/tree.h @@ -10,7 +10,6 @@ #include "common/error.h" #include "common/ostream.h" #include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringRef.h" #include "llvm/ADT/iterator.h" #include "llvm/ADT/iterator_range.h" #include "toolchain/diagnostics/diagnostic_emitter.h" @@ -62,6 +61,26 @@ class Tree : public Printable { class PostorderIterator; class SiblingIterator; + // For PackagingDirective. + enum class ApiOrImpl : uint8_t { + Api, + Impl, + }; + + // Names in packaging, whether the file's packaging or an import. Links back + // to the node for diagnostics. + struct PackagingNames { + Node node; + IdentifierId package_id = IdentifierId::Invalid; + StringLiteralId library_id = StringLiteralId::Invalid; + }; + + // The file's packaging. + struct PackagingDirective { + PackagingNames names; + ApiOrImpl api_or_impl; + }; + // Parses the token buffer into a `Tree`. // // This is the factory function which is used to build parse trees. @@ -107,6 +126,11 @@ class Tree : public Printable { [[nodiscard]] auto node_subtree_size(Node n) const -> int32_t; + auto packaging_directive() const -> const std::optional& { + return packaging_directive_; + } + auto imports() const -> llvm::ArrayRef { return imports_; } + // See the other Print comments. auto Print(llvm::raw_ostream& output) const -> void; @@ -241,6 +265,9 @@ class Tree : public Printable { // is true we do *not* have the expected 1:1 mapping between tokens and parsed // nodes as some tokens may have been skipped. bool has_errors_ = false; + + std::optional packaging_directive_; + llvm::SmallVector imports_; }; // A random-access iterator to the depth-first postorder sequence of parse nodes