NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus,
baloghadamsoftware.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, kristof.beyls, javed.absar.
Herald added a project: clang.
That's a pretty wonky experiment, even if a fairly easy one. I'm trying to add
support for fix-it hints to the bug reporter. In the current shape the patch
does the following:
- Allow attaching fixit hints to Static Analyzer `BugReport`s.
- Add support for fixits in text output (including the default text output that
goes without notes, as the fixit "belongs" to the warning).
- Add support for fixits in the plist output mode (not sure if i'm fully
supporting all kinds of fixits).
- Implement a fixit for the path-insensitive DeadStores checker (only one of
the cases, and i'm still not sure it's a good fixit, but it was an obvious
first thing to experiment with).
- 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).
More TODOs:
- We don't have a good way to test removal fixits (such as the one in the dead
stores checker). Testing plist files is not a good way to test them (though we
definitely need a few such tests). We can't test them via text output because
clang itself generally doesn't display removal fixits in text output (it's
kinda obvious if you look at how it prints them). In clang-tidy they use a more
sophisticated facility for these tests, testing the fixed file instead, but
it's deep within their custom testing scripts. We might need something similar.
- HTML output still doesn't support fixits. Not sure if they are useful in
there because fixits are generally not very useful when there's no button to
apply them. But there should be no harm in displaying them anyway, so i hope
i'll have time to take a look.
- Need more tests with macros and such stuff.
Repository:
rC Clang
https://reviews.llvm.org/D65182
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===================================================================
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -2190,6 +2190,26 @@
<integer>86</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>86</integer>
+ <key>col</key><integer>11</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>86</integer>
+ <key>col</key><integer>40</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
Index: clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
===================================================================
--- clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/objc-arc.m.plist
@@ -403,6 +403,26 @@
<integer>119</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>119</integer>
+ <key>col</key><integer>7</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>119</integer>
+ <key>col</key><integer>25</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -471,6 +491,26 @@
<integer>139</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>139</integer>
+ <key>col</key><integer>10</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>139</integer>
+ <key>col</key><integer>53</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -539,6 +579,26 @@
<integer>144</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>144</integer>
+ <key>col</key><integer>10</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>144</integer>
+ <key>col</key><integer>45</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -607,6 +667,26 @@
<integer>145</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>145</integer>
+ <key>col</key><integer>10</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>145</integer>
+ <key>col</key><integer>44</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -675,6 +755,26 @@
<integer>146</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>146</integer>
+ <key>col</key><integer>10</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>146</integer>
+ <key>col</key><integer>48</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -1085,6 +1185,26 @@
<integer>150</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>150</integer>
+ <key>col</key><integer>16</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>150</integer>
+ <key>col</key><integer>64</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -1153,6 +1273,26 @@
<integer>151</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>151</integer>
+ <key>col</key><integer>18</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>151</integer>
+ <key>col</key><integer>67</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -1221,6 +1361,26 @@
<integer>152</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>152</integer>
+ <key>col</key><integer>16</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>152</integer>
+ <key>col</key><integer>55</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
@@ -1289,6 +1449,26 @@
<integer>153</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>153</integer>
+ <key>col</key><integer>18</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>153</integer>
+ <key>col</key><integer>58</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===================================================================
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -11430,6 +11430,26 @@
<integer>431</integer>
</array>
</dict>
+ <key>fixits</key>
+ <array>
+ <dict>
+ <key>remove_range</key>
+ <array>
+ <dict>
+ <key>line</key><integer>431</integer>
+ <key>col</key><integer>11</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>431</integer>
+ <key>col</key><integer>40</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>insert_string</key>
+ <string></string>
+ </dict>
+ </array>
</dict>
<dict>
<key>path</key>
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -116,8 +116,9 @@
E = Diags.end(); I != E; ++I) {
const PathDiagnostic *PD = *I;
SourceLocation WarnLoc = PD->getLocation().asLocation();
- Diag.Report(WarnLoc, WarnID) << PD->getShortDescription()
- << PD->path.back()->getRanges();
+ Diag.Report(WarnLoc, WarnID)
+ << PD->getShortDescription() << PD->path.back()->getRanges()
+ << PD->getFixits();
// First, add extra notes, even if paths should not be included.
for (const auto &Piece : PD->path) {
Index: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -726,6 +726,22 @@
printCoverage(D, /*IndentLevel=*/2, Fids, FM, o);
+ if (!D->getFixits().empty()) {
+ o << " <key>fixits</key>\n";
+ o << " <array>\n";
+ for (auto Hint : D->getFixits()) {
+ assert(!Hint.isNull());
+ o << " <dict>\n";
+ o << " <key>remove_range</key>\n";
+ EmitRange(o, SM, Lexer::getAsCharRange(Hint.RemoveRange, SM, LangOpts),
+ FM, 4);
+ o << " <key>insert_string</key>\n";
+ o << " <string>" << Hint.CodeToInsert << "</string>\n";
+ o << " </dict>\n";
+ }
+ o << " </array>\n";
+ }
+
// Close up the entry.
o << " </dict>\n";
}
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -134,14 +134,15 @@
StringRef CheckName, const Decl *declWithIssue, StringRef bugtype,
StringRef verboseDesc, StringRef shortDesc, StringRef category,
PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique,
- std::unique_ptr<FilesToLineNumsMap> ExecutedLines)
+ std::unique_ptr<FilesToLineNumsMap> ExecutedLines,
+ ArrayRef<FixItHint> Fixits)
: CheckName(CheckName), DeclWithIssue(declWithIssue),
BugType(StripTrailingDots(bugtype)),
VerboseDesc(StripTrailingDots(verboseDesc)),
ShortDesc(StripTrailingDots(shortDesc)),
Category(StripTrailingDots(category)), UniqueingLoc(LocationToUnique),
UniqueingDecl(DeclToUnique), ExecutedLines(std::move(ExecutedLines)),
- path(pathImpl) {}
+ Fixits(Fixits.begin(), Fixits.end()), path(pathImpl) {}
static PathDiagnosticCallPiece *
getFirstStackedCallToHeaderFile(PathDiagnosticCallPiece *CP,
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -1261,7 +1261,7 @@
R->getBugType().getName(), R->getDescription(),
R->getShortDescription(/*Fallback=*/false), BT.getCategory(),
R->getUniqueingLocation(), R->getUniqueingDecl(),
- findExecutedLines(SM, R->getErrorNode()));
+ findExecutedLines(SM, R->getErrorNode()), R->getFixits());
}
static const Stmt *getStmtParent(const Stmt *S, const ParentMap &PM) {
@@ -3121,23 +3121,26 @@
const CheckerBase *Checker,
StringRef Name, StringRef Category,
StringRef Str, PathDiagnosticLocation Loc,
- ArrayRef<SourceRange> Ranges) {
+ ArrayRef<SourceRange> Ranges,
+ ArrayRef<FixItHint> Fixits) {
EmitBasicReport(DeclWithIssue, Checker->getCheckName(), Name, Category, Str,
- Loc, Ranges);
+ Loc, Ranges, Fixits);
}
void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
- CheckName CheckName,
- StringRef name, StringRef category,
- StringRef str, PathDiagnosticLocation Loc,
- ArrayRef<SourceRange> Ranges) {
+ CheckName CheckName, StringRef name,
+ StringRef category, StringRef str,
+ PathDiagnosticLocation Loc,
+ ArrayRef<SourceRange> Ranges,
+ ArrayRef<FixItHint> Fixits) {
// 'BT' is owned by BugReporter.
BugType *BT = getBugTypeForName(CheckName, name, category);
auto R = llvm::make_unique<BugReport>(*BT, str, Loc);
R->setDeclWithIssue(DeclWithIssue);
- for (ArrayRef<SourceRange>::iterator I = Ranges.begin(), E = Ranges.end();
- I != E; ++I)
- R->addRange(*I);
+ for (auto SR : Ranges)
+ R->addRange(SR);
+ for (auto FH : Fixits)
+ R->addFixItHint(FH);
emitReport(std::move(R));
}
Index: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -141,6 +141,11 @@
return;
auto Report = llvm::make_unique<BugReport>(BT, OS.str(), N);
+ if (!IsPure) {
+ FixItHint Fixit = FixItHint::CreateInsertion(
+ CE->getBeginLoc(), MD->getParent()->getNameAsString() + "::");
+ Report->addFixItHint(Fixit);
+ }
C.emitReport(std::move(Report));
}
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -11,6 +11,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/Lex/Lexer.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Attr.h"
@@ -202,12 +203,23 @@
llvm::raw_svector_ostream os(buf);
const char *BugType = nullptr;
+ SmallVector<FixItHint, 1> Fixits;
+
switch (dsk) {
- case DeadInit:
+ case DeadInit: {
BugType = "Dead initialization";
os << "Value stored to '" << *V
<< "' during its initialization is never read";
+ SourceLocation L1 = Lexer::findNextToken(
+ V->getTypeSourceInfo()->getTypeLoc().getEndLoc(),
+ V->getASTContext().getSourceManager(),
+ V->getASTContext().getLangOpts())->getEndLoc();
+ SourceLocation L2 = Lexer::getLocForEndOfToken(
+ V->getInit()->getEndLoc(), 1, V->getASTContext().getSourceManager(),
+ V->getASTContext().getLangOpts());
+ Fixits.push_back(FixItHint::CreateRemoval({L1, L2}));
break;
+ }
case DeadIncrement:
BugType = "Dead increment";
@@ -225,7 +237,7 @@
}
BR.EmitBasicReport(AC->getDecl(), Checker, BugType, "Dead store", os.str(),
- L, R);
+ L, R, Fixits);
}
void CheckVarDecl(const VarDecl *VD, const Expr *Ex, const Expr *Val,
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -804,13 +804,16 @@
/// Lines executed in the path.
std::unique_ptr<FilesToLineNumsMap> ExecutedLines;
+ SmallVector<FixItHint, 4> Fixits;
+
public:
PathDiagnostic() = delete;
PathDiagnostic(StringRef CheckName, const Decl *DeclWithIssue,
StringRef bugtype, StringRef verboseDesc, StringRef shortDesc,
StringRef category, PathDiagnosticLocation LocationToUnique,
const Decl *DeclToUnique,
- std::unique_ptr<FilesToLineNumsMap> ExecutedLines);
+ std::unique_ptr<FilesToLineNumsMap> ExecutedLines,
+ ArrayRef<FixItHint> Fixits);
~PathDiagnostic();
const PathPieces &path;
@@ -864,6 +867,8 @@
StringRef getBugType() const { return BugType; }
StringRef getCategory() const { return Category; }
+ ArrayRef<FixItHint> getFixits() const { return Fixits; }
+
/// Return the semantic context where an issue occurred. If the
/// issue occurs along a path, this represents the "central" area
/// where the bug manifests.
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -107,6 +107,8 @@
ExtraTextList ExtraText;
NoteList Notes;
+ SmallVector<FixItHint, 4> Fixits;
+
using Symbols = llvm::DenseSet<SymbolRef>;
using Regions = llvm::DenseSet<const MemRegion *>;
@@ -333,9 +335,17 @@
Ranges.push_back(R);
}
+ void addFixItHint(FixItHint F) {
+ Fixits.push_back(F);
+ }
+
/// Get the SourceRanges associated with the report.
virtual llvm::iterator_range<ranges_iterator> getRanges();
+ virtual llvm::ArrayRef<FixItHint> getFixits() {
+ return Fixits;
+ }
+
/// Add custom or predefined bug report visitors to this report.
///
/// The visitors should be used when the default trace is not sufficient.
@@ -502,12 +512,14 @@
void EmitBasicReport(const Decl *DeclWithIssue, const CheckerBase *Checker,
StringRef BugName, StringRef BugCategory,
StringRef BugStr, PathDiagnosticLocation Loc,
- ArrayRef<SourceRange> Ranges = None);
+ ArrayRef<SourceRange> Ranges = None,
+ ArrayRef<FixItHint> Fixits = None);
void EmitBasicReport(const Decl *DeclWithIssue, CheckName CheckName,
StringRef BugName, StringRef BugCategory,
StringRef BugStr, PathDiagnosticLocation Loc,
- ArrayRef<SourceRange> Ranges = None);
+ ArrayRef<SourceRange> Ranges = None,
+ ArrayRef<FixItHint> Fixits = None);
private:
llvm::StringMap<BugType *> StrBugTypes;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
