diff --git a/clang-tools-extra/clang-tidy/ClangTidy.h b/clang-tools-extra/clang-tidy/ClangTidy.h --- a/clang-tools-extra/clang-tidy/ClangTidy.h +++ b/clang-tools-extra/clang-tidy/ClangTidy.h @@ -79,17 +79,28 @@ const tooling::CompilationDatabase &Compilations, ArrayRef InputFiles, llvm::IntrusiveRefCntPtr 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 Errors, - ClangTidyContext &Context, bool Fix, + ClangTidyContext &Context, FixBehaviour Fix, unsigned &WarningsAsErrorsCount, llvm::IntrusiveRefCntPtr BaseFS); diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -95,7 +95,7 @@ class ErrorReporter { public: - ErrorReporter(ClangTidyContext &Context, bool ApplyFixes, + ErrorReporter(ClangTidyContext &Context, FixBehaviour ApplyFixes, llvm::IntrusiveRefCntPtr BaseFS) : Files(FileSystemOptions(), std::move(BaseFS)), DiagOpts(new DiagnosticOptions()), @@ -133,8 +133,9 @@ auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]")) << Message.Message << Name; // FIXME: explore options to support interactive fix selection. - const llvm::StringMap *ChosenFix = selectFirstFix(Error); - if (ApplyFixes && ChosenFix) { + const llvm::StringMap *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 @@ } 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 @@ SourceManager SourceMgr; llvm::StringMap FileReplacements; ClangTidyContext &Context; - bool ApplyFixes; + FixBehaviour ApplyFixes; unsigned TotalFixes; unsigned AppliedFixes; unsigned WarningsAsErrors; @@ -500,7 +501,8 @@ const CompilationDatabase &Compilations, ArrayRef InputFiles, llvm::IntrusiveRefCntPtr BaseFS, - bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) { + bool ApplyAnyFix, bool EnableCheckProfile, + llvm::StringRef StoreCheckProfile) { ClangTool Tool(Compilations, InputFiles, std::make_shared(), BaseFS); @@ -527,7 +529,7 @@ 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 @@ } void handleErrors(llvm::ArrayRef Errors, - ClangTidyContext &Context, bool Fix, + ClangTidyContext &Context, FixBehaviour Fix, unsigned &WarningsAsErrorsCount, llvm::IntrusiveRefCntPtr BaseFS) { ErrorReporter Reporter(Context, Fix, std::move(BaseFS)); diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -226,6 +226,13 @@ 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 * +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 @@ 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 @@ ClangTidyContext &Context; DiagnosticsEngine *ExternalDiagEngine; bool RemoveIncompatibleErrors; + bool GetFixesFromNotes; std::vector Errors; std::unique_ptr HeaderFilter; bool LastErrorRelatesToUserCode; diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -260,11 +260,11 @@ 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 @@ Context, AllowIO); } +const llvm::StringMap * +getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) { + if (!Diagnostic.Message.Fix.empty()) + return &Diagnostic.Message.Fix; + if (!GetFixFromNotes) + return nullptr; + const llvm::StringMap *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 @@ std::pair *>> 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 *>(Fix)); } diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp --- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp +++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp @@ -127,6 +127,15 @@ )"), cl::init(false), cl::cat(ClangTidyCategory)); +static cl::opt 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 FormatStyle("format-style", cl::desc(R"( Style for formatting code around applied fixes: - 'none' (default) turns off formatting @@ -483,18 +492,22 @@ AllowEnablingAnalyzerAlphaCheckers); std::vector 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 @@ 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 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -70,6 +70,10 @@ - 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 --- a/clang-tools-extra/docs/clang-tidy/index.rst +++ b/clang-tools-extra/docs/clang-tidy/index.rst @@ -173,6 +173,12 @@ 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= - 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 --- 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 --- 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 --- 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 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 --- 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 --- 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 --- 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;