Page MenuHomePhabricator

[clangd] Send EOF before resetting diagnostics consumer
ClosedPublic

Authored by kadircet on Jul 5 2020, 12:14 PM.

Details

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.

Diff Detail

Event Timeline

kadircet created this revision.Jul 5 2020, 12:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2020, 12:14 PM

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)

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 accepted this revision.Jul 5 2020, 2:29 PM

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.

This revision is now accepted and ready to land.Jul 5 2020, 2:29 PM

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 added inline comments.
clang-tools-extra/clangd/ParsedAST.cpp
343

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

njames93 added a comment.EditedJul 5 2020, 4:24 PM

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.

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 updated this revision to Diff 275627.Jul 6 2020, 2:52 AM
  • Add unittest to DiagnosticsTest via llvm-include-order

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?

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.

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.

This revision was automatically updated to reflect the committed changes.