[clangd] Added async API to run code completion.

[ilya-biryukov]

Summary

ClangdServer now provides async code completion API.
It is still used synchronously by ClangdLSPServer, more work is needed
to allow processing other requests in parallel while completion (or
any other request) is running.

[klimek]

clangd/ClangdServer.cpp

222

I think we use "const type" everywhere.

225

It doesn't seem like we use Preamble anywhere else but in the lambda - so why not get it in the lambda?

[ilya-biryukov]

• Moved 'const' to the left.

[ilya-biryukov]

clangd/ClangdServer.cpp

222

Sorry, my previously preferred style keeps sneaking in.

225

TLDR; To use in async requests exactly the same preamble that was previously used for sync requests.

Those are two different time points. Our callers might change the file when we'll be executing this lambda. I assume that most of time we'll want the preamble that was built at the point where codeComplete is called, not after that.

On the other hand, after file was changed, code completion results will probably be irrelevant and will be discarded by clients anyway, so that might not matter. I've opted for not changing the behavior and using the same preamble that was previously used by the synchronous version. (Unless Preamble was null in the sync case, where we would only improve performance).

[klimek]

clangd/ClangdServer.cpp

225

That makes sense, but needs a comment in the code :)

clangd/ClangdServer.h

236–237

Extra space before asynchronously.
Replace first ',' with ';'
... wait for the results of the async request...

[ilya-biryukov]

• Added a comment about Preamble assignment.

[ilya-biryukov]

• Fixed grammar in a comment.

[ilya-biryukov]

clangd/ClangdServer.h

236–237

Split into two sentences on , instead of replacing it with ;. Feels even better.

[klimek]

LG

clangd/ClangdServer.h

236–237

..."the" returned future...

[ilya-biryukov]

• Added another missing 'the'.

clangd/ClangdLSPServer.cpp

 
 void ClangdLSPServer::onCompletion(TextDocumentPositionParams Params,
                                    StringRef ID, JSONOutput &Out) {
 
   auto Items = 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;
 
   std::string Completions;
   for (const auto &Item : Items) {
     Completions += CompletionItem::unparse(Item);
     Completions += ",";
   }
   if (!Completions.empty())

clangd/ClangdServer.h

public:
   /// associated resources are freed.
   std::future<void> removeDocument(PathRef File);
   /// Force \p File to be reparsed using the latest contents.
   /// Will also check if CompileCommand, provided by GlobalCompilationDatabase
   /// for \p File has changed. If it has, will remove currently stored Preamble
   /// and AST and rebuild them from scratch.
   std::future<void> forceReparse(PathRef File);
 
-  /// Run code completion for \p File at \p Pos. If \p OverridenContents is not
-  /// None, they will used only for code completion, i.e. no diagnostics update
-  /// will be scheduled and a draft for \p File will not be updated.
-  /// If \p OverridenContents is None, contents of the current draft for \p File
-  /// will be used.
-  /// If \p UsedFS is non-null, it will be overwritten by vfs::FileSystem used
-  /// for completion.
-  /// This method should only be called for currently tracked
-  /// files.
-  Tagged<std::vector<CompletionItem>>
+  /// Run code completion for \p File at \p Pos.
+  ///
+  /// Request is processed asynchronously. You can use the returned future to
+  /// wait for the results of the async request.
+  ///
+  /// If \p OverridenContents is not None, they will used only for code
+  /// completion, i.e. no diagnostics update will be scheduled and a draft for
+  /// \p File will not be updated. If \p OverridenContents is None, contents of
+  /// the current draft for \p File will be used. If \p UsedFS is non-null, it
+  /// will be overwritten by vfs::FileSystem used for completion.
+  ///
+  /// This method should only be called for currently tracked files. However, it
+  /// is safe to call removeDocument for \p File after this method returns, even
+  /// while returned future is not yet ready.
+  std::future<Tagged<std::vector<CompletionItem>>>
   codeComplete(PathRef File, Position Pos,
                llvm::Optional<StringRef> OverridenContents = llvm::None,
                IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
+
   /// Get definition of symbol at a specified \p Line and \p Column in \p File.
   Tagged<std::vector<Location>> findDefinitions(PathRef File, Position Pos);
 
   /// Helper function that returns a path to the corresponding source file when
   /// given a header file and vice versa.
   llvm::Optional<Path> switchSourceHeader(PathRef Path);
 
   /// Run formatting for \p Rng inside \p File.

clangd/ClangdServer.cpp

std::future<void> ClangdServer::forceReparse(PathRef File) {
   // Note that std::future from this cleanup action is ignored.
   scheduleCancelRebuild(std::move(Recreated.RemovedFile));
   // Schedule a reparse.
   return scheduleReparseAndDiags(File, std::move(FileContents),
                                  std::move(Recreated.FileInCollection),
                                  std::move(TaggedFS));
 }
 
-Tagged<std::vector<CompletionItem>>
+std::future<Tagged<std::vector<CompletionItem>>>
 ClangdServer::codeComplete(PathRef File, Position Pos,
                            llvm::Optional<StringRef> OverridenContents,
                            IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
-  std::string DraftStorage;
-  if (!OverridenContents) {
+  std::string Contents;
+  if (OverridenContents) {
+    Contents = *OverridenContents;
+  } else {
     auto FileContents = DraftMgr.getDraft(File);
     assert(FileContents.Draft &&
            "codeComplete is called for non-added document");
 
-    DraftStorage = std::move(*FileContents.Draft);
-    OverridenContents = DraftStorage;
+    Contents = std::move(*FileContents.Draft);
   }
 
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   if (UsedFS)
     *UsedFS = TaggedFS.Value;
 
   std::shared_ptr<CppFile> Resources = Units.getFile(File);
   assert(Resources && "Calling completion on non-added file");
 
-  auto Preamble = Resources->getPossiblyStalePreamble();
+  using PackagedTask =
+      std::packaged_task<Tagged<std::vector<CompletionItem>>()>;
+
+  // Remember the current Preamble and use it when async task starts executing.
+  // At the point when async task starts executing, we may have a different
+  // Preamble in Resources. However, we assume the Preamble that we obtain here
+  // is reusable in completion more often.
+  std::shared_ptr<const PreambleData> Preamble =
+      Resources->getPossiblyStalePreamble();
+  // A task that will be run asynchronously.
+  PackagedTask Task([=]() mutable { // 'mutable' to reassign Preamble variable.
+    if (!Preamble) {
+      // Maybe we built some preamble before processing this request.
+      Preamble = Resources->getPossiblyStalePreamble();
+    }
+    // FIXME(ibiryukov): even if Preamble is non-null, we may want to check
+    // both the old and the new version in case only one of them matches.
+
     std::vector<CompletionItem> Result = clangd::codeComplete(
         File, Resources->getCompileCommand(),
-      Preamble ? &Preamble->Preamble : nullptr, *OverridenContents, Pos,
-      TaggedFS.Value, PCHs, SnippetCompletions, Logger);
-  return make_tagged(std::move(Result), TaggedFS.Tag);
+        Preamble ? &Preamble->Preamble : nullptr, Contents, Pos, TaggedFS.Value,
+        PCHs, SnippetCompletions, Logger);
+    return make_tagged(std::move(Result), std::move(TaggedFS.Tag));
+  });
+
+  auto Future = Task.get_future();
+  // FIXME(ibiryukov): to reduce overhead for wrapping the same callable
+  // multiple times, ClangdScheduler should return future<> itself.
+  WorkScheduler.addToFront([](PackagedTask Task) { Task(); }, std::move(Task));
+  return Future;
 }
 
 std::vector<tooling::Replacement> ClangdServer::formatRange(PathRef File,
                                                             Range Rng) {
   std::string Code = getDocument(File);
 
   size_t Begin = positionToOffset(Code, Rng.start);
   size_t Len = positionToOffset(Code, Rng.end) - Begin;

unittests/clangd/ClangdTests.cpp

TEST_F(ClangdVFSTest, CheckVersions) {
   const auto SourceContents = "int a;";
   FS.Files[FooCpp] = SourceContents;
   FS.ExpectedFile = FooCpp;
 
   // 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}).Tag, FS.Tag);
+  EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).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}).Tag, FS.Tag);
+  EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).get().Tag, FS.Tag);
 }
 
 // Only enable this test on Unix
 #ifdef LLVM_ON_UNIX
 TEST_F(ClangdVFSTest, SearchLibDir) {
   // Checks that searches for GCC installation is done through vfs.
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
)cpp";
   FS.ExpectedFile = FooCpp;
 
   // No need to sync reparses here as there are no asserts on diagnostics (or
   // other async operations).
   Server.addDocument(FooCpp, SourceContents);
 
   {
     auto CodeCompletionResults1 =
-        Server.codeComplete(FooCpp, CompletePos, None).Value;
+        Server.codeComplete(FooCpp, CompletePos, None).get().Value;
     EXPECT_TRUE(ContainsItem(CodeCompletionResults1, "aba"));
     EXPECT_FALSE(ContainsItem(CodeCompletionResults1, "cbc"));
   }
 
   {
     auto CodeCompletionResultsOverriden =
         Server
             .codeComplete(FooCpp, CompletePos,
                           StringRef(OverridenSourceContents))
+            .get()
             .Value;
     EXPECT_TRUE(ContainsItem(CodeCompletionResultsOverriden, "cbc"));
     EXPECT_FALSE(ContainsItem(CodeCompletionResultsOverriden, "aba"));
   }
 
   {
     auto CodeCompletionResults2 =
-        Server.codeComplete(FooCpp, CompletePos, None).Value;
+        Server.codeComplete(FooCpp, CompletePos, None).get().Value;
     EXPECT_TRUE(ContainsItem(CodeCompletionResults2, "aba"));
     EXPECT_FALSE(ContainsItem(CodeCompletionResults2, "cbc"));
   }
 }
 
 class ClangdThreadingTest : public ClangdVFSTest {};
 
 TEST_F(ClangdThreadingTest, StressTest) {
TestDiagConsumer DiagConsumer;
 
     auto CodeCompletionRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
         AddDocument(FileIndex);
 
       Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
-      Server.codeComplete(FilePaths[FileIndex], Pos);
+      // FIXME(ibiryukov): Also test async completion requests.
+      // Simply putting CodeCompletion into async requests now would make
+      // tests slow, since there's no way to cancel previous completion
+      // 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();
     };
 
     auto FindDefinitionsRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
         AddDocument(FileIndex);