Skip to content

Conversation

@pitag-ha
Copy link
Member

The two important commits are:

  • Support binary AST's of any supported version

    Before, the OCaml version of any binary ast input had to coincide with the version the ppxlib driver got compiled with. That was a problem for bucklescript/rescript.

    With this commit, all versions from the Versions module are accepted as input. The input version is preserved by ppxlib when outputting a binary AST.

  • Implement run_as_ppx_rewriter internally

    Besides taking away a burden for Astlib, implementing run_as_ppx_rewriter internally has two advantages:

    • it allows making changes with more flexibility
    • run_as_ppx_rewriter gets syncronized with standalone_run_as_ppx_rewriter

    With this change, the three flags -loc-filename, apply, dont-apply are taken into account when given. Before, they were allowed to be passed in but not taken into account.

@pitag-ha pitag-ha requested a review from xclerc as a code owner January 11, 2021 19:24
@pitag-ha pitag-ha force-pushed the bucklescript-support branch 2 times, most recently from 5d3cd47 to d1b6412 Compare January 11, 2021 19:38
@pitag-ha pitag-ha force-pushed the bucklescript-support branch 2 times, most recently from 05cfe9d to f519d19 Compare January 11, 2021 19:50
@pitag-ha pitag-ha requested review from a user, NathanReb and ceastlund January 11, 2021 19:53
@pitag-ha
Copy link
Member Author

The ocaml-ci arm32 failure isn't related to this PR.

There is a minor change in behavior that I haven't added to the changelog: if someone hands in a binary AST with an unknown OCaml version to the standalone, we now raise instead of packing that error into an extension point and returning the corresponding AST; and same if the input file doesn't exist. Should I mention that in the changelog?

@pitag-ha pitag-ha changed the title Bucklescript support Add bucklescript support Jan 11, 2021
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good overall. The code definitely looks more straightforward now!

@ghost
Copy link

ghost commented Jan 13, 2021

There is a minor change in behavior that I haven't added to the changelog: if someone hands in a binary AST with an unknown OCaml version to the standalone, we now raise instead of packing that error into an extension point and returning the corresponding AST; and same if the input file doesn't exist. Should I mention that in the changelog?

It should go into the changelog, so that users can quickly identify which version introduced this change in behaviour.

But thinking about this again, packing such errors using the compiler AST versions seems like a reasonable behaviour and is more conservative, so I think we should do that instead. Could you do this change?

@pitag-ha pitag-ha force-pushed the bucklescript-support branch 2 times, most recently from e05a533 to f62c4bd Compare January 19, 2021 13:02
@pitag-ha
Copy link
Member Author

pitag-ha commented Jan 19, 2021

But thinking about this again, packing such errors using the compiler AST versions seems like a reasonable behaviour and is more conservative

Ok, I've done that now. Therefore, I've had to restructure the loading logic a bit.

@pitag-ha pitag-ha force-pushed the bucklescript-support branch from f62c4bd to ef5bc6f Compare January 19, 2021 13:23
@ghost
Copy link

ghost commented Jan 19, 2021

The only downside that has is that if the initial exception was raised without loc parameter, here I can't add it anymore. But that only has an impact for input doesn't exist errors (see here), where the input file is reported inside the error message anyways.

About that, looking at the Location.error type in the compiler, it should be possible to write a function update_loc : error -> Location.t -> error that updates the location of an error.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Changes since last review are looking good. Just need to avoid the double file opening, and perhaps factorise a bit more the *run_as_ppx functions

@ghost
Copy link

ghost commented Jan 19, 2021

I also pushed a slightly more idiomatic version of create_versioned_error_f

@pitag-ha pitag-ha force-pushed the bucklescript-support branch 3 times, most recently from 0847c68 to 242167f Compare January 20, 2021 13:30
@pitag-ha
Copy link
Member Author

Thanks! I think I've implemented all the changes you've suggested.

Besides taking away a burden for Astlib, implementing run_as_ppx_rewriter
internally has two advantages:

- it allows making changes with more flexibility
- run_as_ppx_rewriter gets syncronized with standalone_run_as_ppx_rewriter

With this change, the three flags -loc-filename, apply, dont-apply are
taken into account when given. Before, they were allowed to be passed in
but not taken into account.

Signed-off-by: Sonja Heinze <[email protected]>
Signed-off-by: Sonja Heinze <[email protected]>
@pitag-ha pitag-ha force-pushed the bucklescript-support branch from db60b77 to cfd2aa1 Compare January 20, 2021 14:34
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks all good!

Before, the OCaml version of any binary ast input had to coincide with
the version the ppxlib driver got compiled with. That was a problem for
bucklescript/rescript.

With this commit, all versions from the Versions module are accepted as
input. The input version is preserved by ppxlib when outputting a binary
AST.

Signed-off-by: Sonja Heinze <[email protected]>

More idiomatic version of create_versioned_error_f

Signed-off-by: Jeremie Dimino <[email protected]>

tweak

Signed-off-by: Jeremie Dimino <[email protected]>
@pitag-ha pitag-ha force-pushed the bucklescript-support branch from cfd2aa1 to a50690c Compare January 20, 2021 14:55
@pitag-ha pitag-ha merged commit 0235cd2 into ocaml-ppx:master Jan 20, 2021
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Jan 20, 2021
CHANGES:

- Driver (important for bucklescript): handling binary AST's, accept any
  supported version as input; preserve that version (ocaml-ppx/ppxlib#205, @pitag-ha)

- `-as-ppx`: take into account the `-loc-filename` argument (ocaml-ppx/ppxlib#197, @pitag-ha)

- Add input name to expansion context (ocaml-ppx/ppxlib#202, @pitag-ha)

- Add Driver.V2: give access to expansion context in whole file transformation
  callbacks of `register_transformation` (ocaml-ppx/ppxlib#202, @pitag-ha)

- Driver: take `-cookie` argument into account, also when the input is a
  binary AST (@pitag-ha, ocaml-ppx/ppxlib#209)

- `run_as_ppx_rewriter`: take into account the arguments
  `-loc-filename`, `apply` and `dont-apply` (ocaml-ppx/ppxlib#205, @pitag-ha)

- Location.Error: add functions `raise` and `update_loc`
  (ocaml-ppx/ppxlib#205, @pitag-ha)
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Jan 22, 2021
CHANGES:

- Fix ppxlib.traverse declaration and make it a deriver and not a rewriter
  (ocaml-ppx/ppxlib#213, @NathanReb)
- Driver (important for bucklescript): handling binary AST's, accept any
  supported version as input; preserve that version (ocaml-ppx/ppxlib#205, @pitag-ha)

- `-as-ppx`: take into account the `-loc-filename` argument (ocaml-ppx/ppxlib#197, @pitag-ha)

- Add input name to expansion context (ocaml-ppx/ppxlib#202, @pitag-ha)

- Add Driver.V2: give access to expansion context in whole file transformation
  callbacks of `register_transformation` (ocaml-ppx/ppxlib#202, @pitag-ha)

- Driver: take `-cookie` argument into account, also when the input is a
  binary AST (@pitag-ha, ocaml-ppx/ppxlib#209)

- `run_as_ppx_rewriter`: take into account the arguments
  `-loc-filename`, `apply` and `dont-apply` (ocaml-ppx/ppxlib#205, @pitag-ha)

- Location.Error: add functions `raise` and `update_loc`
  (ocaml-ppx/ppxlib#205, @pitag-ha)
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.

1 participant