Page MenuHomePhabricator

[clang-tidy] Make the plugin honor NOLINT
ClosedPublic

Authored by nik on May 3 2019, 3:58 AM.

Details

Summary

Instantiate a ClangTidyDiagnosticConsumer also for the plugin case and
let it forward the diagnostics to the external diagnostic engine that is
already in place.

One minor difference to the clang-tidy executable case is that the
compiler checks/diagnostics are referred to with their original name.
For example, for -Wunused-variable the plugin will refer to the check as
"-Wunused-variable" while the clang-tidy executable will refer to that
as "clang-diagnostic- unused-variable". This is because the compiler
diagnostics never reach ClangTidyDiagnosticConsumer.

Diff Detail

Repository
rL LLVM

Event Timeline

nik created this revision.May 3 2019, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 3:58 AM
nik added a reviewer: alexfh.May 3 2019, 4:01 AM
nik added a reviewer: bkramer.
alexfh added a comment.May 3 2019, 4:52 AM

Apart from NOLINT handling there's more logic in ClangTidyDiagnosticConsumer::HandleDiagnostic, which isn't properly transferred to ClangTidyContext::diag in this patch. The logic that is transferred seems to change the behavior w.r.t. notes that can "unmute" ignored warnings (see https://reviews.llvm.org/D59135#1456108). I suspect that we're missing proper test coverage here. Another issue is that compiler diagnostics don't pass ClangTidyContext::diag in the non-plugin use case. Do all the existing tests pass with your patch?

A better way to implement diagnostic filtering in the plugin would be to make ClangTidyDiagnosticConsumer able to forward diagnostics to the external diagnostics engine. It will still need some sort of a buffering though to handle diagnostics and notes attached to them together.

nik added a comment.May 3 2019, 5:42 AM

Thanks for the fast comments!

I suspect that we're missing proper test coverage here.

True, ideally all the test scripts would also trigger the plugin case code path.

I've started with a modified copy of nolint.cpp as I wasn't able to have the two invocations (clang-tidy, c-index-test plugin) work with the same file. For example, the order of the diagnostics is different (seems solvable by appending using -DAG) and the c-index-test plugin case does not output "Suppressed 13 warnings (13 NOLINT)" - is there a way to exclude the check for this output for the c-index-test case and still having all in the same file?

I haven't tested the plugin case with nolintnextline.cpp yet, but at least this one does not seem to contain the unmute case as far as I see.

Another issue is that compiler diagnostics don't pass ClangTidyContext::diag in the non-plugin use case.

Right, but how is this an issue?

Do all the existing tests pass with your patch?

Yes.

A better way to implement diagnostic filtering in the plugin would be to make ClangTidyDiagnosticConsumer able to forward diagnostics to the external diagnostics engine. It will still need some sort of a buffering though to handle diagnostics and notes attached to them together.

Ahh, you suggest that the plugin should have a separate diagnostic engine and instantiate also a ClangTidyDiagnosticConsumer object, that forwards to the external one? Will check how this could work.

nik updated this revision to Diff 198637.May 8 2019, 6:13 AM

The plugin makes use of ClangTidyDiagnosticConsumer and forwards diagnostics to
the external diagnostic engine.

@alexfh: What do you think? If that looks roughly OK for you, I'll finish it.

nik edited the summary of this revision. (Show Details)May 8 2019, 6:14 AM
nik added a comment.May 15 2019, 4:41 AM

Ping. alexfh?

gribozavr added inline comments.May 22 2019, 3:25 AM
clang-tidy/ClangTidyDiagnosticConsumer.cpp
457 ↗(On Diff #198637)

Indentation is 2 spaces.

472 ↗(On Diff #198637)

It seems like the checkFilters call should not be skipped even if we have another diagnostic engine.

clang-tidy/ClangTidyDiagnosticConsumer.h
197 ↗(On Diff #198637)

Sorry, it is unclear what this struct is, and what its members are, especially given that the member names are L and string...

nik marked 4 inline comments as done.May 22 2019, 7:29 AM

As I've commented on, this change is not finished. However, I've addressed the inline comments nevertheless.

There is one TODO left for which I would like to have an opinion.

clang-tidy/ClangTidyDiagnosticConsumer.cpp
472 ↗(On Diff #198637)

checkFilters() sets some state so that later finalizeLastError() can remove diagnostics/errors from ClangTidyDiagnosticConsumer::Errors and also track some statistics.

Statistics are not relevant for the pluginc case as they should not be printed out for that case.
The filtering happening in finalizeLastError() does not look relevant as the plugin currently only supports the "-checks=..." argument but not the e.g. header and line filter. When checks are created in ClangTidyCheckFactories::createChecks, the "-checks=..." argument is honored, so that the !Context.isCheckEnabled(...) in finalizeLastError() is also not relevant.

I agree that checkFilters()/finalizeLastError() will be needed once the plugin case should support the other filtering options (header + line filter), but note that this goes beyond the scope of this change here, which is NOLINT filtering.

This is all new to me, so if I miss something regarding the checkFilters()/finalizeLastError() thingy, please tell me, preferably with an example if possible :)

nik updated this revision to Diff 200736.May 22 2019, 7:30 AM
nik marked an inline comment as done.

Addressed comments.

gribozavr added inline comments.Jun 3 2019, 1:54 AM
clang-tidy/ClangTidyDiagnosticConsumer.cpp
207 ↗(On Diff #200736)

Did you consider adding this query API to DiagnosticIDs instead? To me it looks weird that creation of custom diag IDs is handled by one class (DiagnosticIDs), and queries -- by another (ClangTidyContext).

472 ↗(On Diff #198637)

It might be not relevant right now, but that code has invariants about how those two functions are called. Even if things happen to work, I think this code is too complicated to allow breaking those invariants in some cases.

test/clang-tidy/nolintnextline-plugin.cpp
47 ↗(On Diff #200736)

Why not on the first line?

nik marked 4 inline comments as done.Jun 6 2019, 1:56 AM
nik added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.cpp
207 ↗(On Diff #200736)

Huch, that query API is already there. For some reason, I've missed it.

Good catch, thanks! :)

test/clang-tidy/nolintnextline-plugin.cpp
47 ↗(On Diff #200736)

As this file is based on test/clang-tidy/nolint.cpp, I left it at is. But having it at top looks more common, so I've moved it there now.

nik updated this revision to Diff 203306.Jun 6 2019, 1:57 AM
nik marked 2 inline comments as done.

Addressed inline comments.

gribozavr accepted this revision.Jun 6 2019, 5:10 AM
This revision is now accepted and ready to land.Jun 6 2019, 5:10 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 6:11 AM
nik added a comment.Jun 6 2019, 6:13 AM

Thanks Dmitri!