[clangd-remote] Replace YAML serialization with proper Protobuf messages

[kbobyrev]

Summary

YAML serialization was used in the Proof of Concept for simplicity.
This patch replaces implements Protobuf (de) serialization of almost all
types that need to be transferred over the protocol.

[sammccall]

It's tedious, but we really should have tests for this now.

clang-tools-extra/clangd/index/remote/Index.proto

68

somewhere in this file there should be a hint that semantics of all fields matches those in the structs in Index/

70

sigh, I wish we'd managed to get rid of this struct by now :-(
Maybe we can still do it before the 11 release

[kbobyrev]

Add tests for Protobuf (de)serialization.

[kbobyrev]

Simplify test setup and rebase on top of master.

[sammccall]

clang-tools-extra/clangd/unittests/TestTU.h

1 ↗

(On Diff #264205)

This header provides a widely used abstraction and has a clear scope, please don't dump things here.

94 ↗

(On Diff #264205)

I don't think the API of this class has been thought through well enough to share.

Why do you need it? Can't you just use TestTU::headerSymbols() (if you need Refs and such as well, just copy its implementation - it's 2 lines long)

[kbobyrev]

Don't complicate SymbolCollectorTest, use TestTU.

[kbobyrev]

Inline HeaderCode and MainCode.

[kbobyrev]

Rebase on top of master.

[sammccall]

Just more stuff about simplifying the tests :-(

This is really just about converting between a struct and a proto, if we have to add indexing-related fixtures to shared libraries something seems to have gone a bit off the rails.
In the worst case we could just create a few symbols with fields populated by hand, it's probably easier to see what parts of the code are being covered by the test...

clang-tools-extra/clangd/unittests/TestTU.cpp

128 ↗

(On Diff #264633)

can you structure the tests to avoid doing this (untested) merge?

Why do we need the example symbols split over multiple files at all?
If we need such symbols can we just construct them manually instead?

clang-tools-extra/clangd/unittests/TestTU.h

73 ↗

(On Diff #264554)

please don't add these for a single test, we can do this inline in the test unless it becomes a common thing

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

2 ↗

(On Diff #264633)

Can we conditionally include these tests into the main ClangdTests, instead of setting up another set of targets?

18 ↗

(On Diff #264633)

I don't think including the same source files in multiple targets is a great path to go down.

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

20 ↗

(On Diff #264633)

Using inheritance to define test *cases* is a bit of a trap, and not particularly idiomatic.
Can we just write a function indexSampleData() that returns a SlabTuple, and call it from the tests?

62 ↗

(On Diff #264633)

similarly i'd consider just inlining the stringsaver/arena into the tests, seems easier to read

66 ↗

(On Diff #264633)

if you want to define the symbols indirectly like this, you probably also want to assert that there are more than 5 or something, to guard against the test fixture being broken

[kbobyrev]

Address some comments.

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

2 ↗

(On Diff #264633)

I can do that, although I saw that XPC has their own set of tests if I am correct, so I thought this should be OK.

Conditionally including tests into the main ClangdTests is not super easy because... CMake stuff. But I. can look into that.

[kbobyrev]

Move remote tests into all Clangd tests

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

2 ↗

(On Diff #264633)

Actually, nevermind, it looks pretty straightforward.

[kbobyrev]

Move a comment to the line it corresponds to.

[sammccall]

Thanks! Sorry about the hassle with the tests.

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

2 ↗

(On Diff #264633)

Yeah we have to pay the cost of *some* cmake stuff either way, this looks like a bit less work.

XPC... yeah, I wish there was less CMake stuff going on there. But in practice, the majority of maintenance in the two dirs has been disjoint sets of people, so it has its advantages to...

[kbobyrev]

In D79862#2044073, @sammccall wrote:

Thanks! Sorry about the hassle with the tests.

No worries, I think it looks cleaner and simpler which is important.

Thanks for the review!

clang-tools-extra/clangd/index/Serialization.h

 };
 // Serializes an index file.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const IndexFileOut &O);
 
 // Convert a single symbol to YAML, a nice debug representation.
 std::string toYAML(const Symbol &);
 std::string toYAML(const std::pair<SymbolID, ArrayRef<Ref>> &);
 std::string toYAML(const Relation &);
-std::string toYAML(const Ref &);
 
 // Deserialize a single symbol from YAML.
 llvm::Expected<clangd::Symbol> symbolFromYAML(StringRef YAML,
                                               llvm::UniqueStringSaver *Strings);
-llvm::Expected<clangd::Ref> refFromYAML(StringRef YAML,
-                                        llvm::UniqueStringSaver *Strings);
 
 // Build an in-memory static index from an index file.
 // The size should be relatively small, so data can be managed in memory.
 std::unique_ptr<SymbolIndex> loadIndex(llvm::StringRef Filename,
                                        bool UseDex = true);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/index/YAMLSerialization.cpp

std::string Buf;
     llvm::raw_string_ostream OS(Buf);
     llvm::yaml::Output Yout(OS);
     Relation Rel = R; // copy: Yout<< requires mutability.
     Yout << Rel;
   }
   return Buf;
 }
 
-std::string toYAML(const Ref &R) {
-  std::string Buf;
-  {
-    llvm::raw_string_ostream OS(Buf);
-    llvm::yaml::Output Yout(OS);
-    Ref Reference = R; // copy: Yout<< requires mutability.
-    Yout << Reference;
-  }
-  return Buf;
-}
-
 llvm::Expected<clangd::Symbol>
 symbolFromYAML(StringRef YAML, llvm::UniqueStringSaver *Strings) {
   clangd::Symbol Deserialized;
   llvm::yaml::Input YAMLInput(YAML, Strings);
   if (YAMLInput.error())
     return llvm::make_error<llvm::StringError>(
         llvm::formatv("Unable to deserialize Symbol from YAML: {0}", YAML),
         llvm::inconvertibleErrorCode());
   YAMLInput >> Deserialized;
   return Deserialized;
 }
 
-llvm::Expected<clangd::Ref> refFromYAML(StringRef YAML,
-                                        llvm::UniqueStringSaver *Strings) {
-  clangd::Ref Deserialized;
-  llvm::yaml::Input YAMLInput(YAML, Strings);
-  if (YAMLInput.error())
-    return llvm::make_error<llvm::StringError>(
-        llvm::formatv("Unable to deserialize Symbol from YAML: {0}", YAML),
-        llvm::inconvertibleErrorCode());
-  YAMLInput >> Deserialized;
-  return Deserialized;
-}
-
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/index/remote/Client.cpp

bool streamRPC(ClangdRequestT Request,
     ReplyT Reply;
     while (Reader->Read(&Reply)) {
       if (!Reply.has_stream_result()) {
         FinalResult = Reply.final_result();
         continue;
       }
       auto Sym = fromProtobuf(Reply.stream_result(), &Strings);
       if (!Sym)
-        elog("Received invalid {0}: {1}", ReplyT::descriptor()->name(),
-             Reply.stream_result().yaml_serialization());
+        elog("Received invalid {0}", ReplyT::descriptor()->name());
       Callback(*Sym);
     }
     SPAN_ATTACH(Tracer, "status", Reader->Finish().ok());
     return FinalResult;
   }
 
 public:
   IndexClient(std::shared_ptr<grpc::Channel> Channel)

clang-tools-extra/clangd/index/remote/Index.proto

 service SymbolIndex {
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
   rpc FuzzyFind(FuzzyFindRequest) returns (stream FuzzyFindReply) {}
 
   rpc Refs(RefsRequest) returns (stream RefsReply) {}
 }
 
-message LookupRequest { repeated string ids = 1; }
+message LookupRequest {
+  repeated string ids = 1;
+}
 
 // The response is a stream of symbol messages and the terminating message
 // indicating the end of stream.
 message LookupReply {
   oneof kind {
     Symbol stream_result = 1;
     bool final_result = 2;
   }
 }
 
 message FuzzyFindRequest {
   string query = 1;
   repeated string scopes = 2;
   bool any_scope = 3;
   uint32 limit = 4;
-  bool resricted_for_code_completion = 5;
+  bool restricted_for_code_completion = 5;
   repeated string proximity_paths = 6;
   repeated string preferred_types = 7;
 }
 
 // The response is a stream of symbol messages, and one terminating has_more
 // message.
 message FuzzyFindReply {
   oneof kind {
     Symbol stream_result = 1;
-    bool final_result = 2; // HasMore
+    bool final_result = 2;  // HasMore
   }
 }
 
 message RefsRequest {
   repeated string ids = 1;
   uint32 filter = 2;
   uint32 limit = 3;
 }
 
 // The response is a stream of reference messages, and one terminating has_more
 // message.
 message RefsReply {
   oneof kind {
     Ref stream_result = 1;
-    bool final_result = 2; // HasMore
+    bool final_result = 2;  // HasMore
   }
 }
 
-// FIXME(kirillbobyrev): Properly serialize symbols and refs instead of passing
-// YAML.
-message Ref { string yaml_serialization = 1; }
-message Symbol { string yaml_serialization = 1; }
+message Symbol {
+  string id = 1;
+  SymbolInfo info = 2;
+  string name = 3;
+  SymbolLocation definition = 4;
+  string scope = 5;
+  SymbolLocation canonical_declarattion = 6;
+  int32 references = 7;
+  uint32 origin = 8;
+  string signature = 9;
+  string template_specialization_args = 10;
+  string completion_snippet_suffix = 11;
+  string documentation = 12;
+  string return_type = 13;
+  string type = 14;
+  repeated HeaderWithReferences headers = 15;
+  uint32 flags = 16;
+}
+
+message Ref {
+  SymbolLocation location = 1;
+  uint32 kind = 2;
+}
+
+message SymbolInfo {
+  uint32 kind = 1;
+  uint32 subkind = 2;
+  uint32 language = 3;
+  uint32 properties = 4;
+}
+
+message SymbolLocation {
+  Position start = 1;
+  Position end = 2;
+  string file_uri = 3;
+}
+
+message Position {
+  uint32 line = 1;
+  uint32 column = 2;
+}
+
+message HeaderWithReferences {
+  string header = 1;
+  uint32 references = 2;
+}

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

 //===--- Marshalling.cpp -----------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Marshalling.h"
+#include "Index.pb.h"
+#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/SmallVector.h"
+#include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace clangd {
 namespace remote {
 
+namespace {
+
+clangd::SymbolLocation::Position fromProtobuf(const Position &Message) {
+  clangd::SymbolLocation::Position Result;
+  Result.setColumn(static_cast<uint32_t>(Message.column()));
+  Result.setLine(static_cast<uint32_t>(Message.line()));
+  return Result;
+}
+
+Position toProtobuf(const clangd::SymbolLocation::Position &Position) {
+  remote::Position Result;
+  Result.set_column(Position.column());
+  Result.set_line(Position.line());
+  return Result;
+}
+
+clang::index::SymbolInfo fromProtobuf(const SymbolInfo &Message) {
+  clang::index::SymbolInfo Result;
+  Result.Kind = static_cast<clang::index::SymbolKind>(Message.kind());
+  Result.SubKind = static_cast<clang::index::SymbolSubKind>(Message.subkind());
+  Result.Lang = static_cast<clang::index::SymbolLanguage>(Message.language());
+  Result.Properties =
+      static_cast<clang::index::SymbolPropertySet>(Message.properties());
+  return Result;
+}
+
+SymbolInfo toProtobuf(const clang::index::SymbolInfo &Info) {
+  SymbolInfo Result;
+  Result.set_kind(static_cast<uint32_t>(Info.Kind));
+  Result.set_subkind(static_cast<uint32_t>(Info.SubKind));
+  Result.set_language(static_cast<uint32_t>(Info.Lang));
+  Result.set_properties(static_cast<uint32_t>(Info.Properties));
+  return Result;
+}
+
+clangd::SymbolLocation fromProtobuf(const SymbolLocation &Message,
+                                    llvm::UniqueStringSaver *Strings) {
+  clangd::SymbolLocation Location;
+  Location.Start = fromProtobuf(Message.start());
+  Location.End = fromProtobuf(Message.end());
+  Location.FileURI = Strings->save(Message.file_uri()).begin();
+  return Location;
+}
+
+SymbolLocation toProtobuf(const clangd::SymbolLocation &Location) {
+  remote::SymbolLocation Result;
+  *Result.mutable_start() = toProtobuf(Location.Start);
+  *Result.mutable_end() = toProtobuf(Location.End);
+  *Result.mutable_file_uri() = Location.FileURI;
+  return Result;
+}
+
+HeaderWithReferences
+toProtobuf(const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader) {
+  HeaderWithReferences Result;
+  Result.set_header(IncludeHeader.IncludeHeader.str());
+  Result.set_references(IncludeHeader.References);
+  return Result;
+}
+
+clangd::Symbol::IncludeHeaderWithReferences
+fromProtobuf(const HeaderWithReferences &Message) {
+  return clangd::Symbol::IncludeHeaderWithReferences{Message.header(),
+                                                     Message.references()};
+}
+
+} // namespace
+
 clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request) {
   clangd::FuzzyFindRequest Result;
   Result.Query = Request->query();
   for (const auto &Scope : Request->scopes())
     Result.Scopes.push_back(Scope);
   Result.AnyScope = Request->any_scope();
   if (Request->limit())
     Result.Limit = Request->limit();
-  Result.RestrictForCodeCompletion = Request->resricted_for_code_completion();
+  Result.RestrictForCodeCompletion = Request->restricted_for_code_completion();
   for (const auto &Path : Request->proximity_paths())
     Result.ProximityPaths.push_back(Path);
   for (const auto &Type : Request->preferred_types())
     Result.ProximityPaths.push_back(Type);
   return Result;
 }
 
 llvm::Optional<clangd::Symbol> fromProtobuf(const Symbol &Message,
                                             llvm::UniqueStringSaver *Strings) {
-  auto Result = symbolFromYAML(Message.yaml_serialization(), Strings);
-  if (!Result) {
-    elog("Cannot convert Symbol from Protobuf: {}", Result.takeError());
+  if (!Message.has_info() || !Message.has_definition() ||
+      !Message.has_canonical_declarattion()) {
+    elog("Cannot convert Symbol from Protobuf: {}", Message.ShortDebugString());
+    return llvm::None;
+  }
+  clangd::Symbol Result;
+  auto ID = SymbolID::fromStr(Message.id());
+  if (!ID) {
+    elog("Cannot convert parse SymbolID {} from Protobuf: {}", ID.takeError(),
+         Message.ShortDebugString());
     return llvm::None;
   }
-  return *Result;
+  Result.ID = *ID;
+  Result.SymInfo = fromProtobuf(Message.info());
+  Result.Name = Message.name();
+  Result.Scope = Message.scope();
+  Result.Definition = fromProtobuf(Message.definition(), Strings);
+  Result.CanonicalDeclaration =
+      fromProtobuf(Message.canonical_declarattion(), Strings);
+  Result.References = Message.references();
+  Result.Origin = static_cast<clangd::SymbolOrigin>(Message.origin());
+  Result.Signature = Message.signature();
+  Result.TemplateSpecializationArgs = Message.template_specialization_args();
+  Result.CompletionSnippetSuffix = Message.completion_snippet_suffix();
+  Result.Documentation = Message.documentation();
+  Result.ReturnType = Message.return_type();
+  Result.Type = Message.type();
+  for (const auto &Header : Message.headers()) {
+    Result.IncludeHeaders.push_back(fromProtobuf(Header));
+  }
+  Result.Flags = static_cast<clangd::Symbol::SymbolFlag>(Message.flags());
+  return Result;
 }
+
 llvm::Optional<clangd::Ref> fromProtobuf(const Ref &Message,
                                          llvm::UniqueStringSaver *Strings) {
-  auto Result = refFromYAML(Message.yaml_serialization(), Strings);
-  if (!Result) {
-    elog("Cannot convert Ref from Protobuf: {}", Result.takeError());
+  if (!Message.has_location()) {
+    elog("Cannot convert Ref from Protobuf: {}", Message.ShortDebugString());
     return llvm::None;
   }
-  return *Result;
+  clangd::Ref Result;
+  Result.Location = fromProtobuf(Message.location(), Strings);
+  Result.Kind = static_cast<clangd::RefKind>(Message.kind());
+  return Result;
 }
 
 LookupRequest toProtobuf(const clangd::LookupRequest &From) {
   LookupRequest RPCRequest;
   for (const auto &SymbolID : From.IDs)
     RPCRequest.add_ids(SymbolID.str());
   return RPCRequest;
 }
 
 FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From) {
   FuzzyFindRequest RPCRequest;
   RPCRequest.set_query(From.Query);
   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_resricted_for_code_completion(From.RestrictForCodeCompletion);
+  RPCRequest.set_restricted_for_code_completion(From.RestrictForCodeCompletion);
   for (const auto &Path : From.ProximityPaths)
     RPCRequest.add_proximity_paths(Path);
   for (const auto &Type : From.PreferredTypes)
     RPCRequest.add_preferred_types(Type);
   return RPCRequest;
 }
 
 RefsRequest toProtobuf(const clangd::RefsRequest &From) {
   RefsRequest RPCRequest;
   for (const auto &ID : From.IDs)
     RPCRequest.add_ids(ID.str());
   RPCRequest.set_filter(static_cast<uint32_t>(From.Filter));
   if (From.Limit)
     RPCRequest.set_limit(*From.Limit);
   return RPCRequest;
 }
 
 Symbol toProtobuf(const clangd::Symbol &From) {
   Symbol Result;
-  Result.set_yaml_serialization(toYAML(From));
+  Result.set_id(From.ID.str());
+  *Result.mutable_info() = toProtobuf(From.SymInfo);
+  Result.set_name(From.Name.str());
+  *Result.mutable_definition() = toProtobuf(From.Definition);
+  Result.set_scope(From.Scope.str());
+  *Result.mutable_canonical_declarattion() =
+      toProtobuf(From.CanonicalDeclaration);
+  Result.set_references(From.References);
+  Result.set_origin(static_cast<uint32_t>(From.Origin));
+  Result.set_signature(From.Signature.str());
+  Result.set_template_specialization_args(
+      From.TemplateSpecializationArgs.str());
+  Result.set_completion_snippet_suffix(From.CompletionSnippetSuffix.str());
+  Result.set_documentation(From.Documentation.str());
+  Result.set_return_type(From.ReturnType.str());
+  Result.set_type(From.Type.str());
+  for (const auto &Header : From.IncludeHeaders) {
+    auto *NextHeader = Result.add_headers();
+    *NextHeader = toProtobuf(Header);
+  }
+  Result.set_flags(static_cast<uint32_t>(From.Flags));
   return Result;
 }
 
 Ref toProtobuf(const clangd::Ref &From) {
   Ref Result;
-  Result.set_yaml_serialization(toYAML(From));
+  Result.set_kind(static_cast<uint32_t>(From.Kind));
+  *Result.mutable_location() = toProtobuf(From.Location);
   return Result;
 }
 
 } // namespace remote
 } // namespace clangd
 } // namespace clang