[clangd-remote] Replace YAML serialization with proper Protobuf messages
authorKirill Bobyrev <kbobyrev@google.com>
Tue, 19 May 2020 14:59:04 +0000 (16:59 +0200)
committerKirill Bobyrev <kbobyrev@google.com>
Tue, 19 May 2020 15:07:38 +0000 (17:07 +0200)
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
clang-tools-extra/clangd/index/remote/Index.proto
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/TestTU.cpp
clang-tools-extra/clangd/unittests/TestTU.h
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp [new file with mode: 0644]

index 6d9cd6d..366a37a 100644 (file)
@@ -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());
index 0cd9738..f3af29d 100644 (file)
@@ -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;
+}
index 60a258a..e95132d 100644 (file)
@@ -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;
 }
 
index a1f6af6..b907dfe 100644 (file)
@@ -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 ()
index 6d9de48..824c4cc 100644 (file)
@@ -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);
index 415326e..2be294f 100644 (file)
@@ -70,6 +70,7 @@ struct TestTU {
   ParsedAST build() const;
   ParseInputs inputs() const;
   SymbolSlab headerSymbols() const;
+  RefSlab headerRefs() const;
   std::unique_ptr<SymbolIndex> index() const;
 };
 
diff --git a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
new file mode 100644 (file)
index 0000000..8e68cfb
--- /dev/null
@@ -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