[clangd] Initial clang-tidy diagnostics support.

[sammccall]

Summary

This runs checks over a restricted subset of the TU:

• preprocessor callbacks just receive the truncated PP events that occur when a preamble is used.
• ASTMatchers run only over the top-level decls in the main-file

This patch just turns on one simple check (bugprone-sizeof-expression)
with no configuration.

• configuration is complex enough to warrant a separate patch
• arbitrary checks don't work well yet - there are various ways that checks can access the whole AST (and thus be incredibly slow). Most notably: the hasAncestor matcher, and using the ASTContext from check().

This depends on a small patch to ASTMatchers to run a MatchFinder on a
certain set of top-level declarations.

[hokein]

I think this is in a good shape as initial patch!

clangd/ClangdUnit.cpp

168 ↗

(On Diff #172937)

It seems we also support clang-tidy checks that analyze preprocessor-dependent properties. I think we can add a clang-tidy check to make sure PPCallback actually work, google-readability-todo is a good candidate.

Ah just realized limitations (truncated PPCallback events) you wrote in the patch description, maybe mention them in the source comment, so that we won't forget in the future when reading the code.

175 ↗

(On Diff #172937)

Maybe add the check names to the Trace?

468 ↗

(On Diff #172937)

I'm curious how much does clangd binary size get increased.

[sammccall]

Address comments and rebase on D54309, addressing performance issues.

[sammccall]

clangd/ClangdUnit.cpp

175 ↗

(On Diff #172937)

that would be nice, but there's no API to get that info :-(

468 ↗

(On Diff #172937)

It's now 21M stripped vs 18M before this patch.
The different with debug info is *much* larger for some reason... sadly this will hurt link times.

[hokein]

Looks mostly good, just a few nits.

This patch contains two parts (clang-tidy and clangd), I think we could split into two, but I'm not insisting, up to you.

clang-tidy/modernize/LoopConvertUtils.h

59 ↗

(On Diff #173357)

The comment is out-of-date.

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

518 ↗

(On Diff #173357)

maybe add a comment describing we are trying to find the top level decl?

566 ↗

(On Diff #173357)

add an assert (MatchOnce)?

clang-tidy/readability/SimplifyBooleanExprCheck.h

82 ↗

(On Diff #173357)

The name seems a bit unclear for readers without any context, maybe FoundTopDecl?

clangd/ClangdUnit.cpp

158 ↗

(On Diff #173357)

just want to make sure -- do we receive all preprocessor events in the main file when we use preamble? We have a few checks that generate #include insertion as part of FixIt.

TestTU doesn't use preamble, it would be nice to have a test running on a main file with a real preamble, but this can be a follow-up I think.

[sammccall]

Moved the clang-tidy changes to D54579.
Sorry for mixing everything up!

clang-tidy/readability/SimplifyBooleanExprCheck.cpp

518 ↗

(On Diff #173357)

We're not, we're trying to match any node (but only one). Extended the comment on the matcher.

566 ↗

(On Diff #173357)

That doesn't compile, I'm not sure what you want here.

clangd/ClangdUnit.cpp

158 ↗

(On Diff #173357)

Nope, you're right: #include from the preamble are not replayed. Can't fix this now, added a FIXME. (Neither of the hardcoded checks care).

TestTU does use a preamble now :-)

[sammccall]

Remove clang-tidy changes, add FIXME comment.

[hokein]

looks good!

clang-tools-extra/trunk/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   clangBasic
   clangDriver
   clangFormat
   clangFrontend
   clangIndex
   clangLex
   clangSema
   clangSerialization
+  clangTidy
+  clangTidyAndroidModule
+  clangTidyAbseilModule
+  clangTidyBoostModule
+  clangTidyBugproneModule
+  clangTidyCERTModule
+  clangTidyCppCoreGuidelinesModule
+  clangTidyFuchsiaModule
+  clangTidyGoogleModule
+  clangTidyHICPPModule
+  clangTidyLLVMModule
+  clangTidyMiscModule
+  clangTidyModernizeModule
+  clangTidyObjCModule
+  clangTidyPerformanceModule
+  clangTidyPortabilityModule
+  clangTidyReadabilityModule
+  clangTidyZirconModule
   clangTooling
   clangToolingCore
   clangToolingInclusions
   clangToolingRefactor
   ${LLVM_PTHREAD_LIB}
   ${CLANGD_ATOMIC_LIB}
   )
 

clang-tools-extra/trunk/clangd/ClangdUnit.cpp

 //===--- ClangdUnit.cpp ------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdUnit.h"
+#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "Logger.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Frontend/CompilerInstance.h"
ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
   auto Action = llvm::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
     log("BeginSourceFile() failed when building AST for {0}",
         MainInput.getFile());
     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<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;
+  ast_matchers::MatchFinder CTFinder;
+  llvm::Optional<tidy::ClangTidyContext> 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::DefaultOptionsProvider>(
+        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) {
+      // FIXME: the PP callbacks skip the entire preamble.
+      // Checks that want to see #includes in the main file do not see them.
+      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{};
   Clang->getPreprocessor().addPPCallbacks(
       collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
 
   if (!Action->Execute())
     log("Execute() failed when building AST for {0}", MainInput.getFile());
 
+  std::vector<Decl *> 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<Decl *> ParsedDecls = Action->takeTopLevelDecls();
   std::vector<Diag> Diags = ASTDiags.take();
   // Add diagnostics from the preamble, if any.
   if (Preamble)
     Diags.insert(Diags.begin(), Preamble->Diags.begin(), Preamble->Diags.end());
   return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
                    std::move(ParsedDecls), std::move(Diags),
                    std::move(Includes));
 }
 
 ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 
 ParsedAST &ParsedAST::operator=(ParsedAST &&Other) = default;
 
 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.
   }
 }
 
 ASTContext &ParsedAST::getASTContext() { return Clang->getASTContext(); }
 
 const ASTContext &ParsedAST::getASTContext() const {
   return Clang->getASTContext();
 }
SourceLocation getBeginningOfIdentifier(ParsedAST &Unit, const Position &Pos,
   if (Before.isValid() &&
       !Lexer::getRawToken(Before, Tok, SourceMgr, AST.getLangOpts(), false) &&
       Tok.is(tok::raw_identifier))
     return SourceMgr.getMacroArgExpandedLocation(Before); // Case 2.
   return SourceMgr.getMacroArgExpandedLocation(InputLoc); // Case 1 or 3.
 }
 
 } // 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

clang-tools-extra/trunk/clangd/XRefs.cpp

if (!SourceLocationBeg.isValid() ||
       !Tok.is(tok::raw_identifier)) {
     return {};
   }
   AST.getPreprocessor().LookUpIdentifierInfo(Tok);
   if (!(Tok.is(tok::kw_auto) || Tok.is(tok::kw_decltype)))
     return {};
 
   DeducedTypeVisitor V(SourceLocationBeg);
-  for (Decl *D : AST.getLocalTopLevelDecls())
-    V.TraverseDecl(D);
+  V.TraverseAST(AST.getASTContext());
   return V.getDeducedType();
 }
 
 Optional<Hover> getHover(ParsedAST &AST, Position Pos) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   SourceLocation SourceLocationBeg =
       getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
   // Identified symbols at a specific position.

clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp

 namespace clang {
 namespace clangd {
 namespace {
 
 using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Pair;
+using testing::UnorderedElementsAre;
 
 testing::Matcher<const Diag &> WithFix(testing::Matcher<Fix> FixMatcher) {
   return Field(&Diag::Fixes, ElementsAre(FixMatcher));
 }
 
 testing::Matcher<const Diag &> WithNote(testing::Matcher<Note> NoteMatcher) {
   return Field(&Diag::Notes, ElementsAre(NoteMatcher));
 }
TEST(DiagnosticsTest, FlagsMatter) {
   TU.Filename = "Plain.c";
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       ElementsAre(AllOf(
           Diag(Test.range(), "return type of 'main' is not 'int'"),
           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:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO