diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -1031,7 +1031,7 @@ PathRef FileName; const tooling::CompileCommand &Command; const PreambleData &Preamble; - const PreamblePatch &Patch; + llvm::Optional Patch; llvm::StringRef Contents; size_t Offset; llvm::IntrusiveRefCntPtr VFS; @@ -1105,7 +1105,8 @@ PreambleBounds PreambleRegion = ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0); bool CompletingInPreamble = PreambleRegion.Size > Input.Offset; - Input.Patch.apply(*CI); + if (Input.Patch) + Input.Patch->apply(*CI); // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise // the remapped buffers do not get freed. auto Clang = prepareCompilerInstance( @@ -1767,7 +1768,8 @@ : std::move(Flow).run({FileName, Command, *Preamble, // We want to serve code completions with // low latency, so don't bother patching. - PreamblePatch(), Contents, *Offset, VFS}); + /*PreamblePatch=*/llvm::None, Contents, + *Offset, VFS}); } SignatureHelp signatureHelp(PathRef FileName, @@ -1792,10 +1794,11 @@ PI.CompileCommand = Command; PI.Contents = Contents.str(); PI.FS = std::move(VFS); - auto PP = PreamblePatch::create(FileName, PI, Preamble); semaCodeComplete( std::make_unique(Options, Index, Result), Options, - {FileName, Command, Preamble, PP, Contents, *Offset, std::move(PI.FS)}); + {FileName, Command, Preamble, + PreamblePatch::create(FileName, PI, Preamble), Contents, *Offset, + std::move(PI.FS)}); return Result; } 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 @@ -14,6 +14,7 @@ #include "Diagnostics.h" #include "Headers.h" #include "IncludeFixer.h" +#include "Preamble.h" #include "SourceCode.h" #include "index/CanonicalIncludes.h" #include "index/Index.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. @@ -268,6 +270,11 @@ StoreDiags ASTDiags; + llvm::Optional Patch; + if (Preamble) { + Patch = PreamblePatch::create(Filename, Inputs, *Preamble); + Patch->apply(*CI); + } auto Clang = prepareCompilerInstance( std::move(CI), PreamblePCH, llvm::MemoryBuffer::getMemBufferCopy(Inputs.Contents, Filename), VFS, @@ -369,12 +376,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; + Includes.MainFileIncludes = Patch->preambleIncludes(); + // 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). 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 @@ -91,12 +91,14 @@ const CompilerInvocation &CI); /// Stores information required to parse a TU using a (possibly stale) Baseline -/// preamble. Updates compiler invocation to approximately reflect additions to -/// the preamble section of Modified contents, e.g. new include directives. +/// preamble. Later on this information can be injected into the main file by +/// updating compiler invocation with \c apply. This injected section +/// approximately reflects additions to the preamble in Modified contents, e.g. +/// new include directives. class PreamblePatch { public: - // With an empty patch, the preamble is used verbatim. - PreamblePatch() = default; + /// \p Preamble is used verbatim. + static PreamblePatch unmodified(const PreambleData &Preamble); /// Builds a patch that contains new PP directives introduced to the preamble /// section of \p Modified compared to \p Baseline. /// FIXME: This only handles include directives, we should at least handle @@ -109,9 +111,21 @@ /// \p CI that contains new directives calculated in create. void apply(CompilerInvocation &CI) const; + /// Returns #include directives from the \c Modified preamble that were + /// resolved using the \c Baseline preamble. This covers the new locations of + /// inclusions that were moved around, but not inclusions of new files. Those + /// will be recorded when parsing the main file: the includes in the injected + /// section will be resolved back to their spelled positions in the main file + /// using the presumed-location mechanism. + std::vector preambleIncludes() const; + private: + PreamblePatch() = default; 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 @@ -24,6 +24,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 +276,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,7 +304,7 @@ } // No patch needed if includes are equal. if (*BaselineIncludes == *ModifiedIncludes) - return {}; + return PreamblePatch::unmodified(Baseline); PreamblePatch PP; // This shouldn't coincide with any real file name. @@ -312,10 +315,15 @@ // 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); // Set default filename for subsequent #line directives @@ -324,9 +332,15 @@ // might lead to problems on windows especially. escapeBackslashAndQuotes(FileName, Patch); Patch << "\"\n"; - for (const auto &Inc : *ModifiedIncludes) { - if (ExistingIncludes.count({Inc.Directive, Inc.Written})) + 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; + } // Include is new in the modified preamble. Inject it into the patch and use // #line to set the presumed location to where it is spelled. auto LineCol = offsetToClangLineColumn(Modified.Contents, Inc.HashOffset); @@ -356,5 +370,15 @@ PPOpts.Includes.push_back(PatchFileName); } +std::vector PreamblePatch::preambleIncludes() const { + return PreambleIncludes; +} + +PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) { + PreamblePatch PP; + PP.PreambleIncludes = Preamble.Includes.MainFileIncludes; + return PP; +} + } // 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.HashLine, Actual.Written) == + std::tie(Expected.HashLine, Expected.Written); +} + TEST(ParsedASTTest, TopLevelDecls) { TestTU TU; TU.HeaderCode = R"( @@ -431,6 +441,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 = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI), + {}, 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 = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI), + {}, 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,51 +32,58 @@ 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, llvm::StringRef MainFileName = "main.cpp") { - std::string MainFile = testPath(MainFileName); - ParseInputs PI; - PI.FS = new llvm::vfs::InMemoryFileSystem; - MockCompilationDatabase CDB; + auto BaselinePreamble = createPreamble(BaselineContents); + // Create the patch. + auto TU = TestTU::withCode(ModifiedContents); + TU.Filename = MainFileName.str(); // 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 + TU.ExtraArgs = {"-fno-ms-compatibility"}; + auto PI = TU.inputs(); + auto PP = PreamblePatch::create(testPath(TU.Filename), PI, *BaselinePreamble); + // Collect patch contents. 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"); - // Create the patch. - PI.Contents = ModifiedContents.str(); - PI.Version = "modified contents"; - auto PP = PreamblePatch::create(MainFile, PI, *BaselinePreamble); - // Collect patch contents. 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"; @@ -163,6 +172,33 @@ EXPECT_THAT(Includes, ElementsAre(AllOf(Field(&Inclusion::Written, ""), Field(&Inclusion::HashLine, 0)))); } + +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); + // 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"; + auto PP = PreamblePatch::create(testPath(TU.Filename), TU.inputs(), + *BaselinePreamble); + // Only a.h should exists in the preamble, as c.h has been dropped and b.h was + // newly introduced. + EXPECT_THAT(PP.preambleIncludes(), + ElementsAre(AllOf(Field(&Inclusion::Written, "\"a.h\""), + Field(&Inclusion::Resolved, testPath("a.h"))))); +} } // namespace } // namespace clangd } // namespace clang