[PATCH] D83178: [clangd] Send EOF before resetting diagnostics consumer

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

In D83178#2131948 <https://reviews.llvm.org/D83178#2131948>, @kadircet wrote:

> In D83178#2131914 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83178/new/

https://reviews.llvm.org/D83178