[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

53

nit: move anon namespace to the top

55

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

292

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

55

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

292

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

55

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.

111

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

106

nit: braces

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

63

printing indices are zero-based so

s/{1}/{0}/g

(same for others)

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

285

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)

295

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)

298

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

300

nit: I would put this one into a separate test

313

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)

317

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

322

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

285

nit: you could inline these into insert calls now.

298

looks like you forgot to delete this

312

s/9000U/Request.Limit

317

s/9000U/Request.Limit

318

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

320

again you forgot to delete this

[kbobyrev]

Address post-LGTM comments.

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

 //===--- Marshalling.h -------------------------------------------*- C++-*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H
 
 #include "Index.pb.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace clangd {
 namespace remote {
 

 /// 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::Optional<clangd::LookupRequest>
+  fromProtobuf(const LookupRequest *Message);
+  llvm::Optional<clangd::FuzzyFindRequest>
+  fromProtobuf(const FuzzyFindRequest *Message);
+  llvm::Optional<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"
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) {
+namespace {
+template <typename MessageT>
+llvm::Optional<llvm::DenseSet<SymbolID>> getIDs(MessageT *Message) {
+  llvm::DenseSet<SymbolID> Result;
+  for (const auto &ID : Message->ids()) {
+    auto SID = SymbolID::fromStr(StringRef(ID));
+    if (!SID) {
+      elog("Invalid SymbolID {1}", SID.takeError());
+      return llvm::None;
+    }
+    Result.insert(*SID);
+  }
+  return Result;
+}
+} // namespace
+
+llvm::Optional<clangd::LookupRequest>
+Marshaller::fromProtobuf(const LookupRequest *Message) {
+  clangd::LookupRequest Req;
+  auto IDs = getIDs(Message);
+  if (!IDs) {
+    elog("Cannot parse LookupRequest from protobuf: invalid Symbol IDs");
+    return llvm::None;
+  }
+  Req.IDs = std::move(*IDs);
+  return Req;
+}
+
+llvm::Optional<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::Optional<clangd::RefsRequest>
+Marshaller::fromProtobuf(const RefsRequest *Message) {
+  clangd::RefsRequest Req;
+  auto IDs = getIDs(Message);
+  if (!IDs) {
+    elog("Cannot parse RefsRequest from protobuf: invalid Symbol IDs");
+    return llvm::None;
+  }
+  Req.IDs = std::move(*IDs);
+  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;
 }
 
if (Header.front() != '<' && Header.front() != '"') {
     if (!URIString)
       return llvm::None;
     Header = *URIString;
   }
   return clangd::Symbol::IncludeHeaderWithReferences{Strings.save(Header),
                                                      Message.references()};
 }
 
 } // namespace remote
 } // namespace clangd
 } // namespace clang

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 "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 
 #include <grpc++/grpc++.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);
-    }
-    Index->lookup(Req, [&](const clangd::Symbol &Sym) {
+    const auto Req = ProtobufMarshaller->fromProtobuf(Request);
+    if (!Req)
+    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) {
+    if (!Req)
+      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);
-    }
-    bool HasMore = Index->refs(Req, [&](const clangd::Ref &Reference) {
+    const auto Req = ProtobufMarshaller->fromProtobuf(Request);
+    if (!Req)
+    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

TEST(RemoteMarshallingTest, IncludeHeaderURIs) {
   EXPECT_TRUE(Deserialized);
 
   Sym.IncludeHeaders = ValidHeaders;
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
 }
 
 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,
+  EXPECT_TRUE(Deserialized);
+  EXPECT_THAT(Deserialized->ProximityPaths,
               testing::ElementsAre(testPath("remote/Header.h"),
                                    testPath("remote/subdir/OtherHeader.h")));
 }
 
 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()));
 }
 
 TEST(RemoteMarshallingTest, URIToRelativePathTranslation) {
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
   Marshaller ProtobufMarshaller(/*RemoteIndexRoot=*/testPath("remote/project/"),
                                 /*LocalIndexRoot=*/"");
   EXPECT_TRUE(ProtobufMarshaller.uriToRelativePath(
       testPathURI("remote/project/lib/File.cpp", Strings)));
   // RemoteIndexRoot has to be be a prefix of the file path.
   Marshaller WrongMarshaller(
       /*RemoteIndexRoot=*/testPath("remote/other/project/"),
       /*LocalIndexRoot=*/"");
   EXPECT_FALSE(WrongMarshaller.uriToRelativePath(
       testPathURI("remote/project/lib/File.cpp", Strings)));
 }
 
 } // namespace
 } // namespace remote
 } // namespace clangd
 } // namespace clang