[PATCH] D95229: [clangd] Treat optional field type mismatches as soft failures

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This is basically the reverse of D89128 <https://reviews.llvm.org/D89128> + D89131 <https://reviews.llvm.org/D89131>.

I'm not sure if I like it, but I might be biased :-)
It was put in place after I screwed up the type of a protocol message and we didn't detect it until someone reported that some setting wasn't respected in VSCode.
A log message would have made it easier to debug, and *may* have led to us catching it, unsure.

I'd probably lean toward fixing https://github.com/clangd/vscode-clangd/issues/134 where we *know* of bad clients, but not based on the possibility alone. WDYT?



================
Comment at: clang-tools-extra/clangd/Protocol.cpp:33
+template <typename T>
+bool mapOptionalOrLog(llvm::json::ObjectMapper &O, llvm::StringLiteral Prop,
+                      T &Out) {
----------------
I would call this tryMap for brevity


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:36
+  if (!O.mapOptional(Prop, Out)) {
+    elog("Unexpected value for optional field '{0}', ignoring", Prop);
+    Out = T{}; // Default-initialize Out to ensure we don't have a
----------------
On the one hand, logging a message seems better than doing this silently.
On the other, if clients have this issue, these messages are likely to be frequent (at least every message) and not so helpful: unlike the current messages they don't include the context.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:37
+    elog("Unexpected value for optional field '{0}', ignoring", Prop);
+    Out = T{}; // Default-initialize Out to ensure we don't have a
+               // half-constructed object.
----------------
this isn't necessarily the *field* default though! e.g. if we had `KeepFingerAttachedToHand = true` then setting it to 1 would result in `false`.
I think this coincides for all the concrete callsites here. But i'm nervous we add a pattern that could bite us later.

We could do something weird like default-allocate a T and swap it if things succeed.


================
Comment at: clang-tools-extra/clangd/Protocol.cpp:40
+  }
+  return true;
+}
----------------
not sure if returning always-true for chaining is helpful or misleading...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95229