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 @@ -99,7 +99,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()), @@ -137,8 +137,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; @@ -191,7 +192,7 @@ } void Finish() { - if (ApplyFixes && TotalFixes > 0) { + if (TotalFixes > 0) { Rewriter Rewrite(SourceMgr, LangOpts); for (const auto &FileAndReplacements : FileReplacements) { StringRef File = FileAndReplacements.first(); @@ -291,7 +292,7 @@ SourceManager SourceMgr; llvm::StringMap FileReplacements; ClangTidyContext &Context; - bool ApplyFixes; + FixBehaviour ApplyFixes; unsigned TotalFixes; unsigned AppliedFixes; unsigned WarningsAsErrors; @@ -504,7 +505,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); @@ -531,7 +533,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); @@ -578,7 +580,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 FixIt attached to \p Diagnostic. +/// If \p AnyFix is true and there is no FixIt attached to the Message, +/// returns the first FixIt attached to any notes in the message. +/// If no FixIt is found, returns 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 @@ -262,11 +262,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()) { @@ -376,6 +376,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 @@ -660,7 +678,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/misc/UnusedUsingDeclsCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp --- a/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp @@ -181,10 +181,7 @@ for (const auto &Context : Contexts) { if (!Context.IsUsed) { diag(Context.FoundUsingDecl->getLocation(), "using decl %0 is unused") - << Context.FoundUsingDecl; - // Emit a fix and a fix description of the check; - diag(Context.FoundUsingDecl->getLocation(), - /*Description=*/"remove the using", DiagnosticIDs::Note) + << Context.FoundUsingDecl << FixItHint::CreateRemoval(Context.UsingDeclRange); } } diff --git a/clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp b/clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp @@ -219,21 +219,12 @@ return ParamInfo.SourceName; }; - auto ParamDiag = - Check->diag(Location, - "differing parameters are named here: (%0), in %1: (%2)", - DiagnosticIDs::Level::Note) + Check->diag(Location, + "differing parameters are named here: (%0), in %1: (%2)", + DiagnosticIDs::Level::Note) << joinParameterNames(DifferingParams, ChooseOtherName) << OtherDeclarationDescription << joinParameterNames(DifferingParams, ChooseSourceName); - - for (const DifferingParamInfo &ParamInfo : DifferingParams) { - if (ParamInfo.GenerateFixItHint) { - ParamDiag << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(ParamInfo.OtherNameRange), - ParamInfo.SourceName); - } - } } void formatDiagnosticsForDeclarations( @@ -241,11 +232,24 @@ const FunctionDecl *ParameterSourceDeclaration, const FunctionDecl *OriginalDeclaration, const InconsistentDeclarationsContainer &InconsistentDeclarations) { - Check->diag( - OriginalDeclaration->getLocation(), - "function %q0 has %1 other declaration%s1 with different parameter names") - << OriginalDeclaration - << static_cast(InconsistentDeclarations.size()); + { + auto MainDiag = Check->diag(OriginalDeclaration->getLocation(), + "function %q0 has %1 other declaration%s1 with " + "different parameter names") + << OriginalDeclaration + << static_cast(InconsistentDeclarations.size()); + for (const InconsistentDeclarationInfo &InconsistentDeclaration : + InconsistentDeclarations) { + for (const DifferingParamInfo &ParamInfo : + InconsistentDeclaration.DifferingParams) { + if (ParamInfo.GenerateFixItHint) { + MainDiag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(ParamInfo.OtherNameRange), + ParamInfo.SourceName); + } + } + } + } int Count = 1; for (const InconsistentDeclarationInfo &InconsistentDeclaration : InconsistentDeclarations) { @@ -270,11 +274,21 @@ StringRef FunctionDescription, StringRef ParameterSourceDescription) { for (const InconsistentDeclarationInfo &InconsistentDeclaration : InconsistentDeclarations) { - Check->diag(InconsistentDeclaration.DeclarationLocation, - "%0 %q1 has a %2 with different parameter names") - << FunctionDescription << OriginalDeclaration - << ParameterSourceDescription; - + { + auto MainDiag = + Check->diag(InconsistentDeclaration.DeclarationLocation, + "%0 %q1 has a %2 with different parameter names") + << FunctionDescription << OriginalDeclaration + << ParameterSourceDescription; + for (const DifferingParamInfo &ParamInfo : + InconsistentDeclaration.DifferingParams) { + if (ParamInfo.GenerateFixItHint) { + MainDiag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(ParamInfo.OtherNameRange), + ParamInfo.SourceName); + } + } + } Check->diag(ParameterSourceDeclaration->getLocation(), "the %0 seen here", DiagnosticIDs::Level::Note) << ParameterSourceDescription; 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,14 @@ )"), cl::init(false), cl::cat(ClangTidyCategory)); +static cl::opt FixNotes("fix-notes", cl::desc(R"( +If a warning has no fix, but one of the +notes attached which contain fixes, apply the +fixes found in there. 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 +491,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 +520,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 @@ -67,6 +67,10 @@ Improvements to clang-tidy -------------------------- + - 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. + - Checks that allow configuring names of headers to include now support wrapping the include in angle brackets to create a system include. For example, :doc:`cppcoreguidelines-init-variables 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,11 @@ 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 one of the + notes attached which contain fixes, apply the + fixes found in there. 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.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 @@ -81,7 +81,6 @@ // eol-comments aren't removed (yet) using n::A; // A // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: using decl 'A' is unused -// CHECK-MESSAGES: :[[@LINE-2]]:10: note: remove the using // CHECK-FIXES: {{^}}// A using n::B; using n::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) { } }