[PATCH] D87335: [json] Create some llvm::Expected-based accessors

Mon Sep 21 02:15:17 PDT 2020

sammccall added a comment.

TL;DR:

- I agree we should solve this, thanks for pushing back
- I think the path-to-error (e.g. `processes[0].triple`) is critical in some cases
- this really seems like an marshaling concern rather than something for Value/Object
- happy to take a stab at a design but I think it'll need some prototyping

In D87335#2282676 <https://reviews.llvm.org/D87335#2282676>, @wallace wrote:

> it'd be interesting to figure out a way to create this API so that callers don't recreate this functionality. My general understanding is that showing some error message, even if incomplete, is better than showing almost nothing.

In D87335#2283040 <https://reviews.llvm.org/D87335#2283040>, @clayborg wrote:

> Right now all users of this code end up creating little extra functions that must work around this. lldb-vscode has it's own methods, the new IntelPT will do its own thing.
>
> I think these APIs make sense for the API itself as a user of the API from my lldb-vscode work. Can we iterate on this and come up with a solution?

I agree there's value here, let's try to build something really nice :-) One advantage of the current situation is it's clear that error-reporting is on the caller, so they have to deal with the question of whether it's good enough. If we provide an opinionated option then I think it's important it's good enough for most use cases.

One disadvantage of the current situation is you have to avoid `fromJSON`/`ObjectMapper` in favor of the caller inventing something much more verbose in order to get error messages - this seems like a comparable-sized problem to me.

> In D87335#2282034 <https://reviews.llvm.org/D87335#2282034>, @sammccall wrote:
>
>> - the error messages aren't *quite* good enough to be directly user-visible in the general case. e.g. "JSON object missing the 'id' field" - some hint to which (sub)object is often needed.
>
> I believe they are good enough, they say 'id' is missing, but then print the full JSON object that they are missing from right?

I have two concerns about this approach:

- the full JSON object may be huge (e.g. missing key in an object where sibling keys may be giant objects)
- the object where the error is found is not unique enough to be clear ("missing key 'id' in value '{}'" in a large object). Wrapping errors could help, but has its own problems.

I think the most important information we should be aiming to capture:

1. what the root object is, in the context of the application (caller must supply this of course)
2. the path from the root to the local error
3. some description of the local error

The second is not present here, and requires a different API to achieve. llvm::Expected is good for *exposing* errors, but both awkward and inefficient for assembling complex errors (especially in cases where failure to parse isn't a real error). So I'd expect conversion of some error description to an llvm::Error to happen at the end of parsing.

>> - because the getAsX() method names are taken, the new accessors are stuck with awkward names
>
> See my "expect" suggestion in inline comments?

Nice! I like `expectString()` etc a lot better. I think the answer for `getOptionalStringOrError()` is to drop that function - it seems fairly nonorthogonal and niche.

While IMO this solves the name problem, it would still be nice if we don't need these on the core data structure classes but rather treated this as a concern of marshaling functions, where it tends to come up. TraceSettingsParser seems to follow this pattern - obviously lack of error handling makes ObjectMapper unsuitable, but do you think it would otherwise work?

>> - Expected<pointer> (for object/array) is an awkward type, because -> doesn't really work and the meaning/impossibility of nullptr isn't obvious.
>
> Since this we are using Expected can we change the value to be "Object&" or "Array &" instead of a pointer?

Indeed! I thought `Expected<T&>` wasn't allowed, but looks like I was hallucinating. (and I have some project code to go clean up!)

> Most of these errors are when we are trying to extract a key/value pair from an Object. So I don't really think we need to pass around more context? to make things make sense.

(I think I covered this above and don't want to bore with repetition, but can go into more examples and stuff if that's interesting)

I have some ideas about this, but this comment is long enough. I'll dump some in the next one and then try to make time to prototype.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87335/new/

https://reviews.llvm.org/D87335