[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 ↗

(On Diff #118225)

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

257 ↗

(On Diff #118225)

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

261 ↗

(On Diff #118225)

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 ↗

(On Diff #118225)

Removed the template, inlined its usage.

257 ↗

(On Diff #118225)

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

261 ↗

(On Diff #118225)

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).

clang-tools-extra/trunk/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);
 
+  /// DEPRECATED. Please use a callback-based version, this API is deprecated
+  /// and will soon be removed.
+  ///
   /// 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);
 
+  /// 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>

clang-tools-extra/trunk/clangd/ClangdServer.cpp

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) {
+  using ResultType = Tagged<std::vector<CompletionItem>>;
+
+  std::promise<ResultType> ResultPromise;
+
+  auto Callback = [](std::promise<ResultType> ResultPromise,
+                     ResultType Result) -> void {
+    ResultPromise.set_value(std::move(Result));
+  };
+
+  std::future<ResultType> ResultFuture = ResultPromise.get_future();
+  codeComplete(BindWithForward(Callback, std::move(ResultPromise)), File, Pos,
+               OverridenContents, UsedFS);
+  return ResultFuture;
+}
+
+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, CodeCompleteOpts, Logger);
-    return make_tagged(std::move(Result), std::move(TaggedFS.Tag));
-  });
+            Preamble ? &Preamble->Preamble : nullptr, Contents, Pos,
+            TaggedFS.Value, PCHs, CodeCompleteOpts, 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) {