[clang-tools-extra] abbe9e2 - [clang-tidy] Added command line option `fix-notes`
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 1 14:07:22 PST 2021
Author: Nathan James
Date: 2021-03-01T22:07:11Z
New Revision: abbe9e227ed31e5dde9bb7567bb9f0dd047689c6
URL: https://github.com/llvm/llvm-project/commit/abbe9e227ed31e5dde9bb7567bb9f0dd047689c6
DIFF: https://github.com/llvm/llvm-project/commit/abbe9e227ed31e5dde9bb7567bb9f0dd047689c6.diff
LOG: [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
Added:
Modified:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidy.h
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/index.rst
clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp
clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp
clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index dc523d0731d5..f65e8ed216f2 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -95,7 +95,7 @@ class AnalyzerDiagnosticConsumer : public ento::PathDiagnosticConsumer {
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 @@ class ErrorReporter {
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 @@ class ErrorReporter {
}
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 @@ class ErrorReporter {
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 99dcbb6a269a..bbe4fe69123f 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 48c5b0b2f4b7..9e45f0aea798 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
diff erent 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 079293149e90..13372cc626b9 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 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
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 2466b647c68c..6147d90eb10b 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 0c7c95dbce3e..77a01b2001aa 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 b8af4d34e203..63b895b7c011 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 25009ebe2bbd..7b017391d5ac 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 ca17e44be8e6..2c2edcf4df77 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 9511160bc2f7..297649e0abce 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 f00ebfa896a5..982243255dd0 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 d5cee68d9b18..9498a726078b 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 15873ed168dc..bb2230803cea 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;
More information about the cfe-commits
mailing list