This is an archive of the discontinued LLVM Phabricator instance.

Continue emitting diagnostics after a fatal error
Needs ReviewPublic

Authored by Dmitry.Kozhevnikov on Aug 8 2018, 9:17 AM.

Details

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.

Diff Detail

Event Timeline

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.

Add a unit test which explicitly builds preamble/AST

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.

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 marked an inline comment as done.Aug 26 2018, 12:52 PM

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.

nridge added a subscriber: nridge.May 28 2021, 12:58 AM