Ppxlib.0.26.0 compatibility #69

pitag-ha · 2022-03-28T11:28:46Z

This is a patch PR to make the PPX compatible with ppxlib.0.26.0 which has bumped the AST to 4.14/5.00.

gasche · 2022-03-28T11:40:00Z

src/ppx_import.ml

@@ -288,6 +288,7 @@ let ptype_decl_of_ttype_decl ~manifest ~subst ptype_name
                 in
                 { pcd_name =
                     {txt = Ocaml_common.Ident.name cd.cd_id; loc = cd.cd_loc}
+                 ; pcd_vars = []


I would expect ; pcd_vars = cd.cd_vars?

Hi @gasche, thanks for reviewing! The reason why I've made it an empty list is that in these patch PRs I simply preserve the behavior from before the AST bump. Sounds good to already implement the good new behavior though.

So it's a good point that here the good new behavior would probably be to use cd.cd_vars! The problem is that cd comes from the Types module, in which no vars field has been added to constructor_declaration. Btw, I was already wondering why no vars field has been added to Types.constructor_declaration. Do you know why (might be an obvious question)?

Actually I don't know; I thought that this came from Typedtree.constructor_declaration, and that one has a cd_vars as expected. I'll ask on the original PR.

ejgallego

Thanks for the PR! CI seems unhappy tho, any idea @pitag-ha ?

Would you mind updating the CHANGES.md file too?

pitag-ha · 2022-03-28T16:59:33Z

Thanks for the PR! CI seems unhappy tho, any idea @pitag-ha ?

For the CI to run, we'll probably have to wait for the patch PR on ppx_sexp_conv to be merged and released first: ppx_import depends on ppx_sexp_conv, which currently has a ppxlip<2.26.0 bound.

tatchi · 2022-04-15T05:14:27Z

Version 0.15.1 of ppx_sexp_conv is available on opam now so it should work 😁

ejgallego · 2022-04-15T13:28:37Z

I tried re-running the CI , but it seems 4.05-4.09 don't find a suitable opam setup. @pitag-ha , what's the next step here?

tatchi · 2022-04-15T14:19:18Z

I forced a minimum version of ppx_sexp_conv to be v0.15.1 and got a better error:

  * No agreement on the version of ocaml:
    - (invariant) → ocaml = 4.09.2
    - ppx_import → ppx_sexp_conv >= v0.15.1 → base >= v0.15 → ocaml >= 4.10.0

So my understanding is that to support ppxlib>=0.26, we need ppx_sexp_conv>=0.15.1. But ppx_sexp_conv.0.15.1 has a dependency of base>=0.15 that itself only supports OCaml version greater or equal to 4.10

ejgallego · 2022-04-15T14:26:28Z

Thanks for the analysis @tatchi , I guess we could maybe drop support for OCaml 4.09 and lower, but that would require doing a 2.0 branch [so maybe we can also then merge #68

Edit, instead of a 2.0 branch, we could do 1.x branch, and move 1.x to maintenance mode.

ejgallego · 2022-04-15T16:19:12Z

So my understanding is that to support ppxlib>=0.26, we need ppx_sexp_conv>=0.15.1. But ppx_sexp_conv.0.15.1 has a dependency of base>=0.15 that itself only supports OCaml version greater or equal to 4.10

I don't indeed see a way out of this problem, seems to me that base bump on the minimal OCaml version is quite restrictive.

ejgallego · 2022-04-15T16:28:50Z

Well, one way to maybe make this work is only to run the test suite in OCaml 4.10 and onwards, that works, I've checked, but maybe a bit risky in terms of testing.

gasche · 2022-04-15T16:38:07Z

The current Debian Stable, released in August 2021, has OCaml 4.11. So maybe requiring 4.10 is okay? If it's not, I guess you need to discuss with ppxlib maintainers to see if they could support older releases.

pitag-ha · 2022-04-16T19:29:13Z

Thanks for having a look at this, @tatchi!

I guess you need to discuss with ppxlib maintainers to see if they could support older releases

@gasche , what are you referring to? Ppxlib supports all compiler versions from 4.04.1 on. Do you mean to see if ppxlib can stay compatible with older versions of ppx_sexp_conv? If so: I could see if the ppx_sexp_comv maintainers (i.e. Jane street) would be willing to also merge and release my patch PR on top of their 0.14 branch.

gasche · 2022-04-16T19:49:43Z

I misunderstood from the above that ppxlib somehow creates the dependency on a recent base. In fact we (ppx_import) depend on ppx_sexp_conv for a test where we import a type to do [@@deriving sexp] (since #54). The test was added because existing tests did not catch a sexp-related issue. I guess that @ejgallego now has to decide whether he prefers to drop support for older OCaml versions or drop the test. (Or: configure the build system to disable this test on older OCaml version, but this way lies more complexity and pain.)

ejgallego · 2022-05-26T21:11:45Z

I'm fine disabling the test in < 4.10 , it is not so complex, just tweak the CI file and add a new target to the test makefile (I'll be happy to help if that's an issue)

ejgallego · 2022-06-13T13:24:27Z

We need to tweak the opam file too as to install the stuff condictionally on the OCaml version.

ejgallego · 2022-06-13T18:41:11Z

Great @pitag-ha , it seems to work, thanks! I will prepare a new release. Would you like to squash the last commits so history is a bit cleaner?

pitag-ha · 2022-06-13T19:14:28Z

I will prepare a new release.

Perfect, thank you!

Would you like to squash the last commits so history is a bit cleaner?

Sure, I've just done that.

pitag-ha · 2022-06-13T19:15:49Z

Or did you want me to sqash more commits?

ejgallego · 2022-06-13T19:37:56Z

Or did you want me to sqash more commits?

As you prefer, IMO it makes sense that commits are structured in such a way that a particular checkout is consistent, so indeed, after a review I think it makes sense to squash them all. For example the first commit only would not pass CI, the second would miss a change, etc...

By the way, was @gasche question above resolved?

This commit makes the ppx compatible with ppxlib.0.25.0. Disable the sexp tests on old compilers Due to its ppxlib constraint, ppx_import needs ppx_sexp_conv >= v.0.15.0, which (transitively via base) requires a compiler >= 4.10.0.

pitag-ha · 2022-06-14T13:00:29Z

As you prefer, IMO it makes sense that commits are structured in such a way that a particular checkout is consistent, so indeed, after a review I think it makes sense to squash them all.

Yes, I agree. I've done that now.

By the way, was @gasche question above resolved?

@gasche's proposal above is still being blocked by the fact that the compiler Types module doesn't reflect that type variables can be explicitly bount now (unless there's something I'm missing). I've just opened an issue at the compiler to ask if it might make sense to change that. I don't expect an answer right away though. I think it would make sense to merge and release this the way it is now. If we do so, ppx_import would become compatible with compilers up to 5.0 (now it's only up to 4.13) with the only caveat that folks using ppx_import can't explicitly bind type variables for now. Once we've sorted that out with the compiler (or found another solution), maybe I can just open another 2-liner PR to fix that and you could cut a point release? What do you think?

ejgallego · 2022-06-14T13:35:12Z

Thanks a lot, I think the plan looks good!

@pitag-ha

CHANGES: * Update ppxlib to 0.26.0 (ocaml-ppx#69, @pitag-ha)

@pitag-ha

CHANGES: * Update ppxlib to 0.26.0 (ocaml-ppx/ppx_import#69, @pitag-ha)

pitag-ha · 2022-06-17T10:23:21Z

Just to come back to the last question that was still kind of open: as pointed out here, explicit binders of type variables don't have any impact on the semantics of the program (and, btw, they don't even have an impact on the error reporting). So it does seem like it's ok for ppx_import to throw them away, doesn't it? The only way people might notice would be by running the PPX driver manually and inspecting the returned AST directly without passing it to the compiler. I think that wouldn't be a problem. Let me know if anyone disagrees though, please.

gasche reviewed Mar 28, 2022

View reviewed changes

ejgallego self-assigned this Mar 28, 2022

ejgallego reviewed Mar 28, 2022

View reviewed changes

ejgallego mentioned this pull request Apr 15, 2022

Ci: use stable version of OCaml 4.14 #70

Merged

pitag-ha force-pushed the ppxlib.0.25.0-compatibility branch 2 times, most recently from fa9bc3b to 4ee94ea Compare June 13, 2022 19:12

Stay up-to-date with the PPX ecosystem

838084a

This commit makes the ppx compatible with ppxlib.0.25.0. Disable the sexp tests on old compilers Due to its ppxlib constraint, ppx_import needs ppx_sexp_conv >= v.0.15.0, which (transitively via base) requires a compiler >= 4.10.0.

pitag-ha force-pushed the ppxlib.0.25.0-compatibility branch from 4ee94ea to 838084a Compare June 14, 2022 12:27

pitag-ha mentioned this pull request Jun 14, 2022

Reflect "explicit binders for type variables" feature in Types? ocaml/ocaml#11319

Closed

ejgallego merged commit a3d84b7 into ocaml-ppx:master Jun 14, 2022

ejgallego added a commit to ejgallego/ppx_import that referenced this pull request Jun 14, 2022

[changes] Release 1.10.0

bba10ee

CHANGES: * Update ppxlib to 0.26.0 (ocaml-ppx#69, @pitag-ha)

ejgallego mentioned this pull request Jun 14, 2022

[new release] ppx_import (1.10.0) ocaml/opam-repository#21530

Merged

kit-ty-kate pushed a commit to ocaml/opam-repository that referenced this pull request Jun 14, 2022

[new release] ppx_import (1.10.0)

c083168

CHANGES: * Update ppxlib to 0.26.0 (ocaml-ppx/ppx_import#69, @pitag-ha)

tatchi mentioned this pull request Jun 27, 2022

implement new syntax for type declaration #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ppxlib.0.26.0 compatibility #69

Ppxlib.0.26.0 compatibility #69

pitag-ha commented Mar 28, 2022

gasche Mar 28, 2022

pitag-ha Mar 28, 2022

gasche Mar 28, 2022

ejgallego left a comment

pitag-ha commented Mar 28, 2022

tatchi commented Apr 15, 2022

ejgallego commented Apr 15, 2022

tatchi commented Apr 15, 2022 •

edited

Loading

ejgallego commented Apr 15, 2022 •

edited

Loading

ejgallego commented Apr 15, 2022

ejgallego commented Apr 15, 2022

gasche commented Apr 15, 2022 •

edited

Loading

pitag-ha commented Apr 16, 2022

gasche commented Apr 16, 2022

ejgallego commented May 26, 2022 •

edited

Loading

ejgallego commented Jun 13, 2022

ejgallego commented Jun 13, 2022

pitag-ha commented Jun 13, 2022

pitag-ha commented Jun 13, 2022

ejgallego commented Jun 13, 2022

pitag-ha commented Jun 14, 2022

ejgallego commented Jun 14, 2022

pitag-ha commented Jun 17, 2022

Ppxlib.0.26.0 compatibility #69

Ppxlib.0.26.0 compatibility #69

Conversation

pitag-ha commented Mar 28, 2022

gasche Mar 28, 2022

Choose a reason for hiding this comment

pitag-ha Mar 28, 2022

Choose a reason for hiding this comment

gasche Mar 28, 2022

Choose a reason for hiding this comment

ejgallego left a comment

Choose a reason for hiding this comment

pitag-ha commented Mar 28, 2022

tatchi commented Apr 15, 2022

ejgallego commented Apr 15, 2022

tatchi commented Apr 15, 2022 • edited Loading

ejgallego commented Apr 15, 2022 • edited Loading

ejgallego commented Apr 15, 2022

ejgallego commented Apr 15, 2022

gasche commented Apr 15, 2022 • edited Loading

pitag-ha commented Apr 16, 2022

gasche commented Apr 16, 2022

ejgallego commented May 26, 2022 • edited Loading

ejgallego commented Jun 13, 2022

ejgallego commented Jun 13, 2022

pitag-ha commented Jun 13, 2022

pitag-ha commented Jun 13, 2022

ejgallego commented Jun 13, 2022

pitag-ha commented Jun 14, 2022

ejgallego commented Jun 14, 2022

pitag-ha commented Jun 17, 2022

tatchi commented Apr 15, 2022 •

edited

Loading

ejgallego commented Apr 15, 2022 •

edited

Loading

gasche commented Apr 15, 2022 •

edited

Loading

ejgallego commented May 26, 2022 •

edited

Loading