Remove json_max type

[Leonidas-from-XIV]

While removing json I came across json_max and wondered what that is for. So when I deleted it, I figured out it works this way:

• There is a top-level Yojson module which has a type t, that contains all the variants.
• That module has another module, Yojson.Pretty which has the prettyprinter for the "biggest" type.
• All the Yojson.{Basic,Safe,Raw} variants upcast their t type to this biggest type and and then use the functions from Yojson.Pretty.
• (Amusingly, it is therefore possible to get a `String in a variant that does not define STRING on the CPPO level, since it always uses `String instead of `Stringlit, though this is mostly a weird behaviour and probably not a bug)

This PR changes it in a way that every of the variants has its own Pretty module which is specialized to only pretty-print the specific variant.

I've made this a separate PR since I would like to know your input:

• The code is in my opinion somewhat easier to understand since all the different variants (Basic, Safe, Raw use their own Pretty module.
• Potentially faster, since the pattern match is smaller, but I'd be surprised if it makes a nontrivial difference
• The generated code is larger since Pretty exists 4 times now instead of once.

This is separated into its separate PR since I don't want to press it on the one that is important for the 2.0.0 release.

(Note this PR is branched off #100)

[NathanReb]

Removing the json_max type seems sound but I'm unsure why you made those changes to Pretty or to be more specific how they are related to the removal of a type alias. I might be missing something!

[Leonidas-from-XIV]

I was also confused why removing this alias would ripple through so much code, until I understood what it is used for. I'll try to explain what is going on.

The current code has one Pretty module, Yojson.Pretty. This module has functions to pretty-print a very specific type — json_max, which includes all the possible variants (note that it includes mutually exclusive variants like `Int and `IntLit, so no parser ever generates values of json_max type).

Then each of the Basic/Safe/Raw has its own write2.ml which takes a {Basic,Safe,Raw}.t and uses Yojson.Pretty to pretty print it. Since these specialized types are subsets of json_max they get upcast (which is safe) to json_max so they can use the functions in the top-level Pretty module.

Removing the json_max name means:

1. Either I have to write out the full type by hand for the upcast which is very tedious
2. Or I have to have a Pretty module that actually matches the type t and not a superset of it.
3. Maybe some other solutions, but I couldn't think of anything on the spot, input welcome

I chose to do number 2, thus I made CPPO generate Pretty modules that would operate on the respective {Basic,Safe,Raw}.t types, so I had to add all the #ifdefs and now each of the modules has its own specialized Pretty module that only pretty-prints the exact subset of t.

Having gone through all this I am not sure it was worth it. It certainly is more code and the printer looks worse. On the other hand, I think it is now slightly more understandable, since the Basic/Safe/Raw variants do not share a mysterious Pretty module under the hood.

Hence this PR is somewhat more on the "well, I'll ask for input from others" side, since I am not sure it is a win or not.

[c-cube]

Haven't looked at the PR, but if `pretty` was just a private module, thanks to dune, it would be an implementation detail so there's no downside in keeping it, I think? Side note: cppo is annoying to me because it breaks crucial tooling (merlin). I tend to use a lot of codegen based on dune these days (I can show examples in containers) instead :-)

[Leonidas-from-XIV]

Haven't looked at the PR, but if pretty was just a private module, thanks to dune, it would be an implementation detail so there's no downside in keeping it, I think?

The json_max type is public so I thought I might remove it for 2.0.0 so I started pulling the string and the whole thing came with it.

But I can try to solve it in a different way, where the Pretty that works on json_max is in its own submodule and can be accessed without needing the json_max alias. Though personally I don't really like how Pretty just has to support everything.

I also dislike CPPO, for the same reason. I was thinking whether replacing the whole misery with functors or just using composition of functions that operate on polymorphic variants would work better. But reducing the need for this duplicated code and code generation (#105) would be even better.

[panglesd]

I think that this can be merged, in the interest of release 2.0 without the json_all type.

I don't think that moving the Pretty module inside the different variants is necessary, but I'm ok with the change. I agree it seems more natural than having one module for all variants, even if it also adds some cppo.

Anyway, I think that after the release we should work on finding a way to remove the cppo, to be able to use ocamlformat to format the code, and get Merlin to work again.

[panglesd]

I think that if we want to remove the json_max alias from the public API without making too much changes to the code, only removing this line is sufficient. It would stay defined in the implementation but hidden from the public. That would be enough for releasing 2.0, and we can decide about the cppo madness later.
Am I wrong?

[Leonidas-from-XIV]

I would agree. Removing it from the public API should free us from compatibility constraints and then we can implement the rest in any other way going forward.

I've incorporated your suggestions and rebased it on current master so the changelog and CI should succeed now.

[panglesd]

LGTM!