std.data.json formal review

[Sönke Ludwig via Digitalmars-d]

Am 21.08.2015 um 19:30 schrieb Andrei Alexandrescu:
> On 8/18/15 12:54 PM, Sönke Ludwig wrote:
>> Am 18.08.2015 um 00:21 schrieb Andrei Alexandrescu:
>>> * On the face of it, dedicating 6 modules to such a small specification
>>> as JSON seems excessive. I'm thinking one module here. (As a simple
>>> point: who would ever want to import only foundation, which in turn has
>>> one exception type and one location type in it?) I think it shouldn't be
>>> up for debate that we must aim for simple and clean APIs.
>>
>> That would mean a single module that is >5k lines long. Spreading out
>> certain things, such as JSONValue into an own module also makes sense to
>> avoid unnecessarily large imports where other parts of the functionality
>> isn't needed. Maybe we could move some private things to "std.internal"
>> or similar and merge some of the modules?
>
> That would help. My point is it's good design to make the response
> proportional to the problem. 5K lines is not a lot, but reducing those
> 5K in the first place would be a noble pursuit. And btw saving parsing
> time is so C++ :o).

Most lines are needed for tests and documentation. Surely dropping some 
functionality would make the module smaller, too. But there is not a lot 
to take away without making severe compromises in terms of actual 
functionality or usability.

>> But I also think that grouping symbols by topic is a good thing and
>> makes figuring out the API easier. There is also always package.d if you
>> really want to import everything.
>
> Figuring out the API easily is a good goal. The best way to achieve that
> is making the API no larger than necessary.

So, what's your suggestion, remove all read*/skip* functions for 
example? Make them member functions of JSONParserRange instead of UFCS 
functions? We could of course also just use the pseudo modules that 
std.algorithm had for example, where we'd create a table in the 
documentation for each category of functions.

>> Another thing I'd like to add is an output range that takes parser nodes
>> and writes to a string output range. This would be the kind of interface
>> that would be most useful for a serialization framework.
>
> Couldn't that be achieved trivially by e.g. using map!(t => t.toString)
> or similar?
>
> This is the nice thing about rangifying everything - suddenly you have a
> host of tools at your disposal.

No, the idea is to have an output range like so:

     Appender!string dst;
     JSONNodeOutputRange r(&dst);
     r.put(beginArray);
     r.put(1);
     r.put(2);
     r.put(endArray);

This would provide a forward interface for code that has to directly 
iterate over its input, which is the case for a serializer - it can't 
provide an input range interface in a sane way. The alternative would be 
to either let the serializer re-implement all of JSON, or to just 
provide some primitives (writeJSON() that takes bool, number or string) 
and to let the serializer implement the rest of JSON (arrays/objects), 
which includes certain options, such as pretty-printing.

>>> - Also, at token level strings should be stored with escapes unresolved.
>>> If the user wants a string with the escapes resolved, a lazy range
>>> does it.
>>
>> To make things efficient, it currently stores escaped strings if slices
>> of the input are used, but stores unescaped strings if allocations are
>> necessary anyway.
>
> That seems a good balance, and probably could be applied to numbers as
> well.

With the difference that numbers stored as numbers never need to 
allocate, so for non-slicable inputs the compromise is not the same.

What about just offering basically three (CT selectable) modes:
- Always parse as double (parse lazily if slicing can be used) (default)
- Parse double or long (again, lazily if slicing can be used)
- Always store the string representation

The question that remains is how to handle this in JSONValue - support 
just double there? Or something like JSONNumber that abstracts away the 
differences, but makes writing generic code against JSONValue difficult? 
Or make it also parameterized in what it can store?

>>> - Validating UTF is tricky; I've seen some discussion in this thread
>>> about it. On the face of it JSON only accepts valid UTF characters. As
>>> such, a modularity-based argument is to pipe UTF validation before
>>> tokenization. (We need a lazy UTF validator and sanitizer stat!) An
>>> efficiency-based argument is to do validation during tokenization. I'm
>>> inclining in favor of modularization, which allows us to focus on one
>>> thing at a time and do it well, instead of duplicationg validation
>>> everywhere. Note that it's easy to write routines that do JSON
>>> tokenization and leave UTF validation for later, so there's a lot of
>>> flexibility in composing validation with JSONization.
>>
>> It's unfortunate to see this change of mind in face of the work that
>> already went into the implementation. I also still think that this is a
>> good optimization opportunity that doesn't really affect the
>> implementation complexity. Validation isn't duplicated, but reused from
>> std.utf.
>
> Well if the validation is reused from std.utf, it can't have been very
> much work. I maintain that separating concerns seems like a good
> strategy here.

There is more than the actual call to validate(), such as writing tests 
and making sure the surroundings work, adjusting the interface and 
writing documentation. It's not *that* much work, but nonetheless wasted 
work.

I also still think that this hasn't been a bad idea at all. Because it 
speeds up the most important use case, parsing JSON from a non-memory 
source that has not yet been validated. I also very much like the idea 
of making it a programming error to have invalid UTF stored in a string, 
i.e. forcing the validation to happen before the cast from bytes to chars.

>>> - Litmus test: if the input type is a forward range AND if the string
>>> type chosen for tokens is the same as input type, successful
>>> tokenization should allocate exactly zero memory. I think this is a
>>> simple way to make sure that the tokenization API works well.
>>
>> Supporting arbitrary forward ranges doesn't seem to be enough, it would
>> at least have to be combined with something like take(), but then the
>> type doesn't equal the string type anymore. I'd suggest to keep it to
>> "if is sliceable and input type equals string type", at least for the
>> initial version.
>
> I had "take" in mind. Don't forget that "take" automatically uses slices
> wherever applicable. So if you just use typeof(take(...)), you get the
> best of all worlds.
>
> The more restrictive version seems reasonable for the first release.

Okay.

>>> - The JSON value does its own internal allocation (for e.g. arrays and
>>> hashtables), which should be fine as long as it's encapsulated and we
>>> can tweak it later (e.g. make it use reference counting inside).
>>
>> Since it's based on (Tagged)Algebraic, the internal types are part of
>> the interface. Changing them later is bound to break some code. So AFICS
>> this would either require to make the types used parameterized (string,
>> array and AA types). Or to abstract them away completely, i.e. only
>> forward operations but deny direct access to the type.
>>
>> ... thinking about it, TaggedAlgebraic could do that, while Algebraic
>> can't.
>
> Well if you figure the general Algebraic type is better replaced by a
> type specialized for JSON, fine.
>
> What we shouldn't endorse is two nearly identical library types
> (Algebraic and TaggedAlgebraic) that are only different in subtle
> matters related to performance in certain use patterns.
>
> If integral tags are better for closed type universes, specialize
> Algebraic to use integral tags where applicable.

TaggedAlgebraic would not be a type specialized for JSON! It's useful 
for all kinds of applications and just happens to have some advantages 
here, too.

An (imperfect) idea for merging this with the existing Algebraic name:

template Algebraic(T)
     if (is(T == struct) || is(T == union))
{
	// ... implementation of TaggedAlgebraic ...
}

To avoid the ambiguity with a single type Algebraic, a UDA could be 
required for T to get the actual TaggedAgebraic behavior.

Everything else would be problematic, because TaggedAlgebraic needs to 
be supplied with names for the different types, so the Algebraic(T...) 
way of specifying allowed types doesn't really work. And, more 
importantly, because exploiting static type information in the generated 
interface means breaking code that currently is built around a Variant 
return value.

>>> - Why both parseJSONStream and parseJSONValue? I'm thinking
>>> parseJSONValue would be enough because then you trivially parse a stream
>>> with repeated calls to parseJSONValue.
>>
>> parseJSONStream is the pull parser (StAX style) interface. It returns
>> the contents of a JSON document as individual nodes instead of storing
>> them in a DOM. This part is vital for high-performance parsing,
>> especially of large documents.
>
> So perhaps this is just a naming issue. The names don't suggest
> everything you said. What I see is "parse a JSON stream" and "parse a
> JSON value". So I naturally assumed we're looking at consuming a full
> stream vs. consuming only one value off a stream and stopping. How about
> better names?

parseToJSONValue/parseToJSONStream? parseAsX?

>>> - readArray suddenly introduces a distinct kind of interacting -
>>> callbacks. Why? Should be a lazy range lazy range lazy range. An adapter
>>> using callbacks is then a two-liner.
>>
>> It just has a more complicated implementation, but is already on the
>> TODO list.
>
> Great. Let me say again that with ranges you get to instantly tap into a
> wealth of tools. I say get rid of the callbacks and let a "tee" take
> care of it for whomever needs it.

The callbacks would surely be dropped when ranges get available. 
foreach() should usually be all that is needed.

>>> - Why is readBool even needed? Just readJSONValue and then enforce it as
>>> a bool. Same reasoning applies to readDouble and readString.
>>
>> This is for lower level access, using parseJSONValue would certainly be
>> possible, but it would have quite some unneeded overhead and would also
>> be non- at nogc.
>
> Meh, fine. But all of this is adding weight to the API in the wrong places.

Frankly, I don't think that this is even the wrong place. The pull 
parser interface is the single most important part of the API when we 
talk about allocation-less and high-performance operation. It also 
really has low weight, as it's just a small function that joins the 
other read* functions quite naturally and doesn't create any additional 
cognitive load.

>
>>> - readObject is with callbacks again - it would be nice if it were a
>>> lazy range.
>>
>> Okay, is also already on the list.
>
> Awes!

It could return a Tuple!(string, JSONNodeRange). But probably there 
should also be an opApply for the object field range, so that foreach 
(key, value; ...) becomes possible.

>> But apart from that, algebraic is unfortunately currently quite unsuited
>> for this kind of abstraction, even if that can be solved in theory (with
>> a lot of work). It requires to write things like
>> obj.get!(JSONValue[string])["foo"].get!JSONValue instead of just
>> obj["foo"], because it simply returns Variant from all of its forwarded
>> operators.
>
> Algebraic does not expose opIndex. We could add it to Algebraic such
> that obj["foo"] returns the same type a "this".

https://github.com/D-Programming-Language/phobos/blob/6df5d551fd8a21feef061483c226e7d9b26d6cd4/std/variant.d#L1088
https://github.com/D-Programming-Language/phobos/blob/6df5d551fd8a21feef061483c226e7d9b26d6cd4/std/variant.d#L1348

> It's easy for anyone to say that what's there is unfit for a particular
> purpose. It's also easy for many to define a ever-so-slightly-different
> new artifact that fits a particular purpose. Where you come as a
> talented hacker is to operate with the understanding of the importance
> of making things work, and make it work.

The problem is that making Algebraic exploit static type information 
means nothing short of a complete reimplementation, which 
TaggedAlgebraic is. It also means breaking existing code, if, for 
example, alg[0] suddenly returns a string instead of just a Variant with 
a string stored inside.

>>> - JSONValue should be more opaque and not expose representation as much
>>> as it does now. In particular, offering a built-in hashtable is bound to
>>> be problematic because those are expensive to construct, create garbage,
>>> and are not customizable. Instead, the necessary lookup and set APIs
>>> should be provided by JSONValue whilst keeping the implementation
>>> hidden. The same goes about array - a JSONValue shall not be exposed;
>>> instead, indexed access primitives should be exposed. Separate types
>>> might be offered (e.g. JSONArray, JSONDictionary) if deemed necessary.
>>> The string type should be a type parameter of JSONValue.
>>
>> This would unfortunately at the same time destroy almost all benefits
>> that using (Tagged)Algebraic has, namely that it would opens up the
>> possibility to have interoperability between different data formats (for
>> example, passing a JSONValue to a BSON generator without letting the
>> BSON generator know about JSON). This is unfortunately an area that I've
>> also not yet properly explored, but I think it's important as we go
>> forward with other data formats.
>
> I think we need to do it. Otherwise we're stuck with "D's JSON API
> cannot be used without the GC". We want to escape that gravitational
> pull. I know it's hard. But it's worth it.

I can't fight the feeling that what Phobos currently has in terms of 
allocators, containters and reference counting is simply not mature 
enough to make a good decision here. Restricting JSONValue as much as 
possible would at least keep the possibility to extend it later, but I 
think that we can and should do better in the long term.