[clang-tidy] Added command line option `fix-notes`

Added an option to control whether to apply the fixes found in notes attached to clang tidy errors or not.
Diagnostics may contain multiple notes each offering different ways to fix the issue, for that reason the default behaviour should be to not look at fixes found in notes.
Instead offer up all the available fix-its in the output but don't try to apply the first one unless `-fix-notes` is supplied.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D84924

13 files changed, 100 insertions, 37 deletions

diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index dc523d0..f65e8ed 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -95,7 +95,7 @@ private:
 
 class ErrorReporter {
 public:
-  ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
+  ErrorReporter(ClangTidyContext &Context, FixBehaviour ApplyFixes,
                 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
       : Files(FileSystemOptions(), std::move(BaseFS)),
         DiagOpts(new DiagnosticOptions()),
@@ -133,8 +133,9 @@ public:
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
                   << Message.Message << Name;
       // FIXME: explore options to support interactive fix selection.
-      const llvm::StringMap<Replacements> *ChosenFix = selectFirstFix(Error);
-      if (ApplyFixes && ChosenFix) {
+      const llvm::StringMap<Replacements> *ChosenFix;
+      if (ApplyFixes != FB_NoFix &&
+          (ChosenFix = getFixIt(Error, ApplyFixes == FB_FixNotes))) {
         for (const auto &FileAndReplacements : *ChosenFix) {
           for (const auto &Repl : FileAndReplacements.second) {
             ++TotalFixes;
@@ -187,7 +188,7 @@ public:
   }
 
   void finish() {
-    if (ApplyFixes && TotalFixes > 0) {
+    if (TotalFixes > 0) {
       Rewriter Rewrite(SourceMgr, LangOpts);
       for (const auto &FileAndReplacements : FileReplacements) {
         StringRef File = FileAndReplacements.first();
@@ -287,7 +288,7 @@ private:
   SourceManager SourceMgr;
   llvm::StringMap<Replacements> FileReplacements;
   ClangTidyContext &Context;
-  bool ApplyFixes;
+  FixBehaviour ApplyFixes;
   unsigned TotalFixes;
   unsigned AppliedFixes;
   unsigned WarningsAsErrors;
@@ -500,7 +501,8 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
              const CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
              llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
-             bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
+             bool ApplyAnyFix, bool EnableCheckProfile,
+             llvm::StringRef StoreCheckProfile) {
   ClangTool Tool(Compilations, InputFiles,
                  std::make_shared<PCHContainerOperations>(), BaseFS);
 
@@ -527,7 +529,7 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
   Context.setEnableProfiling(EnableCheckProfile);
   Context.setProfileStoragePrefix(StoreCheckProfile);
 
-  ClangTidyDiagnosticConsumer DiagConsumer(Context);
+  ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, true, ApplyAnyFix);
   DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
                        &DiagConsumer, /*ShouldOwnClient=*/false);
   Context.setDiagnosticsEngine(&DE);
@@ -574,7 +576,7 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
 }
 
 void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
-                  ClangTidyContext &Context, bool Fix,
+                  ClangTidyContext &Context, FixBehaviour Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
   ErrorReporter Reporter(Context, Fix, std::move(BaseFS));
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.h b/clang-tools-extra/clang-tidy/ClangTidy.h
index 99dcbb6..bbe4fe6 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.h
+++ b/clang-tools-extra/clang-tidy/ClangTidy.h
@@ -79,17 +79,28 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
              const tooling::CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
              llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
-             bool EnableCheckProfile = false,
+             bool ApplyAnyFix, bool EnableCheckProfile = false,
              llvm::StringRef StoreCheckProfile = StringRef());
 
+/// Controls what kind of fixes clang-tidy is allowed to apply.
+enum FixBehaviour {
+  /// Don't try to apply any fix.
+  FB_NoFix,
+  /// Only apply fixes added to warnings.
+  FB_Fix,
+  /// Apply fixes found in notes.
+  FB_FixNotes
+};
+
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
 //
-/// Displays the found \p Errors to the users. If \p Fix is true, \p
-/// Errors containing fixes are automatically applied and reformatted. If no
-/// clang-format configuration file is found, the given \P FormatStyle is used.
+/// Displays the found \p Errors to the users. If \p Fix is \ref FB_Fix or \ref
+/// FB_FixNotes, \p Errors containing fixes are automatically applied and
+/// reformatted. If no clang-format configuration file is found, the given \P
+/// FormatStyle is used.
 void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
-                  ClangTidyContext &Context, bool Fix,
+                  ClangTidyContext &Context, FixBehaviour Fix,
                   unsigned &WarningsAsErrorsCount,
                   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
 
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 48c5b0b..9e45f0a 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -260,11 +260,11 @@ std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
-    bool RemoveIncompatibleErrors)
+    bool RemoveIncompatibleErrors, bool GetFixesFromNotes)
     : Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
       RemoveIncompatibleErrors(RemoveIncompatibleErrors),
-      LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
-      LastErrorWasIgnored(false) {}
+      GetFixesFromNotes(GetFixesFromNotes), LastErrorRelatesToUserCode(false),
+      LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {}
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
   if (!Errors.empty()) {
@@ -374,6 +374,24 @@ bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                        Context, AllowIO);
 }
 
+const llvm::StringMap<tooling::Replacements> *
+getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) {
+  if (!Diagnostic.Message.Fix.empty())
+    return &Diagnostic.Message.Fix;
+  if (!GetFixFromNotes)
+    return nullptr;
+  const llvm::StringMap<tooling::Replacements> *Result = nullptr;
+  for (const auto &Note : Diagnostic.Notes) {
+    if (!Note.Fix.empty()) {
+      if (Result)
+        // We have 2 different fixes in notes, bail out.
+        return nullptr;
+      Result = &Note.Fix;
+    }
+  }
+  return Result;
+}
+
 } // namespace tidy
 } // namespace clang
 
@@ -658,7 +676,7 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
       std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
       ErrorFixes;
   for (auto &Error : Errors) {
-    if (const auto *Fix = tooling::selectFirstFix(Error))
+    if (const auto *Fix = getFixIt(Error, GetFixesFromNotes))
       ErrorFixes.emplace_back(
           &Error, const_cast<llvm::StringMap<tooling::Replacements> *>(Fix));
   }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index 0792931..13372cc 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -226,6 +226,13 @@ bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
                               const Diagnostic &Info, ClangTidyContext &Context,
                               bool AllowIO = true);
 
+/// Gets the Fix attached to \p Diagnostic.
+/// If there isn't a Fix attached to the diagnostic and \p AnyFix is true, Check
+/// to see if exactly one note has a Fix and return it. Otherwise return
+/// nullptr.
+const llvm::StringMap<tooling::Replacements> *
+getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix);
+
 /// A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
@@ -235,7 +242,8 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
 public:
   ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
                               DiagnosticsEngine *ExternalDiagEngine = nullptr,
-                              bool RemoveIncompatibleErrors = true);
+                              bool RemoveIncompatibleErrors = true,
+                              bool GetFixesFromNotes = false);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
   // more generic and should be pulled out into a more useful Diagnostics
@@ -265,6 +273,7 @@ private:
   ClangTidyContext &Context;
   DiagnosticsEngine *ExternalDiagEngine;
   bool RemoveIncompatibleErrors;
+  bool GetFixesFromNotes;
   std::vector<ClangTidyError> Errors;
   std::unique_ptr<llvm::Regex> HeaderFilter;
   bool LastErrorRelatesToUserCode;
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index 2466b64..6147d90 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -127,6 +127,15 @@ well.
 )"),
                                cl::init(false), cl::cat(ClangTidyCategory));
 
+static cl::opt<bool> FixNotes("fix-notes", cl::desc(R"(
+If a warning has no fix, but a single fix can 
+be found through an associated diagnostic note, 
+apply the fix. 
+Specifying this flag will implicitly enable the 
+'--fix' flag.
+)"),
+                              cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt<std::string> FormatStyle("format-style", cl::desc(R"(
 Style for formatting code around applied fixes:
   - 'none' (default) turns off formatting
@@ -483,18 +492,22 @@ int clangTidyMain(int argc, const char **argv) {
                            AllowEnablingAnalyzerAlphaCheckers);
   std::vector<ClangTidyError> Errors =
       runClangTidy(Context, OptionsParser->getCompilations(), PathList, BaseFS,
-                   EnableCheckProfile, ProfilePrefix);
+                   FixNotes, EnableCheckProfile, ProfilePrefix);
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
                        return E.DiagLevel == ClangTidyError::Error;
                      }) != Errors.end();
 
-  const bool DisableFixes = Fix && FoundErrors && !FixErrors;
+  // --fix-errors and --fix-notes imply --fix.
+  FixBehaviour Behaviour = FixNotes             ? FB_FixNotes
+                           : (Fix || FixErrors) ? FB_Fix
+                                                : FB_NoFix;
+
+  const bool DisableFixes = FoundErrors && !FixErrors;
 
   unsigned WErrorCount = 0;
 
-  // -fix-errors implies -fix.
-  handleErrors(Errors, Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
-               BaseFS);
+  handleErrors(Errors, Context, DisableFixes ? FB_NoFix : Behaviour,
+               WErrorCount, BaseFS);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
     std::error_code EC;
@@ -508,7 +521,7 @@ int clangTidyMain(int argc, const char **argv) {
 
   if (!Quiet) {
     printStats(Context.getStats());
-    if (DisableFixes)
+    if (DisableFixes && Behaviour != FB_NoFix)
       llvm::errs()
           << "Found compiler errors, but -fix-errors was not specified.\n"
              "Fixes have NOT been applied.\n\n";
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 0c7c95d..77a01b2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -70,6 +70,10 @@ Improvements to clang-tidy
 - The `run-clang-tidy.py` helper script is now installed in `bin/` as
   `run-clang-tidy`. It was previously installed in `share/clang/`.
 
+- Added command line option `--fix-notes` to apply fixes found in notes
+  attached to warnings. These are typically cases where we are less confident
+  the fix will have the desired effect.
+
 New checks
 ^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst
index b8af4d3..63b895b 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -173,6 +173,12 @@ An overview of all the command-line options:
                                      errors were found. If compiler errors have
                                      attached fix-its, clang-tidy will apply them as
                                      well.
+    --fix-notes                    - 
+                                     If a warning has no fix, but a single fix can 
+                                     be found through an associated diagnostic note, 
+                                     apply the fix. 
+                                     Specifying this flag will implicitly enable the 
+                                     '--fix' flag.
     --format-style=<string>        -
                                      Style for formatting code around applied fixes:
                                        - 'none' (default) turns off formatting
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp b/clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
index 25009eb..7b01739 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
+// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- --fix-notes
 
 int f() {
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers]
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
index ca17e44..2c2edcf 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++17-or-later %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -isystem %S/Inputs/
+// RUN: %check_clang_tidy -std=c++17-or-later %s misc-unused-using-decls %t -- --fix-notes -- -fno-delayed-template-parsing -isystem %S/Inputs/
 
 namespace ns {
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
index 9511160..297649e 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
@@ -1,5 +1,4 @@
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -isystem %S/Inputs/
-
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -- -fno-delayed-template-parsing -isystem %S/Inputs/
 
 // ----- Definitions -----
 template <typename T> class vector {};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
index f00ebfa..9822432 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- --fix-notes -- -fno-delayed-template-parsing
 
 void consistentFunction(int a, int b, int c);
 void consistentFunction(int a, int b, int c);
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
index d5cee68..9498a72 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
@@ -1,9 +1,10 @@
-// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t
+// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t -- --fix-notes
 void foo(int a) {
   if (a = 1) {
-  // CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
-  // CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
-  // CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
-  // CHECK-FIXES: if ((a = 1)) {
+    // CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
+    // CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
+    // CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
+    // As we have 2 conflicting fixes in notes, don't apply any fix.
+    // CHECK-FIXES: if (a = 1) {
   }
 }
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
index 15873ed..bb22308 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
@@ -1,6 +1,6 @@
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -format-style=none --
-// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -format-style=llvm --
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -format-style=none --
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -format-style=llvm --
 namespace a { class A {}; }
 namespace b {
 using a::A;