[clangd] Set completion options per-request.

Summary:
Previously, completion options were set per ClangdServer instance.
It will allow to change completion preferences during the lifetime
of a single ClangdServer instance.

Also rewrote ClangdCompletionTest.CompletionOptions to reuse single
ClangdServer instance, the test now runs 2x faster on my machine.

Reviewers: sammccall, ioeric, hokein

Reviewed By: sammccall, ioeric

Subscribers: klimek, cfe-commits

Differential Revision: https://reviews.llvm.org/D40654

llvm-svn: 319753

6 files changed, 52 insertions, 46 deletions

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index dbce7c87d4b..f8d50c17b64 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -194,14 +194,15 @@ void ClangdLSPServer::onCodeAction(Ctx C, CodeActionParams &Params) {
 }
 
 void ClangdLSPServer::onCompletion(Ctx C, TextDocumentPositionParams &Params) {
-  auto List = Server
-                  .codeComplete(
-                      Params.textDocument.uri.file,
-                      Position{Params.position.line, Params.position.character})
-                  .get() // FIXME(ibiryukov): This could be made async if we
-                         // had an API that would allow to attach callbacks to
-                         // futures returned by ClangdServer.
-                  .Value;
+  auto List =
+      Server
+          .codeComplete(
+              Params.textDocument.uri.file,
+              Position{Params.position.line, Params.position.character}, CCOpts)
+          .get() // FIXME(ibiryukov): This could be made async if we
+                 // had an API that would allow to attach callbacks to
+                 // futures returned by ClangdServer.
+          .Value;
   C.reply(List);
 }
 
@@ -240,9 +241,9 @@ ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                                  llvm::Optional<StringRef> ResourceDir,
                                  llvm::Optional<Path> CompileCommandsDir)
     : Out(Out), CDB(/*Logger=*/Out, std::move(CompileCommandsDir)),
-      Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount,
-             StorePreamblesInMemory, CCOpts,
-             /*Logger=*/Out, ResourceDir) {}
+      CCOpts(CCOpts), Server(CDB, /*DiagConsumer=*/*this, FSProvider,
+                             AsyncThreadsCount, StorePreamblesInMemory,
+                             /*Logger=*/Out, ResourceDir) {}
 
 bool ClangdLSPServer::run(std::istream &In) {
   assert(!IsDone && "Run was called before");
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 45163348f9d..ee4b1a0c61f 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -96,7 +96,8 @@ private:
   // before ClangdServer.
   DirectoryBasedGlobalCompilationDatabase CDB;
   RealFileSystemProvider FSProvider;
-
+  /// Options used for code completion
+  clangd::CodeCompleteOptions CCOpts;
   // Server must be the last member of the class to allow its destructor to exit
   // the worker thread that may otherwise run an async callback on partially
   // destructed instance of ClangdLSPServer.
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 618d69ff253..d1fe1fd7c62 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -173,16 +173,14 @@ ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
                            DiagnosticsConsumer &DiagConsumer,
                            FileSystemProvider &FSProvider,
                            unsigned AsyncThreadsCount,
-                           bool StorePreamblesInMemory,
-                           const clangd::CodeCompleteOptions &CodeCompleteOpts,
-                           clangd::Logger &Logger,
+                           bool StorePreamblesInMemory, clangd::Logger &Logger,
                            llvm::Optional<StringRef> ResourceDir)
     : Logger(Logger), CDB(CDB), DiagConsumer(DiagConsumer),
       FSProvider(FSProvider),
       ResourceDir(ResourceDir ? ResourceDir->str() : getStandardResourceDir()),
       PCHs(std::make_shared<PCHContainerOperations>()),
       StorePreamblesInMemory(StorePreamblesInMemory),
-      CodeCompleteOpts(CodeCompleteOpts), WorkScheduler(AsyncThreadsCount) {}
+      WorkScheduler(AsyncThreadsCount) {}
 
 void ClangdServer::setRootPath(PathRef RootPath) {
   std::string NewRootPath = llvm::sys::path::convert_to_slash(
@@ -226,6 +224,7 @@ std::future<void> ClangdServer::forceReparse(PathRef File) {
 
 std::future<Tagged<CompletionList>>
 ClangdServer::codeComplete(PathRef File, Position Pos,
+                           const clangd::CodeCompleteOptions &Opts,
                            llvm::Optional<StringRef> OverridenContents,
                            IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   using ResultType = Tagged<CompletionList>;
@@ -239,13 +238,14 @@ ClangdServer::codeComplete(PathRef File, Position Pos,
 
   std::future<ResultType> ResultFuture = ResultPromise.get_future();
   codeComplete(BindWithForward(Callback, std::move(ResultPromise)), File, Pos,
-               OverridenContents, UsedFS);
+               Opts, OverridenContents, UsedFS);
   return ResultFuture;
 }
 
 void ClangdServer::codeComplete(
     UniqueFunction<void(Tagged<CompletionList>)> Callback, PathRef File,
-    Position Pos, llvm::Optional<StringRef> OverridenContents,
+    Position Pos, const clangd::CodeCompleteOptions &Opts,
+    llvm::Optional<StringRef> OverridenContents,
     IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   using CallbackType = UniqueFunction<void(Tagged<CompletionList>)>;
 
@@ -273,6 +273,8 @@ void ClangdServer::codeComplete(
   // is reusable in completion more often.
   std::shared_ptr<const PreambleData> Preamble =
       Resources->getPossiblyStalePreamble();
+  // Copy completion options for passing them to async task handler.
+  auto CodeCompleteOpts = Opts;
   // A task that will be run asynchronously.
   auto Task =
       // 'mutable' to reassign Preamble variable.
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index fe067f0a22f..2e703b3cdb7 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -211,7 +211,6 @@ public:
                DiagnosticsConsumer &DiagConsumer,
                FileSystemProvider &FSProvider, unsigned AsyncThreadsCount,
                bool StorePreamblesInMemory,
-               const clangd::CodeCompleteOptions &CodeCompleteOpts,
                clangd::Logger &Logger,
                llvm::Optional<StringRef> ResourceDir = llvm::None);
 
@@ -255,6 +254,7 @@ public:
   /// while returned future is not yet ready.
   std::future<Tagged<CompletionList>>
   codeComplete(PathRef File, Position Pos,
+               const clangd::CodeCompleteOptions &Opts,
                llvm::Optional<StringRef> OverridenContents = llvm::None,
                IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
 
@@ -262,6 +262,7 @@ public:
   /// when codeComplete results become available.
   void codeComplete(UniqueFunction<void(Tagged<CompletionList>)> Callback,
                     PathRef File, Position Pos,
+                    const clangd::CodeCompleteOptions &Opts,
                     llvm::Optional<StringRef> OverridenContents = llvm::None,
                     IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
 
@@ -328,7 +329,6 @@ private:
   llvm::Optional<std::string> RootPath;
   std::shared_ptr<PCHContainerOperations> PCHs;
   bool StorePreamblesInMemory;
-  clangd::CodeCompleteOptions CodeCompleteOpts;
   /// Used to serialize diagnostic callbacks.
   /// FIXME(ibiryukov): get rid of an extra map and put all version counters
   /// into CppFile.
diff --git a/clang-tools-extra/unittests/clangd/ClangdTests.cpp b/clang-tools-extra/unittests/clangd/ClangdTests.cpp
index ef6dec2a0b2..c53591c4034 100644
--- a/clang-tools-extra/unittests/clangd/ClangdTests.cpp
+++ b/clang-tools-extra/unittests/clangd/ClangdTests.cpp
@@ -124,7 +124,6 @@ protected:
     MockCompilationDatabase CDB;
     ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                         /*StorePreamblesInMemory=*/true,
-                        clangd::CodeCompleteOptions(),
                         EmptyLogger::getInstance());
     for (const auto &FileWithContents : ExtraFiles)
       FS.Files[getVirtualTestFilePath(FileWithContents.first)] =
@@ -189,7 +188,6 @@ TEST_F(ClangdVFSTest, Reparse) {
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
 
   const auto SourceContents = R"cpp(
@@ -236,7 +234,6 @@ TEST_F(ClangdVFSTest, ReparseOnHeaderChange) {
 
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
 
   const auto SourceContents = R"cpp(
@@ -286,7 +283,6 @@ TEST_F(ClangdVFSTest, CheckVersions) {
   ClangdServer Server(CDB, DiagConsumer, FS,
                       /*AsyncThreadsCount=*/0,
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
@@ -294,17 +290,22 @@ TEST_F(ClangdVFSTest, CheckVersions) {
   FS.Files[FooCpp] = SourceContents;
   FS.ExpectedFile = FooCpp;
 
+  // Use default completion options.
+  clangd::CodeCompleteOptions CCOpts;
+
   // No need to sync reparses, because requests are processed on the calling
   // thread.
   FS.Tag = "123";
   Server.addDocument(FooCpp, SourceContents);
-  EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).get().Tag, FS.Tag);
+  EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}, CCOpts).get().Tag,
+            FS.Tag);
   EXPECT_EQ(DiagConsumer.lastVFSTag(), FS.Tag);
 
   FS.Tag = "321";
   Server.addDocument(FooCpp, SourceContents);
   EXPECT_EQ(DiagConsumer.lastVFSTag(), FS.Tag);
-  EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).get().Tag, FS.Tag);
+  EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}, CCOpts).get().Tag,
+            FS.Tag);
 }
 
 // Only enable this test on Unix
@@ -322,7 +323,6 @@ TEST_F(ClangdVFSTest, SearchLibDir) {
   ClangdServer Server(CDB, DiagConsumer, FS,
                       /*AsyncThreadsCount=*/0,
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
 
   // Just a random gcc version string
@@ -373,7 +373,6 @@ TEST_F(ClangdVFSTest, ForceReparseCompileCommand) {
   ClangdServer Server(CDB, DiagConsumer, FS,
                       /*AsyncThreadsCount=*/0,
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
   // No need to sync reparses, because reparses are performed on the calling
   // thread to true.
@@ -515,7 +514,6 @@ int d;
     MockCompilationDatabase CDB;
     ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                         /*StorePreamblesInMemory=*/true,
-                        clangd::CodeCompleteOptions(),
                         EmptyLogger::getInstance());
 
     // Prepare some random distributions for the test.
@@ -609,7 +607,10 @@ int d;
       // requests as opposed to AddDocument/RemoveDocument, which are implicitly
       // cancelled by any subsequent AddDocument/RemoveDocument request to the
       // same file.
-      Server.codeComplete(FilePaths[FileIndex], Pos).wait();
+      Server
+          .codeComplete(FilePaths[FileIndex], Pos,
+                        clangd::CodeCompleteOptions())
+          .wait();
     };
 
     auto FindDefinitionsRequest = [&]() {
@@ -676,7 +677,6 @@ TEST_F(ClangdVFSTest, CheckSourceHeaderSwitch) {
 
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
 
   auto SourceContents = R"cpp(
@@ -803,7 +803,6 @@ int d;
 
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, DiagConsumer, FS, 4, /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
   Server.addDocument(FooCpp, SourceContentsWithErrors);
   StartSecondReparse.wait();
diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
index d1fc159debc..fa67415a71d 100644
--- a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -75,7 +75,6 @@ TEST_F(ClangdCompletionTest, CheckContentsOverride) {
 
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true,
-                      clangd::CodeCompleteOptions(),
                       EmptyLogger::getInstance());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
@@ -88,6 +87,9 @@ int b =   ;
 int cbc;
 int b =   ;
 )cpp";
+
+  // Use default options.
+  CodeCompleteOptions CCOpts;
   // Complete after '=' sign. We need to be careful to keep the SourceContents'
   // size the same.
   // We complete on the 3rd line (2nd in zero-based numbering), because raw
@@ -103,7 +105,7 @@ int b =   ;
 
   {
     auto CodeCompletionResults1 =
-        Server.codeComplete(FooCpp, CompletePos, None).get().Value;
+        Server.codeComplete(FooCpp, CompletePos, CCOpts, None).get().Value;
     EXPECT_TRUE(ContainsItem(CodeCompletionResults1, "aba"));
     EXPECT_FALSE(ContainsItem(CodeCompletionResults1, "cbc"));
   }
@@ -111,7 +113,7 @@ int b =   ;
   {
     auto CodeCompletionResultsOverriden =
         Server
-            .codeComplete(FooCpp, CompletePos,
+            .codeComplete(FooCpp, CompletePos, CCOpts,
                           StringRef(OverridenSourceContents))
             .get()
             .Value;
@@ -121,7 +123,7 @@ int b =   ;
 
   {
     auto CodeCompletionResults2 =
-        Server.codeComplete(FooCpp, CompletePos, None).get().Value;
+        Server.codeComplete(FooCpp, CompletePos, CCOpts, None).get().Value;
     EXPECT_TRUE(ContainsItem(CodeCompletionResults2, "aba"));
     EXPECT_FALSE(ContainsItem(CodeCompletionResults2, "cbc"));
   }
@@ -132,10 +134,8 @@ TEST_F(ClangdCompletionTest, Limit) {
   MockCompilationDatabase CDB;
   CDB.ExtraClangFlags.push_back("-xc++");
   IgnoreDiagnostics DiagConsumer;
-  clangd::CodeCompleteOptions Opts;
-  Opts.Limit = 2;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
-                      /*StorePreamblesInMemory=*/true, Opts,
+                      /*StorePreamblesInMemory=*/true,
                       EmptyLogger::getInstance());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
@@ -152,9 +152,12 @@ int main() { ClassWithMembers().{complete} }
                                              "complete");
   Server.addDocument(FooCpp, Completion.Text);
 
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 2;
+
   /// For after-dot completion we must always get consistent results.
   auto Results = Server
-                     .codeComplete(FooCpp, Completion.MarkerPos,
+                     .codeComplete(FooCpp, Completion.MarkerPos, Opts,
                                    StringRef(Completion.Text))
                      .get()
                      .Value;
@@ -171,9 +174,8 @@ TEST_F(ClangdCompletionTest, Filter) {
   MockCompilationDatabase CDB;
   CDB.ExtraClangFlags.push_back("-xc++");
   IgnoreDiagnostics DiagConsumer;
-  clangd::CodeCompleteOptions Opts;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
-                      /*StorePreamblesInMemory=*/true, Opts,
+                      /*StorePreamblesInMemory=*/true,
                       EmptyLogger::getInstance());
 
   auto FooCpp = getVirtualTestFilePath("foo.cpp");
@@ -194,7 +196,8 @@ TEST_F(ClangdCompletionTest, Filter) {
         "complete");
     Server.addDocument(FooCpp, Completion.Text);
     return Server
-        .codeComplete(FooCpp, Completion.MarkerPos, StringRef(Completion.Text))
+        .codeComplete(FooCpp, Completion.MarkerPos,
+                      clangd::CodeCompleteOptions(), StringRef(Completion.Text))
         .get()
         .Value;
   };
@@ -288,7 +291,7 @@ int test() {
 
   auto TestWithOpts = [&](clangd::CodeCompleteOptions Opts) {
     ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
-                        /*StorePreamblesInMemory=*/true, Opts,
+                        /*StorePreamblesInMemory=*/true,
                         EmptyLogger::getInstance());
     // No need to sync reparses here as there are no asserts on diagnostics (or
     // other async operations).
@@ -301,7 +304,7 @@ int test() {
     /// For after-dot completion we must always get consistent results.
     {
       auto Results = Server
-                         .codeComplete(FooCpp, MemberCompletion.MarkerPos,
+                         .codeComplete(FooCpp, MemberCompletion.MarkerPos, Opts,
                                        StringRef(MemberCompletion.Text))
                          .get()
                          .Value;
@@ -337,7 +340,7 @@ int test() {
     // Global completion differs based on the Opts that were passed.
     {
       auto Results = Server
-                         .codeComplete(FooCpp, GlobalCompletion.MarkerPos,
+                         .codeComplete(FooCpp, GlobalCompletion.MarkerPos, Opts,
                                        StringRef(GlobalCompletion.Text))
                          .get()
                          .Value;