Index: clangd/ClangdUnit.cpp =================================================================== --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -119,6 +119,105 @@ 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( + 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 + // + // ... macro definitions like __cplusplus ... + // + // ... 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() == "") + 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 @@ // 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::ClangTidyGlobalOptions(), CTOpts)); CTContext->setDiagnosticsEngine(&Clang->getDiagnostics()); @@ -190,6 +290,12 @@ // 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)); Index: clangd/Diagnostics.cpp =================================================================== --- clangd/Diagnostics.cpp +++ clangd/Diagnostics.cpp @@ -190,8 +190,11 @@ } // 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; } Index: clangd/Headers.h =================================================================== --- clangd/Headers.h +++ clangd/Headers.h @@ -43,7 +43,10 @@ Range R; // Inclusion range. std::string Written; // Inclusion name as written e.g. . 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 Index: clangd/Headers.cpp =================================================================== --- clangd/Headers.cpp +++ clangd/Headers.cpp @@ -34,13 +34,17 @@ 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 @@ 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 Index: unittests/clangd/ClangdUnitTests.cpp =================================================================== --- unittests/clangd/ClangdUnitTests.cpp +++ unittests/clangd/ClangdUnitTests.cpp @@ -131,19 +131,28 @@ 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"), "", + "change '\"assert.h\"' to ''"))), + 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 " Index: unittests/clangd/HeadersTests.cpp =================================================================== --- unittests/clangd/HeadersTests.cpp +++ unittests/clangd/HeadersTests.cpp @@ -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 { using ::testing::AllOf; +using ::testing::ElementsAre; using ::testing::UnorderedElementsAre; class HeadersTest : public ::testing::Test { @@ -132,6 +134,7 @@ 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 @@ 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, DontInsertDuplicatePreferred) { - std::vector 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 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) {