[clangd] Add marshalling code for all request types

[kbobyrev]

Summary

Only FuzzyFindRequest is implemented via Marshaller even though other requests
also follow a similar pattern. Unify them under the marshalling umbrella and
make the server requests even more uniform to complement D84499.

[kadircet]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

69–80

nit: move anon namespace to the top

71

I would make this return an expected instead of an optional and print the error in the callers, instead of logging twice.

(I also think a similar problem is present with the existing fromProtobuf APIs too, they return None in case of failure while logging the error, and then callers again log the error. I am not sure if we plan to implement some error handling strategies but even if we did, it feels like Marshalling is the lowest layer that wouldn't know any other strategy to handle such errors, so it seems like always returning an error from the marshalling and letting the caller handle the error sounds like a safe bet. But no action needed in this patch just stating some ideas in case you find them useful :D)

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

308–309

ASSERT_TRUE to ensure we don't try to dereference in case of None.

[kbobyrev]

Address review comments.

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

71

Good point: we thought about error handling and discussed it with Sam on multiple occasions, we tested several strategies and I think converged to this. I don't think it's optimal, you are right, but this code is consistent with the rest of the file.

The problem here is that in some cases these should actually be Optionals (some fields may or may not be missing) and in some cases it makes sense to have Expecteds, which would leave a weird mixture of these things. But maybe carefully thinking about those cases would simplify the code.

How do you feel about leaving this as is and maybe carefully thinking about Optionals to Expected in the next patches?

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

308–309

Good point, I should do ASSERT_TRUE in most cases throughout the file.

[kbobyrev]

Add missing Limit & Filter for fromProtobuf(RefsRequest *)

[kbobyrev]

Fix formatting.

[kbobyrev]

Rebase on top of master.

[kadircet]

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

71

Good point: we thought about error handling and discussed it with Sam on multiple occasions, we tested several strategies and I think converged to this. I don't think it's optimal, you are right, but this code is consistent with the rest of the file.

i believe rest of the file doesn't handle such low level primitives, i.e. this one is specifically for extracting symbol ids, not for converting bigger proto messages into native structs (as done in the rest of the file)

The problem here is that in some cases these should actually be Optionals (some fields may or may not be missing) and in some cases it makes sense to have Expecteds, which would leave a weird mixture of these things. But maybe carefully thinking about those cases would simplify the code.

i agree that it might be the case in other places, I haven't really looked at them carefully and they are also submitted (hence they were in parentheses in my previous comment). But I think the situation specifically in here is a little bit different.
There is a more clear struct for indicating missing fields in here, an empty set of results(and actually you are using that already), and using None to indicate an error. I believe using an Error (through an Expected) is a lot clear to indicate that.

How do you feel about leaving this as is and maybe carefully thinking about Optionals to Expected in the next patches?

As I mentioned, I believe this one is different than other places, and at least my taste/reasoning says that this should clearly be an Expected rather than an Optional. But it is not a big issue for me totally up to you.

109

can you also add tests for these? (well actually it looks like there are no tests for LookupRequests too)

[kbobyrev]

Need to address the review comments.

[kbobyrev]

Resolve the remaining comments.

[kadircet]

mostly LG. sorry for lots of nits, only two important bits are changing the {1} to {0} in logs, wrapping symbolid generations in llvm::cantFail and making sure Deserialized is exactly the same thing as Request in tests. feel free to ignore the rest (I should've marked them with nit hopefully, but might've missed some)

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

104

nit: braces

clang-tools-extra/clangd/index/remote/server/Server.cpp

65

printing indices are zero-based so

s/{1}/{0}/g

(same for others)

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

277

you can wrap these in llvm::cantFail as the symbolid generation bit is constant, and isn't really something we are testing here

nit: also I would suggest a less random string to indicate that these are arbitrary like 0000...001 (the length still needs to match tho)

287

nit: instead of 2 use Request.IDs.size()

Moreover, SymbolID has an opreator== so in theory you could do an EXPECT_THAT(Serialized.ids(), ElementsAreArray(Request.IDs)) trick to make this check even nicer. But I am not sure about how nicely repeated proto fields plays with gtest version in llvm. So if it doesn't work nvm. (same for the other places with this pattern)

290

you should definitely be able to make sure IDs are exactly the same with Request.IDs

292

nit: I would put this one into a separate test

329

again I would suggest comparing to Requests.IDs.size(), and other relevant fields below instead of repeating some constants (which would be tedious to change in future)

333

again I would make sure IDs are same element-wise in here.

338

nit: again this could be a separate test

[kbobyrev]

Resolve nits.

[kadircet]

thanks for bearing with me, LGTM!

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

277

nit: you could inline these into insert calls now.

290

looks like you forgot to delete this

328

s/9000U/Request.Limit

333

s/9000U/Request.Limit

334

s/RefKind::Spelled | RefKind::Declaration/Request.Filter

336

again you forgot to delete this

[kbobyrev]

Address post-LGTM comments.

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h

 /// relative path and constructs a URI that points to the relevant file in the
 /// filesystem. The relative path is appended to LocalIndexRoot to construct the
 /// full path and build the final URI.
 class Marshaller {
 public:
   Marshaller() = delete;
   Marshaller(llvm::StringRef RemoteIndexRoot, llvm::StringRef LocalIndexRoot);
 
-  clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request);
   llvm::Optional<clangd::Symbol> fromProtobuf(const Symbol &Message);
   llvm::Optional<clangd::Ref> fromProtobuf(const Ref &Message);
 
+  llvm::Expected<clangd::LookupRequest>
+  fromProtobuf(const LookupRequest *Message);
+  llvm::Expected<clangd::FuzzyFindRequest>
+  fromProtobuf(const FuzzyFindRequest *Message);
+  llvm::Expected<clangd::RefsRequest> fromProtobuf(const RefsRequest *Message);
+
   /// toProtobuf() functions serialize native clangd types and strip IndexRoot
   /// from the file paths specific to indexing machine. fromProtobuf() functions
   /// deserialize clangd types and translate relative paths into machine-native
   /// URIs.
   LookupRequest toProtobuf(const clangd::LookupRequest &From);
   FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From);
   RefsRequest toProtobuf(const clangd::RefsRequest &From);
 

clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp

 #include "Protocol.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "index/SymbolLocation.h"
 #include "index/SymbolOrigin.h"
 #include "support/Logger.h"
 #include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace clangd {
 namespace remote {
 
+namespace {
+
+template <typename MessageT>
+llvm::Expected<llvm::DenseSet<SymbolID>> getIDs(MessageT *Message) {
+  llvm::DenseSet<SymbolID> Result;
+  for (const auto &ID : Message->ids()) {
+    auto SID = SymbolID::fromStr(StringRef(ID));
+    if (!SID)
+      return SID.takeError();
+    Result.insert(*SID);
+  }
+  return Result;
+}
+
+} // namespace
+
 Marshaller::Marshaller(llvm::StringRef RemoteIndexRoot,
                        llvm::StringRef LocalIndexRoot)
     : Strings(Arena) {
   if (!RemoteIndexRoot.empty()) {
     assert(llvm::sys::path::is_absolute(RemoteIndexRoot));
     assert(RemoteIndexRoot ==
            llvm::sys::path::convert_to_slash(RemoteIndexRoot));
     assert(RemoteIndexRoot.endswith(llvm::sys::path::get_separator()));
     this->RemoteIndexRoot = RemoteIndexRoot.str();
   }
   if (!LocalIndexRoot.empty()) {
     assert(llvm::sys::path::is_absolute(LocalIndexRoot));
     assert(LocalIndexRoot == llvm::sys::path::convert_to_slash(LocalIndexRoot));
     assert(LocalIndexRoot.endswith(llvm::sys::path::get_separator()));
     this->LocalIndexRoot = LocalIndexRoot.str();
   }
   assert(!RemoteIndexRoot.empty() || !LocalIndexRoot.empty());
 }
 
-clangd::FuzzyFindRequest
-Marshaller::fromProtobuf(const FuzzyFindRequest *Request) {
+llvm::Expected<clangd::LookupRequest>
+Marshaller::fromProtobuf(const LookupRequest *Message) {
+  clangd::LookupRequest Req;
+  auto IDs = getIDs(Message);
+  if (!IDs)
+    return IDs.takeError();
+  Req.IDs = std::move(*IDs);
+  return Req;
+}
+
+llvm::Expected<clangd::FuzzyFindRequest>
+Marshaller::fromProtobuf(const FuzzyFindRequest *Message) {
   assert(RemoteIndexRoot);
   clangd::FuzzyFindRequest Result;
-  Result.Query = Request->query();
-  for (const auto &Scope : Request->scopes())
+  Result.Query = Message->query();
+  for (const auto &Scope : Message->scopes())
     Result.Scopes.push_back(Scope);
-  Result.AnyScope = Request->any_scope();
-  if (Request->limit())
-    Result.Limit = Request->limit();
-  Result.RestrictForCodeCompletion = Request->restricted_for_code_completion();
-  for (const auto &Path : Request->proximity_paths()) {
+  Result.AnyScope = Message->any_scope();
+  if (Message->limit())
+    Result.Limit = Message->limit();
+  Result.RestrictForCodeCompletion = Message->restricted_for_code_completion();
+  for (const auto &Path : Message->proximity_paths()) {
     llvm::SmallString<256> LocalPath = llvm::StringRef(*RemoteIndexRoot);
     llvm::sys::path::append(LocalPath, Path);
     Result.ProximityPaths.push_back(std::string(LocalPath));
   }
-  for (const auto &Type : Request->preferred_types())
+  for (const auto &Type : Message->preferred_types())
     Result.ProximityPaths.push_back(Type);
   return Result;
 }
 
+llvm::Expected<clangd::RefsRequest>
+Marshaller::fromProtobuf(const RefsRequest *Message) {
+  clangd::RefsRequest Req;
+  auto IDs = getIDs(Message);
+  if (!IDs)
+    return IDs.takeError();
+  Req.IDs = std::move(*IDs);
+  Req.Filter = static_cast<RefKind>(Message->filter());
+  if (Message->limit())
+    Req.Limit = Message->limit();
+  return Req;
+}
+
 llvm::Optional<clangd::Symbol> Marshaller::fromProtobuf(const Symbol &Message) {
   if (!Message.has_info() || !Message.has_canonical_declaration()) {
     elog("Cannot convert Symbol from protobuf (missing info, definition or "
          "declaration): {0}",
          Message.DebugString());
     return llvm::None;
   }
   clangd::Symbol Result;
FuzzyFindRequest Marshaller::toProtobuf(const clangd::FuzzyFindRequest &From) {
   for (const auto &Scope : From.Scopes)
     RPCRequest.add_scopes(Scope);
   RPCRequest.set_any_scope(From.AnyScope);
   if (From.Limit)
     RPCRequest.set_limit(*From.Limit);
   RPCRequest.set_restricted_for_code_completion(From.RestrictForCodeCompletion);
   for (const auto &Path : From.ProximityPaths) {
     llvm::SmallString<256> RelativePath = llvm::StringRef(Path);
-    if (llvm::sys::path::replace_path_prefix(RelativePath, *LocalIndexRoot,
-                                             ""))
+    if (llvm::sys::path::replace_path_prefix(RelativePath, *LocalIndexRoot, ""))
       RPCRequest.add_proximity_paths(llvm::sys::path::convert_to_slash(
           RelativePath, llvm::sys::path::Style::posix));
   }
   for (const auto &Type : From.PreferredTypes)
     RPCRequest.add_preferred_types(Type);
   return RPCRequest;
 }
 

clang-tools-extra/clangd/index/remote/server/Server.cpp

 //===--- Server.cpp - gRPC-based Remote Index Server  ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/remote/marshalling/Marshalling.h"
+#include "support/Logger.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 
 #include <grpc++/grpc++.h>
 #include <grpc++/health_check_service_interface.h>
 
ProtobufMarshaller = std::unique_ptr<Marshaller>(new Marshaller(
         /*RemoteIndexRoot=*/llvm::StringRef(NativePath),
         /*LocalIndexRoot=*/""));
   }
 
 private:
   grpc::Status Lookup(grpc::ServerContext *Context,
                       const LookupRequest *Request,
                       grpc::ServerWriter<LookupReply> *Reply) override {
-    clangd::LookupRequest Req;
-    for (const auto &ID : Request->ids()) {
-      auto SID = SymbolID::fromStr(StringRef(ID));
-      if (!SID)
-        return grpc::Status::CANCELLED;
-      Req.IDs.insert(*SID);
+    auto Req = ProtobufMarshaller->fromProtobuf(Request);
+    if (!Req) {
+      elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
     }
-    Index->lookup(Req, [&](const clangd::Symbol &Sym) {
+    Index->lookup(*Req, [&](const clangd::Symbol &Sym) {
       auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
       if (!SerializedSymbol)
         return;
       LookupReply NextMessage;
       *NextMessage.mutable_stream_result() = *SerializedSymbol;
       Reply->Write(NextMessage);
     });
     LookupReply LastMessage;
     LastMessage.set_final_result(true);
     Reply->Write(LastMessage);
     return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
                          const FuzzyFindRequest *Request,
                          grpc::ServerWriter<FuzzyFindReply> *Reply) override {
-    const auto Req = ProtobufMarshaller->fromProtobuf(Request);
-    bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol &Sym) {
+    auto Req = ProtobufMarshaller->fromProtobuf(Request);
+    if (!Req) {
+      elog("Can not parse FuzzyFindRequest from protobuf: {0}",
+           Req.takeError());
+      return grpc::Status::CANCELLED;
+    }
+    bool HasMore = Index->fuzzyFind(*Req, [&](const clangd::Symbol &Sym) {
       auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym);
       if (!SerializedSymbol)
         return;
       FuzzyFindReply NextMessage;
       *NextMessage.mutable_stream_result() = *SerializedSymbol;
       Reply->Write(NextMessage);
     });
     FuzzyFindReply LastMessage;
     LastMessage.set_final_result(HasMore);
     Reply->Write(LastMessage);
     return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
                     grpc::ServerWriter<RefsReply> *Reply) override {
-    clangd::RefsRequest Req;
-    for (const auto &ID : Request->ids()) {
-      auto SID = SymbolID::fromStr(StringRef(ID));
-      if (!SID)
-        return grpc::Status::CANCELLED;
-      Req.IDs.insert(*SID);
+    auto Req = ProtobufMarshaller->fromProtobuf(Request);
+    if (!Req) {
+      elog("Can not parse RefsRequest from protobuf: {0}", Req.takeError());
     }
-    bool HasMore = Index->refs(Req, [&](const clangd::Ref &Reference) {
+    bool HasMore = Index->refs(*Req, [&](const clangd::Ref &Reference) {
       auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference);
       if (!SerializedRef)
         return;
       RefsReply NextMessage;
       *NextMessage.mutable_stream_result() = *SerializedRef;
       Reply->Write(NextMessage);
     });
     RefsReply LastMessage;

clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "index/SymbolLocation.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstring>
 
 namespace clang {
 namespace clangd {
EXPECT_EQ(static_cast<size_t>(Serialized->headers_size()),
             ValidHeaders.size());
   auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
   ASSERT_TRUE(Deserialized);
 
   Sym.IncludeHeaders = ValidHeaders;
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
 }
 
+TEST(RemoteMarshallingTest, LookupRequestSerialization) {
+  clangd::LookupRequest Request;
+  Request.IDs.insert(llvm::cantFail(SymbolID::fromStr("0000000000000001")));
+  Request.IDs.insert(llvm::cantFail(SymbolID::fromStr("0000000000000002")));
+
+  Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
+
+  auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+  EXPECT_EQ(static_cast<unsigned>(Serialized.ids_size()), Request.IDs.size());
+  auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
+  ASSERT_TRUE(bool(Deserialized));
+  EXPECT_EQ(Deserialized->IDs, Request.IDs);
+}
+
+TEST(RemoteMarshallingTest, LookupRequestFailingSerialization) {
+  clangd::LookupRequest Request;
+  Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
+  auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+  Serialized.add_ids("Invalid Symbol ID");
+  auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
+  EXPECT_FALSE(Deserialized);
+  llvm::consumeError(Deserialized.takeError());
+}
+
 TEST(RemoteMarshallingTest, FuzzyFindRequestSerialization) {
   clangd::FuzzyFindRequest Request;
   Request.ProximityPaths = {testPath("local/Header.h"),
                             testPath("local/subdir/OtherHeader.h"),
                             testPath("remote/File.h"), "Not a Path."};
   Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
   auto Serialized = ProtobufMarshaller.toProtobuf(Request);
   EXPECT_EQ(Serialized.proximity_paths_size(), 2);
   auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
-  EXPECT_THAT(Deserialized.ProximityPaths,
+  ASSERT_TRUE(bool(Deserialized));
+  EXPECT_THAT(Deserialized->ProximityPaths,
               testing::ElementsAre(testPath("remote/Header.h"),
                                    testPath("remote/subdir/OtherHeader.h")));
 }
 
+TEST(RemoteMarshallingTest, RefsRequestSerialization) {
+  clangd::RefsRequest Request;
+  Request.IDs.insert(llvm::cantFail(SymbolID::fromStr("0000000000000001")));
+  Request.IDs.insert(llvm::cantFail(SymbolID::fromStr("0000000000000002")));
+
+  Request.Limit = 9000;
+  Request.Filter = RefKind::Spelled | RefKind::Declaration;
+
+  Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
+
+  auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+  EXPECT_EQ(static_cast<unsigned>(Serialized.ids_size()), Request.IDs.size());
+  EXPECT_EQ(Serialized.limit(), Request.Limit);
+  auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
+  ASSERT_TRUE(bool(Deserialized));
+  EXPECT_EQ(Deserialized->IDs, Request.IDs);
+  ASSERT_TRUE(Deserialized->Limit);
+  EXPECT_EQ(*Deserialized->Limit, Request.Limit);
+  EXPECT_EQ(Deserialized->Filter, Request.Filter);
+}
+
+TEST(RemoteMarshallingTest, RefsRequestFailingSerialization) {
+  clangd::RefsRequest Request;
+  Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
+  auto Serialized = ProtobufMarshaller.toProtobuf(Request);
+  Serialized.add_ids("Invalid Symbol ID");
+  auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
+  EXPECT_FALSE(Deserialized);
+  llvm::consumeError(Deserialized.takeError());
+}
+
 TEST(RemoteMarshallingTest, RelativePathToURITranslation) {
   Marshaller ProtobufMarshaller(/*RemoteIndexRoot=*/"",
                                 /*LocalIndexRoot=*/testPath("home/project/"));
   EXPECT_TRUE(ProtobufMarshaller.relativePathToURI("lib/File.cpp"));
   // RelativePath can not be absolute.
   EXPECT_FALSE(ProtobufMarshaller.relativePathToURI("/lib/File.cpp"));
   // RelativePath can not be empty.
   EXPECT_FALSE(ProtobufMarshaller.relativePathToURI(std::string()));