Skip to content

Yojson 3 compat #1534

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Leonidas-from-XIV
Copy link

Yojson 3 removed Tuple and Variant which makes the Json type mismatch with the one defined in this repo. This PR removes these variants and updates the constraint to require Yojson 3 so the type matches again.

Depends on ocaml/opam-repository#27956 merged or pinning Yojson.

@Leonidas-from-XIV Leonidas-from-XIV changed the title Yojson3 compat Yojson 3 compat May 30, 2025
@giltho giltho mentioned this pull request Jun 8, 2025
@coveralls
Copy link

coveralls commented Jun 8, 2025

Pull Request Test Coverage Report for Build 4870

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 23.48%

Totals Coverage Status
Change from base Build 4864: 0.003%
Covered Lines: 6042
Relevant Lines: 25733

💛 - Coveralls

@giltho
Copy link
Collaborator

giltho commented Jun 8, 2025

Re-ran the CI now that yojson3 has been merged, it seems that only the changelog change is required :)

@Leonidas-from-XIV
Copy link
Author

Added a changelog entry, hope it is fine as-is :)

@voodoos
Copy link
Collaborator

voodoos commented Jun 10, 2025

Thank you for taking the time to do the upgrade!

We haven't been doing a release in quite some time, and it is now imminent.
Are these changes going to prevent users that rely on a package that is not compatible with YJS 3 to install ocaml-lsp ?
Are they many incompatible packages ?

@Leonidas-from-XIV
Copy link
Author

Are these changes going to prevent users that rely on a package that is not compatible with YJS 3 to install ocaml-lsp ?

Yes, unfortunately. Maybe the code can be changed so the Tuple and Variant constructors are only conditionally added after parsing, something akin to [ #Yojson.Safe.t | Tuple | Variant ] but that would require some work to make sure that these two constructors get translated to values that can be represented in Yojson.Safe.t before passing it on to any Yojson.Safe.* functions.

If it matters, I can look into it on the next hackday if I can find an implementation that accepts the 2.x style type and always outputs the 3.x style type.

(A bit of self-promotion, but this is not a problem in dune pkg, as LSP is installed in a separate location and its dependencies do not conflict with the users project)

Are they many incompatible packages ?

Unfortunately all packages that currently use atdgen as it hasn't been updated for Yojson 3 yet. Apart from that not that many: ocaml/opam-repository#27956, ocf, ppx_deriving (only when building with tests), kind2, goblint, pa_ppx.

@voodoos
Copy link
Collaborator

voodoos commented Jun 10, 2025

(A bit of self-promotion, but this is not a problem in dune pkg, as LSP is installed in a separate location and its dependencies do not conflict with the users project)

That's great to know !

What I propose is that we hold onto this until the upcoming release, and after the release we will release a version for Yojson 3, hopefully backward compatible if someone finds a way 🙂

@giltho
Copy link
Collaborator

giltho commented Jun 10, 2025

If it matters, I can look into it on the next hackday if I can find an implementation that accepts the 2.x style type and always outputs the 3.x style type.

I tried that for a second, but you need to also modify ppx_yojson_conv (I think) because it hardcodes Yojson.Safe.t as an input to functions.

@voodoos Note that the inverse problem also exists, people using Yojson3 cannot install ocaml-lsp-server.

mseri added a commit to mseri/opam-repository that referenced this pull request Jun 27, 2025
Due to the removal of the Tuple and Variant variants.
See also ocaml/ocaml-lsp#1534.

Signed-off-by: Marcello Seri <[email protected]>
mseri added a commit to mseri/opam-repository that referenced this pull request Jun 27, 2025
Due to the removal of the Tuple and Variant variants.
See also ocaml/ocaml-lsp#1534.

Signed-off-by: Marcello Seri <[email protected]>
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.

4 participants