Skip to content

Commit f2879d8

Browse files
hokeinhokein
committed
[clang-tidy] Add fix descriptions to clang-tidy checks.
Summary: Motivation/Context: in the code review system integrating with clang-tidy, clang-tidy doesn't provide a human-readable description of the fix. Usually developers have to preview a code diff (before vs after apply the fix) to understand what the fix does before applying a fix. This patch proposes that each clang-tidy check provides a short and actional fix description that can be shown in the UI, so that users can know what the fix does without previewing diff. This patch extends clang-tidy framework to support fix descriptions (will add implementations for existing checks in the future). Fix descriptions and fixes are emitted via diagnostic::Note (rather than attaching the main warning diagnostic). Before this patch: ``` void MyCheck::check(...) { ... diag(loc, "my check warning") << FixtItHint::CreateReplacement(...); } ``` After: ``` void MyCheck::check(...) { ... diag(loc, "my check warning"); // Emit a check warning diag(loc, "fix description", DiagnosticIDs::Note) << FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix } ``` Reviewers: sammccall, alexfh Reviewed By: alexfh Subscribers: MyDeveloperDay, Eugene.Zelenko, aaron.ballman, JonasToth, xazax.hun, jdoerfert, cfe-commits Tags: #clang-tools-extra, #clang Differential Revision: https://reviews.llvm.org/D59932 llvm-svn: 358576
1 parent 641caa5 commit f2879d8

File tree

24 files changed

+337
-299
lines changed

24 files changed

+337
-299
lines changed

‎clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "clang/Format/Format.h"
2020
#include "clang/Lex/Lexer.h"
2121
#include "clang/Rewrite/Core/Rewriter.h"
22+
#include "clang/Tooling/Core/Diagnostic.h"
2223
#include "clang/Tooling/DiagnosticsYaml.h"
2324
#include "clang/Tooling/ReplacementsYaml.h"
2425
#include "llvm/ADT/ArrayRef.h"
@@ -169,9 +170,11 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
169170

170171
for (const auto &TU : TUDs)
171172
for (const auto &D : TU.Diagnostics)
172-
for (const auto &Fix : D.Fix)
173-
for (const tooling::Replacement &R : Fix.second)
174-
AddToGroup(R, true);
173+
if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
174+
for (const auto &Fix : *ChoosenFix)
175+
for (const tooling::Replacement &R : Fix.second)
176+
AddToGroup(R, true);
177+
}
175178

176179
// Sort replacements per file to keep consistent behavior when
177180
// clang-apply-replacements run on differents machine.

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

Lines changed: 39 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "clang/Lex/Preprocessor.h"
3636
#include "clang/Rewrite/Frontend/FixItRewriter.h"
3737
#include "clang/Rewrite/Frontend/FrontendActions.h"
38+
#include "clang/Tooling/Core/Diagnostic.h"
3839
#if CLANG_ENABLE_STATIC_ANALYZER
3940
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
4041
#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
@@ -125,15 +126,17 @@ class ErrorReporter {
125126
}
126127
auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
127128
<< Message.Message << Name;
128-
for (const auto &FileAndReplacements : Error.Fix) {
129-
for (const auto &Repl : FileAndReplacements.second) {
130-
SourceLocation FixLoc;
131-
++TotalFixes;
132-
bool CanBeApplied = false;
133-
if (Repl.isApplicable()) {
134-
SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
135-
Files.makeAbsolutePath(FixAbsoluteFilePath);
136-
if (ApplyFixes) {
129+
// FIXME: explore options to support interactive fix selection.
130+
const llvm::StringMap<Replacements> *ChosenFix = selectFirstFix(Error);
131+
if (ApplyFixes && ChosenFix) {
132+
for (const auto &FileAndReplacements : *ChosenFix) {
133+
for (const auto &Repl : FileAndReplacements.second) {
134+
++TotalFixes;
135+
bool CanBeApplied = false;
136+
if (Repl.isApplicable()) {
137+
SourceLocation FixLoc;
138+
SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
139+
Files.makeAbsolutePath(FixAbsoluteFilePath);
137140
tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
138141
Repl.getLength(),
139142
Repl.getReplacementText());
@@ -158,28 +161,17 @@ class ErrorReporter {
158161
llvm::errs()
159162
<< "Can't resolve conflict, skipping the replacement.\n";
160163
}
161-
162164
} else {
163165
CanBeApplied = true;
164166
++AppliedFixes;
165167
}
168+
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
169+
FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
166170
}
167-
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
168-
SourceLocation FixEndLoc =
169-
FixLoc.getLocWithOffset(Repl.getLength());
170-
// Retrieve the source range for applicable fixes. Macro definitions
171-
// on the command line have locations in a virtual buffer and don't
172-
// have valid file paths and are therefore not applicable.
173-
CharSourceRange Range =
174-
CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
175-
Diag << FixItHint::CreateReplacement(Range,
176-
Repl.getReplacementText());
177171
}
178-
179-
if (ApplyFixes)
180-
FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
181172
}
182173
}
174+
reportFix(Diag, Error.Message.Fix);
183175
}
184176
for (auto Fix : FixLocations) {
185177
Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
@@ -250,10 +242,33 @@ class ErrorReporter {
250242
return SourceMgr.getLocForStartOfFile(ID).getLocWithOffset(Offset);
251243
}
252244

245+
void reportFix(const DiagnosticBuilder &Diag,
246+
const llvm::StringMap<Replacements> &Fix) {
247+
for (const auto &FileAndReplacements : Fix) {
248+
for (const auto &Repl : FileAndReplacements.second) {
249+
if (!Repl.isApplicable())
250+
continue;
251+
SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
252+
Files.makeAbsolutePath(FixAbsoluteFilePath);
253+
SourceLocation FixLoc =
254+
getLocation(FixAbsoluteFilePath, Repl.getOffset());
255+
SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Repl.getLength());
256+
// Retrieve the source range for applicable fixes. Macro definitions
257+
// on the command line have locations in a virtual buffer and don't
258+
// have valid file paths and are therefore not applicable.
259+
CharSourceRange Range =
260+
CharSourceRange::getCharRange(SourceRange(FixLoc, FixEndLoc));
261+
Diag << FixItHint::CreateReplacement(Range, Repl.getReplacementText());
262+
}
263+
}
264+
}
265+
253266
void reportNote(const tooling::DiagnosticMessage &Message) {
254267
SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
255-
Diags.Report(Loc, Diags.getCustomDiagID(DiagnosticsEngine::Note, "%0"))
268+
auto Diag =
269+
Diags.Report(Loc, Diags.getCustomDiagID(DiagnosticsEngine::Note, "%0"))
256270
<< Message.Message;
271+
reportFix(Diag, Message.Fix);
257272
}
258273

259274
FileManager Files;

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

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,10 @@
1818
#include "ClangTidyDiagnosticConsumer.h"
1919
#include "ClangTidyOptions.h"
2020
#include "clang/AST/ASTDiagnostic.h"
21+
#include "clang/Basic/Diagnostic.h"
2122
#include "clang/Basic/DiagnosticOptions.h"
2223
#include "clang/Frontend/DiagnosticRenderer.h"
24+
#include "clang/Tooling/Core/Diagnostic.h"
2325
#include "llvm/ADT/STLExtras.h"
2426
#include "llvm/ADT/SmallString.h"
2527
#include <tuple>
@@ -68,6 +70,9 @@ class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
6870
SmallVectorImpl<CharSourceRange> &Ranges,
6971
ArrayRef<FixItHint> Hints) override {
7072
assert(Loc.isValid());
73+
tooling::DiagnosticMessage *DiagWithFix =
74+
Level == DiagnosticsEngine::Note ? &Error.Notes.back() : &Error.Message;
75+
7176
for (const auto &FixIt : Hints) {
7277
CharSourceRange Range = FixIt.RemoveRange;
7378
assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
@@ -77,7 +82,8 @@ class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
7782

7883
tooling::Replacement Replacement(Loc.getManager(), Range,
7984
FixIt.CodeToInsert);
80-
llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
85+
llvm::Error Err =
86+
DiagWithFix->Fix[Replacement.getFilePath()].add(Replacement);
8187
// FIXME: better error handling (at least, don't let other replacements be
8288
// applied).
8389
if (Err) {
@@ -581,9 +587,17 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
581587

582588
// Compute error sizes.
583589
std::vector<int> Sizes;
584-
for (const auto &Error : Errors) {
590+
std::vector<
591+
std::pair<ClangTidyError *, llvm::StringMap<tooling::Replacements> *>>
592+
ErrorFixes;
593+
for (auto &Error : Errors) {
594+
if (const auto *Fix = tooling::selectFirstFix(Error))
595+
ErrorFixes.emplace_back(
596+
&Error, const_cast<llvm::StringMap<tooling::Replacements> *>(Fix));
597+
}
598+
for (const auto &ErrorAndFix : ErrorFixes) {
585599
int Size = 0;
586-
for (const auto &FileAndReplaces : Error.Fix) {
600+
for (const auto &FileAndReplaces : *ErrorAndFix.second) {
587601
for (const auto &Replace : FileAndReplaces.second)
588602
Size += Replace.getLength();
589603
}
@@ -592,8 +606,8 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
592606

593607
// Build events from error intervals.
594608
std::map<std::string, std::vector<Event>> FileEvents;
595-
for (unsigned I = 0; I < Errors.size(); ++I) {
596-
for (const auto &FileAndReplace : Errors[I].Fix) {
609+
for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
610+
for (const auto &FileAndReplace : *ErrorFixes[I].second) {
597611
for (const auto &Replace : FileAndReplace.second) {
598612
unsigned Begin = Replace.getOffset();
599613
unsigned End = Begin + Replace.getLength();
@@ -608,7 +622,7 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
608622
}
609623
}
610624

611-
std::vector<bool> Apply(Errors.size(), true);
625+
std::vector<bool> Apply(ErrorFixes.size(), true);
612626
for (auto &FileAndEvents : FileEvents) {
613627
std::vector<Event> &Events = FileAndEvents.second;
614628
// Sweep.
@@ -627,10 +641,10 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
627641
assert(OpenIntervals == 0 && "Amount of begin/end points doesn't match");
628642
}
629643

630-
for (unsigned I = 0; I < Errors.size(); ++I) {
644+
for (unsigned I = 0; I < ErrorFixes.size(); ++I) {
631645
if (!Apply[I]) {
632-
Errors[I].Fix.clear();
633-
Errors[I].Notes.emplace_back(
646+
ErrorFixes[I].second->clear();
647+
ErrorFixes[I].first->Notes.emplace_back(
634648
"this fix will not be applied because it overlaps with another fix");
635649
}
636650
}

‎clang-tools-extra/clang-tidy/add_new_check.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,8 @@ def write_implementation(module_path, module, check_name_camel):
137137
if (MatchedDecl->getName().startswith("awesome_"))
138138
return;
139139
diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
140-
<< MatchedDecl
140+
<< MatchedDecl;
141+
diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
141142
<< FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
142143
}
143144

‎clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,10 @@ void UnusedUsingDeclsCheck::onEndOfTranslationUnit() {
154154
for (const auto &Context : Contexts) {
155155
if (!Context.IsUsed) {
156156
diag(Context.FoundUsingDecl->getLocation(), "using decl %0 is unused")
157-
<< Context.FoundUsingDecl
157+
<< Context.FoundUsingDecl;
158+
// Emit a fix and a fix description of the check;
159+
diag(Context.FoundUsingDecl->getLocation(),
160+
/*FixDescription=*/"remove the using", DiagnosticIDs::Note)
158161
<< FixItHint::CreateRemoval(Context.UsingDeclRange);
159162
}
160163
}

‎clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file1.yaml‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,25 @@
22
MainSourceFile: source1.cpp
33
Diagnostics:
44
- DiagnosticName: test-basic
5-
Message: Fix
6-
FilePath: $(path)/basic.h
7-
FileOffset: 242
8-
Replacements:
9-
- FilePath: $(path)/basic.h
10-
Offset: 242
11-
Length: 26
12-
ReplacementText: 'auto & elem : ints'
13-
- FilePath: $(path)/basic.h
14-
Offset: 276
15-
Length: 22
16-
ReplacementText: ''
17-
- FilePath: $(path)/basic.h
18-
Offset: 298
19-
Length: 1
20-
ReplacementText: elem
21-
- FilePath: $(path)/../basic/basic.h
22-
Offset: 148
23-
Length: 0
24-
ReplacementText: 'override '
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/basic.h
8+
FileOffset: 242
9+
Replacements:
10+
- FilePath: $(path)/basic.h
11+
Offset: 242
12+
Length: 26
13+
ReplacementText: 'auto & elem : ints'
14+
- FilePath: $(path)/basic.h
15+
Offset: 276
16+
Length: 22
17+
ReplacementText: ''
18+
- FilePath: $(path)/basic.h
19+
Offset: 298
20+
Length: 1
21+
ReplacementText: elem
22+
- FilePath: $(path)/../basic/basic.h
23+
Offset: 148
24+
Length: 0
25+
ReplacementText: 'override '
2526
...

‎clang-tools-extra/test/clang-apply-replacements/Inputs/basic/file2.yaml‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@
22
MainSourceFile: source2.cpp
33
Diagnostics:
44
- DiagnosticName: test-basic
5-
Message: Fix
6-
FilePath: $(path)/basic.h
7-
FileOffset: 148
8-
Replacements:
9-
- FilePath: $(path)/../basic/basic.h
10-
Offset: 298
11-
Length: 1
12-
ReplacementText: elem
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/basic.h
8+
FileOffset: 148
9+
Replacements:
10+
- FilePath: $(path)/../basic/basic.h
11+
Offset: 298
12+
Length: 1
13+
ReplacementText: elem
1314
...

‎clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file1.yaml‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,21 @@
22
MainSourceFile: source1.cpp
33
Diagnostics:
44
- DiagnosticName: test-conflict
5-
Message: Fix
6-
FilePath: $(path)/common.h
7-
FileOffset: 106
8-
Replacements:
9-
- FilePath: $(path)/common.h
10-
Offset: 106
11-
Length: 26
12-
ReplacementText: 'auto & i : ints'
13-
- FilePath: $(path)/common.h
14-
Offset: 140
15-
Length: 7
16-
ReplacementText: i
17-
- FilePath: $(path)/common.h
18-
Offset: 160
19-
Length: 12
20-
ReplacementText: ''
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/common.h
8+
FileOffset: 106
9+
Replacements:
10+
- FilePath: $(path)/common.h
11+
Offset: 106
12+
Length: 26
13+
ReplacementText: 'auto & i : ints'
14+
- FilePath: $(path)/common.h
15+
Offset: 140
16+
Length: 7
17+
ReplacementText: i
18+
- FilePath: $(path)/common.h
19+
Offset: 160
20+
Length: 12
21+
ReplacementText: ''
2122
...

‎clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file2.yaml‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,21 @@
22
MainSourceFile: source2.cpp
33
Diagnostics:
44
- DiagnosticName: test-conflict
5-
Message: Fix
6-
FilePath: $(path)/common.h
7-
FileOffset: 106
8-
Replacements:
9-
- FilePath: $(path)/common.h
10-
Offset: 106
11-
Length: 26
12-
ReplacementText: 'int & elem : ints'
13-
- FilePath: $(path)/common.h
14-
Offset: 140
15-
Length: 7
16-
ReplacementText: elem
17-
- FilePath: $(path)/common.h
18-
Offset: 169
19-
Length: 1
20-
ReplacementText: nullptr
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/common.h
8+
FileOffset: 106
9+
Replacements:
10+
- FilePath: $(path)/common.h
11+
Offset: 106
12+
Length: 26
13+
ReplacementText: 'int & elem : ints'
14+
- FilePath: $(path)/common.h
15+
Offset: 140
16+
Length: 7
17+
ReplacementText: elem
18+
- FilePath: $(path)/common.h
19+
Offset: 169
20+
Length: 1
21+
ReplacementText: nullptr
2122
...

‎clang-tools-extra/test/clang-apply-replacements/Inputs/conflict/file3.yaml‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@
22
MainSourceFile: source1.cpp
33
Diagnostics:
44
- DiagnosticName: test-conflict
5-
Message: Fix
6-
FilePath: $(path)/common.h
7-
FileOffset: 169
8-
Replacements:
9-
- FilePath: $(path)/common.h
10-
Offset: 169
11-
Length: 0
12-
ReplacementText: "(int*)"
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/common.h
8+
FileOffset: 169
9+
Replacements:
10+
- FilePath: $(path)/common.h
11+
Offset: 169
12+
Length: 0
13+
ReplacementText: "(int*)"
1314
...

0 commit comments

Comments
 (0)