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

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 20 02:58:49 PST 2018


Author: sammccall
Date: Tue Nov 20 02:58:48 2018
New Revision: 347298

URL: http://llvm.org/viewvc/llvm-project?rev=347298&view=rev
Log:
[clangd] Replay preamble #includes to clang-tidy checks.

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.

Reviewers: ilya-biryukov

Subscribers: ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D54694

Modified:
    clang-tools-extra/trunk/clangd/ClangdUnit.cpp
    clang-tools-extra/trunk/clangd/Diagnostics.cpp
    clang-tools-extra/trunk/clangd/Headers.cpp
    clang-tools-extra/trunk/clangd/Headers.h
    clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp
    clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=347298&r1=347297&r2=347298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Tue Nov 20 02:58:48 2018
@@ -119,6 +119,105 @@ private:
   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) {
@@ -171,8 +270,9 @@ ParsedAST::build(std::unique_ptr<Compile
     // 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());
@@ -190,6 +290,12 @@ ParsedAST::build(std::unique_ptr<Compile
   // 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));
 

Modified: clang-tools-extra/trunk/clangd/Diagnostics.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Diagnostics.cpp?rev=347298&r1=347297&r2=347298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Diagnostics.cpp (original)
+++ clang-tools-extra/trunk/clangd/Diagnostics.cpp Tue Nov 20 02:58:48 2018
@@ -190,8 +190,11 @@ std::string noteMessage(const Diag &Main
 } // 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;
 }
 

Modified: clang-tools-extra/trunk/clangd/Headers.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Headers.cpp?rev=347298&r1=347297&r2=347298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Headers.cpp (original)
+++ clang-tools-extra/trunk/clangd/Headers.cpp Tue Nov 20 02:58:48 2018
@@ -34,13 +34,17 @@ public:
                           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) {
@@ -168,5 +172,11 @@ Optional<TextEdit> IncludeInserter::inse
   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

Modified: clang-tools-extra/trunk/clangd/Headers.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Headers.h?rev=347298&r1=347297&r2=347298&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Headers.h (original)
+++ clang-tools-extra/trunk/clangd/Headers.h Tue Nov 20 02:58:48 2018
@@ -43,7 +43,10 @@ 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

Modified: clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp?rev=347298&r1=347297&r2=347298&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp Tue Nov 20 02:58:48 2018
@@ -131,19 +131,28 @@ TEST(DiagnosticsTest, FlagsMatter) {
 
 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(...))' "
+               "[bugprone-sizeof-expression]"),
           AllOf(
               Diag(Test.range("macroarg"),
                    "side effects in the 1st macro argument 'X' are repeated in "

Modified: clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp?rev=347298&r1=347297&r2=347298&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp Tue Nov 20 02:58:48 2018
@@ -11,6 +11,7 @@
 
 #include "Compiler.h"
 #include "TestFS.h"
+#include "TestTU.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
@@ -24,6 +25,7 @@ namespace clangd {
 namespace {
 
 using ::testing::AllOf;
+using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 
 class HeadersTest : public ::testing::Test {
@@ -132,6 +134,7 @@ protected:
 
 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)
@@ -179,6 +182,21 @@ TEST_F(HeadersTest, OnlyCollectInclusion
                                    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"
@@ -220,19 +238,20 @@ TEST_F(HeadersTest, PreferredHeader) {
 }
 
 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) {




More information about the cfe-commits mailing list