[clangd] Send EOF before resetting diagnostics consumer

[kadircet]

Summary

Some clang-tidy checkers, e.g. llvm-include-order can emit diagnostics
at this callback (as mentioned in the comments).

Clangd was resetting diag consumer to IgnoreDiags before sending EOF, hence we
were unable to emit diagnostics for such checkers.

This patch changes the order of that reset and preprocosser event to make sure
we emit that diag.

Fixes https://github.com/clangd/clangd/issues/314.

[sammccall]

can emit diagnostics at this callback (as mentioned in the comments).

I'm a bit puzzled about this. The case that the code handles and the comment refers to is if a check installs PPCallbacks and overrides EndSourceFile. And I believe the existing code is ok for this case: we don't detach those callbacks.

It didn't occur to me that EOF could be observed through the DiagnosticConsumer instead, and this patch does look like a reasonable fix for that. However ClangTidyDiagnosticConsumer doesn't override EndSourceFile nor even the destructor, so it doesn't seem to actually happen.

LLVM-include-order is hooking PPCallbacks::EndOfMainFile, i.e. the first case and the one I thought we were already handling, and I wouldn't expect changing the relative order of EOF and resetting the DC to matter for that.

So I believe you that it fixes something but I'd feel better having some idea of how :-)
(We have a test with a fake clang tidy check IIRC, I wonder if we can test this there)

[kadircet]

In D83178#2131914, @sammccall wrote:

can emit diagnostics at this callback (as mentioned in the comments).

I'm a bit puzzled about this. The case that the code handles and the comment refers to is if a check installs PPCallbacks and overrides EndSourceFile. And I believe the existing code is ok for this case: we don't detach those callbacks.

right, we invoke the callback but we replace the diagnostic consumer client with Clang->getDiagnostics().setClient(new IgnoreDiagnostics); before invoking the callback. Hence whenever the clang-tidy check emits the diagnostics it doesn't get handled by StoreDiags, instead it's received by IgnoreDiagnostics.

It didn't occur to me that EOF could be observed through the DiagnosticConsumer instead, and this patch does look like a reasonable fix for that. However ClangTidyDiagnosticConsumer doesn't override EndSourceFile nor even the destructor, so it doesn't seem to actually happen.

LLVM-include-order is hooking PPCallbacks::EndOfMainFile, i.e. the first case and the one I thought we were already handling, and I wouldn't expect changing the relative order of EOF and resetting the DC to matter for that.

as explained above issue isn't about diagsconsumer (or PPCallbacks::EndOfMainFile which is invoked by PP.EndSourceFile()). It is just about having the no-op diagnostics client registered when https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp#L151 is hit.

So I believe you that it fixes something but I'd feel better having some idea of how :-)
(We have a test with a fake clang tidy check IIRC, I wonder if we can test this there)

I was being lazy :( as that fake tidy check is not really re-usable, so we either extend the test, duplicate the logic or refactor it out. I wanted to go with the last option as (IIRC) we already have 2 tests with fake clang-tidy checkers.
Will do that though!

[sammccall]

In D83178#2131948, @kadircet wrote:

In D83178#2131914, @sammccall wrote:

can emit diagnostics at this callback (as mentioned in the comments).

I'm a bit puzzled about this. The case that the code handles and the comment refers to is if a check installs PPCallbacks and overrides EndSourceFile. And I believe the existing code is ok for this case: we don't detach those callbacks.

right, we invoke the callback but we replace the diagnostic consumer client with Clang->getDiagnostics().setClient(new IgnoreDiagnostics); before invoking the callback. Hence whenever the clang-tidy check emits the diagnostics it doesn't get handled by StoreDiags, instead it's received by IgnoreDiagnostics.

Yeah, hmm... How did this ever work! Obviously I never wrote enough tests here, but that line comes very close to fixing a real issue that I feel sure I manually tested.

Maybe this originally worked and there was another change at some point that broke it.

It didn't occur to me that EOF could be observed through the DiagnosticConsumer instead, and this patch does look like a reasonable fix for that. However ClangTidyDiagnosticConsumer doesn't override EndSourceFile nor even the destructor, so it doesn't seem to actually happen.

(This would possibly be relevant if ClangTidyDiagnosticConsumer were used, in clangd, oops)

(We have a test with a fake clang tidy check IIRC, I wonder if we can test this there)

I was being lazy :( as that fake tidy check is not really re-usable, so we either extend the test, duplicate the logic or refactor it out. I wanted to go with the last option as (IIRC) we already have 2 tests with fake clang-tidy checkers.
Will do that though!

Great to pay down the tech debt here, though the bar of "at least understanding why we're doing it" is met (I was just being slow). And it's not like this change adds any complexity.
So if the trade-off between test complexity vs brittleness vs likely-to-catch-a-bug feels poor that's ok.

Another option is to write a test using a real check that (today, and likely in the future) triggers diagnostics on EOF. I think we use this technique to smoke-test clang-tidy already.

[sammccall]

In D83178#2131976, @sammccall wrote:

Maybe this originally worked and there was another change at some point that broke it.

Initial commit looks pretty broken at first glance though: https://github.com/llvm/llvm-project/commit/98ae975187c75ad17a1fdc3e82a844e9ae3f5c84#diff-90ae535abf6a5bdd201b472a160e3e09

I have no excuses :-)

[njames93]

clang-tools-extra/clangd/ParsedAST.cpp

343

Not directly related to this, but surely this comment is redundant.

[njames93]

We should also disable any header-guard related checks. Currently with this patch llvm-header-guard will fire on every header file as no macros get marked as being used for header guards by the pre processor. I'm guessing this is preamble related.

[sammccall]

In D83178#2132034, @njames93 wrote:

We should also disable any header-guard related checks. Currently with this patch llvm-header-guard will fire on every header file as no macros get marked as being used for header guards by the pre processor. I'm guessing this is preamble related.

That's a good point, shouldn't stop us landing this fix but we should do it carefully.

I have a patch https://reviews.llvm.org/D78038 lying around I could clean up that I suspect fixes this. (I hadn't bothered so far as I hadn't seen any really terrible consequences, but I think this qualifies)

[kadircet]

• Add unittest to DiagnosticsTest via llvm-include-order

[kadircet]

Yeah concerns around header-guard checks are real (and likely to be annoying), so will wait for D78038.

I suppose we can split that into multiple patches, and at least land the bit around preserving conditional stack at the end of preamble, WDYT?

[njames93]

In D83178#2132490, @kadircet wrote:

Yeah concerns around header-guard checks are real (and likely to be annoying), so will wait for D78038.

I suppose we can split that into multiple patches, and at least land the bit around preserving conditional stack at the end of preamble, WDYT?

If there are issues with D78038 then I'd say this could be landed so long as header-guard (and other) checks were manually disabled til that can be sorted.

[kadircet]

As discussed offline D78038 is not going to fix header-guard-check issue, as it relies on not only the PP state, but receiving appropriate callbacks for all of the ifndef/define/endif macros, which is something we don't do in current state.

So sent out D83224 to discuss our options around disabling tidy checks. As for checks affected by this change; there seems to be 3 of them, llvm-include-order-check, llvm-header-guard-check and portability-restrict-system-includes, out
of these 3 only the llvm-header-guard requires access to non-include directives, the rest seems to be fine.

clang-tools-extra/clangd/ParsedAST.cpp

ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
       }
       return DiagLevel;
     });
     Preprocessor *PP = &Clang->getPreprocessor();
     for (const auto &Check : CTChecks) {
       if (!Check->isLanguageVersionSupported(CTContext->getLangOpts()))
         continue;
       // FIXME: the PP callbacks skip the entire preamble.
       // Checks that want to see #includes in the main file do not see them.
       Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
       Check->registerMatchers(&CTFinder);
     }
   }
 
   // Add IncludeFixer which can recover diagnostics caused by missing includes
   // (e.g. incomplete type) and attach include insertion fixes to diagnostics.
   llvm::Optional<IncludeFixer> FixIncludes;
ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
   Clang->getASTContext().setTraversalScope(ParsedDecls);
   {
     // Run the AST-dependent part of the clang-tidy checks.
     // (The preprocessor part ran already, via PPCallbacks).
     trace::Span Tracer("ClangTidyMatch");
     CTFinder.matchAST(Clang->getASTContext());
   }
 
+  // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
+  // However Action->EndSourceFile() would destroy the ASTContext!
+  // So just inform the preprocessor of EOF, while keeping everything alive.
+  Clang->getPreprocessor().EndSourceFile();
   // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
   // has a longer lifetime.
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
   // CompilerInstance won't run this callback, do it directly.
   ASTDiags.EndSourceFile();
-  // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
-  // However Action->EndSourceFile() would destroy the ASTContext!
-  // So just inform the preprocessor of EOF, while keeping everything alive.
-  Clang->getPreprocessor().EndSourceFile();
 
   std::vector<Diag> Diags = CompilerInvocationDiags;
   // Add diagnostics from the preamble, if any.
   if (Preamble)
     Diags.insert(Diags.end(), Preamble->Diags.begin(), Preamble->Diags.end());
   // Finally, add diagnostics coming from the AST.
   {
     std::vector<Diag> D = ASTDiags.take(CTContext.getPointer());

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

 #include "gtest/gtest.h"
 #include <algorithm>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::_;
+using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 
 ::testing::Matcher<const Diag &> WithFix(::testing::Matcher<Fix> FixMatcher) {
EXPECT_THAT(
                    "use a trailing return type for this function"),
               DiagSource(Diag::ClangTidy),
               DiagName("modernize-use-trailing-return-type"),
               // Verify that we don't have "[check-name]" suffix in the message.
               WithFix(FixMessage(
                   "use a trailing return type for this function")))));
 }
 
+TEST(DiagnosticsTest, ClangTidyEOF) {
+  // clang-format off
+  Annotations Test(R"cpp(
+  [[#]]include <b.h>
+  #include "a.h")cpp");
+  // clang-format on
+  auto TU = TestTU::withCode(Test.code());
+  TU.ExtraArgs = {"-isystem."};
+  TU.AdditionalFiles["a.h"] = TU.AdditionalFiles["b.h"] = "";
+  TU.ClangTidyChecks = "-*, llvm-include-order";
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      Contains(AllOf(Diag(Test.range(), "#includes are not sorted properly"),
+                     DiagSource(Diag::ClangTidy),
+                     DiagName("llvm-include-order"))));
+}
+
 TEST(DiagnosticTest, TemplatesInHeaders) {
   // Diagnostics from templates defined in headers are placed at the expansion.
   Annotations Main(R"cpp(
     Derived<int> [[y]]; // error-ok
   )cpp");
   Annotations Header(R"cpp(
     template <typename T>
     struct Derived : [[T]] {};