[clangd] Remove codeComplete that returns std::future<>

[ilya-biryukov]

Summary

It was deprecated and callback version and is used everywhere.
Only changes to the testing code were needed.

[ioeric]

unittests/clangd/SyncAPI.h

8 ↗

(On Diff #133402)

Any reason not to put sync APIs in ClangdServer by their async versions, so that they are easier to find? I might be missing context. But if there is a good reason, please explain in the file doc.

[ilya-biryukov]

unittests/clangd/SyncAPI.h

8 ↗

(On Diff #133402)

That's a good question. I don't think we want anyone using the sync versions of the API. The tests are somewhat special, though, as they'll get really complicated if we move everything to callbacks and we don't want that.

This probably deserves a comment.

[ioeric]

unittests/clangd/SyncAPI.h

8 ↗

(On Diff #133402)

If these are expected to be used by tests only, I'd suggest moving the file to the test directory.

[ilya-biryukov]

unittests/clangd/SyncAPI.h

8 ↗

(On Diff #133402)

I think it's in the unittest directory already, so we're covered here :-)

[ioeric]

unittests/clangd/SyncAPI.h

8 ↗

(On Diff #133402)

Sorry! My bad!

[sammccall]

One down!

I'd like to know what you think about a generic "block the call and capture the result" mechanism rather than method-specific wrappers.
But if you're not convinced or just want to land this, I don't want to block the review.

unittests/clangd/SyncAPI.h

1 ↗

(On Diff #133402)

Being able to call synchronously is really nice for tests.
It's a bit unfortunate that to do this for each function we want to call synchronously (giving them a name, writing some boilerplate that repeats the args a few times).

It would be nice if we had a primitive we could compose.

Here's an idea that might work:

Tagged<CompletionList> CompletionResults;
Server.codeComplete(File, Pos, Opts, capture(CompletionResults), OverriddenContents);

capture() returns a callback that writes into CompletionResults. It also magically causes the call to block!

(brief pause for you to consider the API before I suggest a shameful implementation)

Actually capture() returns a proxy object that converts to UniqueFunction. The proxy object itself has a destructor that blocks on the UF being invoked. The proxy will be destroyed at the end of the full-expression, i.e. the line. e.g.

template <typename T> struct CaptureProxy {
  T &Target;
  std::promise<T> Promise;
  std::future<T> Future;
  CaptureProxy(T &Target) : Target(Target) {}
  operator UniqueFunction<void(T)>() {
    return BindWithForward([](T Value, std::promise<T> Promise) {
      Promise.set_value(std::move(Value));
    }, std::move(Promise));
  }
  ~CaptureProxy() { Target = Future.get(); }
};
template <typename T> CaptureProxy<T> capture(T &Target) {
  return CaptureProxy<T>(Target);
}

This is a little bit magic, but it's just for tests :-)

(One caveat - this needs the return value to be default-constructible. We could easily use Optional instead, but these are default-constructible in practice)

8 ↗

(On Diff #133402)

Yep, can you add a file comment describing why these exist?

[ilya-biryukov]

• Added CaptureProxy<T> helper, use it to implement runCodeComplete.
• Documented that we deliberately don't expose the sync API in tests.
• Rebased onto the latest head.

[ilya-biryukov]

unittests/clangd/SyncAPI.h

1 ↗

(On Diff #133402)

As discussed offline, I've opted for both:

• implementing the helper to allow reusing the future/promise code
• providing a function that actually returns the value synchronously.

This approach writing a small amount of wrapper code for each of the LSP methods in ClangdServer, but it, arguably, makes the test code nicer.

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

public:
   /// A version of `codeComplete` that runs \p Callback on the processing thread
   /// when codeComplete results become available.
   void codeComplete(PathRef File, Position Pos,
                     const clangd::CodeCompleteOptions &Opts,
                     UniqueFunction<void(Tagged<CompletionList>)> Callback,
                     llvm::Optional<StringRef> OverridenContents = llvm::None,
                     IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS = nullptr);
 
-  /// DEPRECATED: Please use the callback-based version of codeComplete.
-  std::future<Tagged<CompletionList>>
-  codeComplete(PathRef File, Position Pos,
-               const clangd::CodeCompleteOptions &Opts,
-               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.
   llvm::Expected<Tagged<SignatureHelp>>

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

std::future<void> ClangdServer::forceReparse(PathRef File) {
   // remove any cahced flags.
   CompileArgs.invalidate(File);
 
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);
   return scheduleReparseAndDiags(File, std::move(FileContents),
                                  std::move(TaggedFS));
 }
 
-std::future<Tagged<CompletionList>>
-ClangdServer::codeComplete(PathRef File, Position Pos,
-                           const clangd::CodeCompleteOptions &Opts,
-                           llvm::Optional<StringRef> OverridenContents,
-                           IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
-  std::promise<Tagged<CompletionList>> ResultPromise;
-  auto Callback = [](std::promise<Tagged<CompletionList>> ResultPromise,
-                     Tagged<CompletionList> Result) -> void {
-    ResultPromise.set_value(std::move(Result));
-  };
-
-  auto ResultFuture = ResultPromise.get_future();
-  codeComplete(File, Pos, Opts,
-               BindWithForward(Callback, std::move(ResultPromise)),
-               OverridenContents, UsedFS);
-  return ResultFuture;
-}
-
 void ClangdServer::codeComplete(
     PathRef File, Position Pos, const clangd::CodeCompleteOptions &Opts,
     UniqueFunction<void(Tagged<CompletionList>)> Callback,
     llvm::Optional<StringRef> OverridenContents,
     IntrusiveRefCntPtr<vfs::FileSystem> *UsedFS) {
   using CallbackType = UniqueFunction<void(Tagged<CompletionList>)>;
 
   auto TaggedFS = FSProvider.getTaggedFileSystem(File);

clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

add_extra_unittest(ClangdTests
   ClangdUnitTests.cpp
   CodeCompleteTests.cpp
   CodeCompletionStringsTests.cpp
   ContextTests.cpp
   FileIndexTests.cpp
   FuzzyMatchTests.cpp
   IndexTests.cpp
   JSONExprTests.cpp
-  URITests.cpp
+  SourceCodeTests.cpp
+  SymbolCollectorTests.cpp
+  SyncAPI.cpp
   TestFS.cpp
   ThreadingTests.cpp
   TraceTests.cpp
   TUSchedulerTests.cpp
-  SourceCodeTests.cpp
-  SymbolCollectorTests.cpp
+  URITests.cpp
   XRefsTests.cpp
   )
 
 target_link_libraries(ClangdTests
   PRIVATE
   clangBasic
   clangDaemon
   clangFormat
   clangFrontend
   clangIndex
   clangSema
   clangTooling
   clangToolingCore
   LLVMSupport
   )

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

 //===-- ClangdTests.cpp - Clangd unit tests ---------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "ClangdServer.h"
 #include "Matchers.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "clang/Config/config.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "gmock/gmock.h"
TEST_F(ClangdVFSTest, CheckVersions) {
 
   // 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}, CCOpts).get().Tag,
+  EXPECT_EQ(runCodeComplete(Server, FooCpp, Position{0, 0}, CCOpts).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}, CCOpts).get().Tag,
+  EXPECT_EQ(runCodeComplete(Server, FooCpp, Position{0, 0}, CCOpts).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;
TEST_F(ClangdVFSTest, InvalidCompileCommand) {
   Server.addDocument(FooCpp, "int main() {}");
 
   EXPECT_EQ(Server.dumpAST(FooCpp), "<no-ast>");
   EXPECT_ERROR(Server.findDefinitions(FooCpp, Position{0, 0}));
   EXPECT_ERROR(Server.findDocumentHighlights(FooCpp, Position{0, 0}));
   EXPECT_ERROR(Server.rename(FooCpp, Position{0, 0}, "new_name"));
   // FIXME: codeComplete and signatureHelp should also return errors when they
   // can't parse the file.
-  EXPECT_THAT(
-      Server.codeComplete(FooCpp, Position{0, 0}, clangd::CodeCompleteOptions())
-          .get()
+  EXPECT_THAT(runCodeComplete(Server, FooCpp, Position{0, 0},
+                              clangd::CodeCompleteOptions())
                   .Value.items,
               IsEmpty());
   auto SigHelp = Server.signatureHelp(FooCpp, Position{0, 0});
   ASSERT_TRUE(bool(SigHelp)) << "signatureHelp returned an error";
   EXPECT_THAT(SigHelp->Value.signatures, IsEmpty());
 }
 
 class ClangdThreadingTest : public ClangdVFSTest {};
 
 TEST_F(ClangdThreadingTest, StressTest) {
auto CodeCompletionRequest = [&]() {
 
       Position Pos{LineDist(RandGen), ColumnDist(RandGen)};
       // 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,
-                        clangd::CodeCompleteOptions())
-          .wait();
+      runCodeComplete(Server, FilePaths[FileIndex], Pos,
+                      clangd::CodeCompleteOptions());
     };
 
     auto FindDefinitionsRequest = [&]() {
       unsigned FileIndex = FileIndexDist(RandGen);
       // Make sure we don't violate the ClangdServer's contract.
       if (ReqStats[FileIndex].FileIsRemoved)
         AddDocument(FileIndex);
 

clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

 //===-- CodeCompleteTests.cpp -----------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "ClangdServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
 #include "Matchers.h"
 #include "Protocol.h"
 #include "SourceCode.h"
+#include "SyncAPI.h"
 #include "TestFS.h"
 #include "index/MemIndex.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 // Let GMock print completion items and signature help.
CompletionList completions(StringRef Text,
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true);
   auto File = getVirtualTestFilePath("foo.cpp");
   Annotations Test(Text);
   Server.addDocument(File, Test.code()).wait();
-  auto CompletionList =
-      Server.codeComplete(File, Test.point(), Opts).get().Value;
+  auto CompletionList = runCodeComplete(Server, File, Test.point(), Opts).Value;
   // Sanity-check that filterText is valid.
   EXPECT_THAT(CompletionList.items, Each(NameContainsFilter()));
   return CompletionList;
 }
 
 std::string replace(StringRef Haystack, StringRef Needle, StringRef Repl) {
   std::string Result;
   raw_string_ostream OS(Result);
TEST(CompletionTest, CheckContentsOverride) {
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
                       /*StorePreamblesInMemory=*/true);
   auto File = getVirtualTestFilePath("foo.cpp");
   Server.addDocument(File, "ignored text!");
 
   Annotations Example("int cbc; int b = ^;");
   auto Results =
-      Server
-          .codeComplete(File, Example.point(), clangd::CodeCompleteOptions(),
-                        StringRef(Example.code()))
-          .get()
+      runCodeComplete(Server, File, Example.point(),
+                      clangd::CodeCompleteOptions(), StringRef(Example.code()))
           .Value;
   EXPECT_THAT(Results.items, Contains(Named("cbc")));
 }
 
 TEST(CompletionTest, Priorities) {
   auto Internal = completions(R"cpp(
       class Foo {
         public: void pub();
Annotations Test(R"cpp(
       #include "bar.h"
       namespace ns { int local; }
       void f() { ns::^; }
       void f() { ns::preamble().$2^; }
   )cpp");
   Server.addDocument(File, Test.code()).wait();
   clangd::CodeCompleteOptions Opts = {};
 
-  auto WithoutIndex = Server.codeComplete(File, Test.point(), Opts).get().Value;
+  auto WithoutIndex = runCodeComplete(Server, File, Test.point(), Opts).Value;
   EXPECT_THAT(WithoutIndex.items,
               UnorderedElementsAre(Named("local"), Named("preamble")));
 
   auto I = memIndex({var("ns::index")});
   Opts.Index = I.get();
-  auto WithIndex = Server.codeComplete(File, Test.point(), Opts).get().Value;
+  auto WithIndex = runCodeComplete(Server, File, Test.point(), Opts).Value;
   EXPECT_THAT(WithIndex.items,
               UnorderedElementsAre(Named("local"), Named("index")));
   auto ClassFromPreamble =
-      Server.codeComplete(File, Test.point("2"), Opts).get().Value;
+      runCodeComplete(Server, File, Test.point("2"), Opts).Value;
   EXPECT_THAT(ClassFromPreamble.items, Contains(Named("member")));
 }
 
 TEST(CompletionTest, DynamicIndexMultiFile) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, DiagConsumer, FS, getDefaultAsyncThreadsCount(),
Annotations Test(R"cpp(
       class XXX {};
       /// Doooc
       void fooooo() {}
       }
       void f() { ns::^ }
   )cpp");
   Server.addDocument(File, Test.code()).wait();
 
-  auto Results = Server.codeComplete(File, Test.point(), {}).get().Value;
+  auto Results = runCodeComplete(Server, File, Test.point(), {}).Value;
   // "XYZ" and "foo" are not included in the file being completed but are still
   // visible through the index.
   EXPECT_THAT(Results.items, Has("XYZ", CompletionItemKind::Class));
   EXPECT_THAT(Results.items, Has("foo", CompletionItemKind::Function));
   EXPECT_THAT(Results.items, Has("XXX", CompletionItemKind::Class));
   EXPECT_THAT(Results.items, Contains(AllOf(Named("fooooo"), Filter("fooooo"),
                                             Kind(CompletionItemKind::Function),
                                             Doc("Doooc"), Detail("void"))));

clang-tools-extra/trunk/unittests/clangd/SyncAPI.h

+//===--- SyncAPI.h - Sync version of ClangdServer's API ----------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+// This file contains synchronous versions of ClangdServer's async API. We
+// deliberately don't expose the sync API outside tests to encourage using the
+// async versions in clangd code.
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H
+
+#include "ClangdServer.h"
+#include <future>
+
+namespace clang {
+namespace clangd {
+
+Tagged<CompletionList>
+runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
+                clangd::CodeCompleteOptions Opts,
+                llvm::Optional<StringRef> OverridenContents = llvm::None);
+} // namespace clangd
+} // namespace clang
+
+#endif

clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp

+//===--- SyncAPI.cpp - Sync version of ClangdServer's API --------*- C++-*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===---------------------------------------------------------------------===//
+#include "SyncAPI.h"
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// A helper that waits for async callbacks to fire and exposes their result in
+/// the output variable. Intended to be used in the following way:
+///    T Result;
+///    someAsyncFunc(Param1, Param2, /*Callback=*/capture(Result));
+template <typename T> struct CaptureProxy {
+  CaptureProxy(T &Target) : Target(&Target) {}
+
+  CaptureProxy(const CaptureProxy &) = delete;
+  CaptureProxy &operator=(const CaptureProxy &) = delete;
+  // We need move ctor to return a value from the 'capture' helper.
+  CaptureProxy(CaptureProxy &&Other) : Target(Other.Target) {
+    Other.Target = nullptr;
+  }
+  CaptureProxy &operator=(CaptureProxy &&) = delete;
+
+  operator UniqueFunction<void(T)>() && {
+    assert(!Future.valid() && "conversion to callback called multiple times");
+    Future = Promise.get_future();
+    return BindWithForward([](std::promise<T> Promise,
+                              T Value) { Promise.set_value(std::move(Value)); },
+                           std::move(Promise));
+  }
+
+  ~CaptureProxy() {
+    if (!Target)
+      return;
+    assert(Future.valid() && "conversion to callback was not called");
+    *Target = Future.get();
+  }
+
+private:
+  T *Target;
+  std::promise<T> Promise;
+  std::future<T> Future;
+};
+
+template <typename T> CaptureProxy<T> capture(T &Target) {
+  return CaptureProxy<T>(Target);
+}
+} // namespace
+
+Tagged<CompletionList>
+runCodeComplete(ClangdServer &Server, PathRef File, Position Pos,
+                clangd::CodeCompleteOptions Opts,
+                llvm::Optional<StringRef> OverridenContents) {
+  Tagged<CompletionList> Result;
+  Server.codeComplete(File, Pos, Opts, capture(Result), OverridenContents);
+  return Result;
+}
+
+} // namespace clangd
+} // namespace clang