[JSON] Add error reporting facility, used in fromJSON and ObjectMapper.

[sammccall]

Summary

Translating between JSON objects and C++ strutctures is common.
From experience in clangd, fromJSON/ObjectMapper work well and save a lot of
code, but aren't adopted elsewhere at least partly due to total lack of error
reporting beyond "ok"/"bad".

This new error model should be rich enough for most applications. It comprises:

• a name for the root object, so the user knows what we're parsing
• a path from the root object to the JSON node most associated with the error
• a local error message

These can be presented in two ways:

• An llvm::Error like "expected string at ConfigFile.credentials[0].username"
• A truncated dump of the original object with the error marked, like:

{
  "credentials": [
    {
      "username": /* error: expected string */ 42,
      "password": "secret"
    },
    { ... }
  ]
  "backups": { ... }
}

To track the locations, we exploit the fact that the call graph of recursive
parse functions mirror the structure of the JSON itself.
The current path is represented as a linked list of segments, each of which is
on the stack as a parameter. Concretely, fromJSON now looks like:

bool fromJSON(const Value&, T&, Path);

The heavy parts mostly stay out of the way:

• building path segments is mostly handled in library code for common cases (arrays mapped as std::vector, objects mapped using ObjectMapper)
• no heap allocation at all unless an error is encountered, then one small one
• llvm::Error and error-in-context are created only if needed

The general top-level interface (Path::Root) is independent of fromJSON etc, and
ends up being a bit clunky.
I've added high-level parse<T>(StringRef) -> Expected<T>, but it's not general
enough to be the primary interface I think (at least, not usable in clangd).

---

This can be split into several separate patches if desired:

• the comment support in json::OStream (used to show errors in context)
• the Path type, including llvm::Error representation
• the fromJSON/ObjectMapper changes (and clangd usages - hard to separate)
• support for printing errors in context

I wanted to show it all together first, though.

[wallace]

I like this a lot!! Amazing work. I think it'll be useful for my Trace patch. I'll do a corresponding refactor using probably this new functionality.
I find the diff straightforward to follow. Unless anyone has objections, I'm fine with the patch at it is.

[kadircet]

Thanks! This looks great. I've mostly did the full review anyway but feel free to land in small patches just in case some compiler becomes upset and you need to revert.

Mostly nits, and style related comments. LGTM!

clang-tools-extra/clangd/ClangdLSPServer.cpp

250 ↗

(On Diff #293486)

nit:

auto P = parse ...;
if(!P)
  return Reply(P.takeError());
(Server.*Handler)(*P, std::move(Reply));

clang-tools-extra/clangd/ClangdLSPServer.h

195 ↗

(On Diff #293486)

nit: i would merge with the previous elog, i.e. elog("Failed to decode {0} {1}:\n{2}", ... OS.str());

198 ↗

(On Diff #293486)

why not include Root.err() in the failure?

llvm/include/llvm/Support/JSON.h

579

static_cast<unsigned>

583

.. or a pointer to path::root in case of the "head".

the technique is really need, but do we really need all of this compression ? can't we just have

struct Segment {
  union {
   llvm::StringRef Field;
   unsigned Index;
   Root &R;
  };
  enum { Object, Array, Head } Type;
};

This would be 24 bytes instead of 16, but hopefulyl will get rid of casts and have some extra type checking ?

598

i would could this Path::Start or Path::Head to emphasize the "begining of the linked list" bit, up to you.

603

nit: I would rather have a public: void setError(const Path &P, llvm::StringLiteral Message) but up to you

617

would be nice to have some comments with example output

llvm/lib/Support/JSON.cpp

242

nit: drop static, already in anon namespace

316

nit: I believe this is big enough to be its own function, not sure what we gain by keeping it as a lambda.

322

nit: either drop return or return abbreviateChildren..

[sammccall]

Thanks! I've addressed most of the comments one way or the other, please LMK if it's not convincing!
Meanwhile I'm going to split this up to land in parts.

clang-tools-extra/clangd/ClangdLSPServer.h

195 ↗

(On Diff #293486)

Oops, this was meant to be a vlog (off by default).

198 ↗

(On Diff #293486)

(again, vlog)

llvm/include/llvm/Support/JSON.h

583

Yeah, it probably doesn't matter, but we really are creating a lot of these, and I'm kind of hung up on the idea that this be as close to free as possible.

The casts are really ugly and scattered, I've encapsulated them all into the Segment class which seems much nicer.

598

The point of the name is that it's a path within a tree, and this node is the root of the *tree* rather than the list. Rewrote the class comment to reflect this.

(In a linked list it's kind of the tail, right? But the linked-list is an implementation detail)

603

This is indeed slightly nicer, but this signature loses the path length, so we either have to:

• eat extra allocations to resize the vector
• traverse the linked list a third time to get it
• store it in the path and manage that

I'm not sure the small readability win is worth any of those. I've restricted the friend to just that function.

llvm/lib/Support/JSON.cpp

316

It just leaks a bunch of stuff into the header...
We need the separate function to generalize the signature for recursion.
It needs to be a member so it can access Segment, so its signature needs to be in the header.
It can no longer capture JOS, so that needs to be a parameter, so we need to forward-declare OStream too (or rearrange the header further).

On balance I'm not sure this is a readability win, as the readability of the header is more important.

[sammccall]

Address review comments.

[sammccall]

This was landed as 4 commits, this diff is all 4 as committed.

llvm/include/llvm/Support/JSON.h

if (auto S = E.getAsInteger()) {
     Out = *S;
     return true;
   }
   return false;
 }
 inline bool fromJSON(const Value &E, int64_t &Out) {
   if (auto S = E.getAsInteger()) {
     Out = *S;
     return true;
   }
   return false;
 }
 inline bool fromJSON(const Value &E, double &Out) {
   if (auto S = E.getAsNumber()) {
     Out = *S;
     return true;
   }
   return false;
 }
 inline bool fromJSON(const Value &E, bool &Out) {
   if (auto S = E.getAsBoolean()) {
     Out = *S;
     return true;
   }
   return false;
 }
 inline bool fromJSON(const Value &E, std::nullptr_t &Out) {
   if (auto S = E.getAsNull()) {
     Out = *S;
     return true;
   }
   return false;
 }
 template <typename T> bool fromJSON(const Value &E, llvm::Optional<T> &Out) {
   if (E.getAsNull()) {
     Out = llvm::None;
     return true;
   }
   T Result;
   if (!fromJSON(E, Result))
     return false;
   Out = std::move(Result);
   return true;
 }
 template <typename T> bool fromJSON(const Value &E, std::vector<T> &Out) {
   if (auto *A = E.getAsArray()) {
     Out.clear();
     Out.resize(A->size());
     for (size_t I = 0; I < A->size(); ++I)
       if (!fromJSON((*A)[I], Out[I]))
         return false;
     return true;
   }
   return false;
 }
public:
   std::error_code convertToErrorCode() const override {
     return llvm::inconvertibleErrorCode();
   }
 };
 
 /// json::OStream allows writing well-formed JSON without materializing
 /// all structures as json::Value ahead of time.
 /// It's faster, lower-level, and less safe than OS << json::Value.
+/// It also allows emitting more constructs, such as comments.
 ///
 /// Only one "top-level" object can be written to a stream.
 /// Simplest usage involves passing lambdas (Blocks) to fill in containers:
 ///
 ///   json::OStream J(OS);
 ///   J.array([&]{
 ///     for (const Event &E : Events)
 ///       J.object([&] {
void array(Block Contents) {
     arrayEnd();
   }
   /// Emit an object whose elements are emitted in the provided Block.
   void object(Block Contents) {
     objectBegin();
     Contents();
     objectEnd();
   }
+  /// Emit a JavaScript comment associated with the next printed value.
+  /// The string must be valid until the next attribute or value is emitted.
+  /// Comments are not part of standard JSON, and many parsers reject them!
+  void comment(llvm::StringRef);
 
   // High level functions to output object attributes.
   // Valid only within an object (any number of times).
 
   /// Emit an attribute whose value is self-contained (number, vector<int> etc).
   void attribute(llvm::StringRef Key, const Value& Contents) {
     attributeImpl(Key, [&] { value(Contents); });
   }

  private:
   void attributeImpl(llvm::StringRef Key, Block Contents) {
     attributeBegin(Key);
     Contents();
     attributeEnd();
   }
 
   void valueBegin();
+  void flushComment();
   void newline();
 
   enum Context {
     Singleton, // Top level, or object attribute.
     Array,
     Object,
   };
   struct State {
     Context Ctx = Singleton;
     bool HasValue = false;
   };
   llvm::SmallVector<State, 16> Stack; // Never empty.
+  llvm::StringRef PendingComment;
   llvm::raw_ostream &OS;
   unsigned IndentSize;
   unsigned Indent = 0;
 };
 
 /// Serializes this Value to JSON, writing it to the provided stream.
 /// The formatting is compact (no extra whitespace) and deterministic.
 /// For pretty-printing, use the formatv() format_provider below.

llvm/lib/Support/JSON.cpp

 //=== JSON.cpp - JSON value, parsing and serialization - C++ -----------*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===---------------------------------------------------------------------===//
 
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Format.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cctype>
 
 namespace llvm {
 namespace json {
 
 Value &Object::operator[](const ObjectKey &K) {
   return try_emplace(K, nullptr).first->getSecond();
 }
while (P != End && (*P == ' ' || *P == '\r' || *P == '\n' || *P == '\t'))
       ++P;
   }
 
   // On invalid syntax, parseX() functions return false and set Err.
   bool parseNumber(char First, Value &Out);
   bool parseString(std::string &Out);
   bool parseUnicode(std::string &Out);
   bool parseError(const char *Msg); // always returns false
 
   char next() { return P == End ? 0 : *P++; }
   char peek() { return P == End ? 0 : *P; }
   static bool isNumber(char C) {
     return C == '0' || C == '1' || C == '2' || C == '3' || C == '4' ||
            C == '5' || C == '6' || C == '7' || C == '8' || C == '9' ||
            C == 'e' || C == 'E' || C == '+' || C == '-' || C == '.';
   }
 
case '{': {
     eatWhitespace();
     if (peek() == '}') {
       ++P;
       return true;
     }
     for (;;) {
       if (next() != '"')
         return parseError("Expected object key");
       std::string K;
       if (!parseString(K))
         return false;
       eatWhitespace();
       if (next() != ':')
         return parseError("Expected : after object key");
       eatWhitespace();
       if (!parseValue(O[std::move(K)]))
         return false;
       eatWhitespace();
       switch (next()) {
       case ',':
         eatWhitespace();
         continue;
       case '}':

 void llvm::json::OStream::valueBegin() {
   assert(Stack.back().Ctx != Object && "Only attributes allowed here");
   if (Stack.back().HasValue) {
     assert(Stack.back().Ctx != Singleton && "Only one value allowed here");
     OS << ',';
   }
   if (Stack.back().Ctx == Array)
     newline();
+  flushComment();
   Stack.back().HasValue = true;
 }
 
+void OStream::comment(llvm::StringRef Comment) {
+  assert(PendingComment.empty() && "Only one comment per value!");
+  PendingComment = Comment;
+}
+
+void OStream::flushComment() {
+  if (PendingComment.empty())
+    return;
+  OS << (IndentSize ? "/* " : "/*");
+  // Be sure not to accidentally emit "*/". Transform to "* /".
+  while (!PendingComment.empty()) {
+    auto Pos = PendingComment.find("*/");
+    if (Pos == StringRef::npos) {
+      OS << PendingComment;
+      PendingComment = "";
+    } else {
+      OS << PendingComment.take_front(Pos) << "* /";
+      PendingComment = PendingComment.drop_front(Pos + 2);
+    }
+  }
+  OS << (IndentSize ? " */" : "*/");
+  // Comments are on their own line unless attached to an attribute value.
+  if (Stack.size() > 1 && Stack.back().Ctx == Singleton) {
+    if (IndentSize)
+      OS << ' ';
+  } else {
+    newline();
+  }
+}
+
 void llvm::json::OStream::newline() {
   if (IndentSize) {
     OS.write('\n');
     OS.indent(Indent);
   }
 }
 
 void llvm::json::OStream::arrayBegin() {
   valueBegin();
   Stack.emplace_back();
   Stack.back().Ctx = Array;
   Indent += IndentSize;
   OS << '[';
 }
 
 void llvm::json::OStream::arrayEnd() {
   assert(Stack.back().Ctx == Array);
   Indent -= IndentSize;
   if (Stack.back().HasValue)
     newline();
   OS << ']';
+  assert(PendingComment.empty());
   Stack.pop_back();
   assert(!Stack.empty());
 }
 
 void llvm::json::OStream::objectBegin() {
   valueBegin();
   Stack.emplace_back();
   Stack.back().Ctx = Object;
   Indent += IndentSize;
   OS << '{';
 }
 
 void llvm::json::OStream::objectEnd() {
   assert(Stack.back().Ctx == Object);
   Indent -= IndentSize;
   if (Stack.back().HasValue)
     newline();
   OS << '}';
+  assert(PendingComment.empty());
   Stack.pop_back();
   assert(!Stack.empty());
 }
 
 void llvm::json::OStream::attributeBegin(llvm::StringRef Key) {
   assert(Stack.back().Ctx == Object);
   if (Stack.back().HasValue)
     OS << ',';
   newline();
+  flushComment();
   Stack.back().HasValue = true;
   Stack.emplace_back();
   Stack.back().Ctx = Singleton;
   if (LLVM_LIKELY(isUTF8(Key))) {
     quote(OS, Key);
   } else {
     assert(false && "Invalid UTF-8 in attribute key");
     quote(OS, fixUTF8(Key));
   }
   OS.write(':');
   if (IndentSize)
     OS.write(' ');
 }
 
 void llvm::json::OStream::attributeEnd() {
   assert(Stack.back().Ctx == Singleton);
   assert(Stack.back().HasValue && "Attribute must have a value");
+  assert(PendingComment.empty());
   Stack.pop_back();
   assert(Stack.back().Ctx == Object);
 }
 
 } // namespace json
 } // namespace llvm
 
 void llvm::format_provider<llvm::json::Value>::format(
     const llvm::json::Value &E, raw_ostream &OS, StringRef Options) {
   unsigned IndentAmount = 0;
   if (!Options.empty() && Options.getAsInteger(/*Radix=*/10, IndentAmount))
     llvm_unreachable("json::Value format options should be an integer");
   json::OStream(OS, IndentAmount).value(E);
 }
 

llvm/unittests/Support/JSONTest.cpp

EXPECT_FALSE(fromJSON(Object{{"str", 1}, {"int", "string"}}, V))
       << "Wrong type for Optional<T> " << V;
 }
 
 TEST(JSONTest, Stream) {
   auto StreamStuff = [](unsigned Indent) {
     std::string S;
     llvm::raw_string_ostream OS(S);
     OStream J(OS, Indent);
+    J.comment("top*/level");
     J.object([&] {
       J.attributeArray("foo", [&] {
         J.value(nullptr);
+        J.comment("element");
         J.value(42.5);
         J.arrayBegin();
         J.value(43);
         J.arrayEnd();
       });
+      J.comment("attribute");
       J.attributeBegin("bar");
+      J.comment("attribute value");
       J.objectBegin();
       J.objectEnd();
       J.attributeEnd();
       J.attribute("baz", "xyz");
     });
     return OS.str();
   };
 
-  const char *Plain = R"({"foo":[null,42.5,[43]],"bar":{},"baz":"xyz"})";
+  const char *Plain =
+      R"(/*top* /level*/{"foo":[null,/*element*/42.5,[43]],/*attribute*/"bar":/*attribute value*/{},"baz":"xyz"})";
   EXPECT_EQ(Plain, StreamStuff(0));
-  const char *Pretty = R"({
+  const char *Pretty = R"(/* top* /level */
+{
   "foo": [
     null,
+    /* element */
     42.5,
     [
       43
     ]
   ],
-  "bar": {},
+  /* attribute */
+  "bar": /* attribute value */ {},
   "baz": "xyz"
 })";
   EXPECT_EQ(Pretty, StreamStuff(2));
 }
 
 } // namespace
 } // namespace json
 } // namespace llvm