Tentative lightning update for OCaml 4.11.0 before introducing ppxlib #222

thierry-martinez · 2020-05-22T23:24:09Z

This pull-request tries to propose a small set of changes relatively to the last release to make an intermediate release (let say 4.5.0) before migrating to ppxlib.
It appears that the migration to ppxlib cannot be made without breaking backward compatibility, therefore it makes sense to make it a major release, i.e. 5.0.0.
Meanwhile, it can be useful to propose another release, a minor one this time, just to provide compatibility with OCaml 4.11.0, while breaking backward compatibility as few as possible.
This work was done above the work already done by @kit-ty-kate in #220 . Many thanks to her!

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`).

gasche · 2020-05-23T05:10:14Z

I am not convinced that it is correct to move common logic to ppx_deriving_runtime. The intent of this module is to expose definitions that generated code uses, not helper definitions for the code generators. (Adding a dependency to compilerlibs there feels wrong, for example.) For now I would rather inline the helpers in each module, or add a small library to provide code in ppx_deriving_main and api/ppx_deriving.

Also: we should have a pre-ppxlib branch that this PR would use as target.

@gasche

Suggested by @gasche.

thierry-martinez · 2020-05-23T09:17:17Z

@gasche Thanks for suggesting the branch creation, and you are right, the helpers for destructing strings should be in the API (I don't think we need another module for helpers). I propose to keep Option compatibility in runtime though, next to Result.

gasche · 2020-05-23T09:32:37Z

Thanks! The string_of_*_opt operations are very convincing, they fit nicely within the existing API. I am slightly frustrated that api/ppx_deriving now depends on ppx_deriving_runtime, but this is not problematic, and I agree having an Option compatibility layer there is sensible -- it may be helpful to some plugins.

gasche · 2020-05-23T09:33:15Z

This is good to merge if the CI agrees.

gasche · 2020-05-23T10:06:50Z

Edit: wrong thread..

thierry-martinez · 2020-05-23T12:34:48Z

CI needs that ppx_tools be ported to 4.11 (ocaml-ppx/ppx_tools#82) to succeed.

ocaml-ppx/ppx_deriving#222

gasche

Personally I believe that this is good to merge even if the 4.11 CI is not completely happy -- this is a good PR and we can refine later. That said, you seem to be playing CI golf right now, so I took the opportunity to add a small nitpick remark.

gasche · 2020-05-23T13:13:39Z

src/api/ppx_deriving.cppo.ml

+  | Pconst_string (s, _, _) ->
+  #else
+  | Pconst_string (s, _) ->
+  #endif


Nitpick: it would be a bit more idiomatic in this file to define Pconst_string as a macro (or Pconst_string_patt if we have reasons to believe that we would also need it as a term-former) to have the same interface on all versions, at the top of the file (where we have other definitions in this style), and use the macro here.

#if OCAML_VERSION >= (4, 11, 0) #define Pconst_string(s, loc) Pconst_string(s, loc, _) #endif

Suggested by Gabriel Scherer. ocaml-ppx#222 (comment)

gasche · 2020-05-23T14:17:34Z

@thierry-martinez I will not work on this more this afternoon. If you get to a point where you are satisfied, please feel free to merge. (You may want to cleanup the history first.)

thierry-martinez · 2020-05-23T14:34:43Z

@gasche Ok, I will do that. Thank you very much for your help!

`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).

ocaml-ppx/ppx_tools#82

thierry-martinez · 2020-05-23T22:42:25Z

Finally merged! Sorry for CI golfing!

ocaml-ppx/ppx_deriving#222

@polyprinter

CHANGES: 5.0 (26/10/2020) ---------------- * Migrate to ppxlib ocaml-ppx/ppx_deriving#206, ocaml-ppx/ppx_deriving#210 (Anton Kochkov, Gabriel Scherer, Thierry Martinez) 4.5 --- * Add support for OCaml 4.11. - `Ppx_deriving.string_of_{constant,expression}_opt` to destruct `Pconst_string` in a version-independent way ocaml-ppx/ppx_deriving#220, ocaml-ppx/ppx_deriving#222 (Kate Deplaix, Thierry Martinez, review by Gabriel Scherer) * Stronger type equalities in `Ppx_deriving_runtime` (for instance, `Ppx_deriving_runtime.result` and `Result.result` are now compatible with all OCaml versions) ocaml-ppx/ppx_deriving#223, ocaml-ppx/ppx_deriving#225 (Thierry Martinez, review by Gabriel Scherer) * `Ppx_deriving_runtime.Option` compatibility module ocaml-ppx/ppx_deriving#222 (Thierry Martinez, review by Gabriel Scherer) 4.4.1 ----- * Add support for OCaml 4.10 ocaml-ppx/ppx_deriving#211 (Kate Deplaix, review by Gabriel Scherer) 4.4 --- * Restore support for OCaml 4.02.3 ocaml-ppx/ppx_deriving#188 (ELLIOTTCABLE) * workaround Location.input_filename being empty when using reason-language-server ocaml-ppx/ppx_deriving#196 (Ryan Artecona) * Add support for OCaml 4.08.0 ocaml-ppx/ppx_deriving#193, ocaml-ppx/ppx_deriving#197, ocaml-ppx/ppx_deriving#200 (Gabriel Scherer) 4.3 --- * use Format through Ppx_deriving_runtime to avoid deprecation warning for users of JaneStreet Base (Stephen Bastians and Gabriel Scherer, review by whitequark) * silence a ambiguous-field warning (41) in generated code ocaml-ppx/ppx_deriving#163 (Étienne Millon, review by Gabriel Scherer) * use dune ocaml-ppx/ppx_deriving#170 (Rudi Grinberg, Jérémie Dimino) * silence an unused-value warning for show ocaml-ppx/ppx_deriving#179 (Nathan Rebours) 4.2.1 ----- * Add support for OCaml 4.06.0 ocaml-ppx/ppx_deriving#154, ocaml-ppx/ppx_deriving#155, ocaml-ppx/ppx_deriving#156, ocaml-ppx/ppx_deriving#159 (Gabriel Scherer, Fabian, Leonid Rozenberg) * Consider { with_path = false } when printing record fields ocaml-ppx/ppx_deriving#157 (François Pottier) 4.2 --- * Add support for OCaml 4.05.0. * Use the `ocaml-migrate-parsetree` library to support multiple versions of OCaml. * Fix comparison order of fields in records (ocaml-ppx/ppx_deriving#136). * Silence an `unused rec flag` warning in generated code (ocaml-ppx/ppx_deriving#137). * Monomorphize comparison function for builtin types (ocaml-ppx/ppx_deriving#115) * Raise an error when `type nonrec` is encountered (ocaml-ppx/ppx_deriving#116). * Display an error message when dynamic package loading fails. * Add a `with_path` option to `@@deriving` to skip the module path in generated code (ocaml-ppx/ppx_deriving#120). The homepage for the project has now moved to: <https://github.com/ocaml-ppx/ppx_deriving> 4.1 --- * Fix type error with inheritied polymorphic variant type in [@@deriving map]. * Fix incorrect handling of multi-argument constructors in [@@deriving show]. * Add API hooks for ppx_type_conv. 4.0 --- * Show, eq, ord, map, iter, fold: add support for `Result.result`. * Ppx_deriving.Arg: use Result.result instead of polymorphic variants. * Ppx_deriving.sanitize: parameterize over an opened module. * Add support for `[@@deriving]` in module type declarations. * Add support for loading findlib packages instead of just files in ppx_deriving_main. * Treat types explicitly qualified with Pervasives also as builtin. * Compatibility with statically linked ppx drivers. 3.1 --- * Show, eq, ord: hygienically invoke functions from referenced modules (such as X.pp for X.t when deriving show) to coexist with modules shadowing ones from standard library. * Iter, map, fold: hygienically invoke List and Array functions. 3.0 --- * Implement hygiene: Ppx_deriving.{create_quoter,quote,sanitize,with_quoter}. * Show, eq, ord: add support for `lazy_t`. * Add support for `[@nobuiltin]` attribute. * Add Ppx_deriving.hash_variant. * Remove allow_std_type_shadowing option. * Remove Ppx_deriving.extract_typename_of_type_group. 2.1 --- * Fix breakage occurring with 4.02.2 w.r.t record labels * Fix prefixed attribute names (`[@deriving.foo.attr]` and `[@foo.attr]`). * Add allow_std_type_shadowing option for eq and show. 2.0 --- * Add support for open types. 1.1 --- * New plugin: create. * Show, eq, ord: handle `_`. * Show, eq, ord, map, iter, fold: handle inheriting from a parametric polymorphic variant type. * Make `Ppx_deriving.poly_{fun,arrow}_of_type_decl` construct functions in correct order. This also fixes all derivers with types with more than one parameter. * Add `Ppx_deriving.fold_{left,right}_type_decl`. 1.0 --- * Make deriver names lowercase. * Remove Findlib+dynlink integration. All derivers must now be explicitly required. * Allow shortening [%derive.x:] to [%x:] when deriver x exists. * Make `Ppx_deriving.core_type` field optional to allow ignoring unsupported [%x:] shorthands. * Add support for [@@deriving foo { optional = true }] that does not error out if foo is missing, useful for optional dependencies. * Rename ~name and ~prefix of `Ppx_deriving.attr` and `Ppx_deriving.Arg.payload` to `~deriver`. * Renamed `Ppx_deriving.Arg.payload` to `get_attr`. * Add `Ppx_deriving.Arg.get_expr` and `get_flag`. 0.3 --- * Show, Eq, Ord, Iter, Fold: handle ref. * Show: handle functions. * Show: include break hints in format strings. * Show: pull fprintf into local environment. * Show: add `[@polyprinter]` and `[@opaque]`. * Add `Ppx_deriving.Arg.expr`. 0.2 --- * New plugins: Enum, Iter, Map, Fold. * All plugins: don't concatenate affix if type is named `t`. * Add `[%derive.Foo:]` shorthand. * Show, Eq, Ord: add support for list, array, option. * Show: include full module path in output, including for types with manifest. * A lot of changes in `Ppx_deriving interface`. 0.1 --- * Initial release.

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).

thierry-martinez added 2 commits May 23, 2020 01:09

Test OCaml 4.11 in CI

842032e

thierry-martinez changed the base branch from master to preppx May 23, 2020 08:42

thierry-martinez changed the base branch from preppx to pre-ppxlib May 23, 2020 08:44

thierry-martinez added 2 commits May 23, 2020 10:52

Move string_of_{constant,expression}_opt to api

d6f5149

Suggested by @gasche.

Remove unused cppo definition

38816d5

thierry-martinez added a commit to thierry-martinez/ppx_deriving_yojson that referenced this pull request May 23, 2020

Tentative pinning ppx_deriving pre-ppxlib PR (ppx_deriving#222)

0de0565

ocaml-ppx/ppx_deriving#222

gasche reviewed May 23, 2020

View reviewed changes

thierry-martinez added a commit to thierry-martinez/ppx_deriving that referenced this pull request May 23, 2020

Define Pconst_string_patt

e16e4d0

Suggested by Gabriel Scherer. ocaml-ppx#222 (comment)

thierry-martinez added 3 commits May 23, 2020 20:48

Open Ppx_deriving_runtime for compatibility modules

f89e120

`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.

Compatibility with 4.02

2a6f521

thierry-martinez force-pushed the 411 branch from f4c06e9 to f34405e Compare May 23, 2020 18:56

Pin ppx_tools#82 to pass OCaml 4.11 CI tests

535f76a

ocaml-ppx/ppx_tools#82

thierry-martinez force-pushed the 411 branch from f34405e to 535f76a Compare May 23, 2020 22:13

thierry-martinez merged commit 3b3b1e4 into ocaml-ppx:pre-ppxlib May 23, 2020

thierry-martinez added a commit to thierry-martinez/ppx_deriving_yojson that referenced this pull request May 24, 2020

Pinning ppx_deriving pre-ppxlib branch

b6fbd6b

ocaml-ppx/ppx_deriving#222

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tentative lightning update for OCaml 4.11.0 before introducing ppxlib #222

Tentative lightning update for OCaml 4.11.0 before introducing ppxlib #222

thierry-martinez commented May 22, 2020 •

edited by gasche

Loading

gasche commented May 23, 2020

thierry-martinez commented May 23, 2020

gasche commented May 23, 2020

gasche commented May 23, 2020

gasche commented May 23, 2020 •

edited

Loading

thierry-martinez commented May 23, 2020

gasche left a comment

gasche May 23, 2020

gasche commented May 23, 2020

thierry-martinez commented May 23, 2020

thierry-martinez commented May 23, 2020

Tentative lightning update for OCaml 4.11.0 before introducing ppxlib #222

Tentative lightning update for OCaml 4.11.0 before introducing ppxlib #222

Conversation

thierry-martinez commented May 22, 2020 • edited by gasche Loading

gasche commented May 23, 2020

thierry-martinez commented May 23, 2020

gasche commented May 23, 2020

gasche commented May 23, 2020

gasche commented May 23, 2020 • edited Loading

thierry-martinez commented May 23, 2020

gasche left a comment

Choose a reason for hiding this comment

gasche May 23, 2020

Choose a reason for hiding this comment

gasche commented May 23, 2020

thierry-martinez commented May 23, 2020

thierry-martinez commented May 23, 2020

thierry-martinez commented May 22, 2020 •

edited by gasche

Loading

gasche commented May 23, 2020 •

edited

Loading