From f31c553e65bc47b6005aeedda42177ad6fddb509 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 20 May 2026 13:26:36 -0700 Subject: [PATCH] Add a failing test for Clang modules support. (#7233) Rearrange the file_test infrastructure so that we can customize the mapping of file names to command line arguments. Map `module.modulemap` files to corresponding Clang driver flags. In passing, also clean up the interface for specifying custom argument replacements so that we don't build a string map for each file we process, and stop using `SmallVector::insert`. Assisted-by: Gemini via Antigravity --- testing/file_test/README.md | 10 +-- testing/file_test/file_test_base.h | 15 +++-- testing/file_test/file_test_base_test.cpp | 19 ++++-- testing/file_test/run_test.cpp | 58 +++++++++--------- testing/file_test/testdata/two_files.carbon | 4 ++ .../interop/cpp/modules/import.carbon | 61 +++++++++++++++++++ toolchain/testing/file_test.cpp | 30 ++++++++- 7 files changed, 154 insertions(+), 43 deletions(-) create mode 100644 toolchain/check/testdata/interop/cpp/modules/import.carbon diff --git a/testing/file_test/README.md b/testing/file_test/README.md index 93067b5548a4..35ddd5331c51 100644 --- a/testing/file_test/README.md +++ b/testing/file_test/README.md @@ -144,7 +144,7 @@ content (comment markers don't allow `[[`). Settings in files are provided in comments, similar to `FileCheck` syntax. `bazel run :file_test -- --autoupdate` automatically constructs compatible -CHECK:STDOUT: and CHECK:STDERR: lines. +`CHECK:STDOUT:` and `CHECK:STDERR:` lines. Supported comment markers are: @@ -184,7 +184,8 @@ Supported comment markers are: - `%s` Replaced with the list of files. Currently only allowed as a standalone - argument, not a substring. + argument, not a substring. (The added arguments can be customized by + overriding `FileTestBase::AddArgsForFilename`.) - `%t` @@ -193,10 +194,11 @@ Supported comment markers are: - `%{identifier}` Replaces some implementation-specific identifier with a value. (Mappings - provided by way of an optional `MyFileTest::GetArgReplacements`) + can be provided by overriding `FileTestBase::GetArgReplacement`.) `ARGS` can be specified at most once. If not provided, the `FileTestBase` - child is responsible for providing default arguments. + child is responsible for providing default arguments by overriding + `FileTestBase::GetDefaultArgs`. - ``` // EXTRA-ARGS: diff --git a/testing/file_test/file_test_base.h b/testing/file_test/file_test_base.h index 80738075dd53..02b5f95fa8bc 100644 --- a/testing/file_test/file_test_base.h +++ b/testing/file_test/file_test_base.h @@ -78,10 +78,17 @@ class FileTestBase { // Returns default arguments. Only called when a file doesn't set ARGS. virtual auto GetDefaultArgs() const -> llvm::SmallVector = 0; - // Returns a map of string replacements to implement `%{key}` -> `value` in - // arguments. - virtual auto GetArgReplacements() const -> llvm::StringMap { - return {}; + // Adds arguments to the `args` vector for the given filename. + virtual auto AddArgsForFilename(llvm::SmallVectorImpl& args, + llvm::StringRef filename) const -> void { + args.emplace_back(filename); + } + + // Returns a replacement for the given key. Keys are passed without the + // surrounding %{}. + virtual auto GetArgReplacement(llvm::StringRef /*key*/) const + -> std::optional { + return std::nullopt; } // Returns a regex to match the default file when a line may not be present. diff --git a/testing/file_test/file_test_base_test.cpp b/testing/file_test/file_test_base_test.cpp index 707bb647eb8a..6fb8a1fa5d03 100644 --- a/testing/file_test/file_test_base_test.cpp +++ b/testing/file_test/file_test_base_test.cpp @@ -31,14 +31,25 @@ class FileTestBaseTest : public FileTestBase { llvm::raw_pwrite_stream& error_stream) const -> ErrorOr override; - auto GetArgReplacements() const -> llvm::StringMap override { - return {{"replacement", "replaced"}}; - } - auto GetDefaultArgs() const -> llvm::SmallVector override { return {"default_args", "%s"}; } + auto AddArgsForFilename(llvm::SmallVectorImpl& args, + llvm::StringRef filename) const -> void override { + if (!filename.ends_with(".exclude")) { + args.emplace_back(filename); + } + } + + auto GetArgReplacement(llvm::StringRef key) const + -> std::optional override { + if (key == "replacement") { + return "replaced"; + } + return std::nullopt; + } + auto GetDefaultFileRE(llvm::ArrayRef filenames) const -> std::optional override { return std::make_optional( diff --git a/testing/file_test/run_test.cpp b/testing/file_test/run_test.cpp index 001f99a6a997..a2a8f4467c65 100644 --- a/testing/file_test/run_test.cpp +++ b/testing/file_test/run_test.cpp @@ -31,62 +31,64 @@ using ::testing::internal::GetCapturedStdout; static constexpr llvm::StringLiteral StdinFilename = "STDIN"; // Does replacements in ARGS for %s and %t. -static auto DoArgReplacements(llvm::SmallVector& test_args, - const llvm::StringMap& replacements, +static auto DoArgReplacements(const FileTestBase& test_base, + llvm::SmallVector& test_args, llvm::ArrayRef split_files) -> ErrorOr { - for (auto* it = test_args.begin(); it != test_args.end(); ++it) { - auto percent = it->find("%"); + llvm::SmallVector new_args; + for (auto& arg : test_args) { + auto percent = arg.find('%'); if (percent == std::string::npos) { + new_args.push_back(std::move(arg)); continue; } - if (percent + 1 >= it->size()) { - return ErrorBuilder() << "% is not allowed on its own: " << *it; + if (percent + 1 >= arg.size()) { + return ErrorBuilder() << "% is not allowed on its own: " << arg; } - char c = (*it)[percent + 1]; + char c = arg[percent + 1]; switch (c) { case 's': { - if (*it != "%s") { - return ErrorBuilder() << "%s must be the full argument: " << *it; + if (arg != "%s") { + return ErrorBuilder() << "%s must be the full argument: " << arg; } - it = test_args.erase(it); for (const auto& split : split_files) { - const std::string& filename = split->filename; - if (filename == StdinFilename || filename.ends_with(".h")) { - continue; + // AUTOUPDATE-SPLIT was filtered out when building the list of splits, + // but STDIN is still potentially present. + // TODO: Should we remove STDIN before we reach this point? + if (split->filename != StdinFilename) { + test_base.AddArgsForFilename(new_args, split->filename); } - it = test_args.insert(it, filename); - ++it; } - // Back up once because the for loop will advance. - --it; break; } case 't': { std::filesystem::path tmpdir = GetTempDirectory(); - it->replace(percent, 2, llvm::formatv("{0}/temp_file", tmpdir)); + arg.replace(percent, 2, llvm::formatv("{0}/temp_file", tmpdir)); + new_args.push_back(std::move(arg)); break; } case '{': { - auto end_brace = it->find('}', percent); + auto end_brace = arg.find('}', percent); if (end_brace == std::string::npos) { - return ErrorBuilder() << "%{ without closing }: " << *it; + return ErrorBuilder() << "%{ without closing }: " << arg; } - llvm::StringRef substr(&*(it->begin() + percent + 2), + llvm::StringRef substr(arg.data() + percent + 2, end_brace - percent - 2); - auto replacement = replacements.find(substr); - if (replacement == replacements.end()) { + auto replacement = test_base.GetArgReplacement(substr); + if (!replacement) { return ErrorBuilder() - << "unknown substitution: %{" << substr << "}: " << *it; + << "unknown substitution: %{" << substr << "}: " << arg; } - it->replace(percent, end_brace - percent + 1, replacement->second); + arg.replace(percent, end_brace - percent + 1, *replacement); + new_args.push_back(std::move(arg)); break; } default: - return ErrorBuilder() << "%" << c << " is not supported: " << *it; + return ErrorBuilder() << "%" << c << " is not supported: " << arg; } } + test_args = std::move(new_args); return Success(); } @@ -116,8 +118,8 @@ auto RunTestFile(const FileTestBase& test_base, bool dump_output, test_file.test_args = test_base.GetDefaultArgs(); } test_file.test_args.append(test_file.extra_args); - CARBON_RETURN_IF_ERROR(DoArgReplacements( - test_file.test_args, test_base.GetArgReplacements(), all_splits)); + CARBON_RETURN_IF_ERROR( + DoArgReplacements(test_base, test_file.test_args, all_splits)); // stdin needs to exist on-disk for compatibility. We'll use a pointer for it. FILE* input_stream = nullptr; diff --git a/testing/file_test/testdata/two_files.carbon b/testing/file_test/testdata/two_files.carbon index 2f6cad4e88f3..ba0c6220b5f8 100644 --- a/testing/file_test/testdata/two_files.carbon +++ b/testing/file_test/testdata/two_files.carbon @@ -17,3 +17,7 @@ aaa // --- b.carbon bbb // CHECK:STDOUT: b.carbon:[[@LINE-1]]: bbb + + +// --- c.exclude +ccc diff --git a/toolchain/check/testdata/interop/cpp/modules/import.carbon b/toolchain/check/testdata/interop/cpp/modules/import.carbon new file mode 100644 index 000000000000..6d2488439ad7 --- /dev/null +++ b/toolchain/check/testdata/interop/cpp/modules/import.carbon @@ -0,0 +1,61 @@ +// 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 +// +// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/full.carbon +// EXTRA-ARGS: --clang-arg=-fmodules --clang-arg=-fmodules-cache-path=%t.cache +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/modules/import.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/modules/import.carbon + +// --- module.modulemap + +module A { + header "a.h" + export * +} + +module B { + header "b.h" + export * +} + +// --- a.h + +struct A {}; + +A *_Nonnull makeA(); + +// --- b.h + +#include "a.h" + +struct B {}; + +B *_Nonnull makeB(A *_Nonnull); + +// --- fail_todo_use_a_b.carbon + +library "[[@TEST_NAME]]"; + +import Cpp library "a.h"; +import Cpp library "b.h"; + +// CHECK:STDERR: fail_todo_use_a_b.carbon:[[@LINE+4]]:12: error: member name `B` not found in `Cpp` [MemberNameNotFoundInInstScope] +// CHECK:STDERR: fn Go() -> Cpp.B* { +// CHECK:STDERR: ^~~~~ +// CHECK:STDERR: +fn Go() -> Cpp.B* { + // CHECK:STDERR: fail_todo_use_a_b.carbon:[[@LINE+8]]:10: error: member name `makeB` not found in `Cpp` [MemberNameNotFoundInInstScope] + // CHECK:STDERR: return Cpp.makeB(Cpp.makeA()); + // CHECK:STDERR: ^~~~~~~~~ + // CHECK:STDERR: + // CHECK:STDERR: fail_todo_use_a_b.carbon:[[@LINE+4]]:20: error: member name `makeA` not found in `Cpp` [MemberNameNotFoundInInstScope] + // CHECK:STDERR: return Cpp.makeB(Cpp.makeA()); + // CHECK:STDERR: ^~~~~~~~~ + // CHECK:STDERR: + return Cpp.makeB(Cpp.makeA()); +} diff --git a/toolchain/testing/file_test.cpp b/toolchain/testing/file_test.cpp index a118b66cf33c..b69d4c41c35b 100644 --- a/toolchain/testing/file_test.cpp +++ b/toolchain/testing/file_test.cpp @@ -92,9 +92,15 @@ class ToolchainFileTest : public FileTestBase { // Sets different default flags based on the component being tested. auto GetDefaultArgs() const -> llvm::SmallVector override; - // Returns string replacements to implement `%{key}` -> `value` in arguments. - auto GetArgReplacements() const -> llvm::StringMap override { - return {{"core", data_->installation.core_package().native()}}; + auto AddArgsForFilename(llvm::SmallVectorImpl& args, + llvm::StringRef filename) const -> void override; + + auto GetArgReplacement(llvm::StringRef key) const + -> std::optional override { + if (key == "core") { + return data_->installation.core_package().native(); + } + return std::nullopt; } // Generally uses the parent implementation, with special handling for lex. @@ -212,6 +218,24 @@ auto ToolchainFileTest::Run( return result; } +auto ToolchainFileTest::AddArgsForFilename( + llvm::SmallVectorImpl& args, llvm::StringRef filename) const + -> void { + if (filename.ends_with(".h")) { + // C++ header files don't need a corresponding argument. + return; + } + + if (filename.ends_with("module.modulemap")) { + // Convert module map files to clang module map arguments. + args.push_back("--clang-arg=-fmodule-map-file=" + filename.str()); + return; + } + + // Anything else is expected to be a .carbon input file. + args.push_back(filename.str()); +} + auto ToolchainFileTest::GetDefaultArgs() const -> llvm::SmallVector { llvm::SmallVector args = {"--include-diagnostic-kind"};