[clang-tools-extra] c6b2b78 - [clangd-remote] Replace YAML serialization with proper Protobuf messages
Kirill Bobyrev via cfe-commits
cfe-commits at lists.llvm.org
Tue May 19 08:07:56 PDT 2020
Author: Kirill Bobyrev
Date: 2020-05-19T17:07:38+02:00
New Revision: c6b2b784299b5b8c2081b4e7e76eeab80e9b81ee
URL: https://github.com/llvm/llvm-project/commit/c6b2b784299b5b8c2081b4e7e76eeab80e9b81ee
DIFF: https://github.com/llvm/llvm-project/commit/c6b2b784299b5b8c2081b4e7e76eeab80e9b81ee.diff
LOG: [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
Added:
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/index/remote/Client.cpp b/clang-tools-extra/clangd/index/remote/Client.cpp
index 6d9cd6d1f1d2..366a37a135a4 100644
--- a/clang-tools-extra/clangd/index/remote/Client.cpp
+++ b/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());
diff --git a/clang-tools-extra/clangd/index/remote/Index.proto b/clang-tools-extra/clangd/index/remote/Index.proto
index 0cd9738db58c..f3af29dcd588 100644
--- a/clang-tools-extra/clangd/index/remote/Index.proto
+++ b/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;
+}
diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
index 60a258a6db2e..e95132d000ec 100644
--- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
+++ b/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;
}
diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt
index a1f6af6747f4..b907dfe2c6f3 100644
--- a/clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ b/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 ()
diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp
index 6d9de48f23dc..824c4cc8ff14 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.cpp
+++ b/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);
diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h
index 415326e7d281..2be294f78e7e 100644
--- a/clang-tools-extra/clangd/unittests/TestTU.h
+++ b/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;
};
diff --git a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
new file mode 100644
index 000000000000..8e68cfbdbeee
--- /dev/null
+++ b/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
More information about the cfe-commits
mailing list