Skip to content

Commit abbe9e2

Browse files
njames93njames93
committed
[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
1 parent 0131498 commit abbe9e2

13 files changed

+100
-37
lines changed

‎clang-tools-extra/clang-tidy/ClangTidy.cpp

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ class AnalyzerDiagnosticConsumer : public ento::PathDiagnosticConsumer {
9595

9696
class ErrorReporter {
9797
public:
98-
ErrorReporter(ClangTidyContext &Context, bool ApplyFixes,
98+
ErrorReporter(ClangTidyContext &Context, FixBehaviour ApplyFixes,
9999
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
100100
: Files(FileSystemOptions(), std::move(BaseFS)),
101101
DiagOpts(new DiagnosticOptions()),
@@ -133,8 +133,9 @@ class ErrorReporter {
133133
auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
134134
<< Message.Message << Name;
135135
// FIXME: explore options to support interactive fix selection.
136-
const llvm::StringMap<Replacements> *ChosenFix = selectFirstFix(Error);
137-
if (ApplyFixes && ChosenFix) {
136+
const llvm::StringMap<Replacements> *ChosenFix;
137+
if (ApplyFixes != FB_NoFix &&
138+
(ChosenFix = getFixIt(Error, ApplyFixes == FB_FixNotes))) {
138139
for (const auto &FileAndReplacements : *ChosenFix) {
139140
for (const auto &Repl : FileAndReplacements.second) {
140141
++TotalFixes;
@@ -187,7 +188,7 @@ class ErrorReporter {
187188
}
188189

189190
void finish() {
190-
if (ApplyFixes && TotalFixes > 0) {
191+
if (TotalFixes > 0) {
191192
Rewriter Rewrite(SourceMgr, LangOpts);
192193
for (const auto &FileAndReplacements : FileReplacements) {
193194
StringRef File = FileAndReplacements.first();
@@ -287,7 +288,7 @@ class ErrorReporter {
287288
SourceManager SourceMgr;
288289
llvm::StringMap<Replacements> FileReplacements;
289290
ClangTidyContext &Context;
290-
bool ApplyFixes;
291+
FixBehaviour ApplyFixes;
291292
unsigned TotalFixes;
292293
unsigned AppliedFixes;
293294
unsigned WarningsAsErrors;
@@ -500,7 +501,8 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
500501
const CompilationDatabase &Compilations,
501502
ArrayRef<std::string> InputFiles,
502503
llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
503-
bool EnableCheckProfile, llvm::StringRef StoreCheckProfile) {
504+
bool ApplyAnyFix, bool EnableCheckProfile,
505+
llvm::StringRef StoreCheckProfile) {
504506
ClangTool Tool(Compilations, InputFiles,
505507
std::make_shared<PCHContainerOperations>(), BaseFS);
506508

@@ -527,7 +529,7 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
527529
Context.setEnableProfiling(EnableCheckProfile);
528530
Context.setProfileStoragePrefix(StoreCheckProfile);
529531

530-
ClangTidyDiagnosticConsumer DiagConsumer(Context);
532+
ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, true, ApplyAnyFix);
531533
DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
532534
&DiagConsumer, /*ShouldOwnClient=*/false);
533535
Context.setDiagnosticsEngine(&DE);
@@ -574,7 +576,7 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
574576
}
575577

576578
void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
577-
ClangTidyContext &Context, bool Fix,
579+
ClangTidyContext &Context, FixBehaviour Fix,
578580
unsigned &WarningsAsErrorsCount,
579581
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS) {
580582
ErrorReporter Reporter(Context, Fix, std::move(BaseFS));

‎clang-tools-extra/clang-tidy/ClangTidy.h

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,28 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
7979
const tooling::CompilationDatabase &Compilations,
8080
ArrayRef<std::string> InputFiles,
8181
llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> BaseFS,
82-
bool EnableCheckProfile = false,
82+
bool ApplyAnyFix, bool EnableCheckProfile = false,
8383
llvm::StringRef StoreCheckProfile = StringRef());
8484

85+
/// Controls what kind of fixes clang-tidy is allowed to apply.
86+
enum FixBehaviour {
87+
/// Don't try to apply any fix.
88+
FB_NoFix,
89+
/// Only apply fixes added to warnings.
90+
FB_Fix,
91+
/// Apply fixes found in notes.
92+
FB_FixNotes
93+
};
94+
8595
// FIXME: This interface will need to be significantly extended to be useful.
8696
// FIXME: Implement confidence levels for displaying/fixing errors.
8797
//
88-
/// Displays the found \p Errors to the users. If \p Fix is true, \p
89-
/// Errors containing fixes are automatically applied and reformatted. If no
90-
/// clang-format configuration file is found, the given \P FormatStyle is used.
98+
/// Displays the found \p Errors to the users. If \p Fix is \ref FB_Fix or \ref
99+
/// FB_FixNotes, \p Errors containing fixes are automatically applied and
100+
/// reformatted. If no clang-format configuration file is found, the given \P
101+
/// FormatStyle is used.
91102
void handleErrors(llvm::ArrayRef<ClangTidyError> Errors,
92-
ClangTidyContext &Context, bool Fix,
103+
ClangTidyContext &Context, FixBehaviour Fix,
93104
unsigned &WarningsAsErrorsCount,
94105
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
95106

‎clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -260,11 +260,11 @@ std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
260260

261261
ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
262262
ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
263-
bool RemoveIncompatibleErrors)
263+
bool RemoveIncompatibleErrors, bool GetFixesFromNotes)
264264
: Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
265265
RemoveIncompatibleErrors(RemoveIncompatibleErrors),
266-
LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
267-
LastErrorWasIgnored(false) {}
266+
GetFixesFromNotes(GetFixesFromNotes), LastErrorRelatesToUserCode(false),
267+
LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {}
268268

269269
void ClangTidyDiagnosticConsumer::finalizeLastError() {
270270
if (!Errors.empty()) {
@@ -374,6 +374,24 @@ bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
374374
Context, AllowIO);
375375
}
376376

377+
const llvm::StringMap<tooling::Replacements> *
378+
getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) {
379+
if (!Diagnostic.Message.Fix.empty())
380+
return &Diagnostic.Message.Fix;
381+
if (!GetFixFromNotes)
382+
return nullptr;
383+
const llvm::StringMap<tooling::Replacements> *Result = nullptr;
384+
for (const auto &Note : Diagnostic.Notes) {
385+
if (!Note.Fix.empty()) {
386+
if (Result)
387+
// We have 2 different fixes in notes, bail out.
388+
return nullptr;
389+
Result = &Note.Fix;
390+
}
391+
}
392+
return Result;
393+
}
394+
377395
} // namespace tidy
378396
} // namespace clang
379397

@@ -658,7 +676,7 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
658676
std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
659677
ErrorFixes;
660678
for (auto &Error : Errors) {
661-
if (const auto *Fix = tooling::selectFirstFix(Error))
679+
if (const auto *Fix = getFixIt(Error, GetFixesFromNotes))
662680
ErrorFixes.emplace_back(
663681
&Error, const_cast<llvm::StringMap<tooling::Replacements> *>(Fix));
664682
}

‎clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,13 @@ bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
226226
const Diagnostic &Info, ClangTidyContext &Context,
227227
bool AllowIO = true);
228228

229+
/// Gets the Fix attached to \p Diagnostic.
230+
/// If there isn't a Fix attached to the diagnostic and \p AnyFix is true, Check
231+
/// to see if exactly one note has a Fix and return it. Otherwise return
232+
/// nullptr.
233+
const llvm::StringMap<tooling::Replacements> *
234+
getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix);
235+
229236
/// A diagnostic consumer that turns each \c Diagnostic into a
230237
/// \c SourceManager-independent \c ClangTidyError.
231238
//
@@ -235,7 +242,8 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
235242
public:
236243
ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
237244
DiagnosticsEngine *ExternalDiagEngine = nullptr,
238-
bool RemoveIncompatibleErrors = true);
245+
bool RemoveIncompatibleErrors = true,
246+
bool GetFixesFromNotes = false);
239247

240248
// FIXME: The concept of converting between FixItHints and Replacements is
241249
// more generic and should be pulled out into a more useful Diagnostics
@@ -265,6 +273,7 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
265273
ClangTidyContext &Context;
266274
DiagnosticsEngine *ExternalDiagEngine;
267275
bool RemoveIncompatibleErrors;
276+
bool GetFixesFromNotes;
268277
std::vector<ClangTidyError> Errors;
269278
std::unique_ptr<llvm::Regex> HeaderFilter;
270279
bool LastErrorRelatesToUserCode;

‎clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,15 @@ well.
127127
)"),
128128
cl::init(false), cl::cat(ClangTidyCategory));
129129

130+
static cl::opt<bool> FixNotes("fix-notes", cl::desc(R"(
131+
If a warning has no fix, but a single fix can
132+
be found through an associated diagnostic note,
133+
apply the fix.
134+
Specifying this flag will implicitly enable the
135+
'--fix' flag.
136+
)"),
137+
cl::init(false), cl::cat(ClangTidyCategory));
138+
130139
static cl::opt<std::string> FormatStyle("format-style", cl::desc(R"(
131140
Style for formatting code around applied fixes:
132141
- 'none' (default) turns off formatting
@@ -483,18 +492,22 @@ int clangTidyMain(int argc, const char **argv) {
483492
AllowEnablingAnalyzerAlphaCheckers);
484493
std::vector<ClangTidyError> Errors =
485494
runClangTidy(Context, OptionsParser->getCompilations(), PathList, BaseFS,
486-
EnableCheckProfile, ProfilePrefix);
495+
FixNotes, EnableCheckProfile, ProfilePrefix);
487496
bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
488497
return E.DiagLevel == ClangTidyError::Error;
489498
}) != Errors.end();
490499

491-
const bool DisableFixes = Fix && FoundErrors && !FixErrors;
500+
// --fix-errors and --fix-notes imply --fix.
501+
FixBehaviour Behaviour = FixNotes ? FB_FixNotes
502+
: (Fix || FixErrors) ? FB_Fix
503+
: FB_NoFix;
504+
505+
const bool DisableFixes = FoundErrors && !FixErrors;
492506

493507
unsigned WErrorCount = 0;
494508

495-
// -fix-errors implies -fix.
496-
handleErrors(Errors, Context, (FixErrors || Fix) && !DisableFixes, WErrorCount,
497-
BaseFS);
509+
handleErrors(Errors, Context, DisableFixes ? FB_NoFix : Behaviour,
510+
WErrorCount, BaseFS);
498511

499512
if (!ExportFixes.empty() && !Errors.empty()) {
500513
std::error_code EC;
@@ -508,7 +521,7 @@ int clangTidyMain(int argc, const char **argv) {
508521

509522
if (!Quiet) {
510523
printStats(Context.getStats());
511-
if (DisableFixes)
524+
if (DisableFixes && Behaviour != FB_NoFix)
512525
llvm::errs()
513526
<< "Found compiler errors, but -fix-errors was not specified.\n"
514527
"Fixes have NOT been applied.\n\n";

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ Improvements to clang-tidy
7070
- The `run-clang-tidy.py` helper script is now installed in `bin/` as
7171
`run-clang-tidy`. It was previously installed in `share/clang/`.
7272

73+
- Added command line option `--fix-notes` to apply fixes found in notes
74+
attached to warnings. These are typically cases where we are less confident
75+
the fix will have the desired effect.
76+
7377
New checks
7478
^^^^^^^^^^
7579

‎clang-tools-extra/docs/clang-tidy/index.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,12 @@ An overview of all the command-line options:
173173
errors were found. If compiler errors have
174174
attached fix-its, clang-tidy will apply them as
175175
well.
176+
--fix-notes -
177+
If a warning has no fix, but a single fix can
178+
be found through an associated diagnostic note,
179+
apply the fix.
180+
Specifying this flag will implicitly enable the
181+
'--fix' flag.
176182
--format-style=<string> -
177183
Style for formatting code around applied fixes:
178184
- 'none' (default) turns off formatting

‎clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %check_clang_tidy %s misc-definitions-in-headers %t
1+
// RUN: %check_clang_tidy %s misc-definitions-in-headers %t -- --fix-notes
22

33
int f() {
44
// 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]

‎clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls-cxx17.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %check_clang_tidy -std=c++17-or-later %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -isystem %S/Inputs/
1+
// RUN: %check_clang_tidy -std=c++17-or-later %s misc-unused-using-decls %t -- --fix-notes -- -fno-delayed-template-parsing -isystem %S/Inputs/
22

33
namespace ns {
44

‎clang-tools-extra/test/clang-tidy/checkers/misc-unused-using-decls.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -- -fno-delayed-template-parsing -isystem %S/Inputs/
2-
1+
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -- -fno-delayed-template-parsing -isystem %S/Inputs/
32

43
// ----- Definitions -----
54
template <typename T> class vector {};

‎clang-tools-extra/test/clang-tidy/checkers/readability-inconsistent-declaration-parameter-name.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- -- -fno-delayed-template-parsing
1+
// RUN: %check_clang_tidy %s readability-inconsistent-declaration-parameter-name %t -- --fix-notes -- -fno-delayed-template-parsing
22

33
void consistentFunction(int a, int b, int c);
44
void consistentFunction(int a, int b, int c);

‎clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t
1+
// RUN: %check_clang_tidy %s "llvm-namespace-comment,clang-diagnostic-*" %t -- --fix-notes
22
void foo(int a) {
33
if (a = 1) {
4-
// CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
5-
// CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
6-
// CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
7-
// CHECK-FIXES: if ((a = 1)) {
4+
// CHECK-NOTES: [[@LINE-1]]:9: warning: using the result of an assignment as a condition without parentheses [clang-diagnostic-parentheses]
5+
// CHECK-NOTES: [[@LINE-2]]:9: note: place parentheses around the assignment to silence this warning
6+
// CHECK-NOTES: [[@LINE-3]]:9: note: use '==' to turn this assignment into an equality comparison
7+
// As we have 2 conflicting fixes in notes, don't apply any fix.
8+
// CHECK-FIXES: if (a = 1) {
89
}
910
}

‎clang-tools-extra/test/clang-tidy/infrastructure/clean-up-code.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
// RUN: %check_clang_tidy %s misc-unused-using-decls %t
2-
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -format-style=none --
3-
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- -format-style=llvm --
1+
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes
2+
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -format-style=none --
3+
// RUN: %check_clang_tidy %s misc-unused-using-decls %t -- --fix-notes -format-style=llvm --
44
namespace a { class A {}; }
55
namespace b {
66
using a::A;

0 commit comments

Comments
 (0)