Skip to content

Commit 6cee434

Browse files
haoNoQhaoNoQ
committed
[analyzer] Add minimal support for fix-it hints.
Allow attaching fixit hints to Static Analyzer BugReports. Fixits are attached either to the bug report itself or to its notes (path-sensitive event notes or path-insensitive extra notes). Add support for fixits in text output (including the default text output that goes without notes, as long as the fixit "belongs" to the warning). Add support for fixits in the plist output mode. Implement a fixit for the path-insensitive DeadStores checker. Only dead initialization warning is currently covered. Implement a fixit for the path-sensitive VirtualCall checker when the virtual method is not pure virtual (in this case the "fix" is to suppress the warning by qualifying the call). Both fixits are under an off-by-default flag for now, because they require more careful testing. Differential Revision: https://reviews.llvm.org/D65182 llvm-svn: 371257
1 parent 2b1b4ca commit 6cee434

File tree

15 files changed

+296
-95
lines changed

15 files changed

+296
-95
lines changed

‎clang/include/clang/StaticAnalyzer/Checkers/Checkers.td‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,11 @@ def UninitializedObjectChecker: Checker<"UninitializedObject">,
563563
def VirtualCallChecker : Checker<"VirtualCall">,
564564
HelpText<"Check virtual function calls during construction/destruction">,
565565
CheckerOptions<[
566+
CmdLineOption<Boolean,
567+
"ShowFixIts",
568+
"Enable fix-it hints for this checker",
569+
"false",
570+
InAlpha>,
566571
CmdLineOption<Boolean,
567572
"PureOnly",
568573
"Disables the checker. Keeps cplusplus.PureVirtualCall "
@@ -654,7 +659,12 @@ def DeadStoresChecker : Checker<"DeadStores">,
654659
"Warns for deadstores in nested assignments."
655660
"E.g.: if ((P = f())) where P is unused.",
656661
"true",
657-
Released>
662+
Released>,
663+
CmdLineOption<Boolean,
664+
"ShowFixIts",
665+
"Enable fix-it hints for this checker",
666+
"false",
667+
InAlpha>
658668
]>,
659669
Documentation<HasDocumentation>;
660670

‎clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,14 @@ ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug",
300300
"Whether to place an event at each tracked condition.",
301301
false)
302302

303+
ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks",
304+
"Emit fix-it hints as remarks for testing purposes",
305+
false)
306+
307+
//===----------------------------------------------------------------------===//
308+
// Unsigned analyzer options.
309+
//===----------------------------------------------------------------------===//
310+
303311
ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
304312
"The maximal amount of translation units that is considered "
305313
"for import when inlining functions during CTU analysis. "
@@ -308,10 +316,6 @@ ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold",
308316
"various translation units.",
309317
100u)
310318

311-
//===----------------------------------------------------------------------===//
312-
// Unsinged analyzer options.
313-
//===----------------------------------------------------------------------===//
314-
315319
ANALYZER_OPTION(
316320
unsigned, AlwaysInlineSize, "ipa-always-inline-size",
317321
"The size of the functions (in basic blocks), which should be considered "

‎clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ class BugReport : public llvm::ilist_node<BugReport> {
9696
SmallVector<SourceRange, 4> Ranges;
9797
const SourceRange ErrorNodeRange;
9898
NoteList Notes;
99+
SmallVector<FixItHint, 4> Fixits;
99100

100101
/// A (stack of) a set of symbols that are registered with this
101102
/// report as being "interesting", and thus used to help decide which
@@ -280,20 +281,17 @@ class BugReport : public llvm::ilist_node<BugReport> {
280281
/// allows you to specify where exactly in the auto-generated path diagnostic
281282
/// the extra note should appear.
282283
void addNote(StringRef Msg, const PathDiagnosticLocation &Pos,
283-
ArrayRef<SourceRange> Ranges) {
284+
ArrayRef<SourceRange> Ranges = {},
285+
ArrayRef<FixItHint> Fixits = {}) {
284286
auto P = std::make_shared<PathDiagnosticNotePiece>(Pos, Msg);
285287

286288
for (const auto &R : Ranges)
287289
P->addRange(R);
288290

289-
Notes.push_back(std::move(P));
290-
}
291+
for (const auto &F : Fixits)
292+
P->addFixit(F);
291293

292-
// FIXME: Instead of making an override, we could have default-initialized
293-
// Ranges with {}, however it crashes the MSVC 2013 compiler.
294-
void addNote(StringRef Msg, const PathDiagnosticLocation &Pos) {
295-
std::vector<SourceRange> Ranges;
296-
addNote(Msg, Pos, Ranges);
294+
Notes.push_back(std::move(P));
297295
}
298296

299297
virtual const NoteList &getNotes() {
@@ -319,7 +317,7 @@ class BugReport : public llvm::ilist_node<BugReport> {
319317

320318
const Stmt *getStmt() const;
321319

322-
/// Add a range to a bug report.
320+
/// Add a range to the bug report.
323321
///
324322
/// Ranges are used to highlight regions of interest in the source code.
325323
/// They should be at the same source code line as the BugReport location.
@@ -335,6 +333,20 @@ class BugReport : public llvm::ilist_node<BugReport> {
335333
/// Get the SourceRanges associated with the report.
336334
virtual llvm::iterator_range<ranges_iterator> getRanges() const;
337335

336+
/// Add a fix-it hint to the warning message of the bug report.
337+
///
338+
/// Fix-it hints are the suggested edits to the code that would resolve
339+
/// the problem explained by the bug report. Fix-it hints should be
340+
/// as conservative as possible because it is not uncommon for the user
341+
/// to blindly apply all fixits to their project. It usually is very hard
342+
/// to produce a good fix-it hint for most path-sensitive warnings.
343+
/// Fix-it hints can also be added to notes through the addNote() interface.
344+
void addFixItHint(const FixItHint &F) {
345+
Fixits.push_back(F);
346+
}
347+
348+
ArrayRef<FixItHint> getFixits() const { return Fixits; }
349+
338350
/// Add custom or predefined bug report visitors to this report.
339351
///
340352
/// The visitors should be used when the default trace is not sufficient.
@@ -473,12 +485,14 @@ class BugReporter {
473485
void EmitBasicReport(const Decl *DeclWithIssue, const CheckerBase *Checker,
474486
StringRef BugName, StringRef BugCategory,
475487
StringRef BugStr, PathDiagnosticLocation Loc,
476-
ArrayRef<SourceRange> Ranges = None);
488+
ArrayRef<SourceRange> Ranges = None,
489+
ArrayRef<FixItHint> Fixits = None);
477490

478491
void EmitBasicReport(const Decl *DeclWithIssue, CheckName CheckName,
479492
StringRef BugName, StringRef BugCategory,
480493
StringRef BugStr, PathDiagnosticLocation Loc,
481-
ArrayRef<SourceRange> Ranges = None);
494+
ArrayRef<SourceRange> Ranges = None,
495+
ArrayRef<FixItHint> Fixits = None);
482496

483497
private:
484498
llvm::StringMap<BugType *> StrBugTypes;

‎clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,7 @@ class PathDiagnosticPiece: public llvm::FoldingSetNode {
393393
StringRef Tag;
394394

395395
std::vector<SourceRange> ranges;
396+
std::vector<FixItHint> fixits;
396397

397398
protected:
398399
PathDiagnosticPiece(StringRef s, Kind k, DisplayHint hint = Below);
@@ -437,9 +438,16 @@ class PathDiagnosticPiece: public llvm::FoldingSetNode {
437438
ranges.push_back(SourceRange(B,E));
438439
}
439440

441+
void addFixit(FixItHint F) {
442+
fixits.push_back(F);
443+
}
444+
440445
/// Return the SourceRanges associated with this PathDiagnosticPiece.
441446
ArrayRef<SourceRange> getRanges() const { return ranges; }
442447

448+
/// Return the fix-it hints associated with this PathDiagnosticPiece.
449+
ArrayRef<FixItHint> getFixits() const { return fixits; }
450+
443451
virtual void Profile(llvm::FoldingSetNodeID &ID) const;
444452

445453
void setAsLastInMainSourceFile() {

‎clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp‎

Lines changed: 57 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
//
1212
//===----------------------------------------------------------------------===//
1313

14+
#include "clang/Lex/Lexer.h"
1415
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
1516
#include "clang/AST/ASTContext.h"
1617
#include "clang/AST/Attr.h"
@@ -119,30 +120,37 @@ LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) {
119120
}
120121

121122
namespace {
123+
class DeadStoresChecker : public Checker<check::ASTCodeBody> {
124+
public:
125+
bool ShowFixIts = false;
126+
bool WarnForDeadNestedAssignments = true;
127+
128+
void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
129+
BugReporter &BR) const;
130+
};
131+
122132
class DeadStoreObs : public LiveVariables::Observer {
123133
const CFG &cfg;
124134
ASTContext &Ctx;
125135
BugReporter& BR;
126-
const CheckerBase *Checker;
136+
const DeadStoresChecker *Checker;
127137
AnalysisDeclContext* AC;
128138
ParentMap& Parents;
129139
llvm::SmallPtrSet<const VarDecl*, 20> Escaped;
130140
std::unique_ptr<ReachableCode> reachableCode;
131141
const CFGBlock *currentBlock;
132142
std::unique_ptr<llvm::DenseSet<const VarDecl *>> InEH;
133-
const bool WarnForDeadNestedAssignments;
134143

135144
enum DeadStoreKind { Standard, Enclosing, DeadIncrement, DeadInit };
136145

137146
public:
138147
DeadStoreObs(const CFG &cfg, ASTContext &ctx, BugReporter &br,
139-
const CheckerBase *checker, AnalysisDeclContext *ac,
148+
const DeadStoresChecker *checker, AnalysisDeclContext *ac,
140149
ParentMap &parents,
141150
llvm::SmallPtrSet<const VarDecl *, 20> &escaped,
142151
bool warnForDeadNestedAssignments)
143152
: cfg(cfg), Ctx(ctx), BR(br), Checker(checker), AC(ac), Parents(parents),
144-
Escaped(escaped), currentBlock(nullptr),
145-
WarnForDeadNestedAssignments(warnForDeadNestedAssignments) {}
153+
Escaped(escaped), currentBlock(nullptr) {}
146154

147155
~DeadStoreObs() override {}
148156

@@ -205,12 +213,32 @@ class DeadStoreObs : public LiveVariables::Observer {
205213
llvm::raw_svector_ostream os(buf);
206214
const char *BugType = nullptr;
207215

216+
SmallVector<FixItHint, 1> Fixits;
217+
208218
switch (dsk) {
209-
case DeadInit:
219+
case DeadInit: {
210220
BugType = "Dead initialization";
211221
os << "Value stored to '" << *V
212222
<< "' during its initialization is never read";
223+
224+
ASTContext &ACtx = V->getASTContext();
225+
if (Checker->ShowFixIts) {
226+
if (V->getInit()->HasSideEffects(ACtx,
227+
/*IncludePossibleEffects=*/true)) {
228+
break;
229+
}
230+
SourceManager &SM = ACtx.getSourceManager();
231+
const LangOptions &LO = ACtx.getLangOpts();
232+
SourceLocation L1 =
233+
Lexer::findNextToken(
234+
V->getTypeSourceInfo()->getTypeLoc().getEndLoc(),
235+
SM, LO)->getEndLoc();
236+
SourceLocation L2 =
237+
Lexer::getLocForEndOfToken(V->getInit()->getEndLoc(), 1, SM, LO);
238+
Fixits.push_back(FixItHint::CreateRemoval({L1, L2}));
239+
}
213240
break;
241+
}
214242

215243
case DeadIncrement:
216244
BugType = "Dead increment";
@@ -222,7 +250,7 @@ class DeadStoreObs : public LiveVariables::Observer {
222250

223251
// eg.: f((x = foo()))
224252
case Enclosing:
225-
if (!WarnForDeadNestedAssignments)
253+
if (!Checker->WarnForDeadNestedAssignments)
226254
return;
227255
BugType = "Dead nested assignment";
228256
os << "Although the value stored to '" << *V
@@ -233,7 +261,7 @@ class DeadStoreObs : public LiveVariables::Observer {
233261
}
234262

235263
BR.EmitBasicReport(AC->getDecl(), Checker, BugType, "Dead store", os.str(),
236-
L, R);
264+
L, R, Fixits);
237265
}
238266

239267
void CheckVarDecl(const VarDecl *VD, const Expr *Ex, const Expr *Val,
@@ -479,42 +507,37 @@ class FindEscaped {
479507
// DeadStoresChecker
480508
//===----------------------------------------------------------------------===//
481509

482-
namespace {
483-
class DeadStoresChecker : public Checker<check::ASTCodeBody> {
484-
public:
485-
bool WarnForDeadNestedAssignments = true;
486-
487-
void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
488-
BugReporter &BR) const {
510+
void DeadStoresChecker::checkASTCodeBody(const Decl *D, AnalysisManager &mgr,
511+
BugReporter &BR) const {
489512

490-
// Don't do anything for template instantiations.
491-
// Proving that code in a template instantiation is "dead"
492-
// means proving that it is dead in all instantiations.
493-
// This same problem exists with -Wunreachable-code.
494-
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
495-
if (FD->isTemplateInstantiation())
496-
return;
513+
// Don't do anything for template instantiations.
514+
// Proving that code in a template instantiation is "dead"
515+
// means proving that it is dead in all instantiations.
516+
// This same problem exists with -Wunreachable-code.
517+
if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
518+
if (FD->isTemplateInstantiation())
519+
return;
497520

498-
if (LiveVariables *L = mgr.getAnalysis<LiveVariables>(D)) {
499-
CFG &cfg = *mgr.getCFG(D);
500-
AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D);
501-
ParentMap &pmap = mgr.getParentMap(D);
502-
FindEscaped FS;
503-
cfg.VisitBlockStmts(FS);
504-
DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped,
505-
WarnForDeadNestedAssignments);
506-
L->runOnAllBlocks(A);
507-
}
521+
if (LiveVariables *L = mgr.getAnalysis<LiveVariables>(D)) {
522+
CFG &cfg = *mgr.getCFG(D);
523+
AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D);
524+
ParentMap &pmap = mgr.getParentMap(D);
525+
FindEscaped FS;
526+
cfg.VisitBlockStmts(FS);
527+
DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped,
528+
WarnForDeadNestedAssignments);
529+
L->runOnAllBlocks(A);
508530
}
509-
};
510531
}
511532

512533
void ento::registerDeadStoresChecker(CheckerManager &Mgr) {
513-
auto Chk = Mgr.registerChecker<DeadStoresChecker>();
534+
auto *Chk = Mgr.registerChecker<DeadStoresChecker>();
514535

515536
const AnalyzerOptions &AnOpts = Mgr.getAnalyzerOptions();
516537
Chk->WarnForDeadNestedAssignments =
517538
AnOpts.getCheckerBooleanOption(Chk, "WarnForDeadNestedAssignments");
539+
Chk->ShowFixIts =
540+
AnOpts.getCheckerBooleanOption(Chk, "ShowFixIts");
518541
}
519542

520543
bool ento::shouldRegisterDeadStoresChecker(const LangOptions &LO) {

‎clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ class VirtualCallChecker
4343
public:
4444
// These are going to be null if the respective check is disabled.
4545
mutable std::unique_ptr<BugType> BT_Pure, BT_Impure;
46+
bool ShowFixIts = false;
4647

4748
void checkBeginFunction(CheckerContext &C) const;
4849
void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
@@ -146,6 +147,17 @@ void VirtualCallChecker::checkPreCall(const CallEvent &Call,
146147
}
147148

148149
auto Report = std::make_unique<BugReport>(*BT, OS.str(), N);
150+
151+
if (ShowFixIts && !IsPure) {
152+
// FIXME: These hints are valid only when the virtual call is made
153+
// directly from the constructor/destructor. Otherwise the dispatch
154+
// will work just fine from other callees, and the fix may break
155+
// the otherwise correct program.
156+
FixItHint Fixit = FixItHint::CreateInsertion(
157+
CE->getBeginLoc(), MD->getParent()->getNameAsString() + "::");
158+
Report->addFixItHint(Fixit);
159+
}
160+
149161
C.emitReport(std::move(Report));
150162
}
151163

@@ -206,6 +218,8 @@ void ento::registerVirtualCallChecker(CheckerManager &Mgr) {
206218
Chk->BT_Impure = std::make_unique<BugType>(
207219
Mgr.getCurrentCheckName(), "Unexpected loss of virtual dispatch",
208220
categories::CXXObjectLifecycle);
221+
Chk->ShowFixIts = Mgr.getAnalyzerOptions().getCheckerBooleanOption(
222+
Mgr.getCurrentCheckName(), "ShowFixIts");
209223
}
210224
}
211225

0 commit comments

Comments
 (0)