Don't crash if clang setup fails. (#6804)

Defer creating the CppContext until we have all of its components, so
that we know they're not null. Don't track the action on the context,
since it's not a reliable way of getting back to the compiler invocation
on failure. Don't flush the diagnostics emitter from the emitter
destructor since the derived class emitter will already have been
destroyed at that point. Distinguish between clang setup failing and
clang merely producing errors, and don't connect the check context to
clang if clang setup failed.

---------

Co-authored-by: David Blaikie <dblaikie@gmail.com>

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.

toolchain/check/cpp/context.cpp

@@ -8,8 +8,11 @@
 
 namespace Carbon::Check {
 
-CppContext::CppContext(std::unique_ptr<clang::FrontendAction> action)
-    : action_(std::move(action)) {}
+CppContext::CppContext(clang::CompilerInstance& instance,
+                       std::unique_ptr<clang::Parser> parser)
+    : ast_context_(&instance.getASTContext()),
+      sema_(&instance.getSema()),
+      parser_(std::move(parser)) {}
 
 CppContext::~CppContext() = default;
 

toolchain/check/cpp/context.h

@@ -23,24 +23,13 @@ namespace Carbon::Check {
 // declarations, and similar values.
 class CppContext {
  public:
-  explicit CppContext(std::unique_ptr<clang::FrontendAction> action);
+  explicit CppContext(clang::CompilerInstance& instance,
+                      std::unique_ptr<clang::Parser> 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<clang::Parser> 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<clang::FrontendAction> action_;
+  // The Clang AST context.
+  clang::ASTContext* ast_context_;
+
+  // The Clang semantic analysis engine.
+  clang::Sema* sema_;
+
+  // The Clang parser.
+  std::unique_ptr<clang::Parser> 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::MangleContext> clang_mangle_context_;
-
-  // The Clang parser.
-  std::unique_ptr<clang::Parser> parser_;
 };
 
 }  // namespace Carbon::Check

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<clang::Parser>(
+    auto parser_ptr = std::make_unique<clang::Parser>(
         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<CppContext>(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<clang::CompilerInstance>(invocation);
   auto& clang_instance = *clang_instance_ptr;
@@ -464,15 +465,12 @@ auto GenerateAst(Context& context,
     return false;
   }
 
-  context.set_cpp_context(std::make_unique<CppContext>(
-      std::make_unique<GenerateASTAction>(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 {

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));
 
+  auto name_scope_id = AddNamespace(context, package_id, imports);
   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();
   }
 }

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.

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();
 }
 

toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon

@@ -988,7 +988,6 @@ fn F() {
 // CHECK:STDOUT:   %Cpp: <namespace> = 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] {

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.
   //

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