Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Tuple & Variant #158

Conversation

Leonidas-from-XIV
Copy link
Member

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
Copy link
Member Author

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.

lib/read.mll Outdated Show resolved Hide resolved
lib/read.mll Outdated Show resolved Hide resolved
lib/read.mll Outdated Show resolved Hide resolved
lib/read.mll Outdated Show resolved Hide resolved
lib/read.mll Outdated Show resolved Hide resolved
lib/read.mll Outdated Show resolved Hide resolved
Thanks to @hhugo for pointing them out
This seems to be unused in Yojson and Atd
@Leonidas-from-XIV Leonidas-from-XIV added this to the 3.0.0 milestone May 31, 2024
@c-cube
Copy link
Member

c-cube commented May 31, 2024

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

@Leonidas-from-XIV
Copy link
Member Author

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

@mjambon
Copy link
Member

mjambon commented Jun 3, 2024

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 ahrefs/atd#412

Its not tested anymore and unused by Yojson
@Leonidas-from-XIV
Copy link
Member Author

@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 Leonidas-from-XIV merged commit ce6e250 into ocaml-community:master Jun 28, 2024
3 of 4 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the remove-tuple-variant branch June 28, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Remove support for Tuple and Variant
4 participants