diff --git a/clang-tools-extra/clangd/index/remote/Client.cpp b/clang-tools-extra/clangd/index/remote/Client.cpp --- a/clang-tools-extra/clangd/index/remote/Client.cpp +++ b/clang-tools-extra/clangd/index/remote/Client.cpp @@ -16,6 +16,7 @@ #include "support/Logger.h" #include "support/Trace.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" #include @@ -53,7 +54,9 @@ } auto Response = ProtobufMarshaller->fromProtobuf(Reply.stream_result()); if (!Response) { - elog("Received invalid {0}", ReplyT::descriptor()->name()); + elog("Received invalid {0}: {1}. Reason: {2}", + ReplyT::descriptor()->name(), Reply.stream_result().DebugString(), + Response.takeError()); ++FailedToParse; continue; } diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h @@ -38,10 +38,9 @@ Marshaller() = delete; Marshaller(llvm::StringRef RemoteIndexRoot, llvm::StringRef LocalIndexRoot); - // FIXME(kirillbobyrev): Switch from Optional to Expected. - llvm::Optional fromProtobuf(const Symbol &Message); - llvm::Optional fromProtobuf(const Ref &Message); - llvm::Optional> + llvm::Expected fromProtobuf(const Symbol &Message); + llvm::Expected fromProtobuf(const Ref &Message); + llvm::Expected> fromProtobuf(const Relation &Message); llvm::Expected @@ -61,9 +60,9 @@ RefsRequest toProtobuf(const clangd::RefsRequest &From); RelationsRequest toProtobuf(const clangd::RelationsRequest &From); - llvm::Optional toProtobuf(const clangd::Symbol &From); - llvm::Optional toProtobuf(const clangd::Ref &From); - llvm::Optional toProtobuf(const clangd::SymbolID &Subject, + llvm::Expected toProtobuf(const clangd::Symbol &From); + llvm::Expected toProtobuf(const clangd::Ref &From); + llvm::Expected toProtobuf(const clangd::SymbolID &Subject, const clangd::Symbol &Object); /// Translates \p RelativePath into the absolute path and builds URI for the @@ -72,23 +71,23 @@ /// provided by the client. /// /// The relative path passed over the wire has unix slashes. - llvm::Optional relativePathToURI(llvm::StringRef RelativePath); + llvm::Expected relativePathToURI(llvm::StringRef RelativePath); /// Translates a URI from the server's backing index to a relative path /// suitable to send over the wire to the client. - llvm::Optional uriToRelativePath(llvm::StringRef URI); + llvm::Expected uriToRelativePath(llvm::StringRef URI); private: clangd::SymbolLocation::Position fromProtobuf(const Position &Message); Position toProtobuf(const clangd::SymbolLocation::Position &Position); clang::index::SymbolInfo fromProtobuf(const SymbolInfo &Message); SymbolInfo toProtobuf(const clang::index::SymbolInfo &Info); - llvm::Optional + llvm::Expected fromProtobuf(const SymbolLocation &Message); - llvm::Optional + llvm::Expected toProtobuf(const clangd::SymbolLocation &Location); - llvm::Optional + llvm::Expected toProtobuf(const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader); - llvm::Optional + llvm::Expected fromProtobuf(const HeaderWithReferences &Message); /// RemoteIndexRoot and LocalIndexRoot are absolute paths to the project (on diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp @@ -19,8 +19,6 @@ #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" @@ -46,6 +44,11 @@ return Result; } +llvm::Error makeStringError(llvm::StringRef Message) { + return llvm::make_error(Message, + llvm::inconvertibleErrorCode()); +} + } // namespace Marshaller::Marshaller(llvm::StringRef RemoteIndexRoot, @@ -124,20 +127,13 @@ return Req; } -llvm::Optional 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; - } +llvm::Expected Marshaller::fromProtobuf(const Symbol &Message) { + if (!Message.has_info() || !Message.has_canonical_declaration()) + return makeStringError("Missing info or declaration."); clangd::Symbol Result; auto ID = SymbolID::fromStr(Message.id()); - if (!ID) { - elog("Cannot parse SymbolID {0} given protobuf: {1}", ID.takeError(), - Message.DebugString()); - return llvm::None; - } + if (!ID) + return ID.takeError(); Result.ID = *ID; Result.SymInfo = fromProtobuf(Message.info()); Result.Name = Message.name(); @@ -148,11 +144,8 @@ Result.Definition = *Definition; } auto Declaration = fromProtobuf(Message.canonical_declaration()); - if (!Declaration) { - elog("Cannot convert Symbol from protobuf (invalid declaration): {0}", - Message.DebugString()); - return llvm::None; - } + if (!Declaration) + return Declaration.takeError(); Result.CanonicalDeclaration = *Declaration; Result.References = Message.references(); Result.Origin = static_cast(Message.origin()); @@ -164,49 +157,36 @@ Result.Type = Message.type(); for (const auto &Header : Message.headers()) { auto SerializedHeader = fromProtobuf(Header); - if (SerializedHeader) - Result.IncludeHeaders.push_back(*SerializedHeader); - else - elog("Cannot convert HeaderWithIncludes from protobuf: {0}", - Header.DebugString()); + if (!SerializedHeader) + return SerializedHeader.takeError(); + Result.IncludeHeaders.push_back(*SerializedHeader); } Result.Flags = static_cast(Message.flags()); return Result; } -llvm::Optional Marshaller::fromProtobuf(const Ref &Message) { - if (!Message.has_location()) { - elog("Cannot convert Ref from protobuf (missing location): {0}", - Message.DebugString()); - return llvm::None; - } +llvm::Expected Marshaller::fromProtobuf(const Ref &Message) { + if (!Message.has_location()) + return makeStringError("Missing location."); clangd::Ref Result; auto Location = fromProtobuf(Message.location()); - if (!Location) { - elog("Cannot convert Ref from protobuf (invalid location): {0}", - Message.DebugString()); - return llvm::None; - } + if (!Location) + return Location.takeError(); Result.Location = *Location; Result.Kind = static_cast(Message.kind()); return Result; } -llvm::Optional> +llvm::Expected> Marshaller::fromProtobuf(const Relation &Message) { auto SubjectID = SymbolID::fromStr(Message.subject_id()); - if (!SubjectID) { - elog("Cannot convert Relation from protobuf (invalid Subject ID): {0}", - SubjectID.takeError()); - return llvm::None; - } - if (!Message.has_object()) { - elog("Cannot convert Relation from protobuf (missing Object): {0}"); - return llvm::None; - } + if (!SubjectID) + return SubjectID.takeError(); + if (!Message.has_object()) + return makeStringError("Missing Object."); auto Object = fromProtobuf(Message.object()); if (!Object) - return llvm::None; + return Object.takeError(); return std::make_pair(*SubjectID, *Object); } @@ -258,27 +238,21 @@ return RPCRequest; } -llvm::Optional Marshaller::toProtobuf(const clangd::Symbol &From) { +llvm::Expected Marshaller::toProtobuf(const clangd::Symbol &From) { Symbol Result; Result.set_id(From.ID.str()); *Result.mutable_info() = toProtobuf(From.SymInfo); Result.set_name(From.Name.str()); if (*From.Definition.FileURI) { auto Definition = toProtobuf(From.Definition); - if (!Definition) { - elog("Can not convert Symbol to protobuf (invalid definition) {0}: {1}", - From, From.Definition); - return llvm::None; - } + if (!Definition) + return Definition.takeError(); *Result.mutable_definition() = *Definition; } Result.set_scope(From.Scope.str()); auto Declaration = toProtobuf(From.CanonicalDeclaration); - if (!Declaration) { - elog("Can not convert Symbol to protobuf (invalid declaration) {0}: {1}", - From, From.CanonicalDeclaration); - return llvm::None; - } + if (!Declaration) + return Declaration.takeError(); *Result.mutable_canonical_declaration() = *Declaration; Result.set_references(From.References); Result.set_origin(static_cast(From.Origin)); @@ -291,11 +265,8 @@ Result.set_type(From.Type.str()); for (const auto &Header : From.IncludeHeaders) { auto Serialized = toProtobuf(Header); - if (!Serialized) { - elog("Can not convert IncludeHeaderWithReferences to protobuf: {0}", - Header.IncludeHeader); - continue; - } + if (!Serialized) + return Serialized.takeError(); auto *NextHeader = Result.add_headers(); *NextHeader = *Serialized; } @@ -303,64 +274,52 @@ return Result; } -llvm::Optional Marshaller::toProtobuf(const clangd::Ref &From) { +llvm::Expected Marshaller::toProtobuf(const clangd::Ref &From) { Ref Result; Result.set_kind(static_cast(From.Kind)); auto Location = toProtobuf(From.Location); - if (!Location) { - elog("Can not convert Reference to protobuf (invalid location) {0}: {1}", - From, From.Location); - return llvm::None; - } + if (!Location) + return Location.takeError(); *Result.mutable_location() = *Location; return Result; } -llvm::Optional Marshaller::toProtobuf(const clangd::SymbolID &Subject, +llvm::Expected Marshaller::toProtobuf(const clangd::SymbolID &Subject, const clangd::Symbol &Object) { Relation Result; *Result.mutable_subject_id() = Subject.str(); auto SerializedObject = toProtobuf(Object); - if (!SerializedObject) { - elog("Can not convert Relation to protobuf (invalid symbol): {0}", Object); - return llvm::None; - } + if (!SerializedObject) + return SerializedObject.takeError(); *Result.mutable_object() = *SerializedObject; return Result; } -llvm::Optional +llvm::Expected Marshaller::relativePathToURI(llvm::StringRef RelativePath) { assert(LocalIndexRoot); assert(RelativePath == llvm::sys::path::convert_to_slash( RelativePath, llvm::sys::path::Style::posix)); - if (RelativePath.empty()) { - return llvm::None; - } - if (llvm::sys::path::is_absolute(RelativePath)) { - return llvm::None; - } + if (RelativePath.empty()) + return makeStringError("Empty relative path."); + if (llvm::sys::path::is_absolute(RelativePath)) + return makeStringError("RelativePath is absolute."); llvm::SmallString<256> FullPath = llvm::StringRef(*LocalIndexRoot); llvm::sys::path::append(FullPath, RelativePath); auto Result = URI::createFile(FullPath); return Result.toString(); } -llvm::Optional Marshaller::uriToRelativePath(llvm::StringRef URI) { +llvm::Expected Marshaller::uriToRelativePath(llvm::StringRef URI) { assert(RemoteIndexRoot); auto ParsedURI = URI::parse(URI); - if (!ParsedURI) { - elog("Remote index got bad URI from client {0}: {1}", URI, - ParsedURI.takeError()); - return llvm::None; - } - if (ParsedURI->scheme() != "file") { - return llvm::None; - } + if (!ParsedURI) + return ParsedURI.takeError(); + if (ParsedURI->scheme() != "file") + return makeStringError("Can not use URI schemes other than file."); llvm::SmallString<256> Result = ParsedURI->body(); - if (!llvm::sys::path::replace_path_prefix(Result, *RemoteIndexRoot, "")) { - return llvm::None; - } + if (!llvm::sys::path::replace_path_prefix(Result, *RemoteIndexRoot, "")) + return makeStringError("File path doesn't start with RemoteIndexRoot."); // Make sure the result has UNIX slashes. return llvm::sys::path::convert_to_slash(Result, llvm::sys::path::Style::posix); @@ -401,31 +360,31 @@ return Result; } -llvm::Optional +llvm::Expected Marshaller::fromProtobuf(const SymbolLocation &Message) { clangd::SymbolLocation Location; auto URIString = relativePathToURI(Message.file_path()); if (!URIString) - return llvm::None; + return URIString.takeError(); Location.FileURI = Strings.save(*URIString).begin(); Location.Start = fromProtobuf(Message.start()); Location.End = fromProtobuf(Message.end()); return Location; } -llvm::Optional +llvm::Expected Marshaller::toProtobuf(const clangd::SymbolLocation &Location) { remote::SymbolLocation Result; auto RelativePath = uriToRelativePath(Location.FileURI); if (!RelativePath) - return llvm::None; + return RelativePath.takeError(); *Result.mutable_file_path() = *RelativePath; *Result.mutable_start() = toProtobuf(Location.Start); *Result.mutable_end() = toProtobuf(Location.End); return Result; } -llvm::Optional Marshaller::toProtobuf( +llvm::Expected Marshaller::toProtobuf( const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader) { HeaderWithReferences Result; Result.set_references(IncludeHeader.References); @@ -436,18 +395,18 @@ } auto RelativePath = uriToRelativePath(Header); if (!RelativePath) - return llvm::None; + return RelativePath.takeError(); Result.set_header(*RelativePath); return Result; } -llvm::Optional +llvm::Expected Marshaller::fromProtobuf(const HeaderWithReferences &Message) { std::string Header = Message.header(); - if (Header.front() != '<' && Header.front() != '"') { + if (!isLiteralInclude(Header)) { auto URIString = relativePathToURI(Header); if (!URIString) - return llvm::None; + return URIString.takeError(); Header = *URIString; } return clangd::Symbol::IncludeHeaderWithReferences{Strings.save(Header), diff --git a/clang-tools-extra/clangd/index/remote/server/Server.cpp b/clang-tools-extra/clangd/index/remote/server/Server.cpp --- a/clang-tools-extra/clangd/index/remote/server/Server.cpp +++ b/clang-tools-extra/clangd/index/remote/server/Server.cpp @@ -15,6 +15,7 @@ #include "support/Trace.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/CommandLine.h" +#include "llvm/Support/Error.h" #include "llvm/Support/Path.h" #include "llvm/Support/Signals.h" @@ -82,7 +83,7 @@ grpc::Status Lookup(grpc::ServerContext *Context, const LookupRequest *Request, grpc::ServerWriter *Reply) override { - trace::Span Tracer(LookupRequest::descriptor()->name()); + trace::Span Tracer("LookupRequest"); auto Req = ProtobufMarshaller->fromProtobuf(Request); if (!Req) { elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError()); @@ -93,6 +94,8 @@ Index->lookup(*Req, [&](const clangd::Symbol &Item) { auto SerializedItem = ProtobufMarshaller->toProtobuf(Item); if (!SerializedItem) { + elog("Unable to convert Symbol to protobuf: {0}", + SerializedItem.takeError()); ++FailedToSend; return; } @@ -112,7 +115,7 @@ grpc::Status FuzzyFind(grpc::ServerContext *Context, const FuzzyFindRequest *Request, grpc::ServerWriter *Reply) override { - trace::Span Tracer(FuzzyFindRequest::descriptor()->name()); + trace::Span Tracer("FuzzyFindRequest"); auto Req = ProtobufMarshaller->fromProtobuf(Request); if (!Req) { elog("Can not parse FuzzyFindRequest from protobuf: {0}", @@ -124,6 +127,8 @@ bool HasMore = Index->fuzzyFind(*Req, [&](const clangd::Symbol &Item) { auto SerializedItem = ProtobufMarshaller->toProtobuf(Item); if (!SerializedItem) { + elog("Unable to convert Symbol to protobuf: {0}", + SerializedItem.takeError()); ++FailedToSend; return; } @@ -142,7 +147,7 @@ grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request, grpc::ServerWriter *Reply) override { - trace::Span Tracer(RefsRequest::descriptor()->name()); + trace::Span Tracer("RefsRequest"); auto Req = ProtobufMarshaller->fromProtobuf(Request); if (!Req) { elog("Can not parse RefsRequest from protobuf: {0}", Req.takeError()); @@ -153,6 +158,8 @@ bool HasMore = Index->refs(*Req, [&](const clangd::Ref &Item) { auto SerializedItem = ProtobufMarshaller->toProtobuf(Item); if (!SerializedItem) { + elog("Unable to convert Ref to protobuf: {0}", + SerializedItem.takeError()); ++FailedToSend; return; } @@ -172,7 +179,7 @@ grpc::Status Relations(grpc::ServerContext *Context, const RelationsRequest *Request, grpc::ServerWriter *Reply) override { - trace::Span Tracer(RelationsRequest::descriptor()->name()); + trace::Span Tracer("RelationsRequest"); auto Req = ProtobufMarshaller->fromProtobuf(Request); if (!Req) { elog("Can not parse RelationsRequest from protobuf: {0}", @@ -185,6 +192,8 @@ *Req, [&](const SymbolID &Subject, const clangd::Symbol &Object) { auto SerializedItem = ProtobufMarshaller->toProtobuf(Subject, Object); if (!SerializedItem) { + elog("Unable to convert Relation to protobuf: {0}", + SerializedItem.takeError()); ++FailedToSend; return; } diff --git a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp --- a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp +++ b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "../TestTU.h" +#include "Index.pb.h" #include "TestFS.h" #include "index/Index.h" #include "index/Ref.h" @@ -97,11 +98,11 @@ "clangd/unittests/remote/MarshallingTests.cpp", Strings); auto Serialized = ProtobufMarshaller.toProtobuf(Original); - ASSERT_TRUE(Serialized); + ASSERT_TRUE(bool(Serialized)); EXPECT_EQ(Serialized->location().file_path(), "clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp"); auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); - ASSERT_TRUE(Deserialized); + ASSERT_TRUE(bool(Deserialized)); EXPECT_STREQ(Deserialized->Location.FileURI, testPathURI("home/my-projects/llvm-project/clang-tools-extra/" "clangd/unittests/remote/MarshallingTests.cpp", @@ -109,12 +110,16 @@ // Can't have empty paths. *Serialized->mutable_location()->mutable_file_path() = std::string(); - EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized)); + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + EXPECT_FALSE(bool(Deserialized)); + llvm::consumeError(Deserialized.takeError()); clangd::Ref WithInvalidURI; // Invalid URI results in serialization failure. WithInvalidURI.Location.FileURI = "This is not a URI"; - EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI)); + auto DeserializedRef = ProtobufMarshaller.toProtobuf(WithInvalidURI); + EXPECT_FALSE(bool(DeserializedRef)); + llvm::consumeError(DeserializedRef.takeError()); // Can not use URIs with scheme different from "file". auto UnittestURI = @@ -122,14 +127,18 @@ ASSERT_TRUE(bool(UnittestURI)); WithInvalidURI.Location.FileURI = Strings.save(UnittestURI->toString()).begin(); - EXPECT_FALSE(ProtobufMarshaller.toProtobuf(WithInvalidURI)); + auto DeserializedSymbol = ProtobufMarshaller.toProtobuf(WithInvalidURI); + EXPECT_FALSE(bool(DeserializedSymbol)); + llvm::consumeError(DeserializedSymbol.takeError()); // Paths transmitted over the wire can not be absolute, they have to be // relative. Ref WithAbsolutePath; *WithAbsolutePath.mutable_location()->mutable_file_path() = "/usr/local/user/home/HelloWorld.cpp"; - EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(WithAbsolutePath)); + Deserialized = ProtobufMarshaller.fromProtobuf(WithAbsolutePath); + EXPECT_FALSE(bool(Deserialized)); + llvm::consumeError(Deserialized.takeError()); } TEST(RemoteMarshallingTest, SymbolSerialization) { @@ -142,9 +151,9 @@ // Check that symbols are exactly the same if the path to indexed project is // the same on indexing machine and the client. auto Serialized = ProtobufMarshaller.toProtobuf(Sym); - ASSERT_TRUE(Serialized); + ASSERT_TRUE(bool(Serialized)); auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); - ASSERT_TRUE(Deserialized); + ASSERT_TRUE(bool(Deserialized)); EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized)); // Serialized paths are relative and have UNIX slashes. EXPECT_EQ(convert_to_slash(Serialized->definition().file_path(), @@ -156,36 +165,44 @@ // Missing definition is OK. Sym.Definition = clangd::SymbolLocation(); Serialized = ProtobufMarshaller.toProtobuf(Sym); - ASSERT_TRUE(Serialized); - EXPECT_TRUE(ProtobufMarshaller.fromProtobuf(*Serialized)); + ASSERT_TRUE(bool(Serialized)); + ASSERT_TRUE(bool(ProtobufMarshaller.fromProtobuf(*Serialized))); // Relative path is absolute. *Serialized->mutable_canonical_declaration()->mutable_file_path() = convert_to_slash("/path/to/Declaration.h"); - EXPECT_FALSE(ProtobufMarshaller.fromProtobuf(*Serialized)); + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + EXPECT_FALSE(bool(Deserialized)); + llvm::consumeError(Deserialized.takeError()); // Fail with an invalid URI. Sym.Definition.FileURI = "Not A URI"; - EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym)); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_FALSE(bool(Serialized)); + llvm::consumeError(Serialized.takeError()); // Schemes other than "file" can not be used. auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest"); ASSERT_TRUE(bool(UnittestURI)); Sym.Definition.FileURI = Strings.save(UnittestURI->toString()).begin(); - EXPECT_FALSE(ProtobufMarshaller.toProtobuf(Sym)); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_FALSE(bool(Serialized)); + llvm::consumeError(Serialized.takeError()); // Passing root that is not prefix of the original file path. Sym.Definition.FileURI = testPathURI("home/File.h", Strings); // Check that the symbol is valid and passing the correct path works. Serialized = ProtobufMarshaller.toProtobuf(Sym); - ASSERT_TRUE(Serialized); + ASSERT_TRUE(bool(Serialized)); Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); - ASSERT_TRUE(Deserialized); + ASSERT_TRUE(bool(Deserialized)); EXPECT_STREQ(Deserialized->Definition.FileURI, testPathURI("home/File.h", Strings)); // Fail with a wrong root. Marshaller WrongMarshaller(testPath("nothome/"), testPath("home/")); - EXPECT_FALSE(WrongMarshaller.toProtobuf(Sym)); + Serialized = WrongMarshaller.toProtobuf(Sym); + EXPECT_FALSE(Serialized); + llvm::consumeError(Serialized.takeError()); } TEST(RemoteMarshallingTest, RefSerialization) { @@ -208,9 +225,9 @@ testPath("llvm-project/")); auto Serialized = ProtobufMarshaller.toProtobuf(Ref); - ASSERT_TRUE(Serialized); + ASSERT_TRUE(bool(Serialized)); auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); - ASSERT_TRUE(Deserialized); + ASSERT_TRUE(bool(Deserialized)); EXPECT_EQ(toYAML(Ref), toYAML(*Deserialized)); } @@ -218,60 +235,60 @@ llvm::BumpPtrAllocator Arena; llvm::UniqueStringSaver Strings(Arena); - llvm::SmallVector - ValidHeaders; + clangd::Symbol Sym = createSymbol("remote/", Strings); + clangd::Symbol::IncludeHeaderWithReferences Header; + // Add only valid headers. Header.IncludeHeader = Strings.save( URI::createFile("/usr/local/user/home/project/Header.h").toString()); Header.References = 21; - ValidHeaders.push_back(Header); + Sym.IncludeHeaders.push_back(Header); Header.IncludeHeader = Strings.save(""); Header.References = 100; - ValidHeaders.push_back(Header); + Sym.IncludeHeaders.push_back(Header); Header.IncludeHeader = Strings.save("\"cstdio\""); Header.References = 200; - ValidHeaders.push_back(Header); + Sym.IncludeHeaders.push_back(Header); + + Marshaller ProtobufMarshaller(convert_to_slash("/"), convert_to_slash("/")); + + auto Serialized = ProtobufMarshaller.toProtobuf(Sym); + ASSERT_TRUE(bool(Serialized)); + EXPECT_EQ(static_cast(Serialized->headers_size()), + Sym.IncludeHeaders.size()); + auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + ASSERT_TRUE(bool(Deserialized)); + EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized)); - llvm::SmallVector - InvalidHeaders; // This is an absolute path to a header: can not be transmitted over the wire. Header.IncludeHeader = Strings.save(testPath("project/include/Common.h")); Header.References = 42; - InvalidHeaders.push_back(Header); + Sym.IncludeHeaders.push_back(Header); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_FALSE(bool(Serialized)); + llvm::consumeError(Serialized.takeError()); + + // Remove last invalid header. + Sym.IncludeHeaders.pop_back(); // This is not a valid header: can not be transmitted over the wire; Header.IncludeHeader = Strings.save("NotAHeader"); Header.References = 5; - InvalidHeaders.push_back(Header); - - clangd::Symbol Sym; - // Fill in definition and declaration, Symbool will be invalid otherwise. - clangd::SymbolLocation Location; - Location.Start.setLine(1); - Location.Start.setColumn(2); - Location.End.setLine(3); - Location.End.setColumn(4); - Location.FileURI = testPathURI("File.h", Strings); - Sym.Definition = Location; - Sym.CanonicalDeclaration = Location; - - // Try to serialize all headers but only valid ones will end up in Protobuf - // message. - auto AllHeaders = ValidHeaders; - AllHeaders.insert(AllHeaders.end(), InvalidHeaders.begin(), - InvalidHeaders.end()); - Sym.IncludeHeaders = AllHeaders; - - Marshaller ProtobufMarshaller(convert_to_slash("/"), convert_to_slash("/")); - - auto Serialized = ProtobufMarshaller.toProtobuf(Sym); - ASSERT_TRUE(Serialized); - EXPECT_EQ(static_cast(Serialized->headers_size()), - ValidHeaders.size()); - auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); - ASSERT_TRUE(Deserialized); + Sym.IncludeHeaders.push_back(Header); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_FALSE(bool(Serialized)); + llvm::consumeError(Serialized.takeError()); - Sym.IncludeHeaders = ValidHeaders; - EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized)); + // Try putting an invalid header into already serialized symbol. + Sym.IncludeHeaders.pop_back(); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + ASSERT_TRUE(bool(Serialized)); + HeaderWithReferences InvalidHeader; + InvalidHeader.set_header(convert_to_slash("/absolute/path/Header.h")); + InvalidHeader.set_references(9000); + *Serialized->add_headers() = InvalidHeader; + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + EXPECT_FALSE(bool(Deserialized)); + llvm::consumeError(Deserialized.takeError()); } TEST(RemoteMarshallingTest, LookupRequestSerialization) { @@ -294,7 +311,7 @@ auto Serialized = ProtobufMarshaller.toProtobuf(Request); Serialized.add_ids("Invalid Symbol ID"); auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized); - EXPECT_FALSE(Deserialized); + EXPECT_FALSE(bool(Deserialized)); llvm::consumeError(Deserialized.takeError()); } @@ -340,7 +357,7 @@ auto Serialized = ProtobufMarshaller.toProtobuf(Request); Serialized.add_ids("Invalid Symbol ID"); auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized); - EXPECT_FALSE(Deserialized); + EXPECT_FALSE(bool(Deserialized)); llvm::consumeError(Deserialized.takeError()); } @@ -375,7 +392,7 @@ Serialized.add_subjects("ZZZZZZZZZZZZZZZZ"); Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/")); auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized); - EXPECT_FALSE(Deserialized); + EXPECT_FALSE(bool(Deserialized)); llvm::consumeError(Deserialized.takeError()); } @@ -387,8 +404,9 @@ SymbolID ID = llvm::cantFail(SymbolID::fromStr("0000000000000002")); Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/")); auto Serialized = ProtobufMarshaller.toProtobuf(ID, Sym); + ASSERT_TRUE(bool(Serialized)); auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); - ASSERT_TRUE(Deserialized); + ASSERT_TRUE(bool(Deserialized)); EXPECT_THAT(Deserialized->first, ID); EXPECT_THAT(Deserialized->second.ID, Sym.ID); } @@ -396,11 +414,16 @@ TEST(RemoteMarshallingTest, RelativePathToURITranslation) { Marshaller ProtobufMarshaller(/*RemoteIndexRoot=*/"", /*LocalIndexRoot=*/testPath("home/project/")); - EXPECT_TRUE(ProtobufMarshaller.relativePathToURI("lib/File.cpp")); + auto URIString = ProtobufMarshaller.relativePathToURI("lib/File.cpp"); + ASSERT_TRUE(bool(URIString)); // RelativePath can not be absolute. - EXPECT_FALSE(ProtobufMarshaller.relativePathToURI("/lib/File.cpp")); + URIString = ProtobufMarshaller.relativePathToURI("/lib/File.cpp"); + EXPECT_FALSE(bool(URIString)); + llvm::consumeError(URIString.takeError()); // RelativePath can not be empty. - EXPECT_FALSE(ProtobufMarshaller.relativePathToURI(std::string())); + URIString = ProtobufMarshaller.relativePathToURI(std::string()); + EXPECT_FALSE(bool(URIString)); + llvm::consumeError(URIString.takeError()); } TEST(RemoteMarshallingTest, URIToRelativePathTranslation) { @@ -408,14 +431,17 @@ llvm::UniqueStringSaver Strings(Arena); Marshaller ProtobufMarshaller(/*RemoteIndexRoot=*/testPath("remote/project/"), /*LocalIndexRoot=*/""); - EXPECT_TRUE(ProtobufMarshaller.uriToRelativePath( - testPathURI("remote/project/lib/File.cpp", Strings))); + auto RelativePath = ProtobufMarshaller.uriToRelativePath( + testPathURI("remote/project/lib/File.cpp", Strings)); + ASSERT_TRUE(bool(RelativePath)); // 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))); + RelativePath = WrongMarshaller.uriToRelativePath( + testPathURI("remote/project/lib/File.cpp", Strings)); + EXPECT_FALSE(bool(RelativePath)); + llvm::consumeError(RelativePath.takeError()); } } // namespace