diff --git a/clang-tools-extra/clangd/Headers.h b/clang-tools-extra/clangd/Headers.h --- a/clang-tools-extra/clangd/Headers.h +++ b/clang-tools-extra/clangd/Headers.h @@ -60,6 +60,7 @@ SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User; }; llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion &); +bool operator==(const Inclusion &LHS, const Inclusion &RHS); // Contains information about one file in the build grpah and its direct // dependencies. Doesn't own the strings it references (IncludeGraph is diff --git a/clang-tools-extra/clangd/Headers.cpp b/clang-tools-extra/clangd/Headers.cpp --- a/clang-tools-extra/clangd/Headers.cpp +++ b/clang-tools-extra/clangd/Headers.cpp @@ -253,5 +253,12 @@ << Inc.R; } +bool operator==(const Inclusion &LHS, const Inclusion &RHS) { + return std::tie(LHS.Directive, LHS.FileKind, LHS.HashOffset, LHS.R, + LHS.Resolved, LHS.Written) == + std::tie(RHS.Directive, RHS.FileKind, RHS.HashOffset, RHS.R, + RHS.Resolved, RHS.Written); +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h --- a/clang-tools-extra/clangd/ParsedAST.h +++ b/clang-tools-extra/clangd/ParsedAST.h @@ -48,12 +48,12 @@ /// Attempts to run Clang and store parsed AST. If \p Preamble is non-null /// it is reused during parsing. static llvm::Optional - build(llvm::StringRef Version, std::unique_ptr CI, + build(const ParseInputs &Inputs, + std::unique_ptr CI, llvm::ArrayRef CompilerInvocationDiags, std::shared_ptr Preamble, std::unique_ptr Buffer, - llvm::IntrusiveRefCntPtr VFS, - const SymbolIndex *Index, const ParseOptions &Opts); + llvm::IntrusiveRefCntPtr VFS); ParsedAST(ParsedAST &&Other); ParsedAST &operator=(ParsedAST &&Other); diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -15,6 +15,7 @@ #include "Headers.h" #include "IncludeFixer.h" #include "Logger.h" +#include "Preamble.h" #include "SourceCode.h" #include "Trace.h" #include "index/CanonicalIncludes.h" @@ -48,6 +49,7 @@ #include "llvm/Support/raw_ostream.h" #include #include +#include // Force the linker to link in Clang-tidy modules. // clangd doesn't support the static analyzer. @@ -239,14 +241,15 @@ } llvm::Optional -ParsedAST::build(llvm::StringRef Version, +ParsedAST::build(const ParseInputs &Inputs, std::unique_ptr CI, llvm::ArrayRef CompilerInvocationDiags, std::shared_ptr Preamble, std::unique_ptr Buffer, - llvm::IntrusiveRefCntPtr VFS, - const SymbolIndex *Index, const ParseOptions &Opts) { + llvm::IntrusiveRefCntPtr VFS) { assert(CI); + const ParseOptions &Opts = Inputs.Opts; + const SymbolIndex *Index = Inputs.Index; // Command-line parsing sets DisableFree to true by default, but we don't want // to leak memory in clangd. CI->getFrontendOpts().DisableFree = false; @@ -262,6 +265,10 @@ std::string Filename = std::string(Buffer->getBufferIdentifier()); // Absolute. + PreamblePatch Patch = Preamble + ? PreamblePatch::create(Filename, Inputs, *Preamble) + : PreamblePatch{}; + Patch.apply(*CI); auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH, std::move(Buffer), VFS, ASTDiags); if (!Clang) @@ -360,12 +367,14 @@ Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); } - // 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) + IncludeStructure Includes; + // If we are using a preamble, copy existing includes. + if (Preamble) { + Includes = Preamble->Includes; + Patch.patchPreambleIncludes(Includes); + // Replay the preamble includes so that clang-tidy checks can see them. ReplayPreamble::attach(Includes, *Clang, Preamble->Preamble.getBounds()); + } // 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). @@ -431,7 +440,8 @@ std::vector D = ASTDiags.take(CTContext.getPointer()); Diags.insert(Diags.end(), D.begin(), D.end()); } - return ParsedAST(Version, std::move(Preamble), std::move(Clang), + + return ParsedAST(Inputs.Version, std::move(Preamble), std::move(Clang), std::move(Action), std::move(Tokens), std::move(Macros), std::move(ParsedDecls), std::move(Diags), std::move(Includes), std::move(CanonIncludes)); @@ -551,9 +561,9 @@ } return ParsedAST::build( - Inputs.Version, std::move(Invocation), CompilerInvocationDiags, Preamble, + Inputs, std::move(Invocation), CompilerInvocationDiags, Preamble, llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, FileName), - std::move(VFS), Inputs.Index, Inputs.Opts); + std::move(VFS)); } } // namespace clangd diff --git a/clang-tools-extra/clangd/Preamble.h b/clang-tools-extra/clangd/Preamble.h --- a/clang-tools-extra/clangd/Preamble.h +++ b/clang-tools-extra/clangd/Preamble.h @@ -95,7 +95,7 @@ /// the preamble section of Modified contents, e.g. new include directives. class PreamblePatch { public: - // With an empty patch, the preamble is used verbatim. + /// With an empty patch, the preamble is used verbatim. PreamblePatch() = default; /// Builds a patch that contains new PP directives introduced to the preamble /// section of \p Modified compared to \p Baseline. @@ -109,9 +109,17 @@ /// \p CI that contains new directives calculated in create. void apply(CompilerInvocation &CI) const; + /// Adjusts \p BaselineIncludes to reflect state of \p Modified. This won't + /// add any new includes but rather drop the deleted ones. Note that it won't + /// mutate include depths. + void patchPreambleIncludes(IncludeStructure &BaselineIncludes) const; + private: std::string PatchContents; std::string PatchFileName; + /// Includes that are present in both \p Baseline and \p Modified. Used for + /// patching includes of baseline preamble. + std::vector PreambleIncludes; }; } // namespace clangd diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -10,6 +10,7 @@ #include "Compiler.h" #include "Headers.h" #include "Logger.h" +#include "Path.h" #include "SourceCode.h" #include "Trace.h" #include "clang/Basic/Diagnostic.h" @@ -24,6 +25,8 @@ #include "clang/Lex/PreprocessorOptions.h" #include "clang/Tooling/CompilationDatabase.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" @@ -274,6 +277,7 @@ PreamblePatch PreamblePatch::create(llvm::StringRef FileName, const ParseInputs &Modified, const PreambleData &Baseline) { + assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!"); // First scan the include directives in Baseline and Modified. These will be // used to figure out newly added directives in Modified. Scanning can fail, // the code just bails out and creates an empty patch in such cases, as: @@ -301,6 +305,9 @@ } PreamblePatch PP; + // No patch needed if includes are equal. + if (*BaselineIncludes == *ModifiedIncludes) + return PP; // This shouldn't coincide with any real file name. llvm::SmallString<128> PatchName; llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName), @@ -309,19 +316,26 @@ // We are only interested in newly added includes, record the ones in Baseline // for exclusion. - llvm::DenseSet> + llvm::DenseMap, + /*Resolved=*/llvm::StringRef> ExistingIncludes; + for (const auto &Inc : Baseline.Includes.MainFileIncludes) + ExistingIncludes[{Inc.Directive, Inc.Written}] = Inc.Resolved; + // There might be includes coming from disabled regions, record these for + // exclusion too. note that we don't have resolved paths for those. for (const auto &Inc : *BaselineIncludes) - ExistingIncludes.insert({Inc.Directive, Inc.Written}); + ExistingIncludes.try_emplace({Inc.Directive, Inc.Written}); // Calculate extra includes that needs to be inserted. llvm::raw_string_ostream Patch(PP.PatchContents); - bool EmittedFileName = false; - for (const auto &Inc : *ModifiedIncludes) { - if (ExistingIncludes.count({Inc.Directive, Inc.Written})) + Patch << llvm::formatv("#line 0 \"{0}\"\n", FileName); + for (auto &Inc : *ModifiedIncludes) { + auto It = ExistingIncludes.find({Inc.Directive, Inc.Written}); + // Include already present in the baseline preamble. Set resolved path and + // put into preamble includes. + if (It != ExistingIncludes.end()) { + Inc.Resolved = It->second.str(); + PP.PreambleIncludes.push_back(Inc); continue; - if (!EmittedFileName) { - EmittedFileName = true; - Patch << llvm::formatv("#line 0 \"{0}\"\n", FileName); } // FIXME: Traverse once auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset); @@ -354,5 +368,12 @@ PPOpts.Includes.push_back(PatchFileName); } +void PreamblePatch::patchPreambleIncludes( + IncludeStructure &BaselineIncludes) const { + if (PatchContents.empty()) + return; + BaselineIncludes.MainFileIncludes = PreambleIncludes; +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp b/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp --- a/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp +++ b/clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp @@ -666,7 +666,8 @@ } TEST_F(DocumentSymbolsTest, TempSpecs) { - addFile("foo.cpp", R"cpp( + std::string FilePath = testPath("foo.cpp"); + addFile(FilePath, R"cpp( template class Foo {}; template class Foo {}; template <> class Foo {}; @@ -674,7 +675,7 @@ )cpp"); // Foo is higher ranked because of exact name match. EXPECT_THAT( - getSymbols("foo.cpp"), + getSymbols(FilePath), UnorderedElementsAre( AllOf(WithName("Foo"), WithKind(SymbolKind::Class)), AllOf(WithName("Foo"), WithKind(SymbolKind::Class)), @@ -683,7 +684,8 @@ } TEST_F(DocumentSymbolsTest, Qualifiers) { - addFile("foo.cpp", R"cpp( + std::string FilePath = testPath("foo.cpp"); + addFile(FilePath, R"cpp( namespace foo { namespace bar { struct Cls; @@ -706,7 +708,7 @@ )cpp"); // All the qualifiers should be preserved exactly as written. - EXPECT_THAT(getSymbols("foo.cpp"), + EXPECT_THAT(getSymbols(FilePath), UnorderedElementsAre( WithName("foo"), WithName("foo::bar::Cls"), WithName("foo::bar::func1"), WithName("::foo::bar::func2"), @@ -715,7 +717,8 @@ } TEST_F(DocumentSymbolsTest, QualifiersWithTemplateArgs) { - addFile("foo.cpp", R"cpp( + std::string FilePath = testPath("foo.cpp"); + addFile(FilePath, R"cpp( template class Foo; template <> @@ -738,7 +741,7 @@ int Foo_type::method3() { return 30; } )cpp"); EXPECT_THAT( - getSymbols("foo.cpp"), + getSymbols(FilePath), UnorderedElementsAre(WithName("Foo"), WithName("Foo"), WithName("int_type"), WithName("Foo::method1"), diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp --- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp +++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp @@ -17,7 +17,9 @@ #include "Annotations.h" #include "Compiler.h" #include "Diagnostics.h" +#include "Headers.h" #include "ParsedAST.h" +#include "Preamble.h" #include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" @@ -28,6 +30,7 @@ #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Token.h" #include "clang/Tooling/Syntax/Tokens.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/ScopedPrinter.h" #include "gmock/gmock-matchers.h" @@ -82,6 +85,13 @@ return arg.beginOffset() == R.Begin && arg.endOffset() == R.End; } +MATCHER(EqInc, "") { + Inclusion Actual = testing::get<0>(arg); + Inclusion Expected = testing::get<1>(arg); + return std::tie(Actual.HashOffset, Actual.R, Actual.Written) == + std::tie(Expected.HashOffset, Expected.R, Expected.Written); +} + TEST(ParsedASTTest, TopLevelDecls) { TestTU TU; TU.HeaderCode = R"( @@ -420,6 +430,105 @@ } } +TEST(ParsedASTTest, PatchesAdditionalIncludes) { + llvm::StringLiteral ModifiedContents = R"cpp( + #include "baz.h" + #include "foo.h" + #include "sub/aux.h" + void bar() { + foo(); + baz(); + aux(); + })cpp"; + // Build expected ast with symbols coming from headers. + TestTU TU; + TU.Filename = "foo.cpp"; + TU.AdditionalFiles["foo.h"] = "void foo();"; + TU.AdditionalFiles["sub/baz.h"] = "void baz();"; + TU.AdditionalFiles["sub/aux.h"] = "void aux();"; + TU.ExtraArgs = {"-I" + testPath("sub")}; + TU.Code = ModifiedContents.str(); + auto ExpectedAST = TU.build(); + + // Build preamble with no includes. + TU.Code = ""; + StoreDiags Diags; + auto Inputs = TU.inputs(); + auto CI = buildCompilerInvocation(Inputs, Diags); + auto EmptyPreamble = + buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr); + ASSERT_TRUE(EmptyPreamble); + EXPECT_THAT(EmptyPreamble->Includes.MainFileIncludes, testing::IsEmpty()); + + // Now build an AST using empty preamble and ensure patched includes worked. + TU.Code = ModifiedContents.str(); + Inputs = TU.inputs(); + auto PatchedAST = + buildAST(testPath("foo.cpp"), std::move(CI), {}, Inputs, EmptyPreamble); + ASSERT_TRUE(PatchedAST); + ASSERT_TRUE(PatchedAST->getDiagnostics().empty()); + + // Ensure source location information is correct, including resolved paths. + EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes, + testing::Pointwise( + EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes)); + auto StringMapToVector = [](const llvm::StringMap SM) { + std::vector> Res; + for (const auto &E : SM) + Res.push_back({E.first().str(), E.second}); + llvm::sort(Res); + return Res; + }; + // Ensure file proximity signals are correct. + EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth( + testPath("foo.cpp"))), + StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth( + testPath("foo.cpp")))); +} + +TEST(ParsedASTTest, PatchesDeletedIncludes) { + TestTU TU; + TU.Filename = "foo.cpp"; + TU.Code = ""; + auto ExpectedAST = TU.build(); + + // Build preamble with no includes. + TU.Code = R"cpp(#include )cpp"; + StoreDiags Diags; + auto Inputs = TU.inputs(); + auto CI = buildCompilerInvocation(Inputs, Diags); + auto BaselinePreamble = + buildPreamble(testPath("foo.cpp"), *CI, Inputs, true, nullptr); + ASSERT_TRUE(BaselinePreamble); + EXPECT_THAT(BaselinePreamble->Includes.MainFileIncludes, + ElementsAre(testing::Field(&Inclusion::Written, ""))); + + // Now build an AST using additional includes and check that locations are + // correctly parsed. + TU.Code = ""; + Inputs = TU.inputs(); + auto PatchedAST = buildAST(testPath("foo.cpp"), std::move(CI), {}, Inputs, + BaselinePreamble); + ASSERT_TRUE(PatchedAST); + + // Ensure source location information is correct. + EXPECT_THAT(PatchedAST->getIncludeStructure().MainFileIncludes, + testing::Pointwise( + EqInc(), ExpectedAST.getIncludeStructure().MainFileIncludes)); + auto StringMapToVector = [](const llvm::StringMap SM) { + std::vector> Res; + for (const auto &E : SM) + Res.push_back({E.first().str(), E.second}); + llvm::sort(Res); + return Res; + }; + // Ensure file proximity signals are correct. + EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth( + testPath("foo.cpp"))), + StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth( + testPath("foo.cpp")))); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/PreambleTests.cpp b/clang-tools-extra/clangd/unittests/PreambleTests.cpp --- a/clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ b/clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -8,6 +8,7 @@ #include "Annotations.h" #include "Compiler.h" +#include "Headers.h" #include "Preamble.h" #include "TestFS.h" #include "TestTU.h" @@ -21,6 +22,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include +#include #include #include @@ -30,49 +32,53 @@ namespace clangd { namespace { +MATCHER_P2(Distance, File, D, "") { + return arg.first() == File && arg.second == D; +} + +std::shared_ptr +createPreamble(llvm::StringRef Contents = "") { + auto TU = TestTU::withCode(Contents); + // ms-compatibility changes meaning of #import, make sure it is turned off. + TU.ExtraArgs = {"-fno-ms-compatibility"}; + TU.Filename = "preamble.cpp"; + auto PI = TU.inputs(); + IgnoreDiagnostics Diags; + auto CI = buildCompilerInvocation(PI, Diags); + if (!CI) { + ADD_FAILURE() << "failed to build compiler invocation"; + return nullptr; + } + if (auto Preamble = buildPreamble(TU.Filename, *CI, PI, true, nullptr)) + return Preamble; + ADD_FAILURE() << "failed to build preamble"; + return nullptr; +} + // Builds a preamble for BaselineContents, patches it for ModifiedContents and // returns the includes in the patch. IncludeStructure collectPatchedIncludes(llvm::StringRef ModifiedContents, llvm::StringRef BaselineContents) { - std::string MainFile = testPath("main.cpp"); - ParseInputs PI; - PI.FS = new llvm::vfs::InMemoryFileSystem; - MockCompilationDatabase CDB; - // ms-compatibility changes meaning of #import, make sure it is turned off. - CDB.ExtraClangFlags.push_back("-fno-ms-compatibility"); - PI.CompileCommand = CDB.getCompileCommand(MainFile).getValue(); - // Create invocation - IgnoreDiagnostics Diags; - auto CI = buildCompilerInvocation(PI, Diags); - assert(CI && "failed to create compiler invocation"); - // Build baseline preamble. - PI.Contents = BaselineContents.str(); - PI.Version = "baseline preamble"; - auto BaselinePreamble = buildPreamble(MainFile, *CI, PI, true, nullptr); - assert(BaselinePreamble && "failed to build baseline preamble"); + auto BaselinePreamble = createPreamble(BaselineContents); // Create the patch. - PI.Contents = ModifiedContents.str(); - PI.Version = "modified contents"; - auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble); + auto TU = TestTU::withCode(ModifiedContents); + auto PI = TU.inputs(); + auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble); // Collect patch contents. + IgnoreDiagnostics Diags; + auto CI = buildCompilerInvocation(PI, Diags); PP.apply(*CI); - llvm::StringRef PatchContents; - for (const auto &Rempaped : CI->getPreprocessorOpts().RemappedFileBuffers) { - if (Rempaped.first == testPath("__preamble_patch__.h")) { - PatchContents = Rempaped.second->getBuffer(); - break; - } - } - // Run preprocessor over the modified contents with patched Invocation to and - // BaselinePreamble to collect includes in the patch. We trim the input to - // only preamble section to not collect includes in the mainfile. + // Run preprocessor over the modified contents with patched Invocation. We + // provide a preamble and trim contents to ensure only the implicit header + // introduced by the patch is parsed and nothing else. + // We don't run PP directly over the patch cotents to test production + // behaviour. auto Bounds = Lexer::ComputePreamble(ModifiedContents, *CI->getLangOpts()); auto Clang = prepareCompilerInstance(std::move(CI), &BaselinePreamble->Preamble, llvm::MemoryBuffer::getMemBufferCopy( ModifiedContents.slice(0, Bounds.Size).str()), PI.FS, Diags); - Clang->getPreprocessorOpts().ImplicitPCHInclude.clear(); PreprocessOnlyAction Action; if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) { ADD_FAILURE() << "failed begin source file"; @@ -150,6 +156,38 @@ Field(&Inclusion::Written, ""))); } +TEST(PreamblePatchTest, PatchesPreambleIncludes) { + IgnoreDiagnostics Diags; + auto TU = TestTU::withCode(R"cpp( + #include "a.h" + #include "c.h" + )cpp"); + TU.AdditionalFiles["a.h"] = "#include \"b.h\""; + TU.AdditionalFiles["b.h"] = ""; + TU.AdditionalFiles["c.h"] = ""; + auto PI = TU.inputs(); + auto BaselinePreamble = buildPreamble( + TU.Filename, *buildCompilerInvocation(PI, Diags), PI, true, nullptr); + IncludeStructure PreambleIncludes = BaselinePreamble->Includes; + // We drop c.h from modified and add a new header. Since the latter is patched + // we should only get a.h in preamble includes. + TU.Code = R"cpp( + #include "a.h" + #include "b.h" + )cpp"; + PreamblePatch::create(testPath(TU.Filename), TU.inputs(), *BaselinePreamble) + .patchPreambleIncludes(PreambleIncludes); + // MainFileIncludes should be up-to-date. + EXPECT_THAT(PreambleIncludes.MainFileIncludes, + ElementsAre(AllOf(Field(&Inclusion::Written, "\"a.h\""), + Field(&Inclusion::Resolved, testPath("a.h"))))); + // Include depth should be stale though. + EXPECT_THAT(PreambleIncludes.includeDepth(testPath(TU.Filename)), + testing::UnorderedElementsAre(Distance(testPath(TU.Filename), 0u), + Distance(testPath("a.h"), 1u), + Distance(testPath("b.h"), 2u), + Distance(testPath("c.h"), 1u))); +} } // namespace } // namespace clangd } // namespace clang