Add json mapper for pp_ast #526

pedrobslisboa · 2024-09-24T20:46:44Z

Description

Add json mapper on pp_ast to improve usage in tools such as AST Explorer.
The idea is to increment the @NathanReb PR #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.ml and check the result
Run ppxlib-pp-ast --json test.ml and check the result
Use others flags to see the result with --json

patricoferris

Just a quick comment from me, I don't think we would want to add yojson as a dependency of ppxlib for this feature (I might be mistaken).

The implementation uses the lifter to lift values to JSON values (not actually print them, like pp_simple_val does). So I wonder if this couldn't be its own little tool that depends on ppxlib?

One workaround if this were to be merged here would be to add yojson as a dependency for ppxlib-pp-ast and then ppxlib could just return the polymorphic variants that happen to correspond to the Yojson.Safe.t variants?

NathanReb · 2024-09-26T15:09:56Z

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!

pedrobslisboa · 2024-12-09T21:19:11Z

@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: jchavarri/astexplorer-refmt#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

NathanReb · 2024-12-10T09:37:02Z

That's awesome! Let me know when this is ready for review!

pedrobslisboa · 2024-12-10T17:33:16Z

src/pp_ast.ml

+let pp_simple_val_to_json fmt simple_val =
+  let rec aux indent fmt simple_val =
+    match simple_val with
+    | Unit -> Format.fprintf fmt {|"null"|}
+    | Int i -> Format.fprintf fmt "%d" i
+    | String s -> Format.fprintf fmt {|"%s"|} s
+    | Special s -> Format.fprintf fmt {|"%s"|} s
+    | Bool b -> Format.fprintf fmt "%b" b
+    | Char c -> Format.fprintf fmt {|"%c"|} c
+    | Float f -> Format.fprintf fmt "%f" f
+    | Int32 i32 -> Format.fprintf fmt "%ld" i32
+    | Int64 i64 -> Format.fprintf fmt "%Ld" i64
+    | Nativeint ni -> Format.fprintf fmt "%nd" ni
+    | Array l | Tuple l | List l ->
+        Format.fprintf fmt "[\n";
+        List.iteri
+          ~f:(fun i sv ->
+            if i > 0 then Format.fprintf fmt ",\n";
+            Format.fprintf fmt "%s" (String.make (indent + 2) ' ');
+            aux (indent + 2) fmt sv)
+          l;
+        Format.fprintf fmt "\n%s]" (String.make indent ' ')
+    | Record fields ->
+        Format.fprintf fmt "{\n";
+        List.iteri
+          ~f:(fun i (k, v) ->
+            if i > 0 then Format.fprintf fmt ",\n";
+            Format.fprintf fmt "%s\"%s\": " (String.make (indent + 2) ' ') k;
+            aux (indent + 2) fmt v)
+          fields;
+        Format.fprintf fmt "\n%s}" (String.make indent ' ')
+    | Constr (cname, []) -> Format.fprintf fmt {|"%s"|} cname
+    | Constr (cname, [ (Constr (_, _ :: _) as x) ]) ->
+        Format.fprintf fmt "{\n%s\"%s\": " (String.make (indent + 2) ' ') cname;
+        aux (indent + 2) fmt x;
+        Format.fprintf fmt "\n%s}" (String.make indent ' ')
+    | Constr (cname, [ x ]) ->
+        Format.fprintf fmt "{\n%s\"%s\": " (String.make (indent + 2) ' ') cname;
+        aux (indent + 2) fmt x;
+        Format.fprintf fmt "\n%s}" (String.make indent ' ')
+    | Constr (cname, l) ->
+        Format.fprintf fmt "{\n%s\"%s\": [\n" (String.make (indent + 2) ' ') cname;
+        List.iteri
+          ~f:(fun i sv ->
+            if i > 0 then Format.fprintf fmt ",\n";
+            Format.fprintf fmt "%s" (String.make (indent + 4) ' ');
+            aux (indent + 4) fmt sv)
+          l;
+        Format.fprintf fmt "\n%s]\n%s}" (String.make (indent + 2) ' ') (String.make indent ' ')
+  in
+  aux 0 fmt simple_val


@patricoferris @NathanReb I've just pushed this simple pretty-printing json function to remove the yojson.

One workaround if this were to be merged here would be to add yojson as a dependency for ppxlib-pp-ast and then ppxlib could return the polymorphic variants that happen to correspond to the Yojson.Safe.t variants?

I could let yojson as a ppxlib_pp_ast dep and return the poly-vars to print jsons only at ppxlib_pp_ast or on the usage of Ppxlib.pp_ast, but it looked hacky as we would have the "pretty-printing" not printing the json but return those poly vars.

Another alternative would be ppxlib-tools to have this dependency and pp_ast to use it.

But as it was not hard to create a simple pp for json, I created it. It isn't as pretty as yojson cause we break every array/list/tuple/record line, but it does not feel like a problem right now.

BTW, I tried to follow pp_simple_val way to print, but it had a strange indent structure, and I created this way. If any issue, just let me now.

pedrobslisboa · 2024-12-12T11:55:11Z

@NathanReb BTW, I think now it's ready for review

NathanReb · 2025-01-06T14:53:58Z

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?

NathanReb · 2025-01-06T14:54:41Z

Sorry for the delay by the way, the end of year holidays + the OCaml 5.03 release took quite a lot of my time!

pedrobslisboa · 2025-01-15T08:50:05Z

[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:

ppxlib/src/pp_ast.ml

Lines 368 to 370 in 4889a55

    
           let pp_with_config (type a) (lifter : a -> simple_val) 
        
               ?(config = Config.default) ?(printer = pp_simple_val) fmt (x : a) = 
        
             with_config ~config ~f:(fun () -> printer fmt (lifter x))

For example, on bin/pp_ast:

ppxlib/bin/pp_ast.ml

Lines 21 to 43 in a3210f5

    
           let rec simple_val_to_yojson : Pp_ast.simple_val -> Yojson.Basic.t = function 
        
             | Unit -> `Null 
        
             | Int i -> `Int i 
        
             | String s -> `String s 
        
             | Special s -> `String s 
        
             | Bool b -> `Bool b 
        
             | Char c -> `String (String.make 1 c) 
        
             | Float f -> `Float f 
        
             | Int32 i32 -> `Int (Int32.to_int i32) 
        
             | Int64 i64 -> `Int (Int64.to_int i64) 
        
             | Nativeint ni -> `Int (Nativeint.to_int ni) 
        
             | Array l -> `List (List.map simple_val_to_yojson l) 
        
             | Tuple l -> `List (List.map simple_val_to_yojson l) 
        
             | List l -> `List (List.map simple_val_to_yojson l) 
        
             | Record fields -> 
        
                 `Assoc (List.map (fun (k, v) -> (k, simple_val_to_yojson v)) fields) 
        
             | Constr (cname, []) -> `String cname 
        
             | Constr (cname, [ x ]) -> `Assoc [ (cname, simple_val_to_yojson x) ] 
        
             | Constr (cname, l) -> 
        
                 `Assoc [ (cname, `List (List.map simple_val_to_yojson l)) ] 
        
           let json_printer fmt value = 
        
             Yojson.Basic.pp fmt (simple_val_to_yojson value)

ppxlib/bin/pp_ast.ml

Lines 179 to 182 in a3210f5

    
           let custom_printer = if json then Some json_printer else None in 
        
           pp_ast ~config ?printer:custom_printer ast; 
        
           Format.printf "%!\n"; 
        
           Ok ()

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.

NathanReb

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?

NathanReb · 2025-01-15T09:27:31Z

src/pp_ast.mli

@@ -60,28 +60,46 @@ module Config : sig
          be. *)
 end

+type simple_val =


You're not responsible for this as I'm the one who came up with that name originally but since we're going to expose it, I think we should come up with something better.

I think some commonly used name for this kind of type is repr as in representation. If you have a suggestion I'd gladly hear it as I'm not super inspired here as the original name suggests 😄

repr looks like a good name, though.

NathanReb · 2025-01-15T09:29:28Z

src/pp_ast.mli

+  | List of simple_val list
+  | Special of string
+
+type printer = Format.formatter -> simple_val -> unit


I guess we could remove this alias in favor of a simpler repr pp don't you think?

NathanReb · 2025-01-15T09:32:27Z

src/pp_ast.mli

 type 'a pp = Format.formatter -> 'a -> unit
-type 'a configurable = ?config:Config.t -> 'a pp
+type 'a configurable = ?config:Config.t -> ?printer:printer -> 'a pp


For future reference: this could be considered a breaking change here.

Do you think it would make sense to make the ?printer a part of the config directly? That way this functions API does not change and we just add a new optional argument to Config.make.

We could eventually split the full config into a lifter config and a printing config.

For future reference: this could be considered a breaking change here.

I thought that adding this feature with backward compatibility would avoid that to be considered breaking change 😞

Adding it to the Config like this couldn't also be considered a breaking change, but for the Config?

I think Config.make will pretty much always be applied fully as opposed to the printer which could be partially applied with the ?config argument so it can then be used as a regular 'a pp.

You are right that strictly speaking it is the same kind of change but given the nature of the two functions, I think it's far less likely to break user code that way.

What do you think?

That makes sense to me

pedrobslisboa · 2025-01-15T09:59:26Z

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

pedrobslisboa · 2025-01-16T10:18:30Z

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

NathanReb · 2025-01-20T13:18:29Z

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.

Signed-off-by: Pedro B S Lisboa <[email protected]>

NathanReb · 2025-01-21T09:31:14Z

I'm trying to wrap up the 5.3 compat thing and I'll review and merge right after, sorry for the delay!

pedrobslisboa requested review from ceastlund, NathanReb, panglesd, patricoferris and pitag-ha as code owners September 24, 2024 20:46

pedrobslisboa force-pushed the feat/pp_ast__json branch 2 times, most recently from b73cfdd to a60d33c Compare September 24, 2024 21:30

pedrobslisboa mentioned this pull request Sep 25, 2024

Add json mapper for pp_ast NathanReb/ppxlib#3

Closed

patricoferris reviewed Sep 26, 2024

View reviewed changes

pedrobslisboa mentioned this pull request Dec 9, 2024

Add Pp_ast ppxlib to this repo jchavarri/astexplorer-refmt#1

Open

pedrobslisboa force-pushed the feat/pp_ast__json branch 3 times, most recently from 03a46a1 to 01ffa5b Compare December 10, 2024 14:11

pedrobslisboa commented Dec 11, 2024

View reviewed changes

pedrobslisboa force-pushed the feat/pp_ast__json branch 3 times, most recently from 4889a55 to a3210f5 Compare January 15, 2025 08:47

pedrobslisboa force-pushed the feat/pp_ast__json branch 3 times, most recently from 5dd552d to b12844d Compare January 15, 2025 08:57

NathanReb reviewed Jan 15, 2025

View reviewed changes

pedrobslisboa force-pushed the feat/pp_ast__json branch from b12844d to 2e69e20 Compare January 16, 2025 07:51

pedrobslisboa force-pushed the feat/pp_ast__json branch 2 times, most recently from c721653 to 434b564 Compare January 16, 2025 08:07

pedrobslisboa force-pushed the feat/pp_ast__json branch from 434b564 to 0342632 Compare January 16, 2025 10:19

pedrobslisboa force-pushed the feat/pp_ast__json branch from 0342632 to f4180c2 Compare January 21, 2025 03:07

Add json mapper for pp_ast

11c4ba2

Signed-off-by: Pedro B S Lisboa <[email protected]>

pedrobslisboa force-pushed the feat/pp_ast__json branch from f4180c2 to 11c4ba2 Compare January 21, 2025 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add json mapper for pp_ast #526

Add json mapper for pp_ast #526

pedrobslisboa commented Sep 24, 2024

patricoferris left a comment

NathanReb commented Sep 26, 2024

pedrobslisboa commented Dec 9, 2024

NathanReb commented Dec 10, 2024

pedrobslisboa Dec 10, 2024

pedrobslisboa commented Dec 12, 2024

NathanReb commented Jan 6, 2025

NathanReb commented Jan 6, 2025

pedrobslisboa commented Jan 15, 2025

NathanReb left a comment

NathanReb Jan 15, 2025

pedrobslisboa Jan 16, 2025

NathanReb Jan 15, 2025

NathanReb Jan 15, 2025

NathanReb Jan 15, 2025

pedrobslisboa Jan 16, 2025

NathanReb Jan 16, 2025

pedrobslisboa Jan 16, 2025

pedrobslisboa commented Jan 15, 2025

pedrobslisboa commented Jan 16, 2025

NathanReb commented Jan 20, 2025

NathanReb commented Jan 21, 2025

Add json mapper for pp_ast #526

Are you sure you want to change the base?

Add json mapper for pp_ast #526

Conversation

pedrobslisboa commented Sep 24, 2024

Description

How

How to test

patricoferris left a comment

Choose a reason for hiding this comment

NathanReb commented Sep 26, 2024

pedrobslisboa commented Dec 9, 2024

NathanReb commented Dec 10, 2024

Choose a reason for hiding this comment

pedrobslisboa commented Dec 12, 2024

NathanReb commented Jan 6, 2025

NathanReb commented Jan 6, 2025

pedrobslisboa commented Jan 15, 2025

What I did

NathanReb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobslisboa commented Jan 15, 2025

pedrobslisboa commented Jan 16, 2025

NathanReb commented Jan 20, 2025

NathanReb commented Jan 21, 2025