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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
