From 43aa04eb7a617ee75dfcbbe2d395b8208e66c0e0 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Wed, 1 Apr 2020 11:31:53 +0200 Subject: [PATCH] [clangd] Run semaCodeComplete only with a preamble Summary: It is used by code completion and signature help. Code completion already uses a special no-compile mode for missing preambles, so this change is a no-op for that. As for signature help, it already blocks for a preamble and missing it implies clang has failed to parse the preamble and retrying it in signature help likely will fail again. And even if it doesn't, request latency will be too high to be useful as parsing preambles is expensive. Reviewers: sammccall Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D77204 --- clang-tools-extra/clangd/ClangdServer.cpp | 10 +++++++--- clang-tools-extra/clangd/CodeComplete.cpp | 17 +++++++---------- clang-tools-extra/clangd/CodeComplete.h | 2 +- .../clangd/unittests/ClangdTests.cpp | 4 +--- .../clangd/unittests/CodeCompleteTests.cpp | 8 ++++++-- 5 files changed, 22 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 0882784f7031..c1773258554c 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -271,9 +271,13 @@ void ClangdServer::signatureHelp(PathRef File, Position Pos, if (!IP) return CB(IP.takeError()); - auto PreambleData = IP->Preamble; - CB(clangd::signatureHelp(File, IP->Command, PreambleData, IP->Contents, Pos, - FS, Index)); + const auto *PreambleData = IP->Preamble; + if (!PreambleData) + return CB(llvm::createStringError(llvm::inconvertibleErrorCode(), + "Failed to parse includes")); + + CB(clangd::signatureHelp(File, IP->Command, *PreambleData, IP->Contents, + Pos, FS, Index)); }; // Unlike code completion, we wait for an up-to-date preamble here. diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index 86ea5f26d397..b544510ecea1 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -1022,7 +1022,7 @@ private: struct SemaCompleteInput { PathRef FileName; const tooling::CompileCommand &Command; - const PreambleData *Preamble; + const PreambleData &Preamble; llvm::StringRef Contents; size_t Offset; llvm::IntrusiveRefCntPtr VFS; @@ -1054,8 +1054,8 @@ bool semaCodeComplete(std::unique_ptr Consumer, IncludeStructure *Includes = nullptr) { trace::Span Tracer("Sema completion"); llvm::IntrusiveRefCntPtr VFS = Input.VFS; - if (Input.Preamble && Input.Preamble->StatCache) - VFS = Input.Preamble->StatCache->getConsumingFS(std::move(VFS)); + if (Input.Preamble.StatCache) + VFS = Input.Preamble.StatCache->getConsumingFS(std::move(VFS)); ParseInputs ParseInput; ParseInput.CompileCommand = Input.Command; ParseInput.FS = VFS; @@ -1099,9 +1099,7 @@ bool semaCodeComplete(std::unique_ptr Consumer, // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise // the remapped buffers do not get freed. auto Clang = prepareCompilerInstance( - std::move(CI), - (Input.Preamble && !CompletingInPreamble) ? &Input.Preamble->Preamble - : nullptr, + std::move(CI), !CompletingInPreamble ? &Input.Preamble.Preamble : nullptr, std::move(ContentsBuffer), std::move(VFS), IgnoreDiags); Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble; Clang->setCodeCompletionConsumer(Consumer.release()); @@ -1118,8 +1116,7 @@ bool semaCodeComplete(std::unique_ptr Consumer, // - but Sema code complete won't see them: as part of the preamble, they're // deserialized only when mentioned. // Force them to be deserialized so SemaCodeComplete sees them. - if (Input.Preamble) - loadMainFilePreambleMacros(Clang->getPreprocessor(), *Input.Preamble); + loadMainFilePreambleMacros(Clang->getPreprocessor(), Input.Preamble); if (Includes) Clang->getPreprocessor().addPPCallbacks( collectIncludeStructureCallback(Clang->getSourceManager(), Includes)); @@ -1758,12 +1755,12 @@ codeComplete(PathRef FileName, const tooling::CompileCommand &Command, return (!Preamble || Opts.RunParser == CodeCompleteOptions::NeverParse) ? std::move(Flow).runWithoutSema(Contents, *Offset, VFS) : std::move(Flow).run( - {FileName, Command, Preamble, Contents, *Offset, VFS}); + {FileName, Command, *Preamble, Contents, *Offset, VFS}); } SignatureHelp signatureHelp(PathRef FileName, const tooling::CompileCommand &Command, - const PreambleData *Preamble, + const PreambleData &Preamble, llvm::StringRef Contents, Position Pos, llvm::IntrusiveRefCntPtr VFS, const SymbolIndex *Index) { diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h index df06c156049f..3adea47c89a1 100644 --- a/clang-tools-extra/clangd/CodeComplete.h +++ b/clang-tools-extra/clangd/CodeComplete.h @@ -276,7 +276,7 @@ CodeCompleteResult codeComplete(PathRef FileName, /// Get signature help at a specified \p Pos in \p FileName. SignatureHelp signatureHelp(PathRef FileName, const tooling::CompileCommand &Command, - const PreambleData *Preamble, StringRef Contents, + const PreambleData &Preamble, StringRef Contents, Position Pos, IntrusiveRefCntPtr VFS, const SymbolIndex *Index); diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp index 1e5fcf3d97e1..d15eba80ae29 100644 --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -552,15 +552,13 @@ TEST_F(ClangdVFSTest, InvalidCompileCommand) { EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position())); EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name", clangd::RenameOptions())); + EXPECT_ERROR(runSignatureHelp(Server, FooCpp, Position())); // Identifier-based fallback completion. EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Position(), clangd::CodeCompleteOptions())) .Completions, ElementsAre(Field(&CodeCompletion::Name, "int"), Field(&CodeCompletion::Name, "main"))); - auto SigHelp = runSignatureHelp(Server, FooCpp, Position()); - ASSERT_TRUE(bool(SigHelp)) << "signatureHelp returned an error"; - EXPECT_THAT(SigHelp->signatures, IsEmpty()); } class ClangdThreadingTest : public ClangdVFSTest {}; diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp index 3cb18f6893e9..1084b1550579 100644 --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -1049,8 +1049,12 @@ SignatureHelp signatures(llvm::StringRef Text, Position Point, auto Preamble = buildPreamble(testPath(TU.Filename), *CI, /*OldPreamble=*/nullptr, Inputs, /*InMemory=*/true, /*Callback=*/nullptr); - return signatureHelp(testPath(TU.Filename), Inputs.CompileCommand, - Preamble.get(), Text, Point, Inputs.FS, Index.get()); + if (!Preamble) { + ADD_FAILURE() << "Couldn't build Preamble"; + return {}; + } + return signatureHelp(testPath(TU.Filename), Inputs.CompileCommand, *Preamble, + Text, Point, Inputs.FS, Index.get()); } SignatureHelp signatures(llvm::StringRef Text,