[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfb5588b0ad59: [clangd] Propagate remote index errors via 
Expected (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ 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

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:313
  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 make_error("Empty relative path.",

kadircet wrote:
> kbobyrev wrote:
> > kadircet wrote:
> > > is `RelativePath` user provided? can't we just assert on non-emptiness 
> > > and non-absoluteness and make this function return a std::string instead?
> > 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?
> > 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.
Yes, I agree that spending runtime on this is unfortunate but I guess it's the 
cost we'd have to pay :(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 282166.
kbobyrev marked 6 inline comments as done.
kbobyrev added a comment.

Good points, thanks!

Addressed post-LGTM comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ 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_FALS

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!




Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:313
  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 make_error("Empty relative path.",

kbobyrev wrote:
> kadircet wrote:
> > is `RelativePath` user provided? can't we just assert on non-emptiness and 
> > non-absoluteness and make this function return a std::string instead?
> 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?
> 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.



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:169
+  ASSERT_TRUE(bool(Serialized));
+  EXPECT_TRUE(bool(ProtobufMarshaller.fromProtobuf(*Serialized)));
 

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).



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:418
+  auto URIString = ProtobufMarshaller.relativePathToURI("lib/File.cpp");
+  EXPECT_TRUE(bool(URIString));
   // RelativePath can not be absolute.

again should be ASSERT_TRUE for similar concerns stated above.



Comment at: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp:436
+  testPathURI("remote/project/lib/File.cpp", Strings));
+  EXPECT_TRUE(bool(RelativePath));
   // RemoteIndexRoot has to be be a prefix of the file path.

again should be ASSERT_TRUE for similar concerns stated above.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:313
  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 make_error("Empty relative path.",

kadircet wrote:
> is `RelativePath` user provided? can't we just assert on non-emptiness and 
> non-absoluteness and make this function return a std::string instead?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 281946.
kbobyrev marked 12 inline comments as done.
kbobyrev added a comment.

Resolve most review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ 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));
+  EXPECT_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));

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

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 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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




Comment at: clang-tools-extra/clangd/index/remote/Client.cpp:60
+ Response.takeError());
+llvm::consumeError(Response.takeError());
 ++FailedToParse;

elog already consumes the error



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:131
+  if (!Message.has_info() || !Message.has_canonical_declaration())
+return make_error("Missing info, definition or declaration.",
+   inconvertibleErrorCode());

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



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:162
   Result.IncludeHeaders.push_back(*SerializedHeader);
 else
+  elog("Cannot convert HeaderWithIncludes from protobuf; skipping it: {0}. 
"

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

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



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:163
 else
-  elog("Cannot convert HeaderWithIncludes from protobuf: {0}",
-   Header.DebugString());
+  elog("Cannot convert HeaderWithIncludes from protobuf; skipping it: {0}. 
"
+   "Reason: {1}",

what makes this a non-terminal error?



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:189
+return SubjectID.takeError();
   if (!Message.has_object()) {
+return make_error("Missing Object.", 
inconvertibleErrorCode());

nit: redundant braces 



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:273
 auto Serialized = toProtobuf(Header);
 if (!Serialized) {
+  elog("Can not convert IncludeHeaderWithReferences {0} to protobuf; "

again, what makes this non-terminal?



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:277
+   Header.IncludeHeader, Serialized.takeError());
+  llvm::consumeError(Serialized.takeError());
   continue;

elog already consumes the error



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:313
  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 make_error("Empty relative path.",

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



Comment at: 
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:330
+return ParsedURI.takeError();
+  if (ParsedURI->scheme() != "file")
+return make_error("Can not use URI schemes other than file.",

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



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:89
 if (!Req) {
-  elog("Can not parse LookupRequest from protobuf: {0}", Req.takeError());
+  elog("Can not parse {0} from protobuf: {1}",
+   LookupRequest::descriptor()->name(), Req.takeError());

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.



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:95
 unsigned FailedToSend = 0;
 Index->lookup(*Req, [&](const auto &Item) {
   auto SerializedItem = ProtobufMarshaller->toProtobuf(Item);

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



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:100
+ SerializedItem.takeError());
+llvm::consumeError(SerializedItem.takeError());
 ++FailedToSend;

again, elog consumes



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:124
+  elog("Can not parse {0} from protobuf: {1}",
+   LookupRequest::descriptor()->name(), Req.takeError());
   return grpc::Status::CANCELLED;

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



Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:134
+ SerializedItem.takeError());
+llvm::consumeError(SerializedItem.takeError());
 ++FailedToSend;

again, elog consumes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.or

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 281897.
kbobyrev added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84939/new/

https://reviews.llvm.org/D84939

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
@@ -97,11 +97,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 +109,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 +126,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 +150,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 +164,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));
+  EXPECT_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

[PATCH] D84939: [clangd] Propagate remote index errors via Expected

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous.
Herald added a project: clang.
kbobyrev requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84939

Files:
  clang-tools-extra/clangd/index/remote/Client.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
  clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
  clang-tools-extra/clangd/index/remote/server/Server.cpp
  clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp

Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
@@ -97,11 +97,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 +109,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 +126,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 +150,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 +164,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));
+  EXPECT_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.t