[clangd] Replay preamble #includes to clang-tidy checks.

[sammccall]

Summary

This is needed to correctly handle checks that use IncludeInserter,
which is very common.

I couldn't find a totally safe example of a check to enable for testing,
I picked modernize-deprecated-headers which some will probably hate.
We should get configuration working...

This depends on D54691 which ensures our calls to getFile(open=false)
don't break subsequent accesses via the FileManager.

[ilya-biryukov]

clangd/ClangdUnit.cpp

129 ↗

(On Diff #174575)

Maybe move fields and the private function to the end of the class?
We definitely don't have a strict rule about this in clangd, but that's what most of the code does. The upside is that the public interface of the class is the first thing that readers of the code see.

That's a stylistic thing, so definitely feel free to disagree :-)

159 ↗

(On Diff #174575)

This should be Delegate-> instead of PP.getCallbacks().
Maybe add a test that catches it?

165 ↗

(On Diff #174575)

Another one: should this be Delegate->?

279 ↗

(On Diff #174575)

Maybe assert that the pointer returned from getPPCallbacks() actually changes after the call to addPPCallbacks call here?
A potential breakage might occur if the implementation of adding a callback changes and uses an inheritor of PPCallbacks that stores a list internally and adds new callbacks to a list instead of wrapping an existing one.

[sammccall]

Address comments.

clangd/ClangdUnit.cpp

159 ↗

(On Diff #174575)

So after permuting the code, the existing test catches this :-/
Not sure why it didn't before.

It's hard to construct a test that catches this explicitly, the most obvious way would be to add a PP callbacks inside/outside the replay and ensure it does/doesn't capture the events, but there's deliberately no public API for that.

The offline discussion about using the recorded IncludeStructure seems like the best we can do through the public API, but it's pretty indirect.

279 ↗

(On Diff #174575)

To do this without further cluttering this bloated function, I wrapped this in ReplayPreamble::attach. I'm not sure I like it, but I'm out of ideas, feel free to bikeshed.

[ilya-biryukov]

LGTM

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

 
 private:
   PathRef File;
   PreambleParsedCallback ParsedCallback;
   IncludeStructure Includes;
   SourceManager *SourceMgr = nullptr;
 };
 
+// When using a preamble, only preprocessor events outside its bounds are seen.
+// This is almost what we want: replaying transitive preprocessing wastes time.
+// However this confuses clang-tidy checks: they don't see any #includes!
+// So we replay the *non-transitive* #includes that appear in the main-file.
+// It would be nice to replay other events (macro definitions, ifdefs etc) but
+// this addresses the most common cases fairly cheaply.
+class ReplayPreamble : private PPCallbacks {
+public:
+  // Attach preprocessor hooks such that preamble events will be injected at
+  // the appropriate time.
+  // Events will be delivered to the *currently registered* PP callbacks.
+  static void attach(const IncludeStructure &Includes,
+                     CompilerInstance &Clang) {
+    auto &PP = Clang.getPreprocessor();
+    auto *ExistingCallbacks = PP.getPPCallbacks();
+    PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(
+        new ReplayPreamble(Includes, ExistingCallbacks,
+                           Clang.getSourceManager(), PP, Clang.getLangOpts())));
+    // We're relying on the fact that addPPCallbacks keeps the old PPCallbacks
+    // around, creating a chaining wrapper. Guard against other implementations.
+    assert(PP.getPPCallbacks() != ExistingCallbacks &&
+           "Expected chaining implementation");
+  }
+
+private:
+  ReplayPreamble(const IncludeStructure &Includes, PPCallbacks *Delegate,
+                 const SourceManager &SM, Preprocessor &PP,
+                 const LangOptions &LangOpts)
+      : Includes(Includes), Delegate(Delegate), SM(SM), PP(PP),
+        LangOpts(LangOpts) {}
+
+  // In a normal compile, the preamble traverses the following structure:
+  //
+  // mainfile.cpp
+  //   <built-in>
+  //     ... macro definitions like __cplusplus ...
+  //     <command-line>
+  //       ... macro definitions for args like -Dfoo=bar ...
+  //   "header1.h"
+  //     ... header file contents ...
+  //   "header2.h"
+  //     ... header file contents ...
+  //   ... main file contents ...
+  //
+  // When using a preamble, the "header1" and "header2" subtrees get skipped.
+  // We insert them right after the built-in header, which still appears.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+                   SrcMgr::CharacteristicKind Kind, FileID PrevFID) override {
+    // It'd be nice if there was a better way to identify built-in headers...
+    if (Reason == FileChangeReason::ExitFile &&
+        SM.getBuffer(PrevFID)->getBufferIdentifier() == "<built-in>")
+      replay();
+  }
+
+  void replay() {
+    for (const auto &Inc : Includes.MainFileIncludes) {
+      const FileEntry *File = nullptr;
+      if (Inc.Resolved != "")
+        File = SM.getFileManager().getFile(Inc.Resolved);
+
+      StringRef WrittenFilename =
+          StringRef(Inc.Written).drop_front().drop_back();
+      bool Angled = StringRef(Inc.Written).startswith("<");
+
+      // Re-lex the #include directive to find its interesting parts.
+      StringRef Src = SM.getBufferData(SM.getMainFileID());
+      Lexer RawLexer(SM.getLocForStartOfFile(SM.getMainFileID()), LangOpts,
+                     Src.begin(), Src.begin() + Inc.HashOffset, Src.end());
+      Token HashTok, IncludeTok, FilenameTok;
+      RawLexer.LexFromRawLexer(HashTok);
+      assert(HashTok.getKind() == tok::hash);
+      RawLexer.setParsingPreprocessorDirective(true);
+      RawLexer.LexFromRawLexer(IncludeTok);
+      IdentifierInfo *II = PP.getIdentifierInfo(IncludeTok.getRawIdentifier());
+      IncludeTok.setIdentifierInfo(II);
+      IncludeTok.setKind(II->getTokenID());
+      RawLexer.LexIncludeFilename(FilenameTok);
+
+      Delegate->InclusionDirective(
+          HashTok.getLocation(), IncludeTok, WrittenFilename, Angled,
+          CharSourceRange::getCharRange(FilenameTok.getLocation(),
+                                        FilenameTok.getEndLoc()),
+          File, "SearchPath", "RelPath", /*Imported=*/nullptr, Inc.FileKind);
+      if (File)
+        Delegate->FileSkipped(*File, FilenameTok, Inc.FileKind);
+      else {
+        SmallString<1> UnusedRecovery;
+        Delegate->FileNotFound(WrittenFilename, UnusedRecovery);
+      }
+    }
+  }
+
+  const IncludeStructure &Includes;
+  PPCallbacks *Delegate;
+  const SourceManager &SM;
+  Preprocessor &PP;
+  const LangOptions &LangOpts;
+};
+
 } // namespace
 
 void dumpAST(ParsedAST &AST, raw_ostream &OS) {
   AST.getASTContext().getTranslationUnitDecl()->dump(OS, true);
 }
 
 Optional<ParsedAST>
 ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
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";
+    CTOpts.Checks = "bugprone-sizeof-expression,"
+                    "bugprone-macro-repeated-side-effects,"
+                    "modernize-deprecated-headers";
     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{};
+  // Replay the preamble includes so that clang-tidy checks can see them.
+  if (Preamble)
+    ReplayPreamble::attach(Includes, *Clang);
+  // Important: collectIncludeStructure is registered *after* ReplayPreamble!
+  // Otherwise we would collect the replayed includes again...
+  // (We can't *just* use the replayed includes, they don't have Resolved path).
   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-tools-extra/trunk/clangd/Diagnostics.cpp

std::string noteMessage(const Diag &Main, const DiagBase &Note) {
   OS << "\n\n";
   printDiag(OS, Main);
   OS.flush();
   return capitalize(std::move(Result));
 }
 } // namespace
 
 raw_ostream &operator<<(raw_ostream &OS, const DiagBase &D) {
+  OS << "[";
   if (!D.InsideMainFile)
-    OS << "[in " << D.File << "] ";
+    OS << D.File << ":";
+  OS << D.Range.start << "-" << D.Range.end << "] ";
+
   return OS << D.Message;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const Fix &F) {
   OS << F.Message << " {";
   const char *Sep = "";
   for (const auto &Edit : F.Edits) {
     OS << Sep << Edit;

clang-tools-extra/trunk/clangd/Headers.h

struct HeaderFile {
   bool valid() const;
 };
 
 // An #include directive that we found in the main file.
 struct Inclusion {
   Range R;             // Inclusion range.
   std::string Written; // Inclusion name as written e.g. <vector>.
   Path Resolved;       // Resolved path of included file. Empty if not resolved.
+  unsigned HashOffset = 0; // Byte offset from start of file to #.
+  SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
 };
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion&);
 
 // Information captured about the inclusion graph in a translation unit.
 // This includes detailed information about the direct #includes, and summary
 // information about all transitive includes.
 //
 // It should be built incrementally with collectIncludeStructureCallback().
 // When we build the preamble, we capture and store its include structure along
 // with the preamble data. When we use the preamble, we can copy its

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

public:
 
   // Record existing #includes - both written and resolved paths. Only #includes
   // in the main file are collected.
   void InclusionDirective(SourceLocation HashLoc, const Token & /*IncludeTok*/,
                           StringRef FileName, bool IsAngled,
                           CharSourceRange FilenameRange, const FileEntry *File,
                           StringRef /*SearchPath*/, StringRef /*RelativePath*/,
                           const Module * /*Imported*/,
-                          SrcMgr::CharacteristicKind /*FileType*/) override {
-    if (SM.isInMainFile(HashLoc))
-      Out->MainFileIncludes.push_back({
-          halfOpenToRange(SM, FilenameRange),
-          (IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str(),
-          File ? File->tryGetRealPathName() : "",
-      });
+                          SrcMgr::CharacteristicKind FileKind) override {
+    if (SM.isWrittenInMainFile(HashLoc)) {
+      Out->MainFileIncludes.emplace_back();
+      auto &Inc = Out->MainFileIncludes.back();
+      Inc.R = halfOpenToRange(SM, FilenameRange);
+      Inc.Written =
+          (IsAngled ? "<" + FileName + ">" : "\"" + FileName + "\"").str();
+      Inc.Resolved = File ? File->tryGetRealPathName() : "";
+      Inc.HashOffset = SM.getFileOffset(HashLoc);
+      Inc.FileKind = FileKind;
+    }
     if (File) {
       auto *IncludingFileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc));
       if (!IncludingFileEntry) {
         assert(SM.getBufferName(HashLoc).startswith("<") &&
                "Expected #include location to be a file or <built-in>");
         // Treat as if included from the main file.
         IncludingFileEntry = SM.getFileEntryForID(SM.getMainFileID());
       }

 Optional<TextEdit> IncludeInserter::insert(StringRef VerbatimHeader) const {
   Optional<TextEdit> Edit = None;
   if (auto Insertion = Inserter.insert(VerbatimHeader.trim("\"<>"),
                                        VerbatimHeader.startswith("<")))
     Edit = replacementToEdit(Code, *Insertion);
   return Edit;
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Inclusion &Inc) {
+  return OS << Inc.Written << " = "
+            << (Inc.Resolved.empty() ? Inc.Resolved : "[unresolved]") << " at "
+            << Inc.R;
+}
+
 } // namespace clangd
 } // namespace clang

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

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(
+    #include $deprecated[["assert.h"]]
+
     #define $macrodef[[SQUARE]](X) (X)*(X)
     int main() {
-      return [[sizeof]](sizeof(int));
+      return $doubled[[sizeof]](sizeof(int));
       int y = 4;
       return SQUARE($macroarg[[++]]y);
     }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
+  TU.HeaderFilename = "assert.h"; // Suppress "not found" error.
   EXPECT_THAT(
       TU.build().getDiagnostics(),
       UnorderedElementsAre(
-          Diag(Test.range(), "suspicious usage of 'sizeof(sizeof(...))' "
-                             "[bugprone-sizeof-expression]"),
+          AllOf(Diag(Test.range("deprecated"),
+                     "inclusion of deprecated C++ header 'assert.h'; consider "
+                     "using 'cassert' instead [modernize-deprecated-headers]"),
+                WithFix(Fix(Test.range("deprecated"), "<cassert>",
+                            "change '\"assert.h\"' to '<cassert>'"))),
+          Diag(Test.range("doubled"),
+               "suspicious usage of 'sizeof(sizeof(...))' "
           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]")))));
 }

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

 //===-- HeadersTests.cpp - Include headers unit tests -----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Headers.h"
 
 #include "Compiler.h"
 #include "TestFS.h"
+#include "TestTU.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
 class HeadersTest : public ::testing::Test {
 public:
   HeadersTest() {
     CDB.ExtraClangFlags = {SearchDirArg.c_str()};
     FS.Files[MainFile] = "";
     // Make sure directory sub/ exists.
protected:
   std::string MainFile = testPath("main.cpp");
   std::string Subdir = testPath("sub");
   std::string SearchDirArg = (Twine("-I") + Subdir).str();
   IgnoringDiagConsumer IgnoreDiags;
 };
 
 MATCHER_P(Written, Name, "") { return arg.Written == Name; }
 MATCHER_P(Resolved, Name, "") { return arg.Resolved == Name; }
+MATCHER_P(IncludeLine, N, "") { return arg.R.start.line == N; }
 
 MATCHER_P2(Distance, File, D, "") {
   if (arg.getKey() != File)
     *result_listener << "file =" << arg.getKey().str();
   if (arg.getValue() != D)
     *result_listener << "distance =" << arg.getValue();
   return arg.getKey() == File && arg.getValue() == D;
 }
EXPECT_THAT(collectIncludes().includeDepth(MainFile),
                                    Distance(testPath("sub/bar.h"), 1u),
                                    Distance(testPath("sub/baz.h"), 2u)));
   // includeDepth() also works for non-main files.
   EXPECT_THAT(collectIncludes().includeDepth(testPath("sub/bar.h")),
               UnorderedElementsAre(Distance(testPath("sub/bar.h"), 0u),
                                    Distance(testPath("sub/baz.h"), 1u)));
 }
 
+TEST_F(HeadersTest, PreambleIncludesPresentOnce) {
+  // We use TestTU here, to ensure we use the preamble replay logic.
+  // We're testing that the logic doesn't crash, and doesn't result in duplicate
+  // includes. (We'd test more directly, but it's pretty well encapsulated!)
+  auto TU = TestTU::withCode(R"cpp(
+    #include "a.h"
+    #include "a.h"
+    void foo();
+    #include "a.h"
+  )cpp");
+  TU.HeaderFilename = "a.h"; // suppress "not found".
+  EXPECT_THAT(TU.build().getIncludeStructure().MainFileIncludes,
+              ElementsAre(IncludeLine(1), IncludeLine(2), IncludeLine(4)));
+}
+
 TEST_F(HeadersTest, UnResolvedInclusion) {
   FS.Files[MainFile] = R"cpp(
 #include "foo.h"
 )cpp";
 
   EXPECT_THAT(collectIncludes().MainFileIncludes,
               UnorderedElementsAre(AllOf(Written("\"foo.h\""), Resolved(""))));
   EXPECT_THAT(collectIncludes().includeDepth(MainFile),
TEST_F(HeadersTest, PreferredHeader) {
   std::string BarHeader = testPath("sub/bar.h");
   EXPECT_EQ(calculate(BarHeader, "<bar>"), "<bar>");
 
   std::string BazHeader = testPath("sub/baz.h");
   EXPECT_EQ(calculate(BarHeader, BazHeader), "\"baz.h\"");
 }
 
 TEST_F(HeadersTest, DontInsertDuplicatePreferred) {
-  std::vector<Inclusion> Inclusions = {
-      {Range(), /*Written*/ "\"bar.h\"", /*Resolved*/ ""}};
-  EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", Inclusions), "");
-  EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", Inclusions), "");
+  Inclusion Inc;
+  Inc.Written = "\"bar.h\"";
+  Inc.Resolved = "";
+  EXPECT_EQ(calculate(testPath("sub/bar.h"), "\"bar.h\"", {Inc}), "");
+  EXPECT_EQ(calculate("\"x.h\"", "\"bar.h\"", {Inc}), "");
 }
 
 TEST_F(HeadersTest, DontInsertDuplicateResolved) {
-  std::string BarHeader = testPath("sub/bar.h");
-  std::vector<Inclusion> Inclusions = {
-      {Range(), /*Written*/ "fake-bar.h", /*Resolved*/ BarHeader}};
-  EXPECT_EQ(calculate(BarHeader, "", Inclusions), "");
+  Inclusion Inc;
+  Inc.Written = "fake-bar.h";
+  Inc.Resolved = testPath("sub/bar.h");
+  EXPECT_EQ(calculate(Inc.Resolved, "", {Inc}), "");
   // Do not insert preferred.
-  EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
+  EXPECT_EQ(calculate(Inc.Resolved, "\"BAR.h\"", {Inc}), "");
 }
 
 TEST_F(HeadersTest, PreferInserted) {
   auto Edit = insert("<y>");
   EXPECT_TRUE(Edit.hasValue());
   EXPECT_TRUE(StringRef(Edit->newText).contains("<y>"));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang