[clangd] Treat optional field type mismatches as soft failures

[kadircet]

Summary

Clangd currently throws away any protocol messages whenever an optional
field has an unexpected type. This patch changes the behaviour to treat such
fields as missing.

This enables clangd to be more tolerant against small violations to the LSP
spec.

Fixes https://github.com/clangd/vscode-clangd/issues/134

[sammccall]

This is basically the reverse of D89128 + 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?

clang-tools-extra/clangd/Protocol.cpp

33

I would call this tryMap for brevity

36

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.

37

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.

40

not sure if returning always-true for chaining is helpful or misleading...

[kadircet]

As discussed offline, only treat "null" fields as missing, rather than allowing
type mismatches.

[sammccall]

clang-tools-extra/clangd/Protocol.cpp

33

Sorry... now that it can fail, I don't think tryMap is a great name anymore.

mapOptOrNull? mapNullable?

up to you

838

why are we no longer checking the value?

[kadircet]

• rename to mapOptOrNull
• revert leftover changes.

clang-tools-extra/clangd/Protocol.cpp

33

going with mapOptOrNull.

838

oops, i was trying something different at some point and looks like i forgot to revert some of those changes ..

clang-tools-extra/clangd/Protocol.cpp

 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
+namespace {
+
+// Helper that doesn't treat `null` and absent fields as failures.
+template <typename T>
+bool mapOptOrNull(const llvm::json::Value &Params, llvm::StringLiteral Prop,
+                  T &Out, llvm::json::Path P) {
+  auto *O = Params.getAsObject();
+  assert(O);
+  auto *V = O->get(Prop);
+  // Field is missing or null.
+  if (!V || V->getAsNull().hasValue())
+    return true;
+  return fromJSON(*V, Out, P.field(Prop));
+}
+} // namespace
 
 char LSPError::ID;
 
 URIForFile URIForFile::canonicalize(llvm::StringRef AbsPath,
                                     llvm::StringRef TUPath) {
   assert(llvm::sys::path::is_absolute(AbsPath) && "the path is relative");
   auto Resolved = URI::resolvePath(AbsPath, TUPath);
   if (!Resolved) {

 }
 
 bool fromJSON(const llvm::json::Value &Params, DidChangeTextDocumentParams &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   return O && O.map("textDocument", R.textDocument) &&
          O.map("contentChanges", R.contentChanges) &&
          O.map("wantDiagnostics", R.wantDiagnostics) &&
-         O.mapOptional("forceRebuild", R.forceRebuild);
+         mapOptOrNull(Params, "forceRebuild", R.forceRebuild, P);
 }
 
 bool fromJSON(const llvm::json::Value &E, FileChangeType &Out,
               llvm::json::Path P) {
   if (auto T = E.getAsInteger()) {
     if (*T < static_cast<int>(FileChangeType::Created) ||
         *T > static_cast<int>(FileChangeType::Deleted))
       return false;
llvm::json::Value toJSON(const Diagnostic &D) {
   // FIXME: workaround for older gcc/clang
   return std::move(Diag);
 }
 
 bool fromJSON(const llvm::json::Value &Params, Diagnostic &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   return O && O.map("range", R.range) && O.map("message", R.message) &&
-         O.mapOptional("severity", R.severity) &&
-         O.mapOptional("category", R.category) &&
-         O.mapOptional("code", R.code) && O.mapOptional("source", R.source);
-  return true;
+         mapOptOrNull(Params, "severity", R.severity, P) &&
+         mapOptOrNull(Params, "category", R.category, P) &&
+         mapOptOrNull(Params, "code", R.code, P) &&
+         mapOptOrNull(Params, "source", R.source, P);
 }
 
 llvm::json::Value toJSON(const PublishDiagnosticsParams &PDP) {
   llvm::json::Object Result{
       {"uri", PDP.uri},
       {"diagnostics", PDP.diagnostics},
   };
   if (PDP.version)
return O && O.map("textDocument", R.textDocument) &&
          O.map("position", R.position);
 }
 
 bool fromJSON(const llvm::json::Value &Params, CompletionContext &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   int TriggerKind;
   if (!O || !O.map("triggerKind", TriggerKind) ||
-      !O.mapOptional("triggerCharacter", R.triggerCharacter))
+      !mapOptOrNull(Params, "triggerCharacter", R.triggerCharacter, P))
     return false;
   R.triggerKind = static_cast<CompletionTriggerKind>(TriggerKind);
   return true;
 }
 
 bool fromJSON(const llvm::json::Value &Params, CompletionParams &R,
               llvm::json::Path P) {
   if (!fromJSON(Params, static_cast<TextDocumentPositionParams &>(R), P))
     return false;
   if (auto *Context = Params.getAsObject()->get("context"))
return O && O.map("workingDirectory", CDbUpdate.workingDirectory) &&
          O.map("compilationCommand", CDbUpdate.compilationCommand);
 }
 
 bool fromJSON(const llvm::json::Value &Params, ConfigurationSettings &S,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   if (!O)
     return true; // 'any' type in LSP.
-  return O.mapOptional("compilationDatabaseChanges",
-                       S.compilationDatabaseChanges);
+  return mapOptOrNull(Params, "compilationDatabaseChanges",
+                      S.compilationDatabaseChanges, P);
 }
 
 bool fromJSON(const llvm::json::Value &Params, InitializationOptions &Opts,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   if (!O)
     return true; // 'any' type in LSP.
 
   return fromJSON(Params, Opts.ConfigSettings, P) &&
          O.map("compilationDatabasePath", Opts.compilationDatabasePath) &&
-         O.mapOptional("fallbackFlags", Opts.fallbackFlags) &&
-         O.mapOptional("clangdFileStatus", Opts.FileStatus);
+         mapOptOrNull(Params, "fallbackFlags", Opts.fallbackFlags, P) &&
+         mapOptOrNull(Params, "clangdFileStatus", Opts.FileStatus, P);
 }
 
 bool fromJSON(const llvm::json::Value &E, TypeHierarchyDirection &Out,
               llvm::json::Path P) {
   auto T = E.getAsInteger();
   if (!T)
     return false;
   if (*T < static_cast<int>(TypeHierarchyDirection::Children) ||

 bool fromJSON(const llvm::json::Value &Params, TypeHierarchyItem &I,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
 
   // Required fields.
   return O && O.map("name", I.name) && O.map("kind", I.kind) &&
          O.map("uri", I.uri) && O.map("range", I.range) &&
          O.map("selectionRange", I.selectionRange) &&
-         O.mapOptional("detail", I.detail) &&
-         O.mapOptional("deprecated", I.deprecated) &&
-         O.mapOptional("parents", I.parents) &&
-         O.mapOptional("children", I.children) && O.mapOptional("data", I.data);
+         mapOptOrNull(Params, "detail", I.detail, P) &&
+         mapOptOrNull(Params, "deprecated", I.deprecated, P) &&
+         mapOptOrNull(Params, "parents", I.parents, P) &&
+         mapOptOrNull(Params, "children", I.children, P) &&
+         mapOptOrNull(Params, "data", I.data, P);
 }
 
 bool fromJSON(const llvm::json::Value &Params,
               ResolveTypeHierarchyItemParams &R, llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   return O && O.map("item", R.item) && O.map("resolve", R.resolve) &&
          O.map("direction", R.direction);
 }
bool fromJSON(const llvm::json::Value &Params, CallHierarchyItem &I,
   llvm::json::ObjectMapper O(Params, P);
 
   // Populate the required fields only. We don't care about the
   // optional fields `Tags` and `Detail` for the purpose of
   // client --> server communication.
   return O && O.map("name", I.name) && O.map("kind", I.kind) &&
          O.map("uri", I.uri) && O.map("range", I.range) &&
          O.map("selectionRange", I.selectionRange) &&
-         O.mapOptional("data", I.data);
+         mapOptOrNull(Params, "data", I.data, P);
 }
 
 bool fromJSON(const llvm::json::Value &Params,
               CallHierarchyIncomingCallsParams &C, llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   return O.map("item", C.item);
 }