[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

244

nit: drop static, already in anon namespace

318

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

324

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

318

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

 }
 inline std::pair<Object::iterator, bool> Object::insert(KV E) {
   return try_emplace(std::move(E.K), std::move(E.V));
 }
 inline bool Object::erase(StringRef K) {
   return M.erase(ObjectKey(K));
 }
 
+/// A "cursor" marking a position within a Value.
+/// The Value is a tree, and this is the path from the root to the current node.
+/// This is used to associate errors with particular subobjects.
+class Path {
+public:
+  class Root;
+
+  /// Records that the value at the current path is invalid.
+  /// Message is e.g. "expected number" and becomes part of the final error.
+  /// This overwrites any previously written error message in the root.
+  void report(llvm::StringLiteral Message);
+
+  /// The root may be treated as a Path.
+  Path(Root &R) : Parent(nullptr), Seg(&R) {}
+  /// Derives a path for an array element: this[Index]
+  Path index(unsigned Index) const { return Path(this, Segment(Index)); }
+  /// Derives a path for an object field: this.Field
+  Path field(StringRef Field) const { return Path(this, Segment(Field)); }
+
+private:
+  /// One element in a JSON path: an object field (.foo) or array index [27].
+  /// Exception: the root Path encodes a pointer to the Path::Root.
+  class Segment {
+    uintptr_t Pointer;
+    unsigned Offset;
+
+  public:
+    Segment() = default;
+    Segment(Root *R) : Pointer(reinterpret_cast<uintptr_t>(R)) {}
+    Segment(llvm::StringRef Field)
+        : Pointer(reinterpret_cast<uintptr_t>(Field.data())),
+          Offset(static_cast<unsigned>(Field.size())) {}
+    Segment(unsigned Index) : Pointer(0), Offset(Index) {}
+
+    bool isField() const { return Pointer != 0; }
+    StringRef field() const {
+      return StringRef(reinterpret_cast<const char *>(Pointer), Offset);
+    }
+    unsigned index() const { return Offset; }
+    Root *root() const { return reinterpret_cast<Root *>(Pointer); }
+  };
+
+  const Path *Parent;
+  Segment Seg;
+
+  Path(const Path *Parent, Segment S) : Parent(Parent), Seg(S) {}
+};
+
+/// The root is the trivial Path to the root value.
+/// It also stores the latest reported error and the path where it occurred.
+class Path::Root {
+  llvm::StringRef Name;
+  llvm::StringLiteral ErrorMessage;
+  std::vector<Path::Segment> ErrorPath; // Only valid in error state. Reversed.
+
+  friend void Path::report(llvm::StringLiteral Message);
+
+public:
+  Root(llvm::StringRef Name = "") : Name(Name), ErrorMessage("") {}
+  // No copy/move allowed as there are incoming pointers.
+  Root(Root &&) = delete;
+  Root &operator=(Root &&) = delete;
+  Root(const Root &) = delete;
+  Root &operator=(const Root &) = delete;
+
+  /// Returns the last error reported, or else a generic error.
+  Error getError() const;
+};
+
 // Standard deserializers are provided for primitive types.
 // See comments on Value.
 inline bool fromJSON(const Value &E, std::string &Out) {
   if (auto S = E.getAsString()) {
     Out = std::string(*S);
     return true;
   }
   return false;

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/ADT/STLExtras.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cctype>
 
 namespace llvm {
 namespace json {
 
 Value &Object::operator[](const ObjectKey &K) {
bool operator==(const Value &L, const Value &R) {
   case Value::Array:
     return *L.getAsArray() == *R.getAsArray();
   case Value::Object:
     return *L.getAsObject() == *R.getAsObject();
   }
   llvm_unreachable("Unknown value kind");
 }
 
+void Path::report(llvm::StringLiteral Msg) {
+  // Walk up to the root context, and count the number of segments.
+  unsigned Count = 0;
+  const Path *P;
+  for (P = this; P->Parent != nullptr; P = P->Parent)
+    ++Count;
+  Path::Root *R = P->Seg.root();
+  // Fill in the error message and copy the path (in reverse order).
+  R->ErrorMessage = Msg;
+  R->ErrorPath.resize(Count);
+  auto It = R->ErrorPath.begin();
+  for (P = this; P->Parent != nullptr; P = P->Parent)
+    *It++ = P->Seg;
+}
+
+Error Path::Root::getError() const {
+  std::string S;
+  raw_string_ostream OS(S);
+  OS << (ErrorMessage.empty() ? "invalid JSON contents" : ErrorMessage);
+  if (ErrorPath.empty()) {
+    if (!Name.empty())
+      OS << " when parsing " << Name;
+  } else {
+    OS << " at " << (Name.empty() ? "(root)" : Name);
+    for (const Path::Segment &S : llvm::reverse(ErrorPath)) {
+      if (S.isField())
+        OS << '.' << S.field();
+      else
+        OS << '[' << S.index() << ']';
+    }
+  }
+  return createStringError(llvm::inconvertibleErrorCode(), OS.str());
+}
+
 namespace {
 // Simple recursive-descent JSON parser.
 class Parser {
 public:
   Parser(StringRef JSON)
       : Start(JSON.begin()), P(JSON.begin()), End(JSON.end()) {}
 
   bool checkUTF8() {
     size_t ErrOffset;
     if (isUTF8(StringRef(Start, End - Start), &ErrOffset))
       return true;
     P = Start + ErrOffset; // For line/column calculation.
     return parseError("Invalid UTF-8 sequence");
   }
 
case '"': {
     std::string S;
     if (parseString(S)) {
       Out = std::move(S);
       return true;
     }
     return false;
   }
   case '[': {
     Out = Array{};
     Array &A = *Out.getAsArray();
     eatWhitespace();
     if (peek() == ']') {
       ++P;
       return true;
     }
     for (;;) {
       A.emplace_back(nullptr);
       if (!parseValue(A.back()))
         return false;
       eatWhitespace();
       switch (next()) {
       case ',':
         eatWhitespace();

llvm/unittests/Support/JSONTest.cpp

 //===-- JSONTest.cpp - JSON unit tests --------------------------*- 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/raw_ostream.h"
+#include "llvm/Testing/Support/Error.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace llvm {
 namespace json {
 
 namespace {
TEST(JSONTest, Stream) {
   ],
   /* attribute */
   "bar": /* attribute value */ {},
   "baz": "xyz"
 })";
   EXPECT_EQ(Pretty, StreamStuff(2));
 }
 
+TEST(JSONTest, Path) {
+  Path::Root R("foo");
+  Path P = R, A = P.field("a"), B = P.field("b");
+  A.index(1).field("c").index(2).report("boom");
+  EXPECT_THAT_ERROR(R.getError(), FailedWithMessage("boom at foo.a[1].c[2]"));
+  B.field("d").field("e").report("bam");
+  EXPECT_THAT_ERROR(R.getError(), FailedWithMessage("bam at foo.b.d.e"));
+  P.report("oh no");
+  EXPECT_THAT_ERROR(R.getError(), FailedWithMessage("oh no when parsing foo"));
+}
+
 } // namespace
 } // namespace json
 } // namespace llvm