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

[hokein]

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
}

[hokein]

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

[Eugene.Zelenko]

clang-tidy/ClangTidyCheck.h

109

return {} could be used instead.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

266

return {} could be used instead.

274

return {} could be used instead.

[Eugene.Zelenko]

clang-tidy/ClangTidyCheck.h

107

I think Clang and Clang Static Analyzer should be capitalized.

[JonasToth]

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).

[Eugene.Zelenko]

In D59932#1446533, @JonasToth wrote:

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]

Update the patch:

• move FixDescriptions to tooling diagnostics, YAML serialization support
• add tests

[hokein]

In D59932#1446533, @JonasToth wrote:

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

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

[MyDeveloperDay]

In D59932#1446533, @JonasToth wrote:

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]

clang-tools-extra/clang-tidy/ClangTidy.cpp

75 ↗

(On Diff #193070)

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.

[hokein]

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]

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]

Emit the check fix description via diagnostic::note.

[hokein]

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

[alexfh]

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 #193510)

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

132 ↗

(On Diff #193510)

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

133 ↗

(On Diff #193510)

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

144 ↗

(On Diff #193510)

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

187 ↗

(On Diff #193510)

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

262 ↗

(On Diff #193510)

nit: Early "continue" would make sense here.

if (!Repl.isApplicable())
  continue;
...

clang/include/clang/Tooling/Core/Diagnostic.h

46 ↗

(On Diff #193510)

Some information from the original comment was lost here:

/// Fixes to apply, grouped by file path.

[hokein]

Fix apply-replacements, and address comments.

[hokein]

Remove a stale comment.

[hokein]

In D59932#1453565, @alexfh wrote:

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 #193510)

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

144 ↗

(On Diff #193510)

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.

187 ↗

(On Diff #193510)

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]

clang/include/clang/Tooling/Core/Diagnostic.h

67–71 ↗

(On Diff #193856)

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]

clang/include/clang/Tooling/Core/Diagnostic.h

67–71 ↗

(On Diff #193856)

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

[alexfh]

clang/include/clang/Tooling/Core/Diagnostic.h

67–71 ↗

(On Diff #193856)

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]

clang/include/clang/Tooling/Core/Diagnostic.h

67–71 ↗

(On Diff #193856)

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

Diagnostic.h seems the most suitable place.

[hokein]

Update and rebase.

[hokein]

clang/include/clang/Tooling/Core/Diagnostic.h

67–71 ↗

(On Diff #193856)

moved it out of the structure;

[alexfh]

Thanks! The change looks good now.

clang-tidy/ClangTidy.cpp

public:
   AnalyzerDiagnosticConsumer(ClangTidyContext &Context) : Context(Context) {}
 
   void FlushDiagnosticsImpl(std::vector<const ento::PathDiagnostic *> &Diags,
                             FilesMade *filesMade) override {
     for (const ento::PathDiagnostic *PD : Diags) {
       SmallString<64> CheckName(AnalyzerCheckNamePrefix);
       CheckName += PD->getCheckName();
       Context.diag(CheckName, PD->getLocation().asLocation(),
-                   PD->getShortDescription())
+                   PD->getShortDescription(), /*FixDescription*/ "")
           << PD->path.back()->getRanges();
 
       for (const auto &DiagPiece :
            PD->path.flatten(/*ShouldFlattenMacros=*/true)) {
         Context.diag(CheckName, DiagPiece->getLocation().asLocation(),
-                     DiagPiece->getString(), DiagnosticIDs::Note)
+                     DiagPiece->getString(), /*FixDescription*/ "",
+                     DiagnosticIDs::Note)
             << DiagPiece->getRanges();
       }
     }
   }
 
   StringRef getName() const override { return "ClangTidyDiags"; }
   bool supportsLogicalOpControlFlow() const override { return true; }
   bool supportsCrossFileDiagnostics() const override { return true; }

clang-tidy/ClangTidyCheck.h

public:
   /// store these as members of the derived class. However, note that all
   /// matches occur in the order of the AST traversal.
   virtual void registerMatchers(ast_matchers::MatchFinder *Finder) {}
 
   /// \brief ``ClangTidyChecks`` that register ASTMatchers should do the actual
   /// work in here.
   virtual void check(const ast_matchers::MatchFinder::MatchResult &Result) {}
 
+  /// A short human readable description about the check fix. It should
+  /// descripe what the check fix will do, so users can rely on it to know the
+  /// fix intention without viewing the actual code change.
+  ///
+  /// Example: "Convert to 'switch'"; "Removed unused using"; "Use make_unique".
+  ///
+  /// The description will be attached to ``ClangTidyError``. Returning empty
+  /// string indicates the check doesn't provide this information.
+  ///
+  /// Caveats:
+  ///   - The interface may not fit well with a check that produces different
+  ///     fixes for different cases.
+  ///   - clang compiler diagnostics and clang static analyzer diagnostics are
+  ///     not supported.
+  virtual llvm::StringRef fixDescription() { return ""; }
+
   /// \brief Add a diagnostic with the check's name.
   DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
   /// \brief Should store all options supported by this check with their
   /// current values or default values for options that haven't been overridden.
   ///
   /// The check should use ``Options.store()`` to store each option it supports

clang-tidy/ClangTidyCheck.cpp

ClangTidyCheck::ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context)
     : CheckName(CheckName), Context(Context),
       Options(CheckName, Context->getOptions().CheckOptions) {
   assert(Context != nullptr);
   assert(!CheckName.empty());
 }
 
 DiagnosticBuilder ClangTidyCheck::diag(SourceLocation Loc, StringRef Message,
                                        DiagnosticIDs::Level Level) {
-  return Context->diag(CheckName, Loc, Message, Level);
+  return Context->diag(CheckName, Loc, Message, fixDescription(), Level);
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   // For historical reasons, checks don't implement the MatchFinder run()
   // callback directly. We keep the run()/check() distinction to avoid interface
   // churn, and to allow us to add cross-cutting logic in the future.
   check(Result);
 }

clang-tidy/ClangTidyDiagnosticConsumer.h

 
 #include "ClangTidyOptions.h"
 #include "ClangTidyProfiling.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/Refactoring.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/Timer.h"
 
 namespace clang {
 
 class ASTContext;
 class CompilerInstance;

 /// dependency on a SourceManager.
 ///
 /// FIXME: Make Diagnostics flexible enough to support this directly.
 struct ClangTidyError : tooling::Diagnostic {
   ClangTidyError(StringRef CheckName, Level DiagLevel, StringRef BuildDirectory,
                  bool IsWarningAsError);
 
   bool IsWarningAsError;
+  // A short descrpition of the clang-tidy fix.
+  // Empty if the ClangTidyError doesn't provide any fixes.
+  //
+  // FIXME: this should move to tooling::Diagnostic.
+  std::string FixDescription;
 };
 
 /// \brief Read-only set of strings represented as a list of positive and
 /// negative globs. Positive globs add all matched strings to the set, negative
 /// globs remove them in the order of appearance in the list.
 class GlobList {
 public:
   /// \brief \p GlobList is a comma-separated list of globs (only '*'
public:
   ~ClangTidyContext();
 
   /// \brief Report any errors detected using this method.
   ///
   /// This is still under heavy development and will likely change towards using
   /// tablegen'd diagnostic IDs.
   /// FIXME: Figure out a way to manage ID spaces.
   DiagnosticBuilder diag(StringRef CheckName, SourceLocation Loc,
-                         StringRef Message,
+                         StringRef Message, StringRef CheckFixDescription,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
   /// \brief Sets the \c SourceManager of the used \c DiagnosticsEngine.
   ///
   /// This is called from the \c ClangTidyCheck base class.
   void setSourceManager(SourceManager *SourceMgr);
 
   /// \brief Should be called when starting to process new translation unit.
   void setCurrentFile(StringRef File);
 
   /// \brief Returns the main file name of the current translation unit.
   StringRef getCurrentFile() const { return CurrentFile; }
 
   /// \brief Sets ASTContext for the current translation unit.
   void setASTContext(ASTContext *Context);
 
   /// \brief Gets the language options from the AST context.
   const LangOptions &getLangOpts() const { return LangOpts; }
 
   /// \brief Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
   std::string getCheckName(unsigned DiagnosticID) const;
 
+  /// \brief Returns the fix description of the clang-tidy check which produced
+  /// this diagnostic ID.
+  std::string getCheckFixDescription(unsigned DiagnosticID) const;
+
   /// \brief Returns \c true if the check is enabled for the \c CurrentFile.
   ///
   /// The \c CurrentFile can be changed using \c setCurrentFile.
   bool isCheckEnabled(StringRef CheckName) const;
 
   /// \brief Returns \c true if the check should be upgraded to error for the
   /// \c CurrentFile.
   bool treatAsError(StringRef CheckName) const;
private:
   std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
 
   LangOptions LangOpts;
 
   ClangTidyStats Stats;
 
   std::string CurrentBuildDirectory;
 
-  llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
+  llvm::DenseMap<unsigned, std::pair<std::string, std::string>>
+      CheckNamesByDiagnosticID;
 
   bool Profile;
   std::string ProfilePrefix;
 
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a

clang-tidy/ClangTidyDiagnosticConsumer.cpp

ClangTidyContext::ClangTidyContext(
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
 }
 
 ClangTidyContext::~ClangTidyContext() = default;
 
 DiagnosticBuilder ClangTidyContext::diag(
     StringRef CheckName, SourceLocation Loc, StringRef Description,
+    StringRef CheckFixDescription,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
   assert(Loc.isValid());
   unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
       Level, (Description + " [" + CheckName + "]").str());
-  CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
+  CheckNamesByDiagnosticID[ID] = {CheckName, CheckFixDescription};
   return DiagEngine->Report(Loc, ID);
 }
 
 void ClangTidyContext::setSourceManager(SourceManager *SourceMgr) {
   DiagEngine->setSourceManager(SourceMgr);
 }
 
 void ClangTidyContext::setCurrentFile(StringRef File) {
bool ClangTidyContext::treatAsError(StringRef CheckName) const {
   return WarningAsErrorFilter->contains(CheckName);
 }
 
 std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   std::string ClangWarningOption =
       DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
   if (!ClangWarningOption.empty())
     return "clang-diagnostic-" + ClangWarningOption;
-  llvm::DenseMap<unsigned, std::string>::const_iterator I =
-      CheckNamesByDiagnosticID.find(DiagnosticID);
+  auto I = CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
-    return I->second;
+    return I->second.first;
+  return "";
+}
+
+std::string
+ClangTidyContext::getCheckFixDescription(unsigned DiagnosticID) const {
+  auto I = CheckNamesByDiagnosticID.find(DiagnosticID);
+  if (I != CheckNamesByDiagnosticID.end())
+    return I->second.second;
   return "";
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
     : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
       LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
       LastErrorWasIgnored(false) {}
 
ClangTidyDiagnosticRenderer Converter(
       Errors.back());
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);
   FullSourceLoc Loc;
   if (Info.getLocation().isValid() && Info.hasSourceManager())
     Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
   Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
                            Info.getFixItHints());
+  // Populate the fix description if the diagnostic has fixes.
+  if (!Errors.back().Fix.empty())
+    Errors.back().FixDescription = Context.getCheckFixDescription(Info.getID());
 
   if (Info.hasSourceManager())
     checkFilters(Info.getLocation(), Info.getSourceManager());
 }
 
 bool ClangTidyDiagnosticConsumer::passesLineFilter(StringRef FileName,
                                                    unsigned LineNumber) const {
   if (Context.getGlobalOptions().LineFilter.empty())