yojson icon indicating copy to clipboard operation
yojson copied to clipboard

Remove Tuple & Variant

Open Leonidas-from-XIV opened this issue 2 years ago • 1 comments

It has been long time on my mind but now that Yojson 2.x is finally out, we can start looking at the next step, Yojson 3.0 (which I assume should be a fairly minor issue issue, given most consumers probably don't ever deal with `Tuple or `Variant).

The code itself is pretty self-explanatory, it mostly eliminates the TUPLE and VARIANT CPPO variables and then deletes some support code.

There might be more code to be deleted, like start_any_variant/start_any_tuple. I assume these might be used by atd (need to look into it) so I just removed the part that parses non-standard syntax but maybe it can be removed altogether, along with the unit test that covers it.

Closes #104

Leonidas-from-XIV avatar Dec 27 '22 10:12 Leonidas-from-XIV

A bit of research has shown that atdgen generates both start_any_variant with the `Edgy_bracket case as well as start_any_tuple, so the reading code there would most likely need to be adapted.

Leonidas-from-XIV avatar Dec 27 '22 14:12 Leonidas-from-XIV

Is this going to kill ppx_deriving_yojson ? I think it targets the safe fragment so it might expect Tuple and Variant as inputs…

c-cube avatar May 31 '24 13:05 c-cube

@c-cube I don't think it will break ppx_deriving_yojson too bad, as it encodes tuples as lists and variants as lists with the name of the constructor as first argument, if I remember correctly. Here's the code: https://github.com/ocaml-ppx/ppx_deriving_yojson/blob/61f2d63138ec1efea0a3fb4afa682f40497ec380/src/ppx_deriving_yojson.ml#L115-L148

The biggest (only?) issue will probably be Atd which uses Tuple and Variant, but I believe it would make sense in that case to adopt the same encoding strategy that ppx_deriving_yojson uses. Which has the advantage that the JSON it generates is standard-compliant and thus more interoperable with non-OCaml tools.

Leonidas-from-XIV avatar May 31 '24 13:05 Leonidas-from-XIV

The biggest (only?) issue will probably be Atd which uses Tuple and Variant

I don't think it's a big issue, and it's a good move forward.

Atdgen will make its -j-std option the default, and the old default behavior that was supporting the experimental syntax for tuples and variants will be removed.

To see the dependencies on Yojson functions, I used this:

~/atd/atdgen/src $ git grep -oP 'Yojson.[^" ]+' | sort | uniq -c
      2 ocaml.ml:Yojson.Safe.t
      1 oj_emit.ml:Yojson.End_of_object
      5 oj_emit.ml:Yojson.End_of_tuple
      1 oj_emit.ml:Yojson.Safe.finish_string
      1 oj_emit.ml:Yojson.Safe.init_lexer
      2 oj_emit.ml:Yojson.Safe.lexer_state
      1 oj_emit.ml:Yojson.Safe.map_ident
      1 oj_emit.ml:Yojson.Safe.read_comma
      2 oj_emit.ml:Yojson.Safe.read_gt
      1 oj_emit.ml:Yojson.Safe.read_ident
      1 oj_emit.ml:Yojson.Safe.read_lcurl
      2 oj_emit.ml:Yojson.Safe.read_null_if_possible
      1 oj_emit.ml:Yojson.Safe.read_object_end
      1 oj_emit.ml:Yojson.Safe.read_object_sep
      1 oj_emit.ml:Yojson.Safe.read_rbr
     16 oj_emit.ml:Yojson.Safe.read_space
      1 oj_emit.ml:Yojson.Safe.read_tuple_end2
      4 oj_emit.ml:Yojson.Safe.read_tuple_sep2
      2 oj_emit.ml:Yojson.Safe.skip_json
      1 oj_emit.ml:Yojson.Safe.start_any_tuple
      1 oj_emit.ml:Yojson.Safe.start_any_variant
      1 oj_emit.ml:Yojson.Safe.to_string
      1 oj_emit.ml:Yojson.Safe.write_bool
      1 oj_emit.ml:Yojson.Safe.write_float
      1 oj_emit.ml:Yojson.Safe.write_float_prec
      1 oj_emit.ml:Yojson.Safe.write_int
      1 oj_emit.ml:Yojson.Safe.write_json
      1 oj_emit.ml:Yojson.Safe.write_null
      1 oj_emit.ml:Yojson.Safe.write_std_float
      1 oj_emit.ml:Yojson.Safe.write_std_float_prec
      1 oj_emit.ml:Yojson.Safe.write_string
      1 omelange_emit.ml:Yojson.Safe.to_string

Occurrences such as Yojson.End_of_tuple, Yojson.Safe.read_tuple_end2, and a few others will have to be removed.

See https://github.com/ahrefs/atd/issues/412

mjambon avatar Jun 03 '24 20:06 mjambon

@mjambon That's a great list, will be very handy!

I've been looking at the std variants and with the removal of tuple and variant the only non-standard thing seems to be the printing of floats (NaN, +Infinity, -Infinity). I see that atdgen is currently emitting both write_float(_prec) and write_std_float(_prec). Looking at atdgen it seems to be configurable (p.std) which variant it will use.

I'm thinking that as we're breaking compatibility anyway to become more JSON-compliant, might be sensible to bite the bullet and make the std variants the only option, thus failing to encode unrepresentable floats in JSON. WDYT?

Leonidas-from-XIV avatar Jun 28 '24 08:06 Leonidas-from-XIV

This has broken some dependents of yojson: https://github.com/tmcgilchrist/ocaml-gitlab/issues/101

barracuda156 avatar Oct 15 '25 03:10 barracuda156