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

Add Pp_ast module and ppxlib-pp-ast executable for pretty-printing ppxlib ASTs #517

Merged
merged 17 commits into from
Sep 23, 2024

Conversation

NathanReb
Copy link
Collaborator

This adds pretty-printing functions to our API along with a simple executable that can be used to pretty-print input .ml, .mli, expressions, patterns or core types.

It's API is similar to ppx_tools's dumpast except that it can also read binary ASTs of any version which I assume could come in handy in certain situation.

The printing style is also similar to dumpast:

  • It prints expressions directly as their pexp_desc field and does so for any other such record with a _desc field
  • It does not print locations, positions or attributes to avoid cluttering the output
  • It prints 'a loc types directly as 'a

In the future we can add a bit of configuration there to control what gets printed or not but it is already pretty useful as it is.

I'd even consider using this with our -dparsetree driver option instead of the current sexp printer.

CHANGES.md Outdated Show resolved Hide resolved
@NathanReb
Copy link
Collaborator Author

I still need to add a few tests before this is good to merge.

It uses a bit of private API and re-implements a few small parts of Ppxlib.Utils that aren't exposed but given the amount of code that's impacted I thought it would be simpler to do that than to try and expose those to the executable while keeping them private to the outside world.

here's an example of how let x = 2 + 2 gets printed out:

$ dune exec -- ppxlib-pp-ast test.ml
[ Pstr_value                           
    ( Nonrecursive
    , [ { pvb_pat = Ppat_var "x"
        ; pvb_expr =
            Pexp_apply
              ( Pexp_ident (Lident "+")
              , [ ( Nolabel, Pexp_constant (Pconst_integer ( "2", None)))
                ; ( Nolabel, Pexp_constant (Pconst_integer ( "2", None)))
                ]
              )
        ; pvb_attributes = __attrs
        ; pvb_loc = __loc
        }
      ]
    )
]

I'm not a Format expert yet so there might be a few bugs or quirks but I'm hoping this can be fixed iteratively rather than waiting for it to be perfect before releasing it.

Please let me know what you think!

@NathanReb
Copy link
Collaborator Author

Just added a config option to allow showing attributes as it turns out I needed this to investigate a migration bug.

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I think waiting for the migration bug fix before merging might help uncover more useful features to add (e.g. you needed the --show-attrs configuration flag).

bin/pp_ast.ml Outdated Show resolved Hide resolved
src/pp_ast.mli Outdated Show resolved Hide resolved
bin/pp_ast.ml Outdated Show resolved Hide resolved
@NathanReb
Copy link
Collaborator Author

I'll try to address the documentation related concerns and the exposure of private API parts for internal use but then I think this is good to go!

@NathanReb
Copy link
Collaborator Author

I added loc printing and relevant configuration!

@NathanReb
Copy link
Collaborator Author

I'm also wondering whether the tool should be installed as part of ppxlib's main package or a separate, ppxlib-tools one.

@NathanReb
Copy link
Collaborator Author

I improved the documentation a bit, let me know what you think @patricoferris !

@patricoferris
Copy link
Collaborator

The new documentation looks great!

I'm also wondering whether the tool should be installed as part of ppxlib's main package or a separate, ppxlib-tools one.

ppxlib-tools would just be to attach some executables too?

@NathanReb NathanReb mentioned this pull request Sep 9, 2024
@NathanReb
Copy link
Collaborator Author

ppxlib-tools would just be to attach some executables too?

Yep, in order to keep ppxlib minimal as not every ppx user would be interested in ppxlib-pp-ast.

@NathanReb
Copy link
Collaborator Author

In addition, we could also extend the set of dependencies for such tools to things like cmdliner for instance, whereas we try to keep the set of dependencies for ppxlib to a bare minimum.

@NathanReb
Copy link
Collaborator Author

This should be the final round, I extracted ppxlib-pp-ast to a separate package and rewrote the tool itself using cmdliner.

I also added the ability to read source code directly from the command line which can be extremely convenient to quickly understand how to represent simple expressions or types for instance. This feature was part of ppx_tools' dumpast and I quickly realized it wasn't very convenient to go through a file or stdin for the simplest use cases.

I also took the time to write a bit of documentation in the manpage so hopefully the tool will be easier to use.

Copy link
Collaborator

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! Happy for formatting bugs to be follow up PRs if they do arise, this would already be very useful for debugging migration issues!

bin/pp_ast.ml Show resolved Hide resolved
NathanReb and others added 6 commits September 23, 2024 10:28
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>

Co-authored-by: Patrick Ferris <patrick@sirref.org>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
@NathanReb NathanReb merged commit 456d1a9 into ocaml-ppx:main Sep 23, 2024
5 of 6 checks passed
@patricoferris patricoferris mentioned this pull request Sep 23, 2024
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.

2 participants