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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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



================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:151
       !Message.has_canonical_declaration()) {
-    elog("Cannot convert Symbol from Protobuf: {0}",
+    elog("Cannot convert Symbol from Protobuf (missing info, definition or "
+         "declaration): {1}",
----------------
aren't all these placeholders incorrect? first is {0}


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:167
   Result.Scope = Message.scope();
   auto Definition = fromProtobuf(Message.definition(), Strings, IndexRoot);
+  if (!Definition) {
----------------
missing definition is perfectly valid, so both the old and new code look wrong here


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:310
+  if (!Location) {
+    elog("Can not convet Reference to Potobuf (invalid location) {1}: {2}",
+         From, From.Location);
----------------
convert, protobuf


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:333
   }
   if (llvm::sys::path::is_relative(IndexRoot)) {
+    elog("Remote index client got a relative path as index root: {1}",
----------------
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)


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:349
   assert(!IndexRoot.empty());
   if (llvm::sys::path::is_relative(IndexRoot)) {
+    elog("Index root {1} is not absolute path", IndexRoot);
----------------
and here... IndexRoot shouldn't be checked every time


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83826