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

Port to ppxlib 0.16 / ocaml-migrate-parsetree 2.0.0 #231

Closed
wants to merge 25 commits into from

Conversation

thierry-martinez
Copy link
Collaborator

This PR is an attempt to port ppx_deriving to ppxlib 0.16.

thierry-martinez and others added 24 commits May 23, 2020 01:09
To accomodate `Pconst_string` for OCaml 4.11, I suggest the following
changes:

- constructing a `Pconst_string` can be done with `Ast_helper.Const.string`
- for destructing a `Pconst_string`, this pull-request introduces
  `string_of_constant_opt` and `string_of_expression_opt` in
  `ppx_deriving.runtime`.

I think indeed that both functions can be useful for rev-dependencies
to write OCaml version agnostic code.

I add a compatibility implementation for `Option` module in
`ppx_deriving.runtime` to be able to write more idiomatic code, and
again, I think that it can be useful for rev-dependencies (as we
already do for `Result`).
`Result.result` and `result` seems to be incompatible, at least in
4.04.2: the "include (module type of Pervasives with ...)" above
forgets some equalities and should be replaced in another PR.
Suggested by Gabriel Scherer.
ocaml-ppx#222 (comment)

There should be:

- no space between the macro name Pconst_string_patt and
  the arguments (because the arguments are for the macro), but

- a space between the macro name Pconst_string and the
  arguments (because the arguments are not for the macro but for the
  underlying constructor).
Tentative lightning update for OCaml 4.11.0 before introducing ppxlib
…dencies

This should implement @gasche suggestion:
ocaml-ppx#183

We should do that for `master` as well (I expect there will be
more failures there!) but I begin here.
Suggested by Gabriel Scherer.
I think that CI results on recent versions are often those that are
the most interesting, so it is better to compute them first.
It is the default.
Here is a pull-request to allow reverse-dependencies to be tested.
ocaml/ocaml-ci-scripts#338
@gasche
Copy link
Contributor

gasche commented Sep 22, 2020

(Let me explicitate some context that is shared with @thierry-martinez, but may help other people finding the PR, or ourselves in the future.) In the previous implementation, we were careful to use Ppxlib but without using the OCaml AST version it encourages, rather using the "current" version whenever possible -- using ocaml-migrate-parsetree to transform from one to another. This choice was made to avoid a difficulty coming from Ppxlib's non-versioned design, where users are "stuck" with one version of OCaml that may be lagging a few releases behind the current OCaml release.

In recent months, ppxlib maintainers have taken step to reduce the lag, and announced their intention to follow closely new OCaml releases. This means that we will have to update more often (if we want to follow recent ppxlib releases), but our users will not pay the cost of the "lag", even if we use Ppxlib's AST directly instead of the "current" AST.

This PR applies this design change. This simplifies the implementation quite a bit, which is excellent news.

Another change in this PR is a removal of the driver-registration logic, which was done using ocaml-migrate-parsetree parser facilities. The ppxlib maintainers (who also maintain ocaml-migrate-parsetree) have decided to consolidate all driver logic in ppxlib, so we cannot do a non-ppxlib registration anymore, and we can again simplify the implementation by removing this feature.

In terms of code, this PR thus has very nice properties. I'm not completely sure that the change preserves compatibility for plugin authors, or whether we will have to upgrade plugins again.

@thierry-martinez
Copy link
Collaborator Author

Thanks, @gasche, for the recap. I tried to add REVDEPS to the CI: the result is not good yet, I will try to improve that tomorrow!

@kit-ty-kate
Copy link
Collaborator

Superseded by #237

@kit-ty-kate kit-ty-kate mentioned this pull request Oct 27, 2020
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.

3 participants