Page MenuHomePhabricator

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

Authored by sammccall on Sep 22 2020, 9:18 AM.

Details

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.

Diff Detail

Event Timeline

sammccall created this revision.Sep 22 2020, 9:18 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 22 2020, 9:18 AM
sammccall requested review of this revision.Sep 22 2020, 9:18 AM

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 accepted this revision.Sep 23 2020, 4:57 AM

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–253

nit:

auto P = parse ...;
if(!P)
  return Reply(P.takeError());
(Server.*Handler)(*P, std::move(Reply));
clang-tools-extra/clangd/ClangdLSPServer.h
195

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

198

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

llvm/include/llvm/Support/JSON.h
586

static_cast<unsigned>

590

.. 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 ?

605

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

610

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

624

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..

This revision is now accepted and ready to land.Sep 23 2020, 4:57 AM
sammccall marked 7 inline comments as done.Sep 23 2020, 2:16 PM

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

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

198

(again, vlog)

llvm/include/llvm/Support/JSON.h
590

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.

605

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)

610

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 updated this revision to Diff 293863.Sep 23 2020, 2:17 PM
sammccall marked 2 inline comments as done.

Address review comments.

This revision was landed with ongoing or failed builds.Sep 23 2020, 2:35 PM
This revision was automatically updated to reflect the committed changes.
sammccall reopened this revision.Sep 23 2020, 2:35 PM
This revision is now accepted and ready to land.Sep 23 2020, 2:35 PM
sammccall reopened this revision.Sep 23 2020, 3:34 PM
This revision is now accepted and ready to land.Sep 23 2020, 3:34 PM
This revision was automatically updated to reflect the committed changes.
sammccall reopened this revision.Sep 23 2020, 3:35 PM
This revision is now accepted and ready to land.Sep 23 2020, 3:35 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2020, 4:20 PM
sammccall reopened this revision.Sep 23 2020, 4:26 PM
This revision is now accepted and ready to land.Sep 23 2020, 4:26 PM
sammccall updated this revision to Diff 293892.Sep 23 2020, 4:28 PM

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

sammccall closed this revision.Sep 23 2020, 4:28 PM