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

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.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D79862

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

@@ -46,8 +46,7 @@ class IndexClient : public clangd::SymbolIndex {
       }
       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());

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

@@ -10,6 +10,8 @@ syntax = "proto3";
 
 package clang.clangd.remote;
 
+// Semantics of SymbolIndex match clangd::SymbolIndex with all required
+// structures corresponding to their clangd::* counterparts.
 service SymbolIndex {
   rpc Lookup(LookupRequest) returns (stream LookupReply) {}
 
@@ -34,7 +36,7 @@ message FuzzyFindRequest {
   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;
 }
@@ -63,7 +65,49 @@ message RefsReply {
   }
 }
 
-// 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

@@ -7,13 +7,90 @@
 //===----------------------------------------------------------------------===//
 
 #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();
@@ -22,7 +99,7 @@ clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request) {
   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())
@@ -32,21 +109,50 @@ clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request) {
 
 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;
   }
-  return *Result;
+  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;
+  }
+  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) {
@@ -64,7 +170,7 @@ FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From) {
   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)
@@ -84,13 +190,34 @@ RefsRequest toProtobuf(const clangd::RefsRequest &From) {
 
 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;
 }
 

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

@@ -22,6 +22,12 @@ if(CLANG_BUILT_STANDALONE)
   endif()
 endif()
 
+if (CLANGD_ENABLE_REMOTE)
+  include_directories(${CMAKE_CURRENT_BINARY_DIR}/../index/remote)
+  add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1)
+  set(REMOTE_TEST_SOURCES remote/MarshallingTests.cpp)
+endif()
+
 add_custom_target(ClangdUnitTests)
 add_unittest(ClangdUnitTests ClangdTests
   Annotations.cpp
@@ -87,6 +93,8 @@ add_unittest(ClangdUnitTests ClangdTests
   support/TestTracer.cpp
   support/TraceTests.cpp
 
+  ${REMOTE_TEST_SOURCES}
+
   $<TARGET_OBJECTS:obj.clangDaemonTweaks>
   )
 
@@ -116,6 +124,12 @@ target_link_libraries(ClangdTests
   LLVMTestingSupport
   )
 
+if (CLANGD_ENABLE_REMOTE)
+  target_link_libraries(ClangdTests
+    PRIVATE
+    clangdRemoteMarshalling)
+endif()
+
 if (CLANGD_BUILD_XPC)
   add_subdirectory(xpc)
 endif ()

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

@@ -114,6 +114,11 @@ SymbolSlab TestTU::headerSymbols() const {
                                         AST.getCanonicalIncludes()));
 }
 
+RefSlab TestTU::headerRefs() const {
+  auto AST = build();
+  return std::get<1>(indexMainDecls(AST));
+}
+
 std::unique_ptr<SymbolIndex> TestTU::index() const {
   auto AST = build();
   auto Idx = std::make_unique<FileIndex>(/*UseDex=*/true);

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

@@ -70,6 +70,7 @@ struct TestTU {
   ParsedAST build() const;
   ParseInputs inputs() const;
   SymbolSlab headerSymbols() const;
+  RefSlab headerRefs() const;
   std::unique_ptr<SymbolIndex> index() const;
 };
 

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

@@ -0,0 +1,93 @@
+//===--- MarshallingTests.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 "../TestTU.h"
+#include "index/Serialization.h"
+#include "index/remote/marshalling/Marshalling.h"
+#include "llvm/Support/StringSaver.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace remote {
+namespace {
+
+TEST(RemoteMarshallingTest, SymbolSerialization) {
+  const auto *Header = R"(
+  // This is a class.
+  class Foo {
+  public:
+    Foo();
+
+    int Bar;
+  private:
+    double Number;
+  };
+  /// This is a function.
+  char baz();
+  template <typename T>
+  T getT ();
+  )";
+  const auto TU = TestTU::withHeaderCode(Header);
+  const auto Symbols = TU.headerSymbols();
+  // Sanity check: there are more than 5 symbols available.
+  EXPECT_GE(Symbols.size(), 5UL);
+  llvm::BumpPtrAllocator Arena;
+  llvm::UniqueStringSaver Strings(Arena);
+  for (auto &Sym : Symbols) {
+    const auto ProtobufMeessage = toProtobuf(Sym);
+    const auto SymToProtobufAndBack = fromProtobuf(ProtobufMeessage, &Strings);
+    EXPECT_TRUE(SymToProtobufAndBack.hasValue());
+    EXPECT_EQ(toYAML(Sym), toYAML(*SymToProtobufAndBack));
+  }
+}
+
+TEST(RemoteMarshallingTest, ReferenceSerialization) {
+  TestTU TU;
+  TU.HeaderCode = R"(
+  int foo();
+  int GlobalVariable = 42;
+  class Foo {
+  public:
+    Foo();
+
+    char Symbol = 'S';
+  };
+  template <typename T>
+  T getT() { return T(); }
+  )";
+  TU.Code = R"(
+  int foo() {
+    ++GlobalVariable;
+
+    Foo foo = Foo();
+    if (foo.Symbol - 'a' == 42) {
+      foo.Symbol = 'b';
+    }
+
+    const auto bar = getT<Foo>();
+  }
+  )";
+  const auto References = TU.headerRefs();
+  llvm::BumpPtrAllocator Arena;
+  llvm::UniqueStringSaver Strings(Arena);
+  // Sanity check: there are more than 5 references available.
+  EXPECT_GE(References.numRefs(), 5UL);
+  for (const auto &SymbolWithRefs : References) {
+    for (const auto &Ref : SymbolWithRefs.second) {
+      const auto RefToProtobufAndBack = fromProtobuf(toProtobuf(Ref), &Strings);
+      EXPECT_TRUE(RefToProtobufAndBack.hasValue());
+      EXPECT_EQ(toYAML(Ref), toYAML(*RefToProtobufAndBack));
+    }
+  }
+} // namespace
+
+} // namespace
+} // namespace remote
+} // namespace clangd
+} // namespace clang