diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -280,11 +280,8 @@ Pos, FS, Index)); }; - // Unlike code completion, we wait for an up-to-date preamble here. - // Signature help is often triggered after code completion. If the code - // completion inserted a header to make the symbol available, then using - // the old preamble would yield useless results. - WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Consistent, + // Unlike code completion, we wait for a preamble here. + WorkScheduler.runWithPreamble("SignatureHelp", File, TUScheduler::Stale, std::move(Action)); } 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 @@ -1048,9 +1048,13 @@ // Invokes Sema code completion on a file. // If \p Includes is set, it will be updated based on the compiler invocation. +// If \p PatchAdditionalIncludes is set, calculates newly introduced includes to +// current file contents since \p Input.Preamble was built and parses those +// includes as part of the mainfile. bool semaCodeComplete(std::unique_ptr Consumer, const clang::CodeCompleteOptions &Options, const SemaCompleteInput &Input, + bool PatchAdditionalIncludes, IncludeStructure *Includes = nullptr) { trace::Span Tracer("Sema completion"); llvm::IntrusiveRefCntPtr VFS = Input.VFS; @@ -1068,6 +1072,16 @@ elog("Couldn't create CompilerInvocation"); return false; } + if (PatchAdditionalIncludes) { + for (const auto &Inc : newIncludes( + Input.Preamble.LexedIncludes, + getPreambleIncludes(ParseInput.Contents, *CI->getLangOpts()))) { + // Inc.Written contains quotes or angles, preprocessor requires them to be + // stripped. + CI->getPreprocessorOpts().Includes.emplace_back( + llvm::StringRef(Inc.Written).drop_back().drop_front()); + } + } auto &FrontendOpts = CI->getFrontendOpts(); FrontendOpts.SkipFunctionBodies = true; // Disable typo correction in Sema. @@ -1312,8 +1326,10 @@ Recorder = RecorderOwner.get(); + // Note that we don't patch in case of a stale preamble to improve latency, + // while giving up on correctness. semaCodeComplete(std::move(RecorderOwner), Opts.getClangCompleteOpts(), - SemaCCInput, &Includes); + SemaCCInput, /*PatchAdditionalIncludes=*/false, &Includes); logResults(Output, Tracer); return Output; } @@ -1775,10 +1791,12 @@ Options.IncludeMacros = false; Options.IncludeCodePatterns = false; Options.IncludeBriefComments = false; - IncludeStructure PreambleInclusions; // Unused for signatureHelp + // Patch additional includes since signaturehelp can be trigerred after a code + // completion which insterted a new header. semaCodeComplete( std::make_unique(Options, Index, Result), Options, - {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)}); + {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)}, + /*PatchAdditionalIncludes=*/true); return Result; } 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 @@ -195,6 +195,16 @@ tooling::HeaderIncludes Inserter; // Computers insertion replacement. }; +/// Gets the includes in the preamble section of the file by running lexer over +/// \p Contents. Returned inclusions are sorted according to written filename. +std::vector getPreambleIncludes(llvm::StringRef Contents, + const LangOptions &LangOpts); + +/// Returns set of includes existing in \p New but missing in \p Old. Assumes +/// inputs are sorted w.r.t Written field. +std::vector newIncludes(llvm::ArrayRef Old, + llvm::ArrayRef New); + } // namespace clangd } // namespace clang 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 @@ -10,10 +10,13 @@ #include "Compiler.h" #include "Logger.h" #include "SourceCode.h" +#include "clang/Basic/LangOptions.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Lex/HeaderSearch.h" +#include "llvm/ADT/Optional.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Path.h" @@ -231,5 +234,72 @@ << Inc.R; } +std::vector getPreambleIncludes(llvm::StringRef Contents, + const LangOptions &LangOpts) { + auto ContentsBuffer = llvm::MemoryBuffer::getMemBuffer(Contents, "dummy"); + auto Bounds = ComputePreambleBounds(LangOpts, ContentsBuffer.get(), 0); + + SourceManagerForFile SMFF("dummy", ContentsBuffer->getBuffer()); + auto &SM = SMFF.get(); + auto FID = SM.getMainFileID(); + + Lexer TheLexer(FID, SM.getBuffer(FID), SM, LangOpts); + Token TheTok; + std::vector Incs; + Inclusion Inc; + + // Lex until end of preamble bounds. + while (!TheLexer.LexFromRawLexer(TheTok) && + TheLexer.getCurrentBufferOffset() < Bounds.Size) { + // Skip unless at the start of a PP directive. + if (!TheTok.isAtStartOfLine() || !TheTok.is(tok::hash)) + continue; + Inc.HashOffset = TheLexer.getCurrentBufferOffset() - TheTok.getLength(); + + // Skip if not an include directive. + TheLexer.LexFromRawLexer(TheTok); + if (!TheTok.is(tok::raw_identifier)) + continue; + llvm::StringRef Directive = TheTok.getRawIdentifier(); + if (Directive != "include" && Directive != "import" && + Directive != "include_next") + continue; + + // Skip if on a broken include directive + TheLexer.LexIncludeFilename(TheTok); + if (!TheTok.isLiteral()) + continue; + Inc.Written = + llvm::StringRef(TheTok.getLiteralData(), TheTok.getLength()).str(); + Inc.R = halfOpenToRange(SM, CharSourceRange::getCharRange( + TheTok.getLocation(), TheTok.getEndLoc())); + Incs.push_back(std::move(Inc)); + } + llvm::sort(Incs, [](const Inclusion &LHS, const Inclusion &RHS) { + return LHS.Written < RHS.Written; + }); + + return Incs; +} + +std::vector newIncludes(llvm::ArrayRef Old, + llvm::ArrayRef New) { + std::vector Added; + while (!Old.empty() && !New.empty()) { + // Old.front() doesn't exists in New. Ignored as we are only looking for + // additions. + if (Old.front().Written < New.front().Written) + Old = Old.drop_front(); + else if (Old.front().Written > New.front().Written) { + Added.push_back(New.front()); + New = New.drop_front(); + } else { + Old = Old.drop_front(); + New = New.drop_front(); + } + } + Added.insert(Added.end(), New.begin(), New.end()); + return Added; +} } // namespace clangd } // namespace clang 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 @@ -47,7 +47,8 @@ std::vector Diags, IncludeStructure Includes, MainFileMacros Macros, std::unique_ptr StatCache, - CanonicalIncludes CanonIncludes); + CanonicalIncludes CanonIncludes, + std::vector LexedIncludes); // Version of the ParseInputs this preamble was built from. std::string Version; @@ -65,6 +66,9 @@ // When reusing a preamble, this cache can be consumed to save IO. std::unique_ptr StatCache; CanonicalIncludes CanonIncludes; + /// Contains all of the includes spelled in the preamble section, even the + /// ones in disabled regions. + std::vector LexedIncludes; }; using PreambleParsedCallback = 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 @@ -8,11 +8,13 @@ #include "Preamble.h" #include "Compiler.h" +#include "Headers.h" #include "Logger.h" #include "Trace.h" #include "clang/Basic/SourceLocation.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/PreprocessorOptions.h" +#include namespace clang { namespace clangd { @@ -81,12 +83,13 @@ std::vector Diags, IncludeStructure Includes, MainFileMacros Macros, std::unique_ptr StatCache, - CanonicalIncludes CanonIncludes) + CanonicalIncludes CanonIncludes, + std::vector LexedIncludes) : Version(Inputs.Version), CompileCommand(Inputs.CompileCommand), Preamble(std::move(Preamble)), Diags(std::move(Diags)), Includes(std::move(Includes)), Macros(std::move(Macros)), - StatCache(std::move(StatCache)), CanonIncludes(std::move(CanonIncludes)) { -} + StatCache(std::move(StatCache)), CanonIncludes(std::move(CanonIncludes)), + LexedIncludes(std::move(LexedIncludes)) {} std::shared_ptr buildPreamble(PathRef FileName, CompilerInvocation &CI, @@ -164,7 +167,8 @@ Inputs, std::move(*BuiltPreamble), std::move(Diags), SerializedDeclsCollector.takeIncludes(), SerializedDeclsCollector.takeMacros(), std::move(StatCache), - SerializedDeclsCollector.takeCanonicalIncludes()); + SerializedDeclsCollector.takeCanonicalIncludes(), + getPreambleIncludes(Inputs.Contents, *CI.getLangOpts())); } else { elog("Could not build a preamble for file {0} version {1}", FileName, Inputs.Version); diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h --- a/clang-tools-extra/clangd/TUScheduler.h +++ b/clang-tools-extra/clangd/TUScheduler.h @@ -245,11 +245,6 @@ /// Controls whether preamble reads wait for the preamble to be up-to-date. enum PreambleConsistency { - /// The preamble is generated from the current version of the file. - /// If the content was recently updated, we will wait until we have a - /// preamble that reflects that update. - /// This is the slowest option, and may be delayed by other tasks. - Consistent, /// The preamble may be generated from an older version of the file. /// Reading from locations in the preamble may cause files to be re-read. /// This gives callers two options: diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -595,36 +595,6 @@ return LastBuiltPreamble; } -void ASTWorker::getCurrentPreamble( - llvm::unique_function)> Callback) { - // We could just call startTask() to throw the read on the queue, knowing - // it will run after any updates. But we know this task is cheap, so to - // improve latency we cheat: insert it on the queue after the last update. - std::unique_lock Lock(Mutex); - auto LastUpdate = - std::find_if(Requests.rbegin(), Requests.rend(), - [](const Request &R) { return R.UpdateType.hasValue(); }); - // If there were no writes in the queue, and CurrentRequest is not a write, - // the preamble is ready now. - if (LastUpdate == Requests.rend() && - (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) { - Lock.unlock(); - return Callback(getPossiblyStalePreamble()); - } - assert(!RunSync && "Running synchronously, but queue is non-empty!"); - Requests.insert(LastUpdate.base(), - Request{[Callback = std::move(Callback), this]() mutable { - Callback(getPossiblyStalePreamble()); - }, - "GetPreamble", steady_clock::now(), - Context::current().clone(), - /*UpdateType=*/None, - /*InvalidationPolicy=*/TUScheduler::NoInvalidation, - /*Invalidate=*/nullptr}); - Lock.unlock(); - RequestsCV.notify_all(); -} - void ASTWorker::waitForFirstPreamble() const { PreambleWasBuilt.wait(); } std::shared_ptr ASTWorker::getCurrentFileInputs() const { @@ -1013,38 +983,21 @@ return; } - // Future is populated if the task needs a specific preamble. - std::future> ConsistentPreamble; - if (Consistency == Consistent) { - std::promise> Promise; - ConsistentPreamble = Promise.get_future(); - It->second->Worker->getCurrentPreamble( - [Promise = std::move(Promise)]( - std::shared_ptr Preamble) mutable { - Promise.set_value(std::move(Preamble)); - }); - } - std::shared_ptr Worker = It->second->Worker.lock(); auto Task = [Worker, Consistency, Name = Name.str(), File = File.str(), Contents = It->second->Contents, Command = Worker->getCurrentCompileCommand(), Ctx = Context::current().derive(kFileBeingProcessed, std::string(File)), - ConsistentPreamble = std::move(ConsistentPreamble), Action = std::move(Action), this]() mutable { std::shared_ptr Preamble; - if (ConsistentPreamble.valid()) { - Preamble = ConsistentPreamble.get(); - } else { - if (Consistency != PreambleConsistency::StaleOrAbsent) { - // Wait until the preamble is built for the first time, if preamble - // is required. This avoids extra work of processing the preamble - // headers in parallel multiple times. - Worker->waitForFirstPreamble(); - } - Preamble = Worker->getPossiblyStalePreamble(); + if (Consistency == PreambleConsistency::Stale) { + // Wait until the preamble is built for the first time, if preamble + // is required. This avoids extra work of processing the preamble + // headers in parallel multiple times. + Worker->waitForFirstPreamble(); } + Preamble = Worker->getPossiblyStalePreamble(); std::lock_guard BarrierLock(Barrier); WithContext Guard(std::move(Ctx)); diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -1188,6 +1188,32 @@ } } +TEST(SignatureHelpTest, StalePreamble) { + TestTU TU; + TU.Code = ""; + IgnoreDiagnostics Diags; + auto Inputs = TU.inputs(); + auto CI = buildCompilerInvocation(Inputs, Diags); + ASSERT_TRUE(CI); + auto EmptyPreamble = buildPreamble(testPath(TU.Filename), *CI, + /*OldPreamble=*/nullptr, Inputs, + /*InMemory=*/true, /*Callback=*/nullptr); + ASSERT_TRUE(EmptyPreamble); + + TU.AdditionalFiles["a.h"] = "int foo(int x);"; + const Annotations Test(R"cpp( + #include "a.h" + void bar() { foo(^2); })cpp"); + TU.Code = Test.code().str(); + Inputs = TU.inputs(); + auto Results = + signatureHelp(testPath(TU.Filename), Inputs.CompileCommand, + *EmptyPreamble, TU.Code, Test.point(), Inputs.FS, nullptr); + EXPECT_THAT(Results.signatures, ElementsAre(Sig("foo([[int x]]) -> int"))); + EXPECT_EQ(0, Results.activeSignature); + EXPECT_EQ(0, Results.activeParameter); +} + class IndexRequestCollector : public SymbolIndex { public: bool diff --git a/clang-tools-extra/clangd/unittests/HeadersTests.cpp b/clang-tools-extra/clangd/unittests/HeadersTests.cpp --- a/clang-tools-extra/clangd/unittests/HeadersTests.cpp +++ b/clang-tools-extra/clangd/unittests/HeadersTests.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "Annotations.h" #include "Headers.h" #include "Compiler.h" @@ -15,9 +16,13 @@ #include "clang/Frontend/CompilerInvocation.h" #include "clang/Frontend/FrontendActions.h" #include "clang/Lex/PreprocessorOptions.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/Path.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "gtest/internal/gtest-port.h" +#include +#include namespace clang { namespace clangd { @@ -136,6 +141,13 @@ return arg.getKey() == File && arg.getValue() == D; } +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_F(HeadersTest, CollectRewrittenAndResolved) { FS.Files[MainFile] = R"cpp( #include "sub/bar.h" // not shortest @@ -285,6 +297,74 @@ llvm::None); } +TEST(GetPreambleIncludes, All) { + llvm::StringRef Cases[] = { + // Only preamble + R"cpp(^#include [["a.h"]])cpp", + // Both preamble and mainfile + R"cpp( + ^#include [["a.h"]] + garbage, finishes preamble + #include "a.h")cpp", + // Mixed directives + R"cpp( + ^#include [["a.h"]] + #pragma directive + // some comments + ^#include_next [[]] + #ifdef skipped + ^#import [["a.h"]] + #endif)cpp", + // Broken directives + R"cpp( + #include "a + ^#include [["a.h"]] + #include ]])cpp", + }; + LangOptions LangOpts; + LangOpts.CPlusPlus = true; + for (const auto Case : Cases) { + Annotations Test(Case); + const auto Code = Test.code(); + const auto Points = Test.points(); + const auto Ranges = Test.ranges(); + + std::vector ExpectedIncs; + ExpectedIncs.resize(Points.size()); + for (size_t I = 0, E = ExpectedIncs.size(); I != E; ++I) { + Inclusion &Inc = ExpectedIncs[I]; + Inc.HashOffset = llvm::cantFail(positionToOffset(Code, Points[I])); + Inc.R = Ranges[I]; + auto StartOffset = llvm::cantFail(positionToOffset(Code, Inc.R.start)); + auto EndOffset = llvm::cantFail(positionToOffset(Code, Inc.R.end)); + Inc.Written = Code.substr(StartOffset, EndOffset - StartOffset).str(); + } + + EXPECT_THAT(getPreambleIncludes(Code, LangOpts), + testing::UnorderedPointwise(EqInc(), ExpectedIncs)); + } +} + +TEST(NewIncludes, All) { + std::vector LHS(5); + LHS[0].Written = "\"a.h\""; // Deleted, ignored + LHS[1].Written = "\"b.h\""; + LHS[2].Written = "\"c.h\""; + LHS[3].Written = ""; + LHS[4].Written = ""; + std::vector RHS(6); + RHS[0].Written = "\"b.h\""; + RHS[1].Written = "\"b1.h\""; // Added, in middle. + RHS[2].Written = "\"c.h\""; + RHS[3].Written = ""; + RHS[4].Written = ""; + RHS[5].Written = ""; // Added, at the end. + + EXPECT_THAT(newIncludes(LHS, RHS), + ElementsAre(Written("\"b1.h\""), Written(""))); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -242,63 +242,6 @@ EXPECT_EQ(2, CallbackCount); } -static std::vector includes(const PreambleData *Preamble) { - std::vector Result; - if (Preamble) - for (const auto &Inclusion : Preamble->Includes.MainFileIncludes) - Result.push_back(Inclusion.Written); - return Result; -} - -TEST_F(TUSchedulerTests, PreambleConsistency) { - std::atomic CallbackCount(0); - { - Notification InconsistentReadDone; // Must live longest. - TUScheduler S(CDB, optsForTest()); - auto Path = testPath("foo.cpp"); - // Schedule two updates (A, B) and two preamble reads (stale, consistent). - // The stale read should see A, and the consistent read should see B. - // (We recognize the preambles by their included files). - auto Inputs = getInputs(Path, "#include "); - Inputs.Version = "A"; - updateWithCallback(S, Path, Inputs, WantDiagnostics::Yes, [&]() { - // This callback runs in between the two preamble updates. - - // This blocks update B, preventing it from winning the race - // against the stale read. - // If the first read was instead consistent, this would deadlock. - InconsistentReadDone.wait(); - // This delays update B, preventing it from winning a race - // against the consistent read. The consistent read sees B - // only because it waits for it. - // If the second read was stale, it would usually see A. - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - }); - Inputs.Contents = "#include "; - Inputs.Version = "B"; - S.update(Path, Inputs, WantDiagnostics::Yes); - - S.runWithPreamble("StaleRead", Path, TUScheduler::Stale, - [&](Expected Pre) { - ASSERT_TRUE(bool(Pre)); - EXPECT_EQ(Pre->Preamble->Version, "A"); - EXPECT_THAT(includes(Pre->Preamble), - ElementsAre("")); - InconsistentReadDone.notify(); - ++CallbackCount; - }); - S.runWithPreamble("ConsistentRead", Path, TUScheduler::Consistent, - [&](Expected Pre) { - ASSERT_TRUE(bool(Pre)); - EXPECT_EQ(Pre->Preamble->Version, "B"); - EXPECT_THAT(includes(Pre->Preamble), - ElementsAre("")); - ++CallbackCount; - }); - } - EXPECT_EQ(2, CallbackCount); -} - TEST_F(TUSchedulerTests, Cancellation) { // We have the following update/read sequence // U0