ocaml-ppx
Add json mapper for pp_ast
Description
Add json mapper on pp_ast to improve usage in tools such as AST Explorer.
The idea is to increment the @NathanReb PR https://github.com/ocaml-ppx/ppxlib/pull/517
@NathanReb, your opinion on how to structure it better is very welcome. @jchavarri @davesnx thank you for the help.
How
To use it, we just need to pass --json to ppxlib-pp-ast. It works with all other flags, such as --show-attrs and --show-loc.
How to test
- In this branch build the project
- Create a test.ml on the root and add some content
- Run
ppxlib-pp-ast test.mland check the result - Run
ppxlib-pp-ast --json test.mland check the result - Use others flags to see the result with
--json
Yes, as Patrick said, ppxlib should not depend on external libraries as it otherwise prevents them from being able to depend on ppxlib.
The Yojson dependent parts should go into ppxlib-pp-ast or just a seperate tool shipped with ppxlib-tools.
We could add a json lifter to ppxlib but given it's fairly easy to write one this could be done directly in ppxlib-tools.
Did you try to load the resulting JSON into AST explorer? It would be nice to have a working usecase for this before we merge this feature!
@NathanReb Hey ya!
Did you try to load the resulting JSON into AST explorer? It would be nice to have a working usecase for this before we merge this feature!
I've just pushed a PR on astexplorer refmt to add the ppxlib pp_ast working with this json feature: https://github.com/jchavarri/astexplorer-refmt/pull/1
This same feature could be used on ocaml-platform (Which I'm also working on to migrate to pp_ast). This print is a local demo of the extension running with pp_ast and json:
About the yojson, I'm working on removing it
My impression is that we could simplify this quite a bit by having a lifter exposed that returns a Yojson compatible value.
It could be built on top of the simple_val lifter, inheriting all the config in the process, although I'd imagine that when converting an AST to JSON, we don't care as much about minimizing the output but rather to make it as close to the original AST and let consumer tool do the work.
That way, we can have ppxlib-pp-ast use said lifter from ppxlib and use Yojson to actually print it out as valid JSON.
How does that sound?
Sorry for the delay by the way, the end of year holidays + the OCaml 5.03 release took quite a lot of my time!
[Add json mapper for pp_ast](/ocaml-ppx/ppxlib/pull/526/commits/6ae39ba9bf1b3d4514acf7a822076621ecd6c4a8)
I tried to keep any json syntax away from pp_ast.
What I did
I exposed the simple_value contract and changed the contract to let anyone add any printer to pp:
https://github.com/ocaml-ppx/ppxlib/blob/4889a553ffc7523030299bc6f71b9cec5cf5a6b4/src/pp_ast.ml#L368-L370
For example, on bin/pp_ast:
https://github.com/ocaml-ppx/ppxlib/blob/a3210f53806c80c642054cceac3daac1316afc59/bin/pp_ast.ml#L21-L43
https://github.com/ocaml-ppx/ppxlib/blob/a3210f53806c80c642054cceac3daac1316afc59/bin/pp_ast.ml#L179-L182
So, anyone wanting to print ASTs can choose to map simple_val and print it.
WDYT? That way, pp_ast can be more opened without any json type contract dependency.
This looks good! I think you're right, that's the simplest and most flexible solution here.
Could you add a couple tests in
test/ppxlib-pp-ast/, a changelog entry and update ppxlib-tools deps to reflect the changes?
Ops, I rebased it and forgot to add back the tests. Sorry
When you said, "update ppxlib-tools deps to reflect the changes?", what did you mean by that? I didn't get it.
Besides that, I think I updated the remaining content. LMK if anything is missing
When you said, "update ppxlib-tools deps to reflect the changes?", what did you mean by that? I didn't get it.
Now that ppxlib-tools depends on yojson, we need to declare this dependency in ppxlib-tools' opam file. We use dune to generate our opam file so what you need here is to update the relevant (package ...) stanza in dune-project to add yojson to the list of dependencies there.
The CI's green because we happen to also depend on it via the benchmark package but installing ppxlib-tools via opam would fail.
I'm trying to wrap up the 5.3 compat thing and I'll review and merge right after, sorry for the delay!
Hey, @NathanReb, long time no see. Life happened to me as well, with some tasks to work on. But I'm back to review the PRs I left.
I update this one with the main.
~Hey, @NathanReb, long time no see. Life happened to me as well, with some tasks to work on. But I'm back to review the PRs I left.~
~I update this one with the main.~
~I just noticed that the cram tests were not being executed on my local project, I need to identify why, and then I'll fix the errors and push~
UPDATE: I didn't notice the cram test was skipping when not running on ocaml 5.3.0 😆 Ready for review ✅
@pedrobslisboa nice, thanks! I'm trying to wrap up the 5.4 compat with @patricoferris but I'll review this shortly I promise!