[clangd] Validate optional fields more strictly.

[hokein]

clang-tools-extra/clangd/Protocol.cpp

800

R.failureReason's type is llvm::Optional<std::string>, I think we should probably use the O.map (the overload one for llvm::Optional)?

1130

And this one.

[sammccall]

Sorry, I thought this was accepted. Had addressed the two comments, happy to address more or revert, LMK

[hokein]

ah, I thought I clicked the accepted button.

anyway, LGTM.

clang-tools-extra/clangd/Protocol.cpp

bool fromJSON(const llvm::json::Value &Params, DidSaveTextDocumentParams &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   return O && O.map("textDocument", R.textDocument);
 }
 
 bool fromJSON(const llvm::json::Value &Params, DidChangeTextDocumentParams &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
-  if (!O)
-    return false;
-  O.map("forceRebuild", R.forceRebuild);  // Optional clangd extension.
-  return O.map("textDocument", R.textDocument) &&
+  return O && O.map("textDocument", R.textDocument) &&
          O.map("contentChanges", R.contentChanges) &&
-         O.map("wantDiagnostics", R.wantDiagnostics);
+         O.map("wantDiagnostics", R.wantDiagnostics) &&
+         O.mapOptional("forceRebuild", R.forceRebuild);
 }
 
 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;
if (D.relatedInformation)
     Diag["relatedInformation"] = *D.relatedInformation;
   // 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);
-  if (!O || !O.map("range", R.range) || !O.map("message", R.message))
-    return false;
-  O.map("severity", R.severity);
-  O.map("category", R.category);
-  O.map("code", R.code);
-  O.map("source", R.source);
+  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;
 }
 
 llvm::json::Value toJSON(const PublishDiagnosticsParams &PDP) {
   llvm::json::Object Result{
       {"uri", PDP.uri},
       {"diagnostics", PDP.diagnostics},
   };

 
 llvm::json::Value toJSON(const ApplyWorkspaceEditParams &Params) {
   return llvm::json::Object{{"edit", Params.edit}};
 }
 
 bool fromJSON(const llvm::json::Value &Response, ApplyWorkspaceEditResponse &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Response, P);
-  if (!O || !O.map("applied", R.applied))
-    return false;
+  return O && O.map("applied", R.applied) &&
          O.map("failureReason", R.failureReason);
-  return true;
 }
 
 bool fromJSON(const llvm::json::Value &Params, TextDocumentPositionParams &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   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);
-  if (!O)
-    return false;
-
   int TriggerKind;
-  if (!O.map("triggerKind", TriggerKind))
+  if (!O || !O.map("triggerKind", TriggerKind) ||
+      !O.mapOptional("triggerCharacter", R.triggerCharacter))
     return false;
   R.triggerKind = static_cast<CompletionTriggerKind>(TriggerKind);
-
-  if (auto *TC = Params.getAsObject()->get("triggerCharacter"))
-    return fromJSON(*TC, R.triggerCharacter, P.field("triggerCharacter"));
   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.
-  O.map("compilationDatabaseChanges", S.compilationDatabaseChanges);
-  return true;
+  return O.mapOptional("compilationDatabaseChanges",
+                       S.compilationDatabaseChanges);
 }
 
 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.
 
-  fromJSON(Params, Opts.ConfigSettings, P);
-  O.map("compilationDatabasePath", Opts.compilationDatabasePath);
-  O.map("fallbackFlags", Opts.fallbackFlags);
-  O.map("clangdFileStatus", Opts.FileStatus);
-  return true;
+  return fromJSON(Params, Opts.ConfigSettings, P) &&
+         O.map("compilationDatabasePath", Opts.compilationDatabasePath) &&
+         O.mapOptional("fallbackFlags", Opts.fallbackFlags) &&
+         O.mapOptional("clangdFileStatus", Opts.FileStatus);
 }
 
 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) ||
llvm::json::Value toJSON(const TypeHierarchyItem &I) {
   return std::move(Result);
 }
 
 bool fromJSON(const llvm::json::Value &Params, TypeHierarchyItem &I,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
 
   // Required fields.
-  if (!(O && O.map("name", I.name) && O.map("kind", I.kind) &&
+  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))) {
-    return false;
-  }
-
-  // Optional fields.
-  O.map("detail", I.detail);
-  O.map("deprecated", I.deprecated);
-  O.map("parents", I.parents);
-  O.map("children", I.children);
-  O.map("data", I.data);
-
-  return true;
+         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);
 }
 
 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);
 }