Ppxlib #54

tatchi · 2021-05-16T05:55:22Z

This is my attempt at addressing ocaml-ppx/ppxlib#143. It would need careful review as I'm definitely not a ppx expert. At least the tests are passing which I guess is a good sign 😁

First commit d8b346c only formats the code (should make it easier to review the rest)
Second commit 233d9d0 actually migrate the code to using ppxlib

Closes #44

tatchi · 2021-05-16T05:57:02Z

src/ppx_import.ml

+          (* The equivalent of not specifying the variance explicitly.
+             Since the very purpose of ppx_import is to include the full definition,
+             it should always be sufficient to rely on the inferencer to deduce variance. *)
+          (Ppxlib.Asttypes.NoVariance, Ppxlib.Asttypes.NoInjectivity) ))


Definitely not sure about that one. Is Ppxlib.Asttypes.NoInjectivity correct ?

This change was introduced in ppxlib 0.22.0

Looks like Invariant was replaced by NoVariant but I've no clue what should be the injectivity 😅

This is indeed tricky stuff, @gasche do you know who could help with this?

I just looked at this (sorry for the delay). It's a bit tricky but I think that the current code in the PR is correct:

for abstract type, the default is "non injective", so NoInjectivity is the conservative choice

non-abstract types are always injective, so you could use Injective here, but it looks like the OCaml parser uses NoInjective by default as well and lets the type-checker figure things out afterwards.

tatchi · 2021-05-16T05:59:07Z

src/ppx_import.ml

+  let find_attr s attrs =
+    try
+      Some
+        (List.find (fun { attr_name = x; _ } -> x.txt = s) attrs).attr_payload
+    with Not_found -> None


I didn't find the Ast_convenience.find_attr equivalent so I just inlined the function

ejgallego · 2021-05-16T12:52:05Z

Thanks a lot @tatchi ! I will give it some extended testing this week, I have some development which make extensive use of ppx_import

I'm OK with the code formatting, tho I do believe that it'd be better if you can submit it as a separate PR so we can review this one free of formatting changes.

If we are gonna format ppx_import automatically we should have it happen as part of the CI , add the ocamlformat config, and a makefile target fmt that invokes dune fmt, we should also check that @gasche and @whitequark are Ok with it.

gasche · 2021-05-16T12:57:15Z

Those who do the work decide; @ejgallego if you are doing the main work of integrating this nice-sounding PR, you get to decide on the formatting :-)

ejgallego · 2021-05-16T14:42:26Z

Those who do the work decide; @ejgallego if you are doing the main work of integrating this nice-sounding PR, you get to decide on the formatting :-)

I have a similar view, if @tatchi who is writing this PR would like to have the code formatted, that's fine to me.

ejgallego · 2021-05-16T14:43:57Z

Needs fixing for OCaml versions < 4.12:

File "src/ppx_import.ml", line 193, characters 16-21:
- 193 |       Typ.arrow label lhs (core_type_of_type_expr ~subst rhs)
-                       ^^^^^
- Error: This expression has type Asttypes.arg_label
-        but an expression was expected of type
-          Migrate_parsetree.Ast_412.Asttypes.arg_label
- ```

tatchi · 2021-05-17T07:09:22Z

Those who do the work decide; @ejgallego if you are doing the main work of integrating this nice-sounding PR, you get to decide on the formatting :-)

I have a similar view, if @tatchi who is writing this PR would like to have the code formatted, that's fine to me.

The reason why I formatted the code is because I'm not used to doing that manually. Therefore, I knew I was going to auto-format the files changed by this PR, and figured it would be better to have a first commit that only formats the files. That would make the review easier to avoid extra diff related to formatting changes. I can definitely revert my commit and try to format the changes by hand if you prefer 😄

Needs fixing for OCaml versions < 4.12:

File "src/ppx_import.ml", line 193, characters 16-21:
- 193 |       Typ.arrow label lhs (core_type_of_type_expr ~subst rhs)
-                       ^^^^^
- Error: This expression has type Asttypes.arg_label
-        but an expression was expected of type
-          Migrate_parsetree.Ast_412.Asttypes.arg_label
- ```

I'm a bit lost between all these ast conversions 😅 I tried to re-use the Convert module but it didn't work. After some research, I stumbled upon this example and used the same method. Everything seems to compile now except for OCaml 4.04.2 due to dependencies constraints that can't be met

tatchi · 2021-05-17T07:33:41Z

Would that make sense to restrict ppx_deriving >= 5.0 since this is the version that switched to ppxlib? If so, I guess we would have to drop support for OCaml < 4.05.0 anyway since previous versions are not compatible with ppx_deriving >= 5.0

gasche · 2021-05-17T07:42:28Z

Personally I think that requiring (OCaml >= 4.05) is now very reasonable.
My compass for "which old version should we still support?" is "Debian stable", which currently uses 4.05.0.

ejgallego · 2021-05-17T10:29:05Z

The reason why I formatted the code is because I'm not used to doing that manually. Therefore, I knew I was going to auto-format the files changed by this PR, and figured it would be better to have a first commit that only formats the files. That would make the review easier to avoid extra diff related to formatting changes. I can definitely revert my commit and try to format the changes by hand if you prefer

On the contrary, I'd say let's try to avoid doing more work than what we need. If you pass me your ocamlformat config we can already merge the formatting changes in a separate PR.

gasche · 2021-05-17T11:21:19Z

I think that what @ejgallego has in mind is to have a first PR (or himself committing directly) with your pure-reformat change, and a second PR with the actual changes, in the interest of making the "whole diff" review of the PR easier.

tatchi · 2021-05-17T16:52:40Z

I think that what @ejgallego has in mind is to have a first PR (or himself committing directly) with your pure-reformat change, and a second PR with the actual changes, in the interest of making the "whole diff" review of the PR easier.

Sounds good to me, I'll open a PR with the formatting changes 😊

tatchi · 2021-05-20T11:47:36Z

I rebased my changes on top of master. The second commit bumps ocamlformat to the latest version because the 0.15.0 version was conflicting with ppxlib. Fortunately, it didn't introduce too many formatting changes. Let me know though if you prefer to have that as a separate PR.

ejgallego · 2021-05-26T14:38:00Z

I rebased my changes on top of master. The second commit bumps ocamlformat to the latest version because the 0.15.0 version was conflicting with ppxlib. Fortunately, it didn't introduce too many formatting changes. Let me know though if you prefer to have that as a separate PR.

Thanks @tatchi , I'd prefer to have as a separate PR if you don't mind, so review is easier.

I assume the PR is ready for review, correct? What about the CI failure?

tatchi · 2021-05-31T07:04:59Z

Thanks @tatchi , I'd prefer to have as a separate PR if you don't mind, so review is easier.

Alright, I removed the commit :)

I assume the PR is ready for review, correct? What about the CI failure?

Yes, it should be ready for review. The CI was failing for OCaml 4.04.2. I removed this version since it won't be supported anymore (see #54 (comment)) and now it looks ok :)

ejgallego · 2021-06-02T10:41:05Z

Great, thanks @tatchi , will review ASAP (cc: @gasche in case he want to do a pass too)

ppx_import.opam

ejgallego

Looks good to me, thanks!

I will leave a couple of days to get comments, do some more testing on my own, then merge.

gasche · 2021-06-02T11:39:55Z

I skimmed the code and it looks overall fine (the port is rather non-invasive, which is good news). There is one question on variance that is subtle, I will look at it later. (It's about moving from one version of the AST to another, rather than ppxlib per se, in a corner case of the type declaration syntax.)

My experience with these changes is that there are some failures that are not caught by running the code, but by trying them in the real world. Stuff like breakage with older OCaml versions, or breakage when people combine it with other ppxes, or use a non-Dune build system. I think that the real test with this PR will be building the reverse dependencies of ppx_import on the opam repository. I would encourage @ejgallego to try to do this as soon as possible (after merging the PR), for example (if you don't have a better workflow) by preparing a new ppx_import release on opam-repository, with the idea that we may iterate a bit more to fix issues as they come up.

ejgallego · 2021-06-10T20:16:45Z

Note that the opam file seems to have a couple of issues:

the ocaml lower bound is incorrect
I am not sure why dune 1.2 is now the lower bound for ppx_import, I think that version won't work actually, the 1.11 bound is hard AFAICT [and in fact we should raise it to 2.0]

@tatchi , if you feel like addressing these issues it is fine, otherwise I'll tweak them myself before the release.

Regarding my testing, I'm getting this problem:

Error: Type unsupported for ppx [of_sexp] conversion
File "serlib/ser_sorts.ml", line 19, characters 2-25:
19 |   [%import: Sorts.family]

where the serlib/ser_sorts.ml file is:

type family =
  [%import: Sorts.family]
  [@@deriving sexp,yojson]

and the original type is:

type family = InSProp | InProp | InSet | InType

the build definition for this library is

(library
 (name serlib)
 (public_name coq-serapi.serlib)
 (synopsis "Serialization Library for Coq")
 (preprocess (staged_pps ppx_import ppx_sexp_conv ppx_deriving_yojson))
 (libraries coq.stm sexplib))

I tried a few tweaks without success, didn't investigate more yet.

This is a showstopper I'm afraid, as this is a basic use case of ppx_import IMHO.

Let me of course know if you want me to produce a self-contained reproduction case.

tatchi · 2021-09-20T17:43:09Z

I rebased the branch to bring in #61 (the failing test) and modified the code to use the new ppx_import syntax. The relevant commit is 23fd6a8

To make it work, I slightly had to change the ppxlib code in order to expose an input_name function in the Expansion_context.Extension module (see ocaml-ppx/ppxlib#284).

For the type_declaration, the only thing I had to do is to write the expand function. Then I was able to re-use the type_declaration function that contains all the logic.

For the module_type_declaration, I had to slightly change the code. The module_type function was previously receiving an input of type Ppxlib.module_type. It receives now an input of type Ppxlib__Import.package_type.

The challenge I faced is that there were some code paths that were using the module_type received as input, modifying some of its properties and returned it. Since we no longer receive a module_type as input, I had now to construct one from scratch. Not sure how it matters, especially since all the tests are passing (including the one that was failing before).

src/ppx_import.ml

NathanReb · 2021-11-15T10:43:52Z

What's the status of this PR?

I think it's based on the latest ppxlib additions and should be ready for review isn't it?

I'm currently working on some follow up changes and bug fixes on ppxlib and wanted to make sure that ppx_import does not modify the type declaration attributes. I'll take a look at the code to make sure and if it is indeed the case, that should be part of the contract between ppxlib and ppx_import.

If I was wrong to assume that the need to discuss how ppx_import modifies attributes of type declarations and how to properly interpret this in ppxlib.

tatchi · 2021-11-15T14:07:57Z

What's the status of this PR?

I think it's based on the latest ppxlib additions and should be ready for review isn't it?

From what I remember, I should have adapted the code to what is described in this PR: ocaml-ppx/ppxlib#271 (comment) (or at least from what I understood).

All the current + the failing test are passing. So yes it should be ready for a review 😁

pitag-ha

Nice, thanks for all the work @tatchi and sorry for the delay in having another look at it! By the way, you can always ping me.

The challenge I faced is that there were some code paths that were using the module_type received as input, modifying some of its properties and returned it. Since we no longer receive a module_type as input, I had now to construct one from scratch. Not sure how it matters, especially since all the tests are passing (including the one that was failing before).

See my comment below about that.

src/ppx_import.ml

pitag-ha · 2021-11-19T18:13:02Z

src/ppx_import.ml

+          let psig =
+            psig_of_tsig ~subst (List.map Compat.migrate_signature_item tsig)
+          in
+          Ast_helper.Mty.mk ~attrs:[] (Pmty_signature psig)


Since we no longer receive a module_type as input, I had now to construct one from scratch. Not sure how it matters, especially since all the tests are passing (including the one that was failing before).

This place is what you were referring to, isn't it?

I'm not sure if that ever might happen, but if someone wrote

module type Foo = [%import: ...] [@inner_attribute] [@@deriving ...]

then the inner attribute would get lost now.

@ejgallego, do you think that's a problem? By the way, I'm not really sure: what's the use case of ppx_import in the context of module types? Is it really also for derivers (as in my example above) or is it actually something different? Or both?

I can't tell, I'll test when I can, but I think it should be OK for my use cases.

Thanks! Fingers crossed.

So you have use cases of ppx_import on module types yourself?

So you have use cases of ppx_import on module types yourself?

I was experimenting with something like that to avoid boilerplate as someone recently pointed out to me that this was possible, but not sure we have something in production, need to check anyways, I think coq-serapi has ~700 import statements

@ejgallego, how is this going? Have you already had time to test this?

Nope sorry, but will go ahead ASAP.

This place is what you were referring to, isn't it?

Yes indeed. And also here and here but as you pointed out, these are just for ocamldep so that's probably fine ?

@ejgallego, kind reminder. Have you already had the chance to look into this? :)

@pitag-ha @tatchi , sorry for the delay, I'm happy to report that pinning ppx_import to this PR and using OCaml 4.12.1, coq-serapi test suite seems work fine and passes the test suite! All without a single change to the source code, congrats on the amazing work!

pitag-ha · 2021-12-13T12:28:33Z

Hey @tatchi and @ejgallego, we've now released ppxlib.0.24.0 which contains all the new changes needed (see entries 2, 3 and 4 in release notes). Could you unpin ppxlib and instead bound it to >=0.24.0, @tatchi? And could you dig a bit into the last open question, @ejgallego? We might be close to having this mergable soon 🤞 !

Co-authored-by: Sonja Heinze <[email protected]>

tatchi · 2021-12-13T16:00:25Z

Thanks @pitag-ha for the review 😊 I unpinned ppxlib and bound it to >=0.24.0 as suggested.

ejgallego · 2022-01-06T18:38:10Z

Hi folks, I could finally perform some testing on my end, and this PR seems to work very well!

I thus propose to merge and do a release if everyone is happy.

tatchi · 2022-01-06T18:49:41Z

Thanks for taking the time to test it 🤗 All fine for me to merge it but, to let you know, I noticed that it doesn't work with OCaml 4.13 (due to a change in the AST). I've a local branch where I should have fixed that. I was waiting for that PR to be merged before opening a new one with the above mentioned fix. Up to you to decide if it should be included in the next release :) Le jeu. 6 janv. 2022 à 19:38, Emilio Jesús Gallego Arias < ***@***.***> a écrit :

…

Hi folks, I could finally perform some testing on my end, and this PR seems to work very well! I thus propose to merge and do a release if everyone is happy. — Reply to this email directly, view it on GitHub <#54 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKV7VEEW3N7H4Y6TCLSLE3UUXORZANCNFSM446TYDEQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ejgallego · 2022-01-08T18:46:48Z

I was waiting for that PR to be merged before opening a new one with the above mentioned fix. Up to you to decide if it should be included in the next release :)

I think no problem at all; I propose to merge on Monday evening, so we can maybe cook the release end of next week.

gasche · 2022-01-10T13:39:34Z

Thanks @tatchi and @pitag-ha for the hard work, and thanks @ejgallego for the review!

@ejgallego, will you take care of preparing a new release?
(This is a major change, so I think we should be careful about the build reports for our reverse dependencies, as provided by the opam-repository CI.)

ejgallego · 2022-01-10T13:43:48Z

@ejgallego, will you take care of preparing a new release?

Yup, as soon as @tatchi adds the PR for 4.13 support.

(This is a major change, so I think we should be careful about the build reports for our reverse dependencies, as provided by the opam-repository CI.)

Sure, I think it will be 1.9.0 tho, as 2.0 may be reserved for #62 if finally goes ahead.

tatchi · 2022-01-10T13:57:20Z

Yup, as soon as @tatchi adds the PR for 4.13 support.

I will open a PR for that later on today :)

CHANGES: * Migrate to ppxlib ocaml-ppx/ppx_import#54 , closes ocaml-ppx/ppx_import#44 (tatchi) * Drop support for OCaml `4.04.2`. Minimal supported version is now `4.05.0` ocaml-ppx/ppx_import#54 (tatchi) * Bump minimum dune version to 1.11 ocaml-ppx/ppx_import#56 (tatchi) * Update CI to test OCaml 4.12.0, no changes required (ocaml-ppx/ppx_import#53, Emilio J. Gallego Arias) * Remove the OCaml upper bound in the opam file (ocaml-ppx/ppx_import#53, Emilio J. Gallego Arias, kit-ty-kate)

tatchi commented May 16, 2021

View reviewed changes

tatchi force-pushed the ppxlib branch 3 times, most recently from 8d13b18 to ceb74c5 Compare May 17, 2021 05:37

tatchi mentioned this pull request May 17, 2021

Format codebase #56

Merged

tatchi force-pushed the ppxlib branch 2 times, most recently from 97732da to 7db8969 Compare May 20, 2021 11:43

ejgallego self-assigned this May 26, 2021

tatchi force-pushed the ppxlib branch 3 times, most recently from 72b46db to c51d46f Compare May 31, 2021 06:46

ejgallego reviewed Jun 2, 2021

View reviewed changes

ppx_import.opam Outdated Show resolved Hide resolved

ejgallego approved these changes Jun 2, 2021

View reviewed changes

tatchi added 7 commits September 20, 2021 16:02

use Ppxlib.Longident.flatten_exn

b0e283b

use input_name from expansion context

baeef7f

bump ocaml minimum version to 4.05.0

39f6b6b

inline mknoloc

cf85ac0

bump minimum ppxlib version to 0.22.1

af98c88

inline function to avoid using Outcometree module

cfd93d7

use ppx_import special syntax

23fd6a8

tatchi force-pushed the ppxlib branch from 3fe327f to 23fd6a8 Compare September 20, 2021 16:49

tatchi mentioned this pull request Sep 20, 2021

add input_name to the Expansion_context.Extension module ocaml-ppx/ppxlib#284

Merged

tatchi commented Sep 20, 2021

View reviewed changes

src/ppx_import.ml Show resolved Hide resolved

pitag-ha reviewed Nov 19, 2021

View reviewed changes

tatchi and others added 2 commits December 13, 2021 16:30

Update src/ppx_import.ml

a531321

Co-authored-by: Sonja Heinze <[email protected]>

ppxlib >=0.24.0

5767bb3

ejgallego merged commit 8f1892b into ocaml-ppx:master Jan 10, 2022

ejgallego mentioned this pull request Jan 10, 2022

Migrate to ppxlib #44

Closed

pitag-ha mentioned this pull request Jan 10, 2022

Port ppx_import to ppxlib ocaml-ppx/ppxlib#143

Closed

ejgallego mentioned this pull request Jan 13, 2022

[new release] ppx_import (1.9.0) ocaml/opam-repository#20444

Merged

gasche mentioned this pull request Apr 16, 2022

Ppxlib.0.26.0 compatibility #69

Merged

Ppxlib #54

Ppxlib #54

Conversation

tatchi commented May 16, 2021 • edited by ejgallego Loading

Choose a reason for hiding this comment

tatchi May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejgallego commented May 16, 2021

gasche commented May 16, 2021

ejgallego commented May 16, 2021

ejgallego commented May 16, 2021

tatchi commented May 17, 2021

tatchi commented May 17, 2021

gasche commented May 17, 2021

ejgallego commented May 17, 2021

gasche commented May 17, 2021

tatchi commented May 17, 2021

tatchi commented May 20, 2021 • edited Loading

ejgallego commented May 26, 2021

tatchi commented May 31, 2021

ejgallego commented Jun 2, 2021

ejgallego left a comment

Choose a reason for hiding this comment

gasche commented Jun 2, 2021

ejgallego commented Jun 10, 2021 • edited Loading

tatchi commented Sep 20, 2021

NathanReb commented Nov 15, 2021

tatchi commented Nov 15, 2021

pitag-ha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitag-ha commented Dec 13, 2021

tatchi commented Dec 13, 2021

ejgallego commented Jan 6, 2022

tatchi commented Jan 6, 2022 via email

ejgallego commented Jan 8, 2022

gasche commented Jan 10, 2022

ejgallego commented Jan 10, 2022

tatchi commented Jan 10, 2022 • edited Loading

tatchi commented May 16, 2021 •

edited by ejgallego

Loading

tatchi May 20, 2021 •

edited

Loading

tatchi commented May 20, 2021 •

edited

Loading

ejgallego commented Jun 10, 2021 •

edited

Loading

tatchi commented Jan 10, 2022 •

edited

Loading