[clangd] Propagate remote index errors via Expected

[kbobyrev]

Summary

This is a refactoring: errors should be logged only on the highest level.
Switch from Optional to Expected in the serialization code.

[kbobyrev]

Client: print debug string for the stream_result, not the ReplyT.

[kadircet]

i think you might want to have an helper for creating stringerrors and use it in all places

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

60

elog already consumes the error

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

132

error mentions definition too but conditional only looks for info and declaration?

159–160

nit: swap branches and early exit, i.e.

if(!SerializedHeader) {
 elog...
 continue;
}
push_back;

160–162

what makes this a non-terminal error?

182–183

nit: redundant braces

267–268

again, what makes this non-terminal?

270

elog already consumes the error

303–306

is RelativePath user provided? can't we just assert on non-emptiness and non-absoluteness and make this function return a std::string instead?

318

again why not make this (and the following) an assertion?

clang-tools-extra/clangd/index/remote/server/Server.cpp

89

nit: I would keep LookupRequest instead of going through descriptor()->name() (maybe even change the one in tracer), makes it more explicit especially for the people not experienced with protos.

94

this still has auto in it? (and other callbacks too)

99

again, elog consumes

123

same for usage of descriptor()->name()

132

again, elog consumes

[kbobyrev]

In D84939#2184836, @kadircet wrote:

i think you might want to have an helper for creating stringerrors and use it in all places

Haha, yeah I thought about that but then looked at other examples and saw that in some cases there are even more verbose calls :)

[kbobyrev]

Resolve most review comments.

[kbobyrev]

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

303–306

Could you elaborate on what you mean by user provided? Relative path is something that comes from the remote index. Ideally, this is non-empty and relative but I think the safer option is to maybe check for errors? Actually, I'm not sure about this but it's not user provided as in "given as a flag passed to CLI invocation by the end user".

I think the point here is making errors recoverable and not shutting down the client, otherwise everything could be an assertion? Same in other places.

WDYT?

[kadircet]

thanks, lgtm!

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

303–306

Could you elaborate on what you mean by user provided? Relative path is something that comes from the remote index.

Yes I was asking whether this comes from outside and ideally not processed at all before arriving at this logic.

Ideally, this is non-empty and relative but I think the safer option is to maybe check for errors? Actually, I'm not sure about this but it's not user provided as in "given as a flag passed to CLI invocation by the end user".

Right, the safer is always to check for errors, but if we've already processed the RelativePath before it reaches this code path then there wouldn't be much value in spending more runtime checks (and also it would be callers responsibility generate the errors in such cases). I was asking this because I wasn't aware of the whole structure, but looks like this is literally the entry point (i.e. the first logic that verifies a user input) for path to uri conversions. So no action needed here.

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

169

i would ASSERT_TRUE here, as you'll end up crashing when fromProtoBuf returns an error (because you are not consuming the error and it is destroyed after completion of the statement).

418

again should be ASSERT_TRUE for similar concerns stated above.

436

again should be ASSERT_TRUE for similar concerns stated above.

[kbobyrev]

Good points, thanks!

Addressed post-LGTM comments.

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

303–306

Yes, I agree that spending runtime on this is unfortunate but I guess it's the cost we'd have to pay :(

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

 #include "Client.h"
 #include "Index.grpc.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "marshalling/Marshalling.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 
 #include <chrono>
 
 namespace clang {
 namespace clangd {
 namespace remote {
 namespace {
 
bool streamRPC(ClangdRequestT Request,
     unsigned FailedToParse = 0;
     while (Reader->Read(&Reply)) {
       if (!Reply.has_stream_result()) {
         FinalResult = Reply.final_result();
         continue;
       }
       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;
       }
       Callback(*Response);
       ++Successful;
     }
     SPAN_ATTACH(Tracer, "Status", Reader->Finish().ok());
     SPAN_ATTACH(Tracer, "Successful", Successful);
     SPAN_ATTACH(Tracer, "Failed to parse", FailedToParse);

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

 /// relative path and constructs a URI that points to the relevant file in the
 /// filesystem. The relative path is appended to LocalIndexRoot to construct the
 /// full path and build the final URI.
 class Marshaller {
 public:
   Marshaller() = delete;
   Marshaller(llvm::StringRef RemoteIndexRoot, llvm::StringRef LocalIndexRoot);
 
-  // FIXME(kirillbobyrev): Switch from Optional to Expected.
-  llvm::Optional<clangd::Symbol> fromProtobuf(const Symbol &Message);
-  llvm::Optional<clangd::Ref> fromProtobuf(const Ref &Message);
-  llvm::Optional<std::pair<clangd::SymbolID, clangd::Symbol>>
+  llvm::Expected<clangd::Symbol> fromProtobuf(const Symbol &Message);
+  llvm::Expected<clangd::Ref> fromProtobuf(const Ref &Message);
+  llvm::Expected<std::pair<clangd::SymbolID, clangd::Symbol>>
   fromProtobuf(const Relation &Message);
 
   llvm::Expected<clangd::LookupRequest>
   fromProtobuf(const LookupRequest *Message);
   llvm::Expected<clangd::FuzzyFindRequest>
   fromProtobuf(const FuzzyFindRequest *Message);
   llvm::Expected<clangd::RefsRequest> fromProtobuf(const RefsRequest *Message);
   llvm::Expected<clangd::RelationsRequest>
   fromProtobuf(const RelationsRequest *Message);
 
   /// toProtobuf() functions serialize native clangd types and strip IndexRoot
   /// from the file paths specific to indexing machine. fromProtobuf() functions
   /// deserialize clangd types and translate relative paths into machine-native
   /// URIs.
   LookupRequest toProtobuf(const clangd::LookupRequest &From);
   FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From);
   RefsRequest toProtobuf(const clangd::RefsRequest &From);
   RelationsRequest toProtobuf(const clangd::RelationsRequest &From);
 
-  llvm::Optional<Symbol> toProtobuf(const clangd::Symbol &From);
-  llvm::Optional<Ref> toProtobuf(const clangd::Ref &From);
-  llvm::Optional<Relation> toProtobuf(const clangd::SymbolID &Subject,
+  llvm::Expected<Symbol> toProtobuf(const clangd::Symbol &From);
+  llvm::Expected<Ref> toProtobuf(const clangd::Ref &From);
+  llvm::Expected<Relation> toProtobuf(const clangd::SymbolID &Subject,
                                       const clangd::Symbol &Object);
 
   /// Translates \p RelativePath into the absolute path and builds URI for the
   /// user machine. This translation happens on the client side with the
   /// \p RelativePath received from remote index server and \p IndexRoot is
   /// provided by the client.
   ///
   /// The relative path passed over the wire has unix slashes.
-  llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath);
+  llvm::Expected<std::string> 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<std::string> uriToRelativePath(llvm::StringRef URI);
+  llvm::Expected<std::string> 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<clangd::SymbolLocation>
+  llvm::Expected<clangd::SymbolLocation>
   fromProtobuf(const SymbolLocation &Message);
-  llvm::Optional<SymbolLocation>
+  llvm::Expected<SymbolLocation>
   toProtobuf(const clangd::SymbolLocation &Location);
-  llvm::Optional<HeaderWithReferences>
+  llvm::Expected<HeaderWithReferences>
   toProtobuf(const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader);
-  llvm::Optional<clangd::Symbol::IncludeHeaderWithReferences>
+  llvm::Expected<clangd::Symbol::IncludeHeaderWithReferences>
   fromProtobuf(const HeaderWithReferences &Message);
 
   /// RemoteIndexRoot and LocalIndexRoot are absolute paths to the project (on
   /// remote and local machine respectively) and include a trailing slash. One
   /// of them can be missing (if the machines are different they don't know each
   /// other's specifics and will only do one-way translation), but both can not
   /// be missing at the same time.
   llvm::Optional<std::string> RemoteIndexRoot;

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

 #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/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"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 
 namespace clang {
for (const auto &ID : IDs) {
     auto SID = SymbolID::fromStr(StringRef(ID));
     if (!SID)
       return SID.takeError();
     Result.insert(*SID);
   }
   return Result;
 }
 
+llvm::Error makeStringError(llvm::StringRef Message) {
+  return llvm::make_error<llvm::StringError>(Message,
+                                             llvm::inconvertibleErrorCode());
+}
+
 } // namespace
 
 Marshaller::Marshaller(llvm::StringRef RemoteIndexRoot,
                        llvm::StringRef LocalIndexRoot)
     : Strings(Arena) {
   if (!RemoteIndexRoot.empty()) {
     assert(llvm::sys::path::is_absolute(RemoteIndexRoot));
     assert(RemoteIndexRoot ==
if (!IDs)
     return IDs.takeError();
   Req.Subjects = std::move(*IDs);
   Req.Predicate = static_cast<RelationKind>(Message->predicate());
   if (Message->limit())
     Req.Limit = Message->limit();
   return Req;
 }
 
-llvm::Optional<clangd::Symbol> 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<clangd::Symbol> 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();
   Result.Scope = Message.scope();
   if (Message.has_definition()) {
     auto Definition = fromProtobuf(Message.definition());
     if (Definition)
       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<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()) {
     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.Flags = static_cast<clangd::Symbol::SymbolFlag>(Message.flags());
   return Result;
 }
 
-llvm::Optional<clangd::Ref> 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<clangd::Ref> 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<clangd::RefKind>(Message.kind());
   return Result;
 }
 
-llvm::Optional<std::pair<clangd::SymbolID, clangd::Symbol>>
+llvm::Expected<std::pair<clangd::SymbolID, clangd::Symbol>>
 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);
 }
 
 LookupRequest Marshaller::toProtobuf(const clangd::LookupRequest &From) {
   LookupRequest RPCRequest;
   for (const auto &SymbolID : From.IDs)
     RPCRequest.add_ids(SymbolID.str());
   return RPCRequest;
RelationsRequest Marshaller::toProtobuf(const clangd::RelationsRequest &From) {
   for (const auto &ID : From.Subjects)
     RPCRequest.add_subjects(ID.str());
   RPCRequest.set_predicate(static_cast<uint32_t>(From.Predicate));
   if (From.Limit)
     RPCRequest.set_limit(*From.Limit);
   return RPCRequest;
 }
 
-llvm::Optional<Symbol> Marshaller::toProtobuf(const clangd::Symbol &From) {
+llvm::Expected<Symbol> 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<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 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;
   }
   Result.set_flags(static_cast<uint32_t>(From.Flags));
   return Result;
 }
 
-llvm::Optional<Ref> Marshaller::toProtobuf(const clangd::Ref &From) {
+llvm::Expected<Ref> Marshaller::toProtobuf(const clangd::Ref &From) {
   Ref Result;
   Result.set_kind(static_cast<uint32_t>(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<Relation> Marshaller::toProtobuf(const clangd::SymbolID &Subject,
+llvm::Expected<Relation> 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<std::string>
+llvm::Expected<std::string>
 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<std::string> Marshaller::uriToRelativePath(llvm::StringRef URI) {
+llvm::Expected<std::string> 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);
 }
 
 clangd::SymbolLocation::Position
 Marshaller::fromProtobuf(const Position &Message) {
   clangd::SymbolLocation::Position Result;
SymbolInfo Marshaller::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;
 }
 
-llvm::Optional<clangd::SymbolLocation>
+llvm::Expected<clangd::SymbolLocation>
 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<SymbolLocation>
+llvm::Expected<SymbolLocation>
 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<HeaderWithReferences> Marshaller::toProtobuf(
+llvm::Expected<HeaderWithReferences> Marshaller::toProtobuf(
     const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader) {
   HeaderWithReferences Result;
   Result.set_references(IncludeHeader.References);
   const std::string Header = IncludeHeader.IncludeHeader.str();
   if (isLiteralInclude(Header)) {
     Result.set_header(Header);
     return Result;
   }
   auto RelativePath = uriToRelativePath(Header);
   if (!RelativePath)
-    return llvm::None;
+    return RelativePath.takeError();
   Result.set_header(*RelativePath);
   return Result;
 }
 
-llvm::Optional<clangd::Symbol::IncludeHeaderWithReferences>
+llvm::Expected<clangd::Symbol::IncludeHeaderWithReferences>
 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),
                                                      Message.references()};
 }
 
 } // namespace remote
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/index/remote/server/Server.cpp

 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Logger.h"
 #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"
 
 #include <grpc++/grpc++.h>
 #include <grpc++/health_check_service_interface.h>
 
 #include "Index.grpc.pb.h"
 
ProtobufMarshaller = std::unique_ptr<Marshaller>(new Marshaller(
         /*RemoteIndexRoot=*/llvm::StringRef(NativePath),
         /*LocalIndexRoot=*/""));
   }
 
 private:
   grpc::Status Lookup(grpc::ServerContext *Context,
                       const LookupRequest *Request,
                       grpc::ServerWriter<LookupReply> *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());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
     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;
       }
       LookupReply NextMessage;
       *NextMessage.mutable_stream_result() = *SerializedItem;
       Reply->Write(NextMessage);
       ++Sent;
     });
     LookupReply LastMessage;
     LastMessage.set_final_result(true);
     Reply->Write(LastMessage);
     SPAN_ATTACH(Tracer, "Sent", Sent);
     SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
     return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
                          const FuzzyFindRequest *Request,
                          grpc::ServerWriter<FuzzyFindReply> *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}",
            Req.takeError());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
     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;
       }
       FuzzyFindReply NextMessage;
       *NextMessage.mutable_stream_result() = *SerializedItem;
       Reply->Write(NextMessage);
       ++Sent;
     });
     FuzzyFindReply LastMessage;
     LastMessage.set_final_result(HasMore);
     Reply->Write(LastMessage);
     SPAN_ATTACH(Tracer, "Sent", Sent);
     SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
     return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
                     grpc::ServerWriter<RefsReply> *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());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
     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;
       }
       RefsReply NextMessage;
       *NextMessage.mutable_stream_result() = *SerializedItem;
       Reply->Write(NextMessage);
       ++Sent;
     });
     RefsReply LastMessage;
     LastMessage.set_final_result(HasMore);
     Reply->Write(LastMessage);
     SPAN_ATTACH(Tracer, "Sent", Sent);
     SPAN_ATTACH(Tracer, "Failed to send", FailedToSend);
     return grpc::Status::OK;
   }
 
   grpc::Status Relations(grpc::ServerContext *Context,
                          const RelationsRequest *Request,
                          grpc::ServerWriter<RelationsReply> *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}",
            Req.takeError());
       return grpc::Status::CANCELLED;
     }
     unsigned Sent = 0;
     unsigned FailedToSend = 0;
     Index->relations(
         *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;
           }
           RelationsReply NextMessage;
           *NextMessage.mutable_stream_result() = *SerializedItem;
           Reply->Write(NextMessage);
           ++Sent;
         });

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

 //===--- 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.pb.h"
 #include "TestFS.h"
 #include "index/Index.h"
 #include "index/Ref.h"
 #include "index/Relation.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolID.h"
 #include "index/SymbolLocation.h"
Marshaller ProtobufMarshaller(
       testPath("remote/machine/projects/llvm-project/"),
       testPath("home/my-projects/llvm-project/"));
   clangd::Ref Original;
   Original.Location.FileURI =
       testPathURI("remote/machine/projects/llvm-project/clang-tools-extra/"
                   "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",
                            Strings));
 
   // 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 =
       URI::create(testPath("project/lib/HelloWorld.cpp"), "unittest");
   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) {
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
   clangd::Symbol Sym = createSymbol("home/", Strings);
   Marshaller ProtobufMarshaller(testPath("home/"), testPath("home/"));
 
   // 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(),
                              llvm::sys::path::Style::posix),
             Serialized->definition().file_path());
   EXPECT_TRUE(
       llvm::sys::path::is_relative(Serialized->definition().file_path()));
 
   // 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) {
   clangd::Ref Ref;
   Ref.Kind = clangd::RefKind::Spelled | clangd::RefKind::Declaration;
 
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
   clangd::SymbolLocation Location;
   Location.Start.setLine(124);
   Location.Start.setColumn(21);
   Location.End.setLine(3213);
   Location.End.setColumn(541);
   Location.FileURI = testPathURI(
       "llvm-project/llvm/clang-tools-extra/clangd/Protocol.h", Strings);
   Ref.Location = Location;
 
   Marshaller ProtobufMarshaller(testPath("llvm-project/"),
                                 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));
 }
 
 TEST(RemoteMarshallingTest, IncludeHeaderURIs) {
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
-  llvm::SmallVector<clangd::Symbol::IncludeHeaderWithReferences, 1>
-      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("<iostream>");
   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<size_t>(Serialized->headers_size()),
+            Sym.IncludeHeaders.size());
+  auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized);
+  ASSERT_TRUE(bool(Deserialized));
+  EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
 
-  llvm::SmallVector<clangd::Symbol::IncludeHeaderWithReferences, 1>
-      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<size_t>(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) {
   clangd::LookupRequest Request;
   Request.IDs.insert(llvm::cantFail(SymbolID::fromStr("0000000000000001")));
   Request.IDs.insert(llvm::cantFail(SymbolID::fromStr("0000000000000002")));
 
   Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
 
   auto Serialized = ProtobufMarshaller.toProtobuf(Request);
   EXPECT_EQ(static_cast<unsigned>(Serialized.ids_size()), Request.IDs.size());
   auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized);
   ASSERT_TRUE(bool(Deserialized));
   EXPECT_EQ(Deserialized->IDs, Request.IDs);
 }
 
 TEST(RemoteMarshallingTest, LookupRequestFailingSerialization) {
   clangd::LookupRequest Request;
   Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
   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());
 }
 
 TEST(RemoteMarshallingTest, FuzzyFindRequestSerialization) {
   clangd::FuzzyFindRequest Request;
   Request.ProximityPaths = {testPath("local/Header.h"),
                             testPath("local/subdir/OtherHeader.h"),
                             testPath("remote/File.h"), "Not a Path."};

 }
 
 TEST(RemoteMarshallingTest, RefsRequestFailingSerialization) {
   clangd::RefsRequest Request;
   Marshaller ProtobufMarshaller(testPath("remote/"), testPath("local/"));
   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());
 }
 
 TEST(RemoteMarshallingTest, RelationsRequestSerialization) {
   clangd::RelationsRequest Request;
   Request.Subjects.insert(
       llvm::cantFail(SymbolID::fromStr("0000000000000001")));
   Request.Subjects.insert(
TEST(RemoteMarshallingTest, RelationsRequestSerialization) {
   EXPECT_EQ(Deserialized->Predicate, Request.Predicate);
 }
 
 TEST(RemoteMarshallingTest, RelationsRequestFailingSerialization) {
   RelationsRequest Serialized;
   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());
 }
 
 TEST(RemoteMarshallingTest, RelationsSerializion) {
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
   clangd::Symbol Sym = createSymbol("remote/", Strings);
   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);
 }
 
 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) {
   llvm::BumpPtrAllocator Arena;
   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
 } // namespace remote
 } // namespace clangd
 } // namespace clang