[clangd] Don't send invalid messages from remote index

[kbobyrev]

Summary

Remote server should not send messages that are invalid and will cause problems
on the client side. The client should not be affected by server's failures
whenever possible.

Also add more error messages and logs.

[kbobyrev]

Also do not attempt to use callback on unserialized messages.

[kbobyrev]

Use the same style for elog messages (1-based indexing, message format).

[kbobyrev]

Don't allow empty paths.

[sammccall]

Also add more error messages and logs.

I'm not sure about the error-handling strategy here:

• it's very verbose in the marshalling code. Are we sure it pays for itself? For comparison, the marshalling code for LSP itself silently returns false on error, which is handled at the top level. It's been fine so far.
• some errors are being logged that are not errors. e.g. it's not an error to drop a symbol because its declaration is in a file outside the index root, that's just expected behavior
• it's not clear which levels are responsible for logging errors, leaving the possibility that some errors will be logged twice or not at all
• it's not easy to change if we want a slightly different strategy

I'd suggest leaving out detailed error reporting for now, and reporting at the top level. You may need some mechanism to distinguish invalid vs filtered-out data.
It could be something like a "filtered out" flag that causes the error reporting to be suppressed. There are more design options here if marshalling is a class I think.

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

151–152

aren't all these placeholders incorrect? first is {0}

167

missing definition is perfectly valid, so both the old and new code look wrong here

310

convert, protobuf

331

we shouldn't be checking this every time, IndexRoot is fixed for the lifetime of the index so it should be checked once and asserted here.

(Wrapping the marshalling in a class is a nice clean way to do this)

347

and here... IndexRoot shouldn't be checked every time

[sammccall]

In D83826#2151979, @kbobyrev wrote:

Use the same style for elog messages (1-based indexing, message format).

As far as I can tell, 1-based indexing isn't an option that's available. Where do you see that it is?

[kbobyrev]

In D83826#2152857, @sammccall wrote:

Also add more error messages and logs.

I'm not sure about the error-handling strategy here:

• it's very verbose in the marshalling code. Are we sure it pays for itself? For comparison, the marshalling code for LSP itself silently returns false on error, which is handled at the top level. It's been fine so far.
• some errors are being logged that are not errors. e.g. it's not an error to drop a symbol because its declaration is in a file outside the index root, that's just expected behavior

Can you give an example of this? Missing d

• it's not clear which levels are responsible for logging errors, leaving the possibility that some errors will be logged twice or not at all
• it's not easy to change if we want a slightly different strategy

I'd suggest leaving out detailed error reporting for now, and reporting at the top level. You may need some mechanism to distinguish invalid vs filtered-out data.
It could be something like a "filtered out" flag that causes the error reporting to be suppressed. There are more design options here if marshalling is a class I think.

Fair enough, will do. Thanks!

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

151–152

I think they still work for some reason. Anyway, starting with {0} is OK, I think they're just off in some places.

167

Oh, you're right missing definition is indeed OK, but having an invalid one should still be invalid. I guess I should check if it's empty.

[kbobyrev]

Small update: resolve a couple of comments.

[kbobyrev]

Use DebugStrings instead of ShortDebugStrings.

[kbobyrev]

Wrap marshalling into a class, think about filtering vs failing? I think filtering should be done on the clangd-indexer side and marshalling should deal with valid symbols only and shouldn't have to filter them.

[kbobyrev]

Refactor marshalling into a class, log errors only on the high level.

[kbobyrev]

Remove (now empty) Marshalling.h file comment.

[sammccall]

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

82

this is PratabufMarshaller.reset(new...) or just initialize it in the init list

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

266–267

nit: just if (*From.Definition.FileURI)?

311

potobuf -> protobuf

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

58

why are these needed?

[kbobyrev]

Address post-LGTM comments, rebase on top of HEAD.

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

 //===--- Client.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 <grpc++/grpc++.h>
 
 #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"
bool streamRPC(ClangdRequestT Request,
     ReplyT Reply;
     while (Reader->Read(&Reply)) {
       if (!Reply.has_stream_result()) {
         FinalResult = Reply.final_result();
         continue;
       }
       auto Response =
           fromProtobuf(Reply.stream_result(), &Strings, ProjectRoot);
-      if (!Response)
+      if (!Response) {
         elog("Received invalid {0}", ReplyT::descriptor()->name());
+        continue;
+      }
       Callback(*Response);
     }
     SPAN_ATTACH(Tracer, "status", Reader->Finish().ok());
     return FinalResult;
   }
 
 public:
   IndexClient(
       std::shared_ptr<grpc::Channel> Channel, llvm::StringRef ProjectRoot,
       std::chrono::milliseconds DeadlineTime = std::chrono::milliseconds(1000))
       : Stub(remote::SymbolIndex::NewStub(Channel)), ProjectRoot(ProjectRoot),
         DeadlineWaitingTime(DeadlineTime) {}
 
   void lookup(const clangd::LookupRequest &Request,
               llvm::function_ref<void(const clangd::Symbol &)> Callback) const {
     streamRPC(Request, &remote::SymbolIndex::Stub::Lookup, Callback);
   }
 
   bool
   fuzzyFind(const clangd::FuzzyFindRequest &Request,
             llvm::function_ref<void(const clangd::Symbol &)> Callback) const {
     return streamRPC(Request, &remote::SymbolIndex::Stub::FuzzyFind, Callback);

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

 // deserialize clangd types and translate relative paths into machine-native
 // URIs.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H
 
 #include "Index.pb.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/StringSaver.h"
 
 namespace clang {
 namespace clangd {
 namespace remote {
 
 clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request,
                                       llvm::StringRef IndexRoot);
 llvm::Optional<clangd::Symbol> fromProtobuf(const Symbol &Message,
                                             llvm::UniqueStringSaver *Strings,
                                             llvm::StringRef IndexRoot);
 llvm::Optional<clangd::Ref> fromProtobuf(const Ref &Message,
                                          llvm::UniqueStringSaver *Strings,
                                          llvm::StringRef IndexRoot);
 
 LookupRequest toProtobuf(const clangd::LookupRequest &From);
 FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From,
                             llvm::StringRef IndexRoot);
 RefsRequest toProtobuf(const clangd::RefsRequest &From);
 
-Ref toProtobuf(const clangd::Ref &From, llvm::StringRef IndexRoot);
-Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot);
+llvm::Optional<Ref> toProtobuf(const clangd::Ref &From,
+                               llvm::StringRef IndexRoot);
+llvm::Optional<Symbol> toProtobuf(const clangd::Symbol &From,
+                                  llvm::StringRef IndexRoot);
 
 /// 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.
 llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath,
                                               llvm::StringRef IndexRoot);
 /// Translates a URI from the server's backing index to a relative path suitable

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

clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request,
   return Result;
 }
 
 llvm::Optional<clangd::Symbol> fromProtobuf(const Symbol &Message,
                                             llvm::UniqueStringSaver *Strings,
                                             llvm::StringRef IndexRoot) {
   if (!Message.has_info() || !Message.has_definition() ||
       !Message.has_canonical_declaration()) {
-    elog("Cannot convert Symbol from Protobuf: {0}",
+    elog("Cannot convert Symbol from Protobuf (missing info, definition or "
+         "declaration): {1}",
          Message.ShortDebugString());
     return llvm::None;
   }
   clangd::Symbol Result;
   auto ID = SymbolID::fromStr(Message.id());
   if (!ID) {
-    elog("Cannot parse SymbolID {0} given Protobuf: {1}", ID.takeError(),
+    elog("Cannot parse SymbolID {1} given Protobuf: {2}", ID.takeError(),
          Message.ShortDebugString());
     return llvm::None;
   }
   Result.ID = *ID;
   Result.SymInfo = fromProtobuf(Message.info());
   Result.Name = Message.name();
   Result.Scope = Message.scope();
   auto Definition = fromProtobuf(Message.definition(), Strings, IndexRoot);
-  if (!Definition)
+  if (!Definition) {
+    elog("Cannot convert Symbol from Protobuf (invalid definition): {1}",
+         Message.ShortDebugString());
     return llvm::None;
+  }
   Result.Definition = *Definition;
   auto Declaration =
       fromProtobuf(Message.canonical_declaration(), Strings, IndexRoot);
-  if (!Declaration)
+  if (!Declaration) {
+    elog("Cannot convert Symbol from Protobuf (invalid declaration): {1}",
+         Message.ShortDebugString());
     return llvm::None;
+  }
   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, Strings, IndexRoot);
     if (SerializedHeader)
       Result.IncludeHeaders.push_back(*SerializedHeader);
+    else
+      elog("Cannot convert HeaderWithIncludes from Protobuf: {1}",
+           Header.ShortDebugString());
   }
   Result.Flags = static_cast<clangd::Symbol::SymbolFlag>(Message.flags());
   return Result;
 }
 
 llvm::Optional<clangd::Ref> fromProtobuf(const Ref &Message,
                                          llvm::UniqueStringSaver *Strings,
                                          llvm::StringRef IndexRoot) {
   if (!Message.has_location()) {
-    elog("Cannot convert Ref from Protobuf: {}", Message.ShortDebugString());
+    elog("Cannot convert Ref from Protobuf (missing location): {1}",
+         Message.ShortDebugString());
     return llvm::None;
   }
   clangd::Ref Result;
   auto Location = fromProtobuf(Message.location(), Strings, IndexRoot);
-  if (!Location)
+  if (!Location) {
+    elog("Cannot convert Ref from Protobuf (invalid location): {1}",
+         Message.ShortDebugString());
     return llvm::None;
+  }
   Result.Location = *Location;
   Result.Kind = static_cast<clangd::RefKind>(Message.kind());
   return Result;
 }
 
 LookupRequest toProtobuf(const clangd::LookupRequest &From) {
   LookupRequest RPCRequest;
   for (const auto &SymbolID : From.IDs)
RefsRequest toProtobuf(const clangd::RefsRequest &From) {
   for (const auto &ID : From.IDs)
     RPCRequest.add_ids(ID.str());
   RPCRequest.set_filter(static_cast<uint32_t>(From.Filter));
   if (From.Limit)
     RPCRequest.set_limit(*From.Limit);
   return RPCRequest;
 }
 
-Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot) {
+llvm::Optional<Symbol> toProtobuf(const clangd::Symbol &From,
+                                  llvm::StringRef IndexRoot) {
   Symbol Result;
   Result.set_id(From.ID.str());
   *Result.mutable_info() = toProtobuf(From.SymInfo);
   Result.set_name(From.Name.str());
   auto Definition = toProtobuf(From.Definition, IndexRoot);
-  if (Definition)
-    *Result.mutable_definition() = *Definition;
+  if (!Definition) {
+    elog("Can not convert Symbol to Protobuf (invalid definition) {1}: {2}",
+         From, From.Definition);
+    return llvm::None;
+  }
   Result.set_scope(From.Scope.str());
   auto Declaration = toProtobuf(From.CanonicalDeclaration, IndexRoot);
-  if (Declaration)
-    *Result.mutable_canonical_declaration() = *Declaration;
+  if (!Declaration) {
+    elog("Can not convert Symbol to Protobuf (invalid declaration) {1}: {2}",
+         From, From.CanonicalDeclaration);
+    return llvm::None;
+  }
   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, IndexRoot);
-    if (!Serialized)
+    if (!Serialized) {
+      elog("Can not convert IncludeHeaderWithReferences to Protobuf: {1}",
+           Header.IncludeHeader);
       continue;
+    }
     auto *NextHeader = Result.add_headers();
     *NextHeader = *Serialized;
   }
   Result.set_flags(static_cast<uint32_t>(From.Flags));
   return Result;
 }
 
-// FIXME(kirillbobyrev): A reference without location is invalid.
-// llvm::Optional<Ref> here and on the server side?
-Ref toProtobuf(const clangd::Ref &From, llvm::StringRef IndexRoot) {
+llvm::Optional<Ref> toProtobuf(const clangd::Ref &From,
+                               llvm::StringRef IndexRoot) {
   Ref Result;
   Result.set_kind(static_cast<uint32_t>(From.Kind));
   auto Location = toProtobuf(From.Location, IndexRoot);
-  if (Location)
-    *Result.mutable_location() = *Location;
+  if (!Location) {
+    elog("Can not convet Reference to Potobuf (invalid location) {1}: {2}",
+         From, From.Location);
+    return llvm::None;
+  }
   return Result;
 }
 
 llvm::Optional<std::string> relativePathToURI(llvm::StringRef RelativePath,
                                               llvm::StringRef IndexRoot) {
   assert(RelativePath == llvm::sys::path::convert_to_slash(
                              RelativePath, llvm::sys::path::Style::posix));
   assert(IndexRoot == llvm::sys::path::convert_to_slash(IndexRoot));
   assert(IndexRoot.endswith(llvm::sys::path::get_separator()));
   if (RelativePath.empty())
     return std::string();
   if (llvm::sys::path::is_absolute(RelativePath)) {
-    elog("Remote index client got absolute path from server: {0}",
+    elog("Remote index client got absolute path from server: {1}",
          RelativePath);
     return llvm::None;
   }
   if (llvm::sys::path::is_relative(IndexRoot)) {
-    elog("Remote index client got a relative path as index root: {0}",
+    elog("Remote index client got a relative path as index root: {1}",
          IndexRoot);
     return llvm::None;
   }
   llvm::SmallString<256> FullPath = IndexRoot;
   llvm::sys::path::append(FullPath, RelativePath);
   auto Result = URI::createFile(FullPath);
   return Result.toString();
 }
 
 llvm::Optional<std::string> uriToRelativePath(llvm::StringRef URI,
                                               llvm::StringRef IndexRoot) {
   assert(IndexRoot.endswith(llvm::sys::path::get_separator()));
   assert(IndexRoot == llvm::sys::path::convert_to_slash(IndexRoot));
   assert(!IndexRoot.empty());
   if (llvm::sys::path::is_relative(IndexRoot)) {
-    elog("Index root {0} is not absolute path", IndexRoot);
+    elog("Index root {1} is not absolute path", IndexRoot);
     return llvm::None;
   }
   auto ParsedURI = URI::parse(URI);
   if (!ParsedURI) {
-    elog("Remote index got bad URI from client {0}: {1}", URI,
+    elog("Remote index got bad URI from client {1}: {2}", URI,
          ParsedURI.takeError());
     return llvm::None;
   }
   if (ParsedURI->scheme() != "file") {
-    elog("Remote index got URI with scheme other than \"file\" {0}: {1}", URI,
+    elog("Remote index got URI with scheme other than \"file\" {1}: {2}", URI,
          ParsedURI->scheme());
     return llvm::None;
   }
   llvm::SmallString<256> Result = ParsedURI->body();
   if (!llvm::sys::path::replace_path_prefix(Result, IndexRoot, "")) {
     elog("Can not get relative path from the URI {0} given the index root {1}",
          URI, IndexRoot);
     return llvm::None;

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

 //===--- Server.cpp - gRPC-based Remote Index Server  ---------------------===//
 //
 // 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 "index/Index.h"
 #include "index/Serialization.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Signals.h"
 
 #include <grpc++/grpc++.h>
grpc::Status Lookup(grpc::ServerContext *Context,
     clangd::LookupRequest Req;
     for (const auto &ID : Request->ids()) {
       auto SID = SymbolID::fromStr(StringRef(ID));
       if (!SID)
         return grpc::Status::CANCELLED;
       Req.IDs.insert(*SID);
     }
     Index->lookup(Req, [&](const clangd::Symbol &Sym) {
+      auto SerializedSymbol = toProtobuf(Sym, IndexedProjectRoot);
+      if (!SerializedSymbol)
+        return;
       LookupReply NextMessage;
-      *NextMessage.mutable_stream_result() =
-          toProtobuf(Sym, IndexedProjectRoot);
+      *NextMessage.mutable_stream_result() = *SerializedSymbol;
       Reply->Write(NextMessage);
     });
     LookupReply LastMessage;
     LastMessage.set_final_result(true);
     Reply->Write(LastMessage);
     return grpc::Status::OK;
   }
 
   grpc::Status FuzzyFind(grpc::ServerContext *Context,
                          const FuzzyFindRequest *Request,
                          grpc::ServerWriter<FuzzyFindReply> *Reply) override {
     const auto Req = fromProtobuf(Request, IndexedProjectRoot);
     bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol &Sym) {
+      auto SerializedSymbol = toProtobuf(Sym, IndexedProjectRoot);
+      if (!SerializedSymbol)
+        return;
       FuzzyFindReply NextMessage;
-      *NextMessage.mutable_stream_result() =
-          toProtobuf(Sym, IndexedProjectRoot);
+      *NextMessage.mutable_stream_result() = *SerializedSymbol;
       Reply->Write(NextMessage);
     });
     FuzzyFindReply LastMessage;
     LastMessage.set_final_result(HasMore);
     Reply->Write(LastMessage);
     return grpc::Status::OK;
   }
 
   grpc::Status Refs(grpc::ServerContext *Context, const RefsRequest *Request,
                     grpc::ServerWriter<RefsReply> *Reply) override {
     clangd::RefsRequest Req;
     for (const auto &ID : Request->ids()) {
       auto SID = SymbolID::fromStr(StringRef(ID));
       if (!SID)
         return grpc::Status::CANCELLED;
       Req.IDs.insert(*SID);
     }
     bool HasMore = Index->refs(Req, [&](const clangd::Ref &Reference) {
+      auto SerializedRef = toProtobuf(Reference, IndexedProjectRoot);
+      if (!SerializedRef)
+        return;
       RefsReply NextMessage;
-      *NextMessage.mutable_stream_result() = toProtobuf(Reference, IndexRoot);
+      *NextMessage.mutable_stream_result() = *SerializedRef;
       Reply->Write(NextMessage);
     });
     RefsReply LastMessage;
     LastMessage.set_final_result(HasMore);
     Reply->Write(LastMessage);
     return grpc::Status::OK;
   }
 

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

TEST(RemoteMarshallingTest, URITranslation) {
   llvm::UniqueStringSaver Strings(Arena);
   clangd::Ref Original;
   Original.Location.FileURI =
       testPathURI("remote/machine/projects/llvm-project/clang-tools-extra/"
                   "clangd/unittests/remote/MarshallingTests.cpp",
                   Strings);
   auto Serialized =
       toProtobuf(Original, testPath("remote/machine/projects/llvm-project/"));
-  EXPECT_EQ(Serialized.location().file_path(),
+  EXPECT_TRUE(Serialized);
+  EXPECT_EQ(Serialized->location().file_path(),
             "clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp");
   const std::string LocalIndexPrefix = testPath("local/machine/project/");
-  auto Deserialized = fromProtobuf(Serialized, &Strings,
+  auto Deserialized = fromProtobuf(*Serialized, &Strings,
                                    testPath("home/my-projects/llvm-project/"));
   EXPECT_TRUE(Deserialized);
   EXPECT_EQ(Deserialized->Location.FileURI,
             testPathURI("home/my-projects/llvm-project/clang-tools-extra/"
                         "clangd/unittests/remote/MarshallingTests.cpp",
                         Strings));
 
   clangd::Ref WithInvalidURI;
-  // Invalid URI results in empty path.
+  // Invalid URI results in serialization failure.
   WithInvalidURI.Location.FileURI = "This is not a URI";
   Serialized = toProtobuf(WithInvalidURI, testPath("home/"));
-  EXPECT_EQ(Serialized.location().file_path(), "");
+  EXPECT_FALSE(Serialized);
 
   // Can not use URIs with scheme different from "file".
   auto UnittestURI =
       URI::create(testPath("project/lib/HelloWorld.cpp"), "unittest");
   EXPECT_TRUE(bool(UnittestURI));
   WithInvalidURI.Location.FileURI =
       Strings.save(UnittestURI->toString()).begin();
   Serialized = toProtobuf(WithInvalidURI, testPath("project/lib/"));
-  EXPECT_EQ(Serialized.location().file_path(), "");
+  EXPECT_FALSE(Serialized);
 
   Ref WithAbsolutePath;
   *WithAbsolutePath.mutable_location()->mutable_file_path() =
       "/usr/local/user/home/HelloWorld.cpp";
   Deserialized = fromProtobuf(WithAbsolutePath, &Strings, LocalIndexPrefix);
   // Paths transmitted over the wire can not be absolute, they have to be
   // relative.
   EXPECT_FALSE(Deserialized);
TEST(RemoteMarshallingTest, SymbolSerialization) {
   Sym.Documentation = Strings.save("This is my amazing Foo constructor!");
   Sym.ReturnType = Strings.save("Foo");
 
   Sym.Flags = clangd::Symbol::SymbolFlag::IndexedForCodeCompletion;
 
   // Check that symbols are exactly the same if the path to indexed project is
   // the same on indexing machine and the client.
   auto Serialized = toProtobuf(Sym, testPath("home/"));
-  auto Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/"));
+  EXPECT_TRUE(Serialized);
+  auto Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/"));
   EXPECT_TRUE(Deserialized);
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
   // Serialized paths are relative and have UNIX slashes.
-  EXPECT_EQ(convert_to_slash(Serialized.definition().file_path(),
+  EXPECT_EQ(convert_to_slash(Serialized->definition().file_path(),
                              llvm::sys::path::Style::posix),
-            Serialized.definition().file_path());
+            Serialized->definition().file_path());
   EXPECT_TRUE(
-      llvm::sys::path::is_relative(Serialized.definition().file_path()));
+      llvm::sys::path::is_relative(Serialized->definition().file_path()));
+
+  // Relative path is absolute.
+  *Serialized->mutable_canonical_declaration()->mutable_file_path() =
+      convert_to_slash("/path/to/Declaration.h");
+  Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/"));
+  EXPECT_FALSE(Deserialized);
 
   // Fail with an invalid URI.
   Location.FileURI = "Not A URI";
   Sym.Definition = Location;
   Serialized = toProtobuf(Sym, testPath("home/"));
-  Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/"));
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(Serialized);
 
   // Schemes other than "file" can not be used.
   auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest");
   EXPECT_TRUE(bool(UnittestURI));
   Location.FileURI = Strings.save(UnittestURI->toString()).begin();
   Sym.Definition = Location;
   Serialized = toProtobuf(Sym, testPath("home/"));
-  Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/"));
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(Serialized);
 
   // Passing root that is not prefix of the original file path.
   Location.FileURI = testPathURI("home/File.h", Strings);
   Sym.Definition = Location;
   // Check that the symbol is valid and passing the correct path works.
   Serialized = toProtobuf(Sym, testPath("home/"));
-  Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/"));
+  EXPECT_TRUE(Serialized);
+  Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/"));
   EXPECT_TRUE(Deserialized);
   EXPECT_EQ(Deserialized->Definition.FileURI,
             testPathURI("home/File.h", Strings));
   // Fail with a wrong root.
   Serialized = toProtobuf(Sym, testPath("nothome/"));
-  Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/"));
-  EXPECT_FALSE(Deserialized);
+  EXPECT_FALSE(Serialized);
 }
 
 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;
 
   auto Serialized = toProtobuf(Ref, testPath("llvm-project/"));
+  EXPECT_TRUE(Serialized);
   auto Deserialized =
-      fromProtobuf(Serialized, &Strings, testPath("llvm-project/"));
+      fromProtobuf(*Serialized, &Strings, testPath("llvm-project/"));
   EXPECT_TRUE(Deserialized);
   EXPECT_EQ(toYAML(Ref), toYAML(*Deserialized));
 }
 
 TEST(RemoteMarshallingTest, IncludeHeaderURIs) {
   llvm::BumpPtrAllocator Arena;
   llvm::UniqueStringSaver Strings(Arena);
 
TEST(RemoteMarshallingTest, IncludeHeaderURIs) {
   // 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;
 
   auto Serialized = toProtobuf(Sym, convert_to_slash("/"));
-  EXPECT_EQ(static_cast<size_t>(Serialized.headers_size()),
+  EXPECT_EQ(static_cast<size_t>(Serialized->headers_size()),
             ValidHeaders.size());
-  auto Deserialized = fromProtobuf(Serialized, &Strings, convert_to_slash("/"));
+  EXPECT_TRUE(Serialized);
+  auto Deserialized =
+      fromProtobuf(*Serialized, &Strings, convert_to_slash("/"));
   EXPECT_TRUE(Deserialized);
 
   Sym.IncludeHeaders = ValidHeaders;
   EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized));
 }
 
 TEST(RemoteMarshallingTest, FuzzyFindRequestSerialization) {
   clangd::FuzzyFindRequest Request;