diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index f517f8ff7303..4087d2b13276 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -619,6 +619,8 @@ auto CheckUnit::FinishRun() -> void { context_.inst_blocks().ReplacePlaceholder(reserved_id, block); } + emitter_.Flush(); + context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error()); // Verify that Context cleanly finished. diff --git a/toolchain/check/cpp/context.cpp b/toolchain/check/cpp/context.cpp index 49bffcb5c7b5..aba3f6d51105 100644 --- a/toolchain/check/cpp/context.cpp +++ b/toolchain/check/cpp/context.cpp @@ -8,8 +8,11 @@ namespace Carbon::Check { -CppContext::CppContext(std::unique_ptr action) - : action_(std::move(action)) {} +CppContext::CppContext(clang::CompilerInstance& instance, + std::unique_ptr parser) + : ast_context_(&instance.getASTContext()), + sema_(&instance.getSema()), + parser_(std::move(parser)) {} CppContext::~CppContext() = default; diff --git a/toolchain/check/cpp/context.h b/toolchain/check/cpp/context.h index 7cdaaab1167f..92234f3bde84 100644 --- a/toolchain/check/cpp/context.h +++ b/toolchain/check/cpp/context.h @@ -23,24 +23,13 @@ namespace Carbon::Check { // declarations, and similar values. class CppContext { public: - explicit CppContext(std::unique_ptr action); + explicit CppContext(clang::CompilerInstance& instance, + std::unique_ptr parser); ~CppContext(); - auto action() -> clang::FrontendAction& { return *action_; } - - auto ast_context() -> clang::ASTContext& { - return action_->getCompilerInstance().getASTContext(); - } - - auto sema() -> clang::Sema& { - return action_->getCompilerInstance().getSema(); - } - + auto ast_context() -> clang::ASTContext& { return *ast_context_; } + auto sema() -> clang::Sema& { return *sema_; } auto parser() -> clang::Parser& { return *parser_; } - auto set_parser(std::unique_ptr parser) { - CARBON_CHECK(!parser_); - parser_ = std::move(parser); - } auto clang_mangle_context() -> clang::MangleContext&; @@ -49,8 +38,14 @@ class CppContext { } private: - // The clang action that is generating the C++ AST. - std::unique_ptr action_; + // The Clang AST context. + clang::ASTContext* ast_context_; + + // The Clang semantic analysis engine. + clang::Sema* sema_; + + // The Clang parser. + std::unique_ptr parser_; // Per-Carbon-file start locations for corresponding Clang source buffers. // Owned and managed by code in location.cpp. @@ -58,9 +53,6 @@ class CppContext { // The Clang mangle context for the target in the ASTContext. std::unique_ptr clang_mangle_context_; - - // The Clang parser. - std::unique_ptr parser_; }; } // namespace Carbon::Check diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index 00c03b62d22e..2e8acefe6cb9 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/toolchain/check/cpp/generate_ast.cpp @@ -374,10 +374,10 @@ class GenerateASTAction : public clang::ASTFrontendAction { clang_instance.createSema(getTranslationUnitKind(), /*CompletionConsumer=*/nullptr); - context_->cpp_context()->set_parser(std::make_unique( + auto parser_ptr = std::make_unique( clang_instance.getPreprocessor(), clang_instance.getSema(), - /*SkipFunctionBodies=*/false)); - auto& parser = context_->cpp_context()->parser(); + /*SkipFunctionBodies=*/false); + auto& parser = *parser_ptr; clang_instance.getPreprocessor().EnterMainSourceFile(); if (auto* source = clang_instance.getASTContext().getExternalSource()) { @@ -387,6 +387,9 @@ class GenerateASTAction : public clang::ASTFrontendAction { parser.Initialize(); clang_instance.getSema().ActOnStartOfTranslationUnit(); + context_->set_cpp_context( + std::make_unique(clang_instance, std::move(parser_ptr))); + // Don't allow C++20 module declarations in inline Cpp code fragments. auto module_import_state = clang::Sema::ModuleImportState::NotACXX20Module; @@ -448,8 +451,6 @@ auto GenerateAst(Context& context, invocation->getPreprocessorOpts().addRemappedFile(file_name, includes_buffer.release()); - clang::DiagnosticErrorTrap trap(*diags); - auto clang_instance_ptr = std::make_unique(invocation); auto& clang_instance = *clang_instance_ptr; @@ -464,15 +465,12 @@ auto GenerateAst(Context& context, return false; } - context.set_cpp_context(std::make_unique( - std::make_unique(context))); - - if (!context.cpp_context()->action().BeginSourceFile(clang_instance, - inputs[0])) { + GenerateASTAction action(context); + if (!action.BeginSourceFile(clang_instance, inputs[0])) { return false; } - if (llvm::Error error = context.cpp_context()->action().Execute()) { + if (llvm::Error error = action.Execute()) { // `Execute` currently never fails, but its contract allows it to. context.TODO(SemIR::LocId::None, "failed to execute clang action: " + llvm::toString(std::move(error))); @@ -483,7 +481,7 @@ auto GenerateAst(Context& context, // diagnostic now. context.emitter().Flush(); - return !trap.hasErrorOccurred(); + return true; } auto FinishAst(Context& context) -> void { diff --git a/toolchain/check/cpp/import.cpp b/toolchain/check/cpp/import.cpp index 62f2428a9708..61512e80e15f 100644 --- a/toolchain/check/cpp/import.cpp +++ b/toolchain/check/cpp/import.cpp @@ -130,19 +130,17 @@ auto ImportCpp(Context& context, llvm::all_of(imports, [&](const Parse::Tree::PackagingNames& import) { return import.package_id == package_id; })); + auto name_scope_id = AddNamespace(context, package_id, imports); - - bool ast_has_error = - !GenerateAst(context, imports, fs, llvm_context, std::move(invocation)); - SemIR::NameScope& name_scope = context.name_scopes().Get(name_scope_id); name_scope.set_is_closed_import(true); - name_scope.set_clang_decl_context_id(context.clang_decls().Add( - {.key = - SemIR::ClangDeclKey(context.ast_context().getTranslationUnitDecl()), - .inst_id = name_scope.inst_id()})); - if (ast_has_error) { + if (GenerateAst(context, imports, fs, llvm_context, std::move(invocation))) { + name_scope.set_clang_decl_context_id(context.clang_decls().Add( + {.key = SemIR::ClangDeclKey( + context.ast_context().getTranslationUnitDecl()), + .inst_id = name_scope.inst_id()})); + } else { name_scope.set_has_error(); } } diff --git a/toolchain/check/testdata/interop/cpp/bad_flags.carbon b/toolchain/check/testdata/interop/cpp/bad_flags.carbon new file mode 100644 index 000000000000..096b3f628a6a --- /dev/null +++ b/toolchain/check/testdata/interop/cpp/bad_flags.carbon @@ -0,0 +1,22 @@ +// 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/none.carbon +// EXTRA-ARGS: --clang-arg=-include-pch --clang-arg=does_not_exist.pch +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/bad_flags.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/bad_flags.carbon +// CHECK:STDERR: error: unable to read PCH file does_not_exist.pch: 'No such file or directory' [CppInteropParseError] +// CHECK:STDERR: +// CHECK:STDERR: error: PCH file 'does_not_exist.pch' not found: module file not found [CppInteropParseError] +// CHECK:STDERR: + +// --- fail_bad_args.carbon + +import Cpp inline ""; + +// Nothing to see here. We're just checking that this does not crash. diff --git a/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon b/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon index 13cdb1c8c38b..ba2d8580ba08 100644 --- a/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon +++ b/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon @@ -338,6 +338,10 @@ library "[[@TEST_NAME]]"; import Cpp library "fix_it_hints.h"; fn F() { + // CHECK:STDERR: fail_import_fix_it_hints.carbon:[[@LINE+4]]:3: error: member name `foo` not found in `Cpp` [MemberNameNotFoundInInstScope] + // CHECK:STDERR: Cpp.foo(); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: Cpp.foo(); } diff --git a/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon b/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon index 70dba8c2cecd..ebdecd792ea7 100644 --- a/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon +++ b/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon @@ -988,7 +988,6 @@ fn F() { // CHECK:STDOUT: %Cpp: = namespace file.%Cpp.import_cpp, [concrete] { // CHECK:STDOUT: .foo = %foo.cpp_overload_set.value // CHECK:STDOUT: import Cpp//... -// CHECK:STDOUT: has_error // CHECK:STDOUT: } // CHECK:STDOUT: %foo.cpp_overload_set.value: %foo.cpp_overload_set.type = cpp_overload_set_value @foo.cpp_overload_set [concrete = constants.%foo.cpp_overload_set.value] // CHECK:STDOUT: %foo.decl: %foo.type = fn_decl @foo [concrete = constants.%foo] { diff --git a/toolchain/diagnostics/emitter.h b/toolchain/diagnostics/emitter.h index e495f84dca46..a21da0507438 100644 --- a/toolchain/diagnostics/emitter.h +++ b/toolchain/diagnostics/emitter.h @@ -164,7 +164,7 @@ class Emitter { // `consumer` is required to outlive the diagnostic emitter. explicit Emitter(Consumer* consumer) : consumer_(consumer) {} - virtual ~Emitter() { Flush(); } + virtual ~Emitter() = default; // Emits an error. // diff --git a/toolchain/diagnostics/emitter_test.cpp b/toolchain/diagnostics/emitter_test.cpp index 4a83d0350e20..40999754b7b0 100644 --- a/toolchain/diagnostics/emitter_test.cpp +++ b/toolchain/diagnostics/emitter_test.cpp @@ -251,9 +251,9 @@ TEST_F(EmitterTest, Flush) { flushed = false; } - // Destroying the emitter should flush. - EXPECT_TRUE(flushed); - flushed = false; + // Destroying the emitter should not flush, as that could call back into the + // base class emitter after the derived-class emitter has been destroyed. + EXPECT_FALSE(flushed); } } // namespace