Page MenuHomePhabricator

[clang-tidy] Add fix descriptions to clang-tidy checks.
ClosedPublic

Authored by hokein on Mar 28 2019, 7:13 AM.

Details

Summary

Motivation/Context: in the code review system integrating with clang-tidy,
clang-tidy doesn't provide a human-readable description of the fix. Usually
developers have to preview a code diff (before vs after apply the fix) to
understand what the fix does before applying a fix.

This patch proposes that each clang-tidy check provides a short and
actional fix description that can be shown in the UI, so that users can know
what the fix does without previewing diff.

This patch extends clang-tidy framework to support fix descriptions (will add implementations for
existing checks in the future). Fix descriptions and fixes are emitted via diagnostic::Note (rather than
attaching the main warning diagnostic).

Before this patch:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
}

After:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning"); // Emit a check warning
   diag(loc, "fix description", DiagnosticIDs::Note) << FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}

Event Timeline

hokein created this revision.Mar 28 2019, 7:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 7:13 AM
hokein retitled this revision from [clang-tidy] **Prototype**: Add fix descrption to clang-tidy checks. to [clang-tidy] **Prototype**: Add fix description to clang-tidy checks..Mar 28 2019, 7:21 AM
hokein edited the summary of this revision. (Show Details)
hokein added reviewers: sammccall, alexfh.
hokein added subscribers: JonasToth, aaron.ballman.

This is my first attempt, still missing tests, but it should be in a shape to get early feedbacks.

Eugene.Zelenko added inline comments.
clang-tidy/ClangTidyCheck.h
109 ↗(On Diff #192627)

return {} could be used instead.

clang-tidy/ClangTidyDiagnosticConsumer.cpp
266 ↗(On Diff #192627)

return {} could be used instead.

274 ↗(On Diff #192627)

return {} could be used instead.

Eugene.Zelenko edited projects, added Restricted Project; removed Restricted Project.Mar 28 2019, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2019, 8:46 AM
clang-tidy/ClangTidyCheck.h
107 ↗(On Diff #192627)

I think Clang and Clang Static Analyzer should be capitalized.

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone and cert differ from readability for example).

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone and cert differ from readability for example).

Also Clazy split checks by severity level.

hokein updated this revision to Diff 193070.Apr 1 2019, 5:44 AM
hokein marked 2 inline comments as done.

Update the patch:

  • move FixDescriptions to tooling diagnostics, YAML serialization support
  • add tests
hokein added a comment.Apr 1 2019, 5:45 AM

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

I know that code-checker and code-compass have something like this to signal importance of problems (say bugprone and cert differ from readability for example).

Thanks. Unfortunately, we have to iterate all checks no matter which solution we use ;(

Adding a severity to checks is an interesting idea, we need to define the semantic of the severity, looks like different analyzers define them in different ways (some defined by check quality, like stable enough/production ready; some defined by importance of the problem that the check find). And the existing modules of clang-tidy checks address this problem in some degree (like bugprone, readability modules). I think it is a separate topic that needs more thoughts and discussions.

clang-tidy/ClangTidyCheck.h
109 ↗(On Diff #192627)

yes, {} works here, but I'd use "" which is more explicit.

I think the idea is good and implementation, too. If we iterate all checks anyway (probably?) we could think about adding a severity to the checks, too?

In a similar vein, I often feel like I'd like to be able to define a AllowFixIt on every checker, so I could turn off the FixIts for things we just want to warn on.

I often run clang-tidy with -fix but I don't want to necessarily want clang-tidy to fix everything I have marked as -check. readability-identifier-naming is one such checker that isn't mature enough to get everything correct, I want to see the warning to prevent new violations from creeping in, but don't actually want it to do a fixit.

However I don't want to have to keep commenting the check out of my .clang-tidy file just so I can run with -fix, it would be great if we could add *.AllowFixIt

i.e.

- key:             <checkername>-naming.AllowFixIt
  value:           'true/false'

To allow us to disable FixIts for certain checkers

- key:             readability-identifier-naming.AllowFixIt
  value:           'false'

Just and idea, sorry for hijacking the thread.

alexfh requested changes to this revision.Apr 2 2019, 2:25 AM
alexfh added inline comments.
clang-tools-extra/clang-tidy/ClangTidy.cpp
76 ↗(On Diff #194817)

nit: Use the format that the bugprone-argument-comment check understands: /*FixDescription=*/

Same below.

clang-tools-extra/clang-tidy/ClangTidyCheck.h
109 ↗(On Diff #193070)

Checks can provide different fixes with different semantics depending on the context. It seems incorrect to assume that there can be one reasonable description for all of them.

This revision now requires changes to proceed.Apr 2 2019, 2:25 AM
hokein marked an inline comment as done.Apr 2 2019, 6:18 AM
hokein added inline comments.
clang-tools-extra/clang-tidy/ClangTidyCheck.h
109 ↗(On Diff #193070)

Yes, as mentioned in the comment, this is the limitation of this approach -- I assume we don't have a large number of these checks, most checks are providing a single fix.

Another alternative is to use some heuristic mechanisms to do a translation from the Replacement, like we do in clangd, https://github.com/llvm-mirror/clang-tools-extra/blob/master/clangd/Diagnostics.cpp#L350.

hokein marked an inline comment as done.Apr 3 2019, 2:39 AM

As discussed offline, the current approach only works for checks provide a single fix, providing such API is somehow misleading.

Instead, we'd emit the check fix and the fix description via diagnostic::Note, rather than attaching to the main diagnostic of the check:

  • match the data model of the checks provide different semantic fixes depending on the context;
  • align with the way how existing clang diagnostics emit alternative fixes (via diagnostic::Note);
  • open a door to write clang-tidy checks that provide alternative fixes; we don't have these checks at the moment, but some clang diagnostics like clang-diagnostic-parentheses do (and our current implementation just aggregates all the fixes together, which is not correct);

It would require some changes in clang-tidy check side:

Before this patch:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
}

After this patch:

void MyCheck::check(...) {
   ...
   diag(loc, "my check warning"); // Emit a check warning
   // We might want to introduce an utility method like `diagFix` to save some verbosed words.
   diag(loc, "fix description", DiagnosticIDs::Note) << FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}

An example of unused-using-decls check (clang-tidy command line output)

  • before:
1 warning generated.
/tmp/test.cpp:8:12: warning: using decl 'Foo' is unused [misc-unused-using-decls]
using foo::Foo;
~~~~~~~~~~~^~~~
  • after:
1 warning generated.
/tmp/test.cpp:8:12: warning: using decl 'Foo' is unused [misc-unused-using-decls]
using foo::Foo;
           ^
/tmp/test.cpp:8:12: note: remove the using
using foo::Foo;
~~~~~~~~~~~^~~~
hokein updated this revision to Diff 193510.Apr 3 2019, 8:20 AM

Emit the check fix description via diagnostic::note.

hokein added a comment.Apr 3 2019, 8:24 AM

Running out of time today, the patch is not finished yet, but it should be good for another initial review/comments.

This looks like a more promising direction. Thanks for the readiness to experiment with this.

See the comments inline.

clang-tools-extra/clang-tidy/ClangTidy.cpp
130 ↗(On Diff #194817)

Could you leave a FIXME here to explore options around interactive fix selection?

132 ↗(On Diff #194817)

ErrorFix brings more questions than it answers. Maybe SelectedFix, ChosenFix, or just Fix?

133 ↗(On Diff #194817)

nit: I'd add braces here, since the else branch has them.

144 ↗(On Diff #194817)

The nesting level starts getting out of control here. I'd try to pull the loop into a function.

174 ↗(On Diff #194817)

Why Error.Message.Fix? Should this use *SelectedFix instead?

249 ↗(On Diff #194817)

nit: Early "continue" would make sense here.

if (!Repl.isApplicable())
  continue;
...
clang/include/clang/Tooling/Core/Diagnostic.h
46 ↗(On Diff #194817)

Some information from the original comment was lost here:

/// Fixes to apply, grouped by file path.
hokein updated this revision to Diff 193856.Apr 5 2019, 4:59 AM
hokein marked 8 inline comments as done.

Fix apply-replacements, and address comments.

hokein updated this revision to Diff 193857.Apr 5 2019, 5:06 AM

Remove a stale comment.

hokein added a comment.Apr 5 2019, 5:06 AM

This looks like a more promising direction. Thanks for the readiness to experiment with this.

See the comments inline.

Thanks for the comments. Now all existing tests are passed, the patch is in a good shape for review.

There is one missing point -- we don't test the fix-description notes in the lit tests, the current test mechanism (CHECK-MESSAGE, CHECK-NOTES) doesn't handle it well, we need to feature it out. I think this can be done in a separate patch.

clang-tools-extra/clang-tidy/ClangTidy.cpp
130 ↗(On Diff #194817)

Done. I moved this code to Diagnostic::getChosenFix() as we have a few places using it.

144 ↗(On Diff #194817)

Agree, but making such refactoring change in this patch will add noise to the diff, I tend to make the change in this patch as mini as possible.

I think this can be improved in a follow-up change.

174 ↗(On Diff #194817)

I think we still want to report all the alternative fixes to clients to align with clang's behavior. We only use the SelectedFix when applying the fix.

alexfh added inline comments.Apr 5 2019, 5:06 AM
clang/include/clang/Tooling/Core/Diagnostic.h
67–71 ↗(On Diff #194817)

Do we actually need this method here? This whole structure is sort of a data-only serialization helper, and this method is adding some (arbitrary) logic into it.

hokein added inline comments.Apr 5 2019, 5:16 AM
clang/include/clang/Tooling/Core/Diagnostic.h
67–71 ↗(On Diff #194817)

We have a few places using this method, or we could move this method out of this structure?

alexfh added inline comments.Apr 5 2019, 5:50 AM
clang/include/clang/Tooling/Core/Diagnostic.h
67–71 ↗(On Diff #194817)

I'd move it out closer to the code which uses it and call it "selectFirstFix", for example (the name should be a verb and it should be less vague about what the function is doing).

hokein marked an inline comment as done.Apr 5 2019, 6:04 AM
hokein added inline comments.
clang/include/clang/Tooling/Core/Diagnostic.h
67–71 ↗(On Diff #194817)

hmm, this function is used in ApplyReplacements.cpp, ClangTidy.cpp, ClangTidyTest.h, ClangTidyDiagnosticConsumer.cpp.

Diagnostic.h seems the most suitable place.

hokein updated this revision to Diff 194817.Apr 12 2019, 1:51 AM
hokein marked an inline comment as done.

Update and rebase.

hokein added inline comments.Apr 12 2019, 1:52 AM
clang/include/clang/Tooling/Core/Diagnostic.h
67–71 ↗(On Diff #194817)

moved it out of the structure;

alexfh accepted this revision.Apr 15 2019, 7:51 AM

Thanks! The change looks good now.

This revision is now accepted and ready to land.Apr 15 2019, 7:51 AM
hokein retitled this revision from [clang-tidy] **Prototype**: Add fix description to clang-tidy checks. to [clang-tidy] Add fix descriptions to clang-tidy checks..Apr 16 2019, 7:32 AM
hokein edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.