From: Artem Dergachev Date: Fri, 6 Sep 2019 20:55:29 +0000 (+0000) Subject: [analyzer] Add minimal support for fix-it hints. X-Git-Tag: llvmorg-11-init~9797 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=6cee434ed10ead6b7416ca5ee9592b2b207eeb0f;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 0626d29..4d52655 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -564,6 +564,11 @@ def VirtualCallChecker : Checker<"VirtualCall">, HelpText<"Check virtual function calls during construction/destruction">, CheckerOptions<[ CmdLineOption, + CmdLineOption, "Warns for deadstores in nested assignments." "E.g.: if ((P = f())) where P is unused.", "true", - Released> + Released>, + CmdLineOption ]>, Documentation; diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def index 9a0225e..d853fb7 100644 --- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def +++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def @@ -300,6 +300,14 @@ ANALYZER_OPTION(bool, ShouldTrackConditionsDebug, "track-conditions-debug", "Whether to place an event at each tracked condition.", false) +ANALYZER_OPTION(bool, ShouldEmitFixItHintsAsRemarks, "fixits-as-remarks", + "Emit fix-it hints as remarks for testing purposes", + false) + +//===----------------------------------------------------------------------===// +// Unsigned analyzer options. +//===----------------------------------------------------------------------===// + ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold", "The maximal amount of translation units that is considered " "for import when inlining functions during CTU analysis. " @@ -308,10 +316,6 @@ ANALYZER_OPTION(unsigned, CTUImportThreshold, "ctu-import-threshold", "various translation units.", 100u) -//===----------------------------------------------------------------------===// -// Unsinged analyzer options. -//===----------------------------------------------------------------------===// - ANALYZER_OPTION( unsigned, AlwaysInlineSize, "ipa-always-inline-size", "The size of the functions (in basic blocks), which should be considered " diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index 76c282a..28e9e94 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -96,6 +96,7 @@ protected: SmallVector Ranges; const SourceRange ErrorNodeRange; NoteList Notes; + SmallVector Fixits; /// A (stack of) a set of symbols that are registered with this /// report as being "interesting", and thus used to help decide which @@ -280,20 +281,17 @@ public: /// allows you to specify where exactly in the auto-generated path diagnostic /// the extra note should appear. void addNote(StringRef Msg, const PathDiagnosticLocation &Pos, - ArrayRef Ranges) { + ArrayRef Ranges = {}, + ArrayRef Fixits = {}) { auto P = std::make_shared(Pos, Msg); for (const auto &R : Ranges) P->addRange(R); - Notes.push_back(std::move(P)); - } + for (const auto &F : Fixits) + P->addFixit(F); - // FIXME: Instead of making an override, we could have default-initialized - // Ranges with {}, however it crashes the MSVC 2013 compiler. - void addNote(StringRef Msg, const PathDiagnosticLocation &Pos) { - std::vector Ranges; - addNote(Msg, Pos, Ranges); + Notes.push_back(std::move(P)); } virtual const NoteList &getNotes() { @@ -319,7 +317,7 @@ public: const Stmt *getStmt() const; - /// Add a range to a bug report. + /// Add a range to the bug report. /// /// Ranges are used to highlight regions of interest in the source code. /// They should be at the same source code line as the BugReport location. @@ -335,6 +333,20 @@ public: /// Get the SourceRanges associated with the report. virtual llvm::iterator_range getRanges() const; + /// Add a fix-it hint to the warning message of the bug report. + /// + /// Fix-it hints are the suggested edits to the code that would resolve + /// the problem explained by the bug report. Fix-it hints should be + /// as conservative as possible because it is not uncommon for the user + /// to blindly apply all fixits to their project. It usually is very hard + /// to produce a good fix-it hint for most path-sensitive warnings. + /// Fix-it hints can also be added to notes through the addNote() interface. + void addFixItHint(const FixItHint &F) { + Fixits.push_back(F); + } + + ArrayRef getFixits() const { return Fixits; } + /// Add custom or predefined bug report visitors to this report. /// /// The visitors should be used when the default trace is not sufficient. @@ -473,12 +485,14 @@ public: void EmitBasicReport(const Decl *DeclWithIssue, const CheckerBase *Checker, StringRef BugName, StringRef BugCategory, StringRef BugStr, PathDiagnosticLocation Loc, - ArrayRef Ranges = None); + ArrayRef Ranges = None, + ArrayRef Fixits = None); void EmitBasicReport(const Decl *DeclWithIssue, CheckName CheckName, StringRef BugName, StringRef BugCategory, StringRef BugStr, PathDiagnosticLocation Loc, - ArrayRef Ranges = None); + ArrayRef Ranges = None, + ArrayRef Fixits = None); private: llvm::StringMap StrBugTypes; diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index 52d2584..6b5a2eb 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -393,6 +393,7 @@ private: StringRef Tag; std::vector ranges; + std::vector fixits; protected: PathDiagnosticPiece(StringRef s, Kind k, DisplayHint hint = Below); @@ -437,9 +438,16 @@ public: ranges.push_back(SourceRange(B,E)); } + void addFixit(FixItHint F) { + fixits.push_back(F); + } + /// Return the SourceRanges associated with this PathDiagnosticPiece. ArrayRef getRanges() const { return ranges; } + /// Return the fix-it hints associated with this PathDiagnosticPiece. + ArrayRef getFixits() const { return fixits; } + virtual void Profile(llvm::FoldingSetNodeID &ID) const; void setAsLastInMainSourceFile() { diff --git a/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp index e4889c1..6144188 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp +++ b/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" @@ -119,30 +120,37 @@ LookThroughTransitiveAssignmentsAndCommaOperators(const Expr *Ex) { } namespace { +class DeadStoresChecker : public Checker { +public: + bool ShowFixIts = false; + bool WarnForDeadNestedAssignments = true; + + void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr, + BugReporter &BR) const; +}; + class DeadStoreObs : public LiveVariables::Observer { const CFG &cfg; ASTContext &Ctx; BugReporter& BR; - const CheckerBase *Checker; + const DeadStoresChecker *Checker; AnalysisDeclContext* AC; ParentMap& Parents; llvm::SmallPtrSet Escaped; std::unique_ptr reachableCode; const CFGBlock *currentBlock; std::unique_ptr> InEH; - const bool WarnForDeadNestedAssignments; enum DeadStoreKind { Standard, Enclosing, DeadIncrement, DeadInit }; public: DeadStoreObs(const CFG &cfg, ASTContext &ctx, BugReporter &br, - const CheckerBase *checker, AnalysisDeclContext *ac, + const DeadStoresChecker *checker, AnalysisDeclContext *ac, ParentMap &parents, llvm::SmallPtrSet &escaped, bool warnForDeadNestedAssignments) : cfg(cfg), Ctx(ctx), BR(br), Checker(checker), AC(ac), Parents(parents), - Escaped(escaped), currentBlock(nullptr), - WarnForDeadNestedAssignments(warnForDeadNestedAssignments) {} + Escaped(escaped), currentBlock(nullptr) {} ~DeadStoreObs() override {} @@ -205,12 +213,32 @@ public: llvm::raw_svector_ostream os(buf); const char *BugType = nullptr; + SmallVector Fixits; + switch (dsk) { - case DeadInit: + case DeadInit: { BugType = "Dead initialization"; os << "Value stored to '" << *V << "' during its initialization is never read"; + + ASTContext &ACtx = V->getASTContext(); + if (Checker->ShowFixIts) { + if (V->getInit()->HasSideEffects(ACtx, + /*IncludePossibleEffects=*/true)) { + break; + } + SourceManager &SM = ACtx.getSourceManager(); + const LangOptions &LO = ACtx.getLangOpts(); + SourceLocation L1 = + Lexer::findNextToken( + V->getTypeSourceInfo()->getTypeLoc().getEndLoc(), + SM, LO)->getEndLoc(); + SourceLocation L2 = + Lexer::getLocForEndOfToken(V->getInit()->getEndLoc(), 1, SM, LO); + Fixits.push_back(FixItHint::CreateRemoval({L1, L2})); + } break; + } case DeadIncrement: BugType = "Dead increment"; @@ -222,7 +250,7 @@ public: // eg.: f((x = foo())) case Enclosing: - if (!WarnForDeadNestedAssignments) + if (!Checker->WarnForDeadNestedAssignments) return; BugType = "Dead nested assignment"; os << "Although the value stored to '" << *V @@ -233,7 +261,7 @@ public: } 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, @@ -479,42 +507,37 @@ public: // DeadStoresChecker //===----------------------------------------------------------------------===// -namespace { -class DeadStoresChecker : public Checker { -public: - bool WarnForDeadNestedAssignments = true; - - void checkASTCodeBody(const Decl *D, AnalysisManager& mgr, - BugReporter &BR) const { +void DeadStoresChecker::checkASTCodeBody(const Decl *D, AnalysisManager &mgr, + BugReporter &BR) const { - // Don't do anything for template instantiations. - // Proving that code in a template instantiation is "dead" - // means proving that it is dead in all instantiations. - // This same problem exists with -Wunreachable-code. - if (const FunctionDecl *FD = dyn_cast(D)) - if (FD->isTemplateInstantiation()) - return; + // Don't do anything for template instantiations. + // Proving that code in a template instantiation is "dead" + // means proving that it is dead in all instantiations. + // This same problem exists with -Wunreachable-code. + if (const FunctionDecl *FD = dyn_cast(D)) + if (FD->isTemplateInstantiation()) + return; - if (LiveVariables *L = mgr.getAnalysis(D)) { - CFG &cfg = *mgr.getCFG(D); - AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D); - ParentMap &pmap = mgr.getParentMap(D); - FindEscaped FS; - cfg.VisitBlockStmts(FS); - DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped, - WarnForDeadNestedAssignments); - L->runOnAllBlocks(A); - } + if (LiveVariables *L = mgr.getAnalysis(D)) { + CFG &cfg = *mgr.getCFG(D); + AnalysisDeclContext *AC = mgr.getAnalysisDeclContext(D); + ParentMap &pmap = mgr.getParentMap(D); + FindEscaped FS; + cfg.VisitBlockStmts(FS); + DeadStoreObs A(cfg, BR.getContext(), BR, this, AC, pmap, FS.Escaped, + WarnForDeadNestedAssignments); + L->runOnAllBlocks(A); } -}; } void ento::registerDeadStoresChecker(CheckerManager &Mgr) { - auto Chk = Mgr.registerChecker(); + auto *Chk = Mgr.registerChecker(); const AnalyzerOptions &AnOpts = Mgr.getAnalyzerOptions(); Chk->WarnForDeadNestedAssignments = AnOpts.getCheckerBooleanOption(Chk, "WarnForDeadNestedAssignments"); + Chk->ShowFixIts = + AnOpts.getCheckerBooleanOption(Chk, "ShowFixIts"); } bool ento::shouldRegisterDeadStoresChecker(const LangOptions &LO) { diff --git a/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp index 903ba04..a210128 100644 --- a/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp @@ -43,6 +43,7 @@ class VirtualCallChecker public: // These are going to be null if the respective check is disabled. mutable std::unique_ptr BT_Pure, BT_Impure; + bool ShowFixIts = false; void checkBeginFunction(CheckerContext &C) const; void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; @@ -146,6 +147,17 @@ void VirtualCallChecker::checkPreCall(const CallEvent &Call, } auto Report = std::make_unique(*BT, OS.str(), N); + + if (ShowFixIts && !IsPure) { + // FIXME: These hints are valid only when the virtual call is made + // directly from the constructor/destructor. Otherwise the dispatch + // will work just fine from other callees, and the fix may break + // the otherwise correct program. + FixItHint Fixit = FixItHint::CreateInsertion( + CE->getBeginLoc(), MD->getParent()->getNameAsString() + "::"); + Report->addFixItHint(Fixit); + } + C.emitReport(std::move(Report)); } @@ -206,6 +218,8 @@ void ento::registerVirtualCallChecker(CheckerManager &Mgr) { Chk->BT_Impure = std::make_unique( Mgr.getCurrentCheckName(), "Unexpected loss of virtual dispatch", categories::CXXObjectLifecycle); + Chk->ShowFixIts = Mgr.getAnalyzerOptions().getCheckerBooleanOption( + Mgr.getCurrentCheckName(), "ShowFixIts"); } } diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index 3ce998f..f5d747c 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2928,6 +2928,9 @@ void BugReporter::FlushReport(BugReportEquivClass& EQ) { Pieces.push_front(*I); } + for (const auto &I : report->getFixits()) + Pieces.back()->addFixit(I); + updateExecutedLinesWithDiagnosticPieces(*PD); Consumer->HandlePathDiagnostic(std::move(PD)); } @@ -3048,26 +3051,29 @@ BugReporter::generateDiagnosticForConsumerMap( } void BugReporter::EmitBasicReport(const Decl *DeclWithIssue, - const CheckerBase *Checker, - StringRef Name, StringRef Category, - StringRef Str, PathDiagnosticLocation Loc, - ArrayRef Ranges) { + const CheckerBase *Checker, StringRef Name, + StringRef Category, StringRef Str, + PathDiagnosticLocation Loc, + ArrayRef Ranges, + ArrayRef 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 Ranges) { + ArrayRef Ranges, + ArrayRef Fixits) { // 'BT' is owned by BugReporter. BugType *BT = getBugTypeForName(CheckName, name, category); auto R = std::make_unique(*BT, str, Loc); R->setDeclWithIssue(DeclWithIssue); - for (ArrayRef::iterator I = Ranges.begin(), E = Ranges.end(); - I != E; ++I) - R->addRange(*I); + for (const auto &SR : Ranges) + R->addRange(SR); + for (const auto &FH : Fixits) + R->addFixItHint(FH); emitReport(std::move(R)); } diff --git a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp index 04640eb..3ba050f 100644 --- a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp +++ b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp @@ -134,6 +134,7 @@ private: void EmitRanges(raw_ostream &o, const ArrayRef Ranges, unsigned indent); void EmitMessage(raw_ostream &o, StringRef Message, unsigned indent); + void EmitFixits(raw_ostream &o, ArrayRef fixits, unsigned indent); void ReportControlFlow(raw_ostream &o, const PathDiagnosticControlFlowPiece& P, @@ -222,6 +223,33 @@ void PlistPrinter::EmitMessage(raw_ostream &o, StringRef Message, EmitString(o, Message) << '\n'; } +void PlistPrinter::EmitFixits(raw_ostream &o, ArrayRef fixits, + unsigned indent) { + if (fixits.size() == 0) + return; + + const SourceManager &SM = PP.getSourceManager(); + const LangOptions &LangOpts = PP.getLangOpts(); + + Indent(o, indent) << "fixits\n"; + Indent(o, indent) << "\n"; + for (const auto &fixit : fixits) { + assert(!fixit.isNull()); + // FIXME: Add support for InsertFromRange and BeforePreviousInsertion. + assert(!fixit.InsertFromRange.isValid() && "Not implemented yet!"); + assert(!fixit.BeforePreviousInsertions && "Not implemented yet!"); + Indent(o, indent) << " \n"; + Indent(o, indent) << " remove_range\n"; + EmitRange(o, SM, Lexer::getAsCharRange(fixit.RemoveRange, SM, LangOpts), + FM, indent + 2); + Indent(o, indent) << " insert_string"; + EmitString(o, fixit.CodeToInsert); + o << "\n"; + Indent(o, indent) << " \n"; + } + Indent(o, indent) << "\n"; +} + void PlistPrinter::ReportControlFlow(raw_ostream &o, const PathDiagnosticControlFlowPiece& P, unsigned indent) { @@ -272,6 +300,9 @@ void PlistPrinter::ReportControlFlow(raw_ostream &o, EmitString(o, s) << '\n'; } + assert(P.getFixits().size() == 0 && + "Fixits on constrol flow pieces are not implemented yet!"); + --indent; Indent(o, indent) << "\n"; } @@ -308,6 +339,9 @@ void PlistPrinter::ReportEvent(raw_ostream &o, const PathDiagnosticEventPiece& P // Output the text. EmitMessage(o, P.getString(), indent); + // Output the fixits. + EmitFixits(o, P.getFixits(), indent); + // Finish up. --indent; Indent(o, indent); o << "\n"; @@ -335,6 +369,9 @@ void PlistPrinter::ReportCall(raw_ostream &o, const PathDiagnosticCallPiece &P, if (auto callExit = P.getCallExitEvent()) ReportPiece(o, *callExit, indent, depth, /*includeControlFlow*/ true); + + assert(P.getFixits().size() == 0 && + "Fixits on call pieces are not implemented yet!"); } void PlistPrinter::ReportMacroSubPieces(raw_ostream &o, @@ -347,6 +384,9 @@ void PlistPrinter::ReportMacroSubPieces(raw_ostream &o, I != E; ++I) { ReportPiece(o, **I, indent, depth, /*includeControlFlow*/ false); } + + assert(P.getFixits().size() == 0 && + "Fixits on constrol flow pieces are not implemented yet!"); } void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) { @@ -404,6 +444,9 @@ void PlistPrinter::ReportNote(raw_ostream &o, const PathDiagnosticNotePiece& P, // Output the text. EmitMessage(o, P.getString(), indent); + // Output the fixits. + EmitFixits(o, P.getFixits(), indent); + // Finish up. --indent; Indent(o, indent); o << "\n"; @@ -432,6 +475,9 @@ void PlistPrinter::ReportPopUp(raw_ostream &o, // Output the text. EmitMessage(o, P.getString(), indent); + assert(P.getFixits().size() == 0 && + "Fixits on pop-up pieces are not implemented yet!"); + // Finish up. --indent; Indent(o, indent) << "\n"; diff --git a/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp b/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp index 0dcb9db..9f03035 100644 --- a/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp +++ b/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp @@ -83,11 +83,11 @@ void ento::createTextPathDiagnosticConsumer( namespace { class ClangDiagPathDiagConsumer : public PathDiagnosticConsumer { DiagnosticsEngine &Diag; - bool IncludePath, ShouldEmitAsError; + bool IncludePath = false, ShouldEmitAsError = false, FixitsAsRemarks = false; public: ClangDiagPathDiagConsumer(DiagnosticsEngine &Diag) - : Diag(Diag), IncludePath(false), ShouldEmitAsError(false) {} + : Diag(Diag) {} ~ClangDiagPathDiagConsumer() override {} StringRef getName() const override { return "ClangDiags"; } @@ -98,11 +98,9 @@ public: return IncludePath ? Minimal : None; } - void enablePaths() { - IncludePath = true; - } - + void enablePaths() { IncludePath = true; } void enableWerror() { ShouldEmitAsError = true; } + void enableFixitsAsRemarks() { FixitsAsRemarks = true; } void FlushDiagnosticsImpl(std::vector &Diags, FilesMade *filesMade) override { @@ -111,22 +109,46 @@ public: ? Diag.getCustomDiagID(DiagnosticsEngine::Error, "%0") : Diag.getCustomDiagID(DiagnosticsEngine::Warning, "%0"); unsigned NoteID = Diag.getCustomDiagID(DiagnosticsEngine::Note, "%0"); - - for (std::vector::iterator I = Diags.begin(), - E = Diags.end(); I != E; ++I) { + unsigned RemarkID = Diag.getCustomDiagID(DiagnosticsEngine::Remark, "%0"); + + auto reportPiece = + [&](unsigned ID, SourceLocation Loc, StringRef String, + ArrayRef Ranges, ArrayRef Fixits) { + if (!FixitsAsRemarks) { + Diag.Report(Loc, ID) << String << Ranges << Fixits; + } else { + Diag.Report(Loc, ID) << String << Ranges; + for (const FixItHint &Hint : Fixits) { + SourceManager &SM = Diag.getSourceManager(); + llvm::SmallString<128> Str; + llvm::raw_svector_ostream OS(Str); + // FIXME: Add support for InsertFromRange and + // BeforePreviousInsertion. + assert(!Hint.InsertFromRange.isValid() && "Not implemented yet!"); + assert(!Hint.BeforePreviousInsertions && "Not implemented yet!"); + OS << SM.getSpellingColumnNumber(Hint.RemoveRange.getBegin()) + << "-" << SM.getSpellingColumnNumber(Hint.RemoveRange.getEnd()) + << ": '" << Hint.CodeToInsert << "'"; + Diag.Report(Loc, RemarkID) << OS.str(); + } + } + }; + + for (std::vector::iterator I = Diags.begin(), + 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(); + reportPiece(WarnID, PD->getLocation().asLocation(), + PD->getShortDescription(), PD->path.back()->getRanges(), + PD->path.back()->getFixits()); // First, add extra notes, even if paths should not be included. for (const auto &Piece : PD->path) { if (!isa(Piece.get())) continue; - SourceLocation NoteLoc = Piece->getLocation().asLocation(); - Diag.Report(NoteLoc, NoteID) << Piece->getString() - << Piece->getRanges(); + reportPiece(NoteID, Piece->getLocation().asLocation(), + Piece->getString(), Piece->getRanges(), Piece->getFixits()); } if (!IncludePath) @@ -138,9 +160,8 @@ public: if (isa(Piece.get())) continue; - SourceLocation NoteLoc = Piece->getLocation().asLocation(); - Diag.Report(NoteLoc, NoteID) << Piece->getString() - << Piece->getRanges(); + reportPiece(NoteID, Piece->getLocation().asLocation(), + Piece->getString(), Piece->getRanges(), Piece->getFixits()); } } } @@ -241,6 +262,9 @@ public: if (Opts->AnalyzerWerror) clangDiags->enableWerror(); + if (Opts->ShouldEmitFixItHintsAsRemarks) + clangDiags->enableFixitsAsRemarks(); + if (Opts->AnalysisDiagOpt == PD_TEXT) { clangDiags->enablePaths(); diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c index c8a24eea..6c6883d 100644 --- a/clang/test/Analysis/analyzer-config.c +++ b/clang/test/Analysis/analyzer-config.c @@ -30,6 +30,7 @@ // CHECK-NEXT: ctu-dir = "" // CHECK-NEXT: ctu-import-threshold = 100 // CHECK-NEXT: ctu-index-name = externalDefMap.txt +// CHECK-NEXT: deadcode.DeadStores:ShowFixIts = false // CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = true // CHECK-NEXT: debug.AnalysisOrder:* = false // CHECK-NEXT: debug.AnalysisOrder:Bind = false @@ -54,6 +55,7 @@ // CHECK-NEXT: experimental-enable-naive-ctu-analysis = false // CHECK-NEXT: exploration_strategy = unexplored_first_queue // CHECK-NEXT: faux-bodies = true +// CHECK-NEXT: fixits-as-remarks = false // CHECK-NEXT: graph-trim-interval = 1000 // CHECK-NEXT: inline-lambdas = true // CHECK-NEXT: ipa = dynamic-bifurcate @@ -74,6 +76,7 @@ // CHECK-NEXT: optin.cplusplus.UninitializedObject:NotesAsWarnings = false // CHECK-NEXT: optin.cplusplus.UninitializedObject:Pedantic = false // CHECK-NEXT: optin.cplusplus.VirtualCall:PureOnly = false +// CHECK-NEXT: optin.cplusplus.VirtualCall:ShowFixIts = false // CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false // CHECK-NEXT: optin.performance.Padding:AllowedPad = 24 // CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false @@ -94,4 +97,4 @@ // CHECK-NEXT: unroll-loops = false // CHECK-NEXT: widen-loops = false // CHECK-NEXT: [stats] -// CHECK-NEXT: num-entries = 91 +// CHECK-NEXT: num-entries = 94 diff --git a/clang/test/Analysis/dead-stores.c b/clang/test/Analysis/dead-stores.c index 26377f7..cbfdabd 100644 --- a/clang/test/Analysis/dead-stores.c +++ b/clang/test/Analysis/dead-stores.c @@ -1,17 +1,16 @@ -// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \ -// RUN: -analyzer-checker=core,deadcode.DeadStores \ -// RUN: -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\ -// RUN: -analyzer-opt-analyze-nested-blocks -verify=non-nested %s -// -// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \ -// RUN: -analyzer-checker=core,deadcode.DeadStores \ -// RUN: -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\ -// RUN: -analyzer-opt-analyze-nested-blocks -verify=non-nested \ -// RUN: -analyzer-store=region %s -// -// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \ -// RUN: -analyzer-checker=core,deadcode.DeadStores \ -// RUN: -analyzer-opt-analyze-nested-blocks -verify=non-nested,nested %s +// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \ +// RUN: -analyzer-checker=core,deadcode.DeadStores \ +// RUN: -analyzer-config deadcode.DeadStores:ShowFixIts=true \ +// RUN: -analyzer-config fixits-as-remarks=true \ +// RUN: -analyzer-config \ +// RUN: deadcode.DeadStores:WarnForDeadNestedAssignments=false \ +// RUN: -verify=non-nested %s + +// RUN: %clang_analyze_cc1 -Wunused-variable -fblocks -Wno-unreachable-code \ +// RUN: -analyzer-checker=core,deadcode.DeadStores \ +// RUN: -analyzer-config deadcode.DeadStores:ShowFixIts=true \ +// RUN: -analyzer-config fixits-as-remarks=true \ +// RUN: -verify=non-nested,nested %s void f1() { int k, y; // non-nested-warning {{unused variable 'k'}} @@ -19,12 +18,14 @@ void f1() { int abc = 1; long idx = abc + 3 * 5; // non-nested-warning {{never read}} // non-nested-warning@-1 {{unused variable 'idx'}} + // non-nested-remark@-2 {{11-24: ''}} } void f2(void *b) { char *c = (char *)b; // no-warning char *d = b + 1; // non-nested-warning {{never read}} // non-nested-warning@-1 {{unused variable 'd'}} + // non-nested-remark@-2 {{10-17: ''}} printf("%s", c); // non-nested-warning@-1 {{implicitly declaring library function 'printf' with type 'int (const char *, ...)'}} // non-nested-note@-2 {{include the header or explicitly provide a declaration for 'printf'}} @@ -50,6 +51,7 @@ void f5() { int x = 4; // no-warning int *p = &x; // non-nested-warning {{never read}} // non-nested-warning@-1 {{unused variable 'p'}} + // non-nested-remark@-2 {{9-13: ''}} } int f6() { @@ -413,6 +415,7 @@ void f23_pos(int argc, char **argv) { int shouldLog = (argc > 1); // non-nested-warning@-1 {{Value stored to 'shouldLog' during its initialization is never read}} // non-nested-warning@-2 {{unused variable 'shouldLog'}} + // non-nested-remark@-3 {{16-28: ''}} ^{ f23_aux("I did too use it!\n"); }(); @@ -425,6 +428,7 @@ void f24_A(int y) { int z = x + y; // non-nested-warning@-1 {{Value stored to 'z' during its initialization is never read}} // non-nested-warning@-2 {{unused variable 'z'}} + // non-nested-remark@-3 {{10-17: ''}} }(); } diff --git a/clang/test/Analysis/edges-new.mm b/clang/test/Analysis/edges-new.mm index dda1bfb..6bddbef 100644 --- a/clang/test/Analysis/edges-new.mm +++ b/clang/test/Analysis/edges-new.mm @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,deadcode.DeadStores,osx.cocoa.RetainCount,unix.Malloc,unix.MismatchedDeallocator -analyzer-output=plist -o %t -w %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,deadcode.DeadStores,osx.cocoa.RetainCount,unix.Malloc,unix.MismatchedDeallocator -analyzer-output=plist -analyzer-config deadcode.DeadStores:ShowFixIts=true -o %t -w %s // RUN: %normalize_plist <%t | diff -ub %S/Inputs/expected-plists/edges-new.mm.plist - //===----------------------------------------------------------------------===// diff --git a/clang/test/Analysis/objc-arc.m b/clang/test/Analysis/objc-arc.m index cab6618..7127232 100644 --- a/clang/test/Analysis/objc-arc.m +++ b/clang/test/Analysis/objc-arc.m @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.cocoa.RetainCount,deadcode -verify -fblocks -analyzer-opt-analyze-nested-blocks -fobjc-arc -analyzer-output=plist-multi-file -o %t.plist %s +// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,osx.cocoa.RetainCount,deadcode -verify -fblocks -analyzer-opt-analyze-nested-blocks -fobjc-arc -analyzer-output=plist-multi-file -analyzer-config deadcode.DeadStores:ShowFixIts=true -o %t.plist %s // RUN: %normalize_plist <%t.plist | diff -ub %S/Inputs/expected-plists/objc-arc.m.plist - typedef signed char BOOL; diff --git a/clang/test/Analysis/plist-output.m b/clang/test/Analysis/plist-output.m index 3291633..553982b 100644 --- a/clang/test/Analysis/plist-output.m +++ b/clang/test/Analysis/plist-output.m @@ -1,4 +1,4 @@ -// RUN: %clang_analyze_cc1 -analyzer-config eagerly-assume=false %s -analyzer-checker=osx.cocoa.RetainCount,deadcode.DeadStores,core -analyzer-output=plist -o %t.plist +// RUN: %clang_analyze_cc1 -analyzer-config eagerly-assume=false %s -analyzer-checker=osx.cocoa.RetainCount,deadcode.DeadStores,core -analyzer-output=plist -analyzer-config deadcode.DeadStores:ShowFixIts=true -o %t.plist // RUN: %normalize_plist <%t.plist | diff -ub %S/Inputs/expected-plists/plist-output.m.plist - void test_null_init(void) { diff --git a/clang/test/Analysis/virtualcall-fixits.cpp b/clang/test/Analysis/virtualcall-fixits.cpp new file mode 100644 index 0000000..ea149d5 --- /dev/null +++ b/clang/test/Analysis/virtualcall-fixits.cpp @@ -0,0 +1,45 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \ +// RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \ +// RUN: %s 2>&1 | FileCheck -check-prefix=TEXT %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.cplusplus.VirtualCall \ +// RUN: -analyzer-config optin.cplusplus.VirtualCall:ShowFixIts=true \ +// RUN: -analyzer-config fixits-as-remarks=true \ +// RUN: -analyzer-output=plist -o %t.plist -verify %s +// RUN: cat %t.plist | FileCheck -check-prefix=PLIST %s + +struct S { + virtual void foo(); + S() { + foo(); + // expected-warning@-1{{Call to virtual method 'S::foo' during construction bypasses virtual dispatch}} + // expected-remark@-2{{5-5: 'S::'}} + } + ~S(); +}; + +// TEXT: warning: Call to virtual method 'S::foo' during construction +// TEXT-SAME: bypasses virtual dispatch +// TEXT-NEXT: foo(); +// TEXT-NEXT: ^~~~~ +// TEXT-NEXT: S:: +// TEXT-NEXT: 1 warning generated. + +// PLIST: fixits +// PLIST-NEXT: +// PLIST-NEXT: +// PLIST-NEXT: remove_range +// PLIST-NEXT: +// PLIST-NEXT: +// PLIST-NEXT: line13 +// PLIST-NEXT: col5 +// PLIST-NEXT: file0 +// PLIST-NEXT: +// PLIST-NEXT: +// PLIST-NEXT: line13 +// PLIST-NEXT: col4 +// PLIST-NEXT: file0 +// PLIST-NEXT: +// PLIST-NEXT: +// PLIST-NEXT: insert_stringS:: +// PLIST-NEXT: +// PLIST-NEXT: