From 54461eb38953d17a9a9b7aeff90ef59f65884a86 Mon Sep 17 00:00:00 2001 From: Alexander Kornienko Date: Thu, 6 Feb 2014 14:50:10 +0000 Subject: [PATCH] Improve clang-tidy diagnostic output and filtering. Summary: This patch introduces several improvements to clang-tidy diagnostic; 1. Make filtering of messages from non-user code more reliable. Output an error when it or any of the related notes touches user code. This fixes an assertion when an error has a location in a system header, and one of the notes relates to user code. 2. In order for 1. to work, subscribe to the static analyzer diagnostics using a custom PathDiagnosticConsumer. 3. Enable colors on supported terminals. 4. Output FixItHints. Reviewers: djasper Reviewed By: djasper CC: cfe-commits Differential Revision: http://llvm-reviews.chandlerc.com/D2714 llvm-svn: 200924 --- clang-tools-extra/clang-tidy/ClangTidy.cpp | 130 +++++++++++++----- .../ClangTidyDiagnosticConsumer.cpp | 43 +++--- .../clang-tidy/ClangTidyDiagnosticConsumer.h | 5 +- 3 files changed, 126 insertions(+), 52 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 4129414c10e2..927c5910cb28 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -35,6 +35,7 @@ #include "clang/Tooling/Refactoring.h" #include "clang/Tooling/Tooling.h" #include "llvm/Support/Path.h" +#include "llvm/Support/Process.h" #include "llvm/Support/Signals.h" #include #include @@ -48,9 +49,9 @@ namespace clang { namespace tidy { namespace { -static const char *AnalyzerCheckerNamePrefix = "clang-analyzer-"; +static const char *AnalyzerCheckNamePrefix = "clang-analyzer-"; -static StringRef StaticAnalyzerCheckers[] = { +static StringRef StaticAnalyzerChecks[] = { #define GET_CHECKERS #define CHECKER(FULLNAME, CLASS, DESCFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ FULLNAME, @@ -59,6 +60,56 @@ static StringRef StaticAnalyzerCheckers[] = { #undef GET_CHECKERS }; +class AnalyzerDiagnosticConsumer : public ento::PathDiagnosticConsumer { +public: + AnalyzerDiagnosticConsumer(ClangTidyContext &Context) : Context(Context) {} + + virtual void + FlushDiagnosticsImpl(std::vector &Diags, + FilesMade *filesMade) LLVM_OVERRIDE { + for (std::vector::iterator I = Diags.begin(), + E = Diags.end(); + I != E; ++I) { + const ento::PathDiagnostic *PD = *I; + StringRef CheckName(AnalyzerCheckNamePrefix); + addRanges(Context.diag(CheckName, PD->getLocation().asLocation(), + PD->getShortDescription()), + PD->path.back()->getRanges()); + + ento::PathPieces FlatPath = + PD->path.flatten(/*ShouldFlattenMacros=*/true); + for (ento::PathPieces::const_iterator PI = FlatPath.begin(), + PE = FlatPath.end(); + PI != PE; ++PI) { + addRanges(Context.diag(CheckName, (*PI)->getLocation().asLocation(), + (*PI)->getString(), DiagnosticIDs::Note), + (*PI)->getRanges()); + } + } + } + + virtual StringRef getName() const { return "ClangTidyDiags"; } + + virtual bool supportsLogicalOpControlFlow() const LLVM_OVERRIDE { + return true; + } + virtual bool supportsCrossFileDiagnostics() const LLVM_OVERRIDE { + return true; + } + +private: + ClangTidyContext &Context; + + // FIXME: Convert to operator<<(DiagnosticBuilder&, ArrayRef). + static const DiagnosticBuilder &addRanges(const DiagnosticBuilder &DB, + ArrayRef Ranges) { + for (ArrayRef::iterator I = Ranges.begin(), E = Ranges.end(); + I != E; ++I) + DB << *I; + return DB; + } +}; + } // namespace ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory( @@ -103,15 +154,15 @@ clang::ASTConsumer *ClangTidyASTConsumerFactory::CreateASTConsumer( AnalyzerOptionsRef Options = Compiler.getAnalyzerOpts(); Options->CheckersControlList = getCheckersControlList(); Options->AnalysisStoreOpt = RegionStoreModel; - Options->AnalysisDiagOpt = PD_TEXT; + Options->AnalysisDiagOpt = PD_NONE; Options->AnalyzeNestedBlocks = true; Options->eagerlyAssumeBinOpBifurcation = true; - ASTConsumer *Consumers[] = { - Finder.newASTConsumer(), - ento::CreateAnalysisConsumer(Compiler.getPreprocessor(), - Compiler.getFrontendOpts().OutputFile, Options, - Compiler.getFrontendOpts().Plugins) - }; + ento::AnalysisASTConsumer *AnalysisConsumer = ento::CreateAnalysisConsumer( + Compiler.getPreprocessor(), Compiler.getFrontendOpts().OutputFile, + Options, Compiler.getFrontendOpts().Plugins); + AnalysisConsumer->AddDiagnosticConsumer( + new AnalyzerDiagnosticConsumer(Context)); + ASTConsumer *Consumers[] = { Finder.newASTConsumer(), AnalysisConsumer }; return new MultiplexConsumer(Consumers); } @@ -129,7 +180,7 @@ std::vector ClangTidyASTConsumerFactory::getCheckNames() { for (CheckersList::const_iterator I = AnalyzerChecks.begin(), E = AnalyzerChecks.end(); I != E; ++I) - CheckNames.push_back(AnalyzerCheckerNamePrefix + I->first); + CheckNames.push_back(AnalyzerCheckNamePrefix + I->first); std::sort(CheckNames.begin(), CheckNames.end()); return CheckNames; @@ -138,13 +189,13 @@ std::vector ClangTidyASTConsumerFactory::getCheckNames() { ClangTidyASTConsumerFactory::CheckersList ClangTidyASTConsumerFactory::getCheckersControlList() { CheckersList List; - ArrayRef Checkers(StaticAnalyzerCheckers); + ArrayRef Checks(StaticAnalyzerChecks); bool AnalyzerChecksEnabled = false; - for (unsigned i = 0; i < Checkers.size(); ++i) { - std::string Checker((AnalyzerCheckerNamePrefix + Checkers[i]).str()); + for (unsigned i = 0; i < Checks.size(); ++i) { + std::string Checker((AnalyzerCheckNamePrefix + Checks[i]).str()); AnalyzerChecksEnabled |= - Filter.IsCheckEnabled(Checker) && !Checkers[i].startswith("debug"); + Filter.IsCheckEnabled(Checker) && !Checks[i].startswith("debug"); } if (AnalyzerChecksEnabled) { @@ -155,12 +206,12 @@ ClangTidyASTConsumerFactory::getCheckersControlList() { // Always add all core checkers if any other static analyzer checks are // enabled. This is currently necessary, as other path sensitive checks // rely on the core checkers. - for (unsigned i = 0; i < Checkers.size(); ++i) { - std::string Checker((AnalyzerCheckerNamePrefix + Checkers[i]).str()); + for (unsigned i = 0; i < Checks.size(); ++i) { + std::string Checker((AnalyzerCheckNamePrefix + Checks[i]).str()); - if (Checkers[i].startswith("core") || - (!Checkers[i].startswith("debug") && Filter.IsCheckEnabled(Checker))) - List.push_back(std::make_pair(Checkers[i], true)); + if (Checks[i].startswith("core") || + (!Checks[i].startswith("debug") && Filter.IsCheckEnabled(Checker))) + List.push_back(std::make_pair(Checks[i], true)); } } return List; @@ -237,29 +288,42 @@ void runClangTidy(StringRef EnableChecksRegex, StringRef DisableChecksRegex, EnableChecksRegex, DisableChecksRegex, Context))); } +static SourceLocation getLocation(SourceManager &SourceMgr, StringRef FilePath, + unsigned Offset) { + if (FilePath.empty()) + return SourceLocation(); + + const FileEntry *File = SourceMgr.getFileManager().getFile(FilePath); + FileID ID = SourceMgr.createFileID(File, SourceLocation(), SrcMgr::C_User); + return SourceMgr.getLocForStartOfFile(ID).getLocWithOffset(Offset); +} + static void reportDiagnostic(const ClangTidyMessage &Message, SourceManager &SourceMgr, DiagnosticsEngine::Level Level, DiagnosticsEngine &Diags, - StringRef CheckName = "") { - SourceLocation Loc; - if (!Message.FilePath.empty()) { - const FileEntry *File = - SourceMgr.getFileManager().getFile(Message.FilePath); - FileID ID = SourceMgr.createFileID(File, SourceLocation(), SrcMgr::C_User); - Loc = SourceMgr.getLocForStartOfFile(ID); - Loc = Loc.getLocWithOffset(Message.FileOffset); + tooling::Replacements *Fixes = NULL) { + SourceLocation Loc = + getLocation(SourceMgr, Message.FilePath, Message.FileOffset); + DiagnosticBuilder Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0")) + << Message.Message; + if (Fixes != NULL) { + for (tooling::Replacements::const_iterator I = Fixes->begin(), + E = Fixes->end(); + I != E; ++I) { + SourceLocation FixLoc = + getLocation(SourceMgr, I->getFilePath(), I->getOffset()); + Diag << FixItHint::CreateReplacement( + SourceRange(FixLoc, FixLoc.getLocWithOffset(I->getLength())), + I->getReplacementText()); + } } - if (CheckName.empty()) - Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0")) << Message.Message; - else - Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]")) - << Message.Message << CheckName; } void handleErrors(SmallVectorImpl &Errors, bool Fix) { FileManager Files((FileSystemOptions())); IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions(); + DiagOpts->ShowColors = llvm::sys::Process::StandardErrHasColors(); DiagnosticConsumer *DiagPrinter = new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts); DiagnosticsEngine Diags(IntrusiveRefCntPtr(new DiagnosticIDs), @@ -271,7 +335,7 @@ void handleErrors(SmallVectorImpl &Errors, bool Fix) { E = Errors.end(); I != E; ++I) { reportDiagnostic(I->Message, SourceMgr, DiagnosticsEngine::Warning, Diags, - I->CheckName); + &I->Fix); for (unsigned i = 0, e = I->Notes.size(); i != e; ++i) { reportDiagnostic(I->Notes[i], SourceMgr, DiagnosticsEngine::Note, Diags); } diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 631fdd18542c..e83c8c7bc52f 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -37,11 +37,11 @@ ClangTidyError::ClangTidyError(StringRef CheckName, const ClangTidyMessage &Message) : CheckName(CheckName), Message(Message) {} -DiagnosticBuilder ClangTidyContext::diag(StringRef CheckName, - SourceLocation Loc, - StringRef Description) { +DiagnosticBuilder ClangTidyContext::diag( + StringRef CheckName, SourceLocation Loc, StringRef Description, + DiagnosticIDs::Level Level /* = DiagnosticsEngine::Warning*/) { unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID( - DiagnosticIDs::Warning, Description); + Level, (Description + " [" + CheckName + "]").str()); if (CheckNamesByDiagnosticID.count(ID) == 0) CheckNamesByDiagnosticID.insert(std::make_pair(ID, CheckName.str())); return DiagEngine->Report(Loc, ID); @@ -69,7 +69,7 @@ StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const { } ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx) - : Context(Ctx) { + : Context(Ctx), LastErrorRelatesToUserCode(false) { IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions(); Diags.reset(new DiagnosticsEngine( IntrusiveRefCntPtr(new DiagnosticIDs), &*DiagOpts, this, @@ -77,31 +77,38 @@ ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx) Context.setDiagnosticsEngine(Diags.get()); } +void ClangTidyDiagnosticConsumer::finalizeLastError() { + if (!LastErrorRelatesToUserCode && !Errors.empty()) + Errors.pop_back(); + LastErrorRelatesToUserCode = false; +} + void ClangTidyDiagnosticConsumer::HandleDiagnostic( DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) { - // FIXME: Demultiplex diagnostics. - // FIXME: Ensure that we don't get notes from user code related to errors - // from non-user code. - // Let argument parsing-related warnings through. - if (Diags->hasSourceManager() && - Diags->getSourceManager().isInSystemHeader(Info.getLocation())) - return; - if (DiagLevel != DiagnosticsEngine::Note) { - Errors.push_back( - ClangTidyError(Context.getCheckName(Info.getID()), getMessage(Info))); - } else { + // FIXME: Deduplicate diagnostics. + if (DiagLevel == DiagnosticsEngine::Note) { assert(!Errors.empty() && "A diagnostic note can only be appended to a message."); Errors.back().Notes.push_back(getMessage(Info)); + } else { + finalizeLastError(); + Errors.push_back( + ClangTidyError(Context.getCheckName(Info.getID()), getMessage(Info))); } addFixes(Info, Errors.back()); + + // Let argument parsing-related warnings through. + if (!Diags->hasSourceManager() || + !Diags->getSourceManager().isInSystemHeader(Info.getLocation())) { + LastErrorRelatesToUserCode = true; + } } // Flushes the internal diagnostics buffer to the ClangTidyContext. void ClangTidyDiagnosticConsumer::finish() { - for (unsigned i = 0, e = Errors.size(); i != e; ++i) { + finalizeLastError(); + for (unsigned i = 0, e = Errors.size(); i != e; ++i) Context.storeError(Errors[i]); - } Errors.clear(); } diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index f5e2857f66f5..f86d28409c4e 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -75,7 +75,8 @@ public: /// tablegen'd diagnostic IDs. /// FIXME: Figure out a way to manage ID spaces. DiagnosticBuilder diag(StringRef CheckName, SourceLocation Loc, - StringRef Message); + StringRef Message, + DiagnosticIDs::Level Level = DiagnosticIDs::Warning); /// \brief Sets the \c DiagnosticsEngine so that Diagnostics can be generated /// correctly. @@ -124,10 +125,12 @@ public: private: void addFixes(const Diagnostic &Info, ClangTidyError &Error); ClangTidyMessage getMessage(const Diagnostic &Info) const; + void finalizeLastError(); ClangTidyContext &Context; OwningPtr Diags; SmallVector Errors; + bool LastErrorRelatesToUserCode; }; } // end namespace tidy