Index: clang-tidy/misc/UnusedParametersCheck.cpp =================================================================== --- clang-tidy/misc/UnusedParametersCheck.cpp +++ clang-tidy/misc/UnusedParametersCheck.cpp @@ -74,7 +74,7 @@ class UnusedParametersCheck::IndexerVisitor : public RecursiveASTVisitor { public: - IndexerVisitor(TranslationUnitDecl *Top) { TraverseDecl(Top); } + IndexerVisitor(ASTContext &Ctx) { TraverseAST(Ctx); } const std::unordered_set & getFnCalls(const FunctionDecl *Fn) { @@ -136,8 +136,7 @@ auto MyDiag = diag(Param->getLocation(), "parameter %0 is unused") << Param; if (!Indexer) { - Indexer = llvm::make_unique( - Result.Context->getTranslationUnitDecl()); + Indexer = llvm::make_unique(*Result.Context); } // Comment out parameter name for non-local functions. Index: clang-tidy/modernize/LoopConvertCheck.cpp =================================================================== --- clang-tidy/modernize/LoopConvertCheck.cpp +++ clang-tidy/modernize/LoopConvertCheck.cpp @@ -899,7 +899,7 @@ // variable declared inside the loop outside of it. // FIXME: Determine when the external dependency isn't an expression converted // by another loop. - TUInfo->getParentFinder().gatherAncestors(Context->getTranslationUnitDecl()); + TUInfo->getParentFinder().gatherAncestors(*Context); DependencyFinderASTVisitor DependencyFinder( &TUInfo->getParentFinder().getStmtToParentStmtMap(), &TUInfo->getParentFinder().getDeclToParentStmtMap(), Index: clang-tidy/modernize/LoopConvertUtils.h =================================================================== --- clang-tidy/modernize/LoopConvertUtils.h +++ clang-tidy/modernize/LoopConvertUtils.h @@ -59,9 +59,9 @@ /// \brief Run the analysis on the TranslationUnitDecl. /// /// In case we're running this analysis multiple times, don't repeat the work. - void gatherAncestors(const clang::TranslationUnitDecl *T) { + void gatherAncestors(ASTContext &Ctx) { if (StmtAncestors.empty()) - TraverseDecl(const_cast(T)); + TraverseAST(Ctx); } /// Accessor for StmtAncestors. Index: clang-tidy/readability/SimplifyBooleanExprCheck.h =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.h +++ clang-tidy/readability/SimplifyBooleanExprCheck.h @@ -79,6 +79,7 @@ SourceLocation Loc, StringRef Description, SourceRange ReplacementRange, StringRef Replacement); + bool MatchedOnce = false; const bool ChainedConditionalReturn; const bool ChainedConditionalAssignment; }; Index: clang-tidy/readability/SimplifyBooleanExprCheck.cpp =================================================================== --- clang-tidy/readability/SimplifyBooleanExprCheck.cpp +++ clang-tidy/readability/SimplifyBooleanExprCheck.cpp @@ -507,8 +507,15 @@ ChainedConditionalAssignment); } +// This is a silly hack to let us run a RecursiveASTVisitor on the Context. +AST_MATCHER_P(Decl, matchOnce, bool *, Matched) { + if (*Matched) + return false; + return *Matched = true; +} + void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(translationUnitDecl().bind("top"), this); + Finder->addMatcher(matchOnce(&MatchedOnce), this); matchBoolCondition(Finder, true, ConditionThenStmtId); matchBoolCondition(Finder, false, ConditionElseStmtId); @@ -556,8 +563,8 @@ else if (const auto *Compound = Result.Nodes.getNodeAs(CompoundNotBoolId)) replaceCompoundReturnWithCondition(Result, Compound, true); - else if (const auto TU = Result.Nodes.getNodeAs("top")) - Visitor(this, Result).TraverseDecl(const_cast(TU)); + else // MatchOnce matcher + Visitor(this, Result).TraverseAST(*Result.Context); } void SimplifyBooleanExprCheck::issueDiag( Index: clangd/CMakeLists.txt =================================================================== --- clangd/CMakeLists.txt +++ clangd/CMakeLists.txt @@ -64,6 +64,24 @@ clangLex clangSema clangSerialization + clangTidy + clangTidyAndroidModule + clangTidyAbseilModule + clangTidyBoostModule + clangTidyBugproneModule + clangTidyCERTModule + clangTidyCppCoreGuidelinesModule + clangTidyFuchsiaModule + clangTidyGoogleModule + clangTidyHICPPModule + clangTidyLLVMModule + clangTidyMiscModule + clangTidyModernizeModule + clangTidyObjCModule + clangTidyPerformanceModule + clangTidyPortabilityModule + clangTidyReadabilityModule + clangTidyZirconModule clangTooling clangToolingCore clangToolingInclusions Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -8,6 +8,8 @@ //===----------------------------------------------------------------------===// #include "ClangdUnit.h" +#include "../clang-tidy/ClangTidyDiagnosticConsumer.h" +#include "../clang-tidy/ClangTidyModuleRegistry.h" #include "Compiler.h" #include "Diagnostics.h" #include "Logger.h" @@ -151,6 +153,38 @@ return None; } + // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext exists. + // Clang-tidy has some limitiations to ensure reasonable performance: + // - checks don't see all preprocessor events in the preamble + // - matchers run only over the main-file top-level decls (and can't see + // ancestors outside this scope). + // In practice almost all checks work well without modifications. + std::vector> CTChecks; + ast_matchers::MatchFinder CTFinder; + llvm::Optional CTContext; + { + trace::Span Tracer("ClangTidyInit"); + tidy::ClangTidyCheckFactories CTFactories; + for (const auto &E : tidy::ClangTidyModuleRegistry::entries()) + E.instantiate()->addCheckFactories(CTFactories); + auto CTOpts = tidy::ClangTidyOptions::getDefaults(); + // FIXME: this needs to be configurable, and we need to support .clang-tidy + // files and other options providers. + // These checks exercise the matcher- and preprocessor-based hooks. + CTOpts.Checks = + "bugprone-sizeof-expression,bugprone-macro-repeated-side-effects"; + CTContext.emplace(llvm::make_unique( + tidy::ClangTidyGlobalOptions(), CTOpts)); + CTContext->setDiagnosticsEngine(&Clang->getDiagnostics()); + CTContext->setASTContext(&Clang->getASTContext()); + CTContext->setCurrentFile(MainInput.getFile()); + CTFactories.createChecks(CTContext.getPointer(), CTChecks); + for (const auto &Check : CTChecks) { + Check->registerPPCallbacks(*Clang); + Check->registerMatchers(&CTFinder); + } + } + // Copy over the includes from the preamble, then combine with the // non-preamble includes below. auto Includes = Preamble ? Preamble->Includes : IncludeStructure{}; @@ -160,13 +194,26 @@ if (!Action->Execute()) log("Execute() failed when building AST for {0}", MainInput.getFile()); + std::vector ParsedDecls = Action->takeTopLevelDecls(); + // AST traversals should exclude the preamble, to avoid performance cliffs. + Clang->getASTContext().setTraversalScope(ParsedDecls); + { + // Run the AST-dependent part of the clang-tidy checks. + // (The preprocessor part ran already, via PPCallbacks). + trace::Span Tracer("ClangTidyMatch"); + CTFinder.matchAST(Clang->getASTContext()); + } + // UnitDiagsConsumer is local, we can not store it in CompilerInstance that // has a longer lifetime. Clang->getDiagnostics().setClient(new IgnoreDiagnostics); // CompilerInstance won't run this callback, do it directly. ASTDiags.EndSourceFile(); + // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF. + // However Action->EndSourceFile() would destroy the ASTContext! + // So just inform the preprocessor of EOF, while keeping everything alive. + Clang->getPreprocessor().EndSourceFile(); - std::vector ParsedDecls = Action->takeTopLevelDecls(); std::vector Diags = ASTDiags.take(); // Add diagnostics from the preamble, if any. if (Preamble) @@ -182,7 +229,12 @@ ParsedAST::~ParsedAST() { if (Action) { - Action->EndSourceFile(); + // We already notified the PP of end-of-file earlier, so detach it first. + // We must keep it alive until after EndSourceFile(), Sema relies on this. + auto PP = Clang->getPreprocessorPtr(); // Keep PP alive for now. + Clang->setPreprocessor(nullptr); // Detach so we don't send EOF again. + Action->EndSourceFile(); // Destroy ASTContext and Sema. + // Now Sema is gone, it's safe for PP to go out of scope. } } @@ -416,4 +468,29 @@ } } // namespace clangd +namespace tidy { +// Force the linker to link in Clang-tidy modules. +#define LINK_TIDY_MODULE(X) \ + extern volatile int X##ModuleAnchorSource; \ + static int LLVM_ATTRIBUTE_UNUSED X##ModuleAnchorDestination = \ + X##ModuleAnchorSource +LINK_TIDY_MODULE(CERT); +LINK_TIDY_MODULE(Abseil); +LINK_TIDY_MODULE(Boost); +LINK_TIDY_MODULE(Bugprone); +LINK_TIDY_MODULE(LLVM); +LINK_TIDY_MODULE(CppCoreGuidelines); +LINK_TIDY_MODULE(Fuchsia); +LINK_TIDY_MODULE(Google); +LINK_TIDY_MODULE(Android); +LINK_TIDY_MODULE(Misc); +LINK_TIDY_MODULE(Modernize); +LINK_TIDY_MODULE(Performance); +LINK_TIDY_MODULE(Portability); +LINK_TIDY_MODULE(Readability); +LINK_TIDY_MODULE(ObjC); +LINK_TIDY_MODULE(HICPP); +LINK_TIDY_MODULE(Zircon); +#undef LINK_TIDY_MODULE +} // namespace tidy } // namespace clang Index: clangd/XRefs.cpp =================================================================== --- clangd/XRefs.cpp +++ clangd/XRefs.cpp @@ -672,8 +672,7 @@ return {}; DeducedTypeVisitor V(SourceLocationBeg); - for (Decl *D : AST.getLocalTopLevelDecls()) - V.TraverseDecl(D); + V.TraverseAST(AST.getASTContext()); return V.getDeducedType(); } Index: unittests/clangd/ClangdUnitTests.cpp =================================================================== --- unittests/clangd/ClangdUnitTests.cpp +++ unittests/clangd/ClangdUnitTests.cpp @@ -24,6 +24,7 @@ using testing::Field; using testing::IsEmpty; using testing::Pair; +using testing::UnorderedElementsAre; testing::Matcher WithFix(testing::Matcher FixMatcher) { return Field(&Diag::Fixes, ElementsAre(FixMatcher)); @@ -128,6 +129,30 @@ WithFix(Fix(Test.range(), "int", "change return type to 'int'"))))); } +TEST(DiagnosticsTest, ClangTidy) { + Annotations Test(R"cpp( + #define $macrodef[[SQUARE]](X) (X)*(X) + int main() { + return [[sizeof]](sizeof(int)); + int y = 4; + return SQUARE($macroarg[[++]]y); + } + )cpp"); + auto TU = TestTU::withCode(Test.code()); + EXPECT_THAT( + TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Test.range(), "suspicious usage of 'sizeof(sizeof(...))' " + "[bugprone-sizeof-expression]"), + AllOf( + Diag(Test.range("macroarg"), + "side effects in the 1st macro argument 'X' are repeated in " + "macro expansion [bugprone-macro-repeated-side-effects]"), + WithNote(Diag(Test.range("macrodef"), + "macro 'SQUARE' defined here " + "[bugprone-macro-repeated-side-effects]"))))); +} + TEST(DiagnosticsTest, Preprocessor) { // This looks like a preamble, but there's an #else in the middle! // Check that: