[clangd] Added a callback-based codeComplete in clangd.

[ilya-biryukov]

I hope the whole design doesn't seem overly complicated. Very much open to suggestions on how to simplify it :-)

[sammccall]

I'm missing some context for this one:

• what's the goal? Make the code read more naturally, change the async model, or something else?
• what's the end state for codeComplete specifically? will we switch to the new overload and delete the other, or is makeFutureAPIFromCallback here to stay?

[ilya-biryukov]

• Added a deprecation notice to function description.

[ilya-biryukov]

We had a discussion with @sammccall offline, probably

• what's the goal? Make the code read more naturally, change the async model, or something else?

Callback API is more flexible (if std::future that we use had then, they'd be equivalent).
We have internal clients that want to be notified about results of code completion without

• what's the end state for codeComplete specifically? will we switch to the new overload and delete the other, or is makeFutureAPIFromCallback here to stay?

I think the way to go would be exactly that, switch to the new API and remove the older one. There are usages in tests, ClangdLSPServer and in our internal client, but I'll remove them with a separate commit later.
Added a deprecation notice for the API.

[sammccall]

clangd/ClangdServer.cpp

51

I'm not sure we need a template here rather than just writing the code directly.

257

Isn't this exactly where you'd want to use ForwardBinder?
(Yeah, it looks like the work scheduler itself has this functionality too)

clangd/ClangdServer.h

258

Hmm, generally having a callback as the last parameter is nicer, but this doesn't play nicely with optional params...

[ilya-biryukov]

• Inlined makeFutureAPIFromCallback.

[ilya-biryukov]

clangd/ClangdServer.cpp

51

Removed the template, inlined its usage.

257

Yes, ClangdServer uses ForwardBinder internally.

We could refactor it to accept UniqueFunction instead, but I suggest we do that in a separate commit since there are more call sites that we'll have to change.

clangd/ClangdServer.h

258

Right, so I opted for making it a first parameter.
I don't know if there's a better alternative. Do you think making it a last non-optional parameter would make things better? (Probably it will, at least calls without non-optional params would look nicer).

clangd/ClangdServer.h

public:
   /// 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);
 
+  /// A version of `codeComplete` that runs \p Callback on the processing thread
+  /// when codeComplete results become available.
+  void codeComplete(
+      UniqueFunction<void(Tagged<std::vector<CompletionItem>>)> Callback,
+      PathRef File, Position Pos,
+      llvm::Optional<StringRef> OverridenContents = llvm::None,
+      IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
+
   /// Provide signature help for \p File at \p Pos. If \p OverridenContents is
   /// not None, they will used only for signature help, 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 signature help. This method should only be called
   /// for currently tracked files.
   Tagged<SignatureHelp>

clangd/ClangdServer.cpp

std::vector<tooling::Replacement> formatCode(StringRef Code, StringRef Filename,
   return std::vector<tooling::Replacement>(Result.begin(), Result.end());
 }
 
 std::string getStandardResourceDir() {
   static int Dummy; // Just an address in this process.
   return CompilerInvocation::GetResourcesPath("clangd", (void *)&Dummy);
 }
 
+template <class Ret, class... Args>
+std::future<Ret> makeFutureAPIFromCallback(
+    ClangdServer *Server,
+    void (ClangdServer::*CallbackFunPtr)(UniqueFunction<void(Ret)>, Args...),
+    Args... As) {
+  std::promise<Ret> ResultPromise;
+  std::future<Ret> ResultFuture = ResultPromise.get_future();
+
+  auto Callback = [](std::promise<Ret> ResultPromise, Ret Result) -> void {
+    ResultPromise.set_value(std::move(Result));
+  };
+
+  (Server->*CallbackFunPtr)(
+      BindWithForward(std::move(Callback), std::move(ResultPromise)),
+      std::forward<Args>(As)...);
+
+  return ResultFuture;
+}
+
 } // namespace
 
 size_t clangd::positionToOffset(StringRef Code, Position P) {
   size_t Offset = 0;
   for (int I = 0; I != P.line; ++I) {
     // FIXME: \r\n
     // FIXME: UTF-8
     size_t F = Code.find('\n', Offset);
return scheduleReparseAndDiags(File, std::move(FileContents),
                                  std::move(Recreated.FileInCollection),
                                  std::move(TaggedFS));
 }
 
 std::future<Tagged<std::vector<CompletionItem>>>
 ClangdServer::codeComplete(PathRef File, Position Pos,
                            llvm::Optional<StringRef> OverridenContents,
                            IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
+  return makeFutureAPIFromCallback(this, &ClangdServer::codeComplete, File, Pos,
+                                   OverridenContents, UsedFS);
+}
+
+void ClangdServer::codeComplete(
+    UniqueFunction<void(Tagged<std::vector<CompletionItem>>)> Callback,
+    PathRef File, Position Pos, llvm::Optional<StringRef> OverridenContents,
+    IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
+  using CallbackType =
+      UniqueFunction<void(Tagged<std::vector<CompletionItem>>)>;
+
   std::string Contents;
   if (OverridenContents) {
     Contents = *OverridenContents;
   } else {
     auto FileContents = DraftMgr.getDraft(File);
     assert(FileContents.Draft &&
            "codeComplete is called for non-added document");
 
     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");
 
-  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.
+  auto Task =
+      // 'mutable' to reassign Preamble variable.
+      [=](CallbackType Callback) mutable {
         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, Contents, Pos, TaggedFS.Value,
-        PCHs, SnippetCompletions, Logger);
-    return make_tagged(std::move(Result), std::move(TaggedFS.Tag));
-  });
+            Preamble ? &Preamble->Preamble : nullptr, Contents, Pos,
+            TaggedFS.Value, PCHs, SnippetCompletions, Logger);
+
+        Callback(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;
+  WorkScheduler.addToFront(std::move(Task), std::move(Callback));
 }
 
 Tagged<SignatureHelp>
 ClangdServer::signatureHelp(PathRef File, Position Pos,
                             llvm::Optional<StringRef> OverridenContents,
                             IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   std::string DraftStorage;
   if (!OverridenContents) {