Continue emitting diagnostics after a fatal error

[Dmitry.Kozhevnikov]

Summary

By default, diagnostics are suppressed after a fatal error. Some fatal errors (notably, "file not found" for include directive) are common for incomplete code and we probably want to have further diagnostics anyway.

Currently, this flag is optionally set by libclang (see CXTranslationUnit_KeepGoing option).

There are also a bunch of related problems when AST is not fully built in presence of fatal errors (templates are not instantiated and include directives are not processed), I'll address these in separate patches.

[Dmitry.Kozhevnikov]

See also https://reviews.llvm.org/D50462

[ilya-biryukov]

The new behavior looks reasonable for interactive tools like clangd, but I'm a bit worried that clang may emit too many irrelevant errors, because it may not cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that mode.
Nevertheless, I think this is moving clangd in the right direction and if we see too many irrelevant errors, we should look into improving clang to show less of those.

test/clangd/missing-includes.test

1

This tests looks useful, but could we also unit-test (with a C++ gtest) that fatal errors do not block other errors when:

1. building the preamble (i.e. when calling buildPreamble),
2. building the AST (i.e. when calling ParsedAST::build).

To have a regression test for potential changes in those areas.

[Dmitry.Kozhevnikov]

Add a unit test which explicitly builds preamble/AST

[Dmitry.Kozhevnikov]

In D50455#1193468, @ilya-biryukov wrote:

I'm a bit worried that clang may emit too many irrelevant errors, because it may not cope nicely with those fatal errors, i.e. wasn't tested that thoroughly in that mode.

It sort of happens already: even if you have a fatal error in a preamble, diagnostics for the main file are still produced. This just expands it for the case the fatal error and the rest of the code happen to exist both in the preamble/main file.

Nevertheless, I think this is moving clangd in the right direction and if we see too many irrelevant errors, we should look into improving clang to show less of those.

We've enabled it in CLion by default, hopefully, we'll get some feedback.

[ilya-biryukov]

Sorry for the delay with this one

unittests/clangd/ClangdTests.cpp

1002 ↗

(On Diff #160837)

Just reuse PreambleCI?

1009 ↗

(On Diff #160837)

Maybe fold all asserts into one, e.g.:

EXPECT_THAT(AST->getDiagnostics(), ElementsAre(Field(&Diag::Message, HasSubstr("preamble1")), ...)

Could be made shorter by introducing a matcher for Field(&Diag::Message, HasSubstr(...

[Dmitry.Kozhevnikov]

In D50455#1193468, @ilya-biryukov wrote:

Nevertheless, I think this is moving clangd in the right direction and if we see too many irrelevant errors, we should look into improving clang to show less of those.

A more conservative fix (demoting "include file not found" from fatal errors) was proposed in https://reviews.llvm.org/D50462, I'll try exploring that instead.

clangd/ClangdUnit.cpp

std::shared_ptr<const PreambleData> clangd::buildPreamble(
   // Skip function bodies when building the preamble to speed up building
   // the preamble and make it smaller.
   assert(!CI.getFrontendOpts().SkipFunctionBodies);
   CI.getFrontendOpts().SkipFunctionBodies = true;
   // We don't want to write comment locations into PCH. They are racy and slow
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
+  PreambleDiagsEngine->setSuppressAfterFatalError(false);
+
   CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback);
   if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
     log("Couldn't set working directory when building the preamble.");
     // We proceed anyway, our lit-tests rely on results for non-existing working
     // dirs.
   }
   auto BuiltPreamble = PrecompiledPreamble::Build(
       CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine, Inputs.FS, PCHs,

clangd/Compiler.cpp

prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
   } else {
     CI->getPreprocessorOpts().addRemappedFile(
         CI->getFrontendOpts().Inputs[0].getFile(), Buffer.get());
   }
 
   auto Clang = llvm::make_unique<CompilerInstance>(PCHs);
   Clang->setInvocation(std::move(CI));
   Clang->createDiagnostics(&DiagsClient, false);
+  Clang->getDiagnostics().setSuppressAfterFatalError(false);
 
   if (auto VFSWithRemapping = createVFSFromCompilerInvocation(
           Clang->getInvocation(), Clang->getDiagnostics(), VFS))
     VFS = VFSWithRemapping;
   Clang->setVirtualFileSystem(VFS);
 
   Clang->setTarget(TargetInfo::CreateTargetInfo(
       Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));

test/clangd/missing-includes.test

+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"#include <a>\n#include <b>\nint x;\n#include <c>\n#include <d>\n"}}}
+# CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK:  "message": "'a' file not found",
+# CHECK:  "message": "'b' file not found",
+# CHECK:  "message": "'c' file not found",
+# CHECK:  "message": "'d' file not found",
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}