search

ocaml-ppx / ppx_import

Less redundancy in type declarations and signatures
MIT License
89 stars 29 forks source link

Ppxlib #54

Closed tatchi closed 2 years ago

tatchi commented 3 years ago

This is my attempt at addressing https://github.com/ocaml-ppx/ppxlib/issues/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 😁

Closes #44

ejgallego commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/ocaml-ppx/ppx_import/pull/54#issuecomment-842092027) and now it looks ok :)

ejgallego commented 3 years ago

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

gasche commented 3 years ago

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 commented 3 years ago

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

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

ejgallego commented 3 years ago

Actually the test suite should catch this, as it doesn't happen with some derivers, but it does happen with others, I'm gonna strengthen the test-suite adding ppx_deriving_sexp so this PR will properly fail in CI.

ejgallego commented 3 years ago

@tatchi I took the liberty to rebase this branch and push, now that #59 is in master, so CI properly fails with the testing issue we detected.

pitag-ha commented 3 years ago

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]

To me it seems like this problem is that ppxlib tries to apply ppx_sexp_conv before ppx_import and therefore fails. I've proposed trying this to make sure that's what happens.

it doesn't happen with some derivers, but it does happen with others

For what kind of derivers does it not happen? Are the ones where it happens maybe the ones that rely on ppx_import being applied them before and the ones where it doesn't happen don't rely on that?

pitag-ha commented 3 years ago

After some research, I stumbled upon this example and used the same method.

I haven't found that anywhere in the PR, but just to make sure: @tatchi, are you using that trick in the end? I'm asking, because using that internal ppxlib API would lead to compatibility problems with future ppxlib versions.

tatchi commented 3 years ago

After some research, I stumbled upon this example and used the same method.

I haven't found that anywhere in the PR, but just to make sure: @tatchi, are you using that trick in the end? I'm asking, because using that internal ppxlib API would lead to compatibility problems with future ppxlib versions.

Not strictly the same but something very similar that I've used here: https://github.com/ocaml-ppx/ppx_import/pull/54/files#diff-5f520cdcf9624f27ab1dc90a5a6e92042a28d5b52f5cc9036ae60820ee045ca2R2

Looking at the current unreleased changes from ppxlib, the Compiler_version module will soon be removed from the public API. Do you have any idea how we could achieve the same without that module 😄?

tatchi commented 3 years ago

Alright, except for the comment I left here, I should have addressed all your comments. Thanks again for the review @ejgallego and @pitag-ha 🤗

ejgallego commented 3 years ago

Alright, except for the comment I left here, I should have addressed all your comments. Thanks again for the review @ejgallego and @pitag-ha

Thanks! Let me know folks when you want this merged and I will attempt a release.

bbc2 commented 3 years ago

I tested this branch and I'm afraid there might be an issue with interface files. When using [%import: Bar.t] in a .mli file, I get:

> dune build @check
File "foo.mli", line 1, characters 11-17:
1 | type t = [%import: Bar.t]
               ^^^^^^
Error: Uninterpreted extension 'import'.

It may be related to the change in how this ppx is registered.

I was able to reproduce this in CI with a new test case (src_test/ppx_deriving/test_ppx_import.mli):

I'm quite excited by this change so I went ahead and tested it on our code base. This looks unexpected so I thought you might want to know. Hopefully, I'm not missing something obvious.

ejgallego commented 3 years ago

Thanks a lot @bbc2 , indeed it is great we are strengthening ppx_import's test suite , would you mind submitting the test case as a separate PR?

bbc2 commented 3 years ago

Sure, here it is: https://github.com/ocaml-ppx/ppx_import/pull/61

pitag-ha commented 3 years ago

I'm afraid there might be an issue with interface files.

Thanks for noticing! What's happening is that currently, both with the "normal" registration and with the Instrument registration, only the structure of the Ast_traverse object is passed to the register function. To also rewrite interface files, the signature of that object also needs to be passed to it:

Ppxlib.Driver.V2.register_transformation ~impl:mapper#structure ~intf:mapper#signature "ppx_import"

But now that we're using the Instrument trick, adding signature isn't possible.

So either we have to implement a way in ppxlib to say when a rewriter should be applied that rewrites both implementation and interface files or we directly implement all the features needed for ppx_import to fit well with ppxlib. I'll come back to you on this.

pitag-ha commented 3 years ago

I'll come back to you on this.

I've discussed this with @NathanReb (thanks @NathanReb!) and we've come up with a solution that would be great as a long term solution. For that, the ppx_import syntax would have to be made conform with the OCaml AST structure. Since breaking changes are always controversial, I've opened a separate issue to discuss that. Please, have a look at that issue.

To make the port of ppx_import to ppxlib work, we've also already considered for a while adding a virtual extension node in ppxlib that would capture the current ppx_import syntax. That solution has two downsides: one, it would add appxlib layer that's not conform with the OCaml AST just for ppx_import; and two, if someone also uses a standard core type extension node along with ppx_import, we'd have to raise.

So let's first discuss what you think about the issue I've opened. If you think that's a good idea, we could proceed as follows:

  1. We make a ppxlib release in which we add a "hidden" function register_ppx_import just serving the purpose to register ppx_import with its current syntax as whole file transformation and making sure it's applied before sexp and yojson and all.
  2. You use that function here in this port and release the port as 1.x.
  3. You make a major release 2.0.0 of ppx_import which fixes the ppx_import syntax as explained in the issue.
  4. We make a PR to register ppx_import as extension node rewriter.

What do you think?

gasche commented 3 years ago

As someone who only looks at ppx stuff intermittently (when it stops working), some extra explanations would help.

  1. What is the current problem with supporting ppx_import as a ppxlib extension? What I understood from your previous message is that the (current) ppxlib implementation rewrites structures / ml files, but not signatures / mli files. Is that the problem that makes a ppxlib-based ppx_import currently unsatisfying?
  2. How is your suggestion to change the syntax going to help with this problem?
  3. When you say that ppx_import does not conform to the OCaml AST, you mean that the interpretation of the extension point depends on its placement in a wider context, and transforms other parts of the program than just the AST node it is placed at. Is it technically impossible to do this with ppxlib, or rather a question of recommandation/taste, or something in between? I would expect that "generating code outside the place where the extension is placed" would be a not-uncommon need among ppx rewriters (for example: the extension runs on an expression, but it generates helper definitions that should be placed at the toplevel of the currently processed module, to be evaluated only once).
gasche commented 3 years ago

Looking at the Ppxlib.Driver documentation, my impression is that the issue is that unlike many other transformation passes ({lint,preprocess,enclose}_{intf,impl}, etc.), instrument only comes with a structure-processing version, and the signature-processing counterpart (instrument_intf) is missing. (And the way ppx_import is setup requires that it runs before "deriving"-style derivers, so impl/intf passes are too late.) Would a reasonable fix for this be to implement an instrument_signature pass?

pitag-ha commented 3 years ago

As someone who only looks at ppx stuff intermittently (when it stops working), some extra explanations would help.

Sorry for that!

What is the current problem with supporting ppx_import as a ppxlib extension?

Due to its syntax, ppx_import cannot be registered as a normal extension node rewriter. Unless we implement a virtual extension node in ppxlib just for ppx_import, ppx_import with its current syntax has to be registered as a whole file transformation. If you register it as a whole file transformation in the usual way (with impl/intf), it will be applied after sexp etc (see here for order of application) and therefore applying sexp will fail. So as a temporary "hack" around that I suggested registering it with instrument for now until we come up with a long-term solution: instrument has a feature to specify when it should be applied. But instrument doesn't allow to rewrite interface files. So that's not an option.

Would a reasonable fix for this be to implement an instrument_signature pass?

I would say that would be a "practical" fix, but I'm not sure about "reasonable". ppx_import is not an instrumentation. Making use of the existing instrument feature was meant as a temporary trick. If we go down the road of adding something to ppxlib to make ppx_import work as whole file transformation, I would rather add a "hidden" register_ppx_import function to the driver.

How is your suggestion to change the syntax going to help with this problem?

If ppx_import had the syntax I suggested in this issue, it could be registered as extension node, which would be far more efficient and furthermore we wouldn't have this problem of wrong order of application (I've made sure of that in this quick ppxlib test).

About your question 3. here:

When you say that ppx_import does not conform to the OCaml AST, you mean that the interpretation of the extension point depends on its placement in a wider context, and transforms other parts of the program than just the AST node it is placed at.

Yes, the end of the sentence captures exactly what I want to say: the expansion of the node spreads onto the rest of the tree. From what I understand, extension nodes are meant to be expanded (as nodes), not to expand the rest of the tree.

Is it technically impossible to do this with ppxlib, or rather a question of recommandation/taste, or something in between?

It is possible to do that with ppxlib, but only if you register your PPX as a whole file transformation. But that's far less efficient and should only be done if necessary: if you register you PPX as whole file transformation, it takes up one whole AST pass on its own; if you register it as context-free rule (e.g. extension node), it is applied together with all other registered context-free rules in one single AST pass. ppx_import does not depend on its context by nature but only due to its syntax. With the proposed syntax it would be context-free. What do you think about discussing the proposed syntax (probably better on the issue)? I'm curious about the pro's for the current syntax and/or con's for the proposed syntax (apart from the breaking change) in general.

gasche commented 3 years ago

I would say that would be a "practical" fix, but I'm not sure about "reasonable". ppx_import is not an instrumentation. Making use of the existing instrument feature was meant as a temporary trick. If we go down the road of adding something to ppxlib to make ppx_import work as whole file transformation, I would rather add a "hidden" register_ppx_import function to the driver.

The way I understand things, ppxlib may actually benefit from giving people the ability to order whole-pass transformations. I'm not sure what "an instrumentation" is, but it seems to provide this feature; yet it is limited in an artificial way (why wouldn't instrumentations want to rewrite signature files as well?). Making instrument more coherent with other ppxlib passes seems less ad-hoc, and not harder to do, than adding a "hidden register_ppx_import" function, and it may be of benefit to other users of ppxlib. Is there a reason why that would be a bad idea? Could it not be helpful to help transition other existing extensions?

What do you think about discussing the proposed syntax (probably better on the issue)?

Yes, we can discuss the proposed syntax. However in principle I'm not so satisfied with the idea that we have to break backward-compatibility of the extension to be able to use ppxlib. I think that the suggestion makes sense, I understand the benefit of locality for predictability and performance, but this is a transition that I would prefer to see done without hurry, instead of being forced to do it. You propose to add an ugly hack within ppxlib to enable a gradual transition; and in any case it's going to be your choice, or in general the choice of the people actually doing the ppxlib side of the work (not me). But to me this ugly workaround sounds inferior to finding a general solution that would work for ppx_import and also other people. (With the caveat that I barely understand the design space on the ppxlib side.)

pitag-ha commented 3 years ago

I'm not sure what "an instrumentation" is

I would say an instrumentation is a way to analyze your program in terms of test coverage, performance etc. One example that uses that feature is bisect_ppx (to be applied after other PPXs).

Making instrument more coherent with other ppxlib passes seems less ad-hoc, and not harder to do, than adding a "hidden register_ppx_import" function, and it may be of benefit to other users of ppxlib. Is there a reason why that would be a bad idea? Could it not be helpful to help transition other existing extensions?

We don't want ppxlib users to extensively use the Before or After feature for the following reason: that feature doesn't fix an exact position at which your PPX is applied. It only puts PPXs into buckets. There is a clear order of application between buckets. But PPXs inside the same bucket are simply applied in alphabetic order. So if the bucket gets big, it looses its purpose. We made clear that we don't want the Before or After feature to be used extensively by labeling them as "meant only for instrumentations". When I suggested using that feature as a hack for the ppx_import port, I added a clear comment that that's just a temporary solution and I thought that the feature could be used as is without extending it. But I would prefer keeping clear signs that it shouldn't be used extensively (only supports impl rewriters; it's named instrument).

Also notice: There is another feature for non-instrumentation whole-file transformations that really need to be applied before all other PPXs, called preprocess_impl. For example ppx_optcomp uses that. But there can be only one preprocess_impl PPX registered at once in a project! So also using that extensively is not a good idea and in the case of ppx_import it's not necessary.

You propose to add an ugly hack within ppxlib to enable a gradual transition

Is it clearer now why I've proposed that ugly hack instead of proposing to extend the Before/After feature? Of course I'd only want to implement that ugly hack as a temporary solution and only if we have a nice long-term solution such as the change of syntax (of course without hurry, since we would have this temporary hack). If not, I would try to add a virtual extension node to ppxlib to try registering ppx_import as extension node rewriter even with its current syntax.

gasche commented 3 years ago

One approach could be to implement the ppx_import logic using the[%import type ...] syntax, and then add an instrumentation rule (or preprocessing rule, except we can't because preprocessing rules are un-composable by choice) that simply transforms type t = [%import ..] [@@foo] into [%import type t = ... [@foo]]. That transformation is "obviously harmless", and the fact that it gets ordered with other instrumentation rules in an arbitrary way does not seem to be an issue. But we would still need the ability to instrument signatures/mlis.

pitag-ha commented 3 years ago

I like that approach from the syntax point of view! We'd allow the new syntax while not breaking the projects using the old syntax. There are a couple of other things I don't like too much about that approach though.

One downside is that it doesn't improve anything from the performance point of view: even though the ppx_import logic would be applied together with all other context-free rules, the syntax transformation would still take up one whole AST pass on its own (even for folks using the new syntax there would be one whole identity AST pass).

Another downside is the one you've already mentioned that we'd have to extend the instrument feature for signatures for a PPX that's not an instrumentation but a PPX that rewrites the syntax. But it's also true what you're saying: having that simple transformation in the same bucket as instrumentations wouldn't do any real harm. I'll ask the other ppxlib folks what they think about that. (cc e.g. @NathanReb , since I think Jeremie is on vacation).

The last thing is not really a downside, but more of a theoretical remark about that approach: taking out the machinery of a PPX rewriter, in particular a whole file transformation, for a transformation that could be done once and for all with a command as simple as sed 's/type\(.*\)\[%import:\([^]]*\)\]/type%import\1\2/g' isn't ideal.

NathanReb commented 3 years ago

I think @pitag-ha covered this pretty well but to summarize it all simply: We don't want to encourage users to write whole AST transformations, everyone should use context-free rules as much as possible. It is a much better approach to keep good performances and a clear and predictable transformation semantic.

We are aware that some rare and very specific ppx-es need to be written as whole AST passes, and we added support for those but this should remain an exception. Handling the order of transformations through priorities is a part of OMP's heritage, that we don't want as we are convinced it was a mistake for quite a while now.

ppx_import doesn't have to be written in such a way and that's why we don't want to add generic support for doing so.

That being said, we want to make the transition as smooth as possible both for ppx_import maintainers and users and we agree whatever solution we choose need to be compatible with the current syntax. I think we established above that there is no satisfying solution using the current ppxlib API that will allow porting ppx_import without changing the syntax so we will have to add a workaround in ppxlib. Out of of all the proposed workarounds, I think adding an exception in the context-free rules handling so that ppx_import can be registered as a context-free rule for expanding type declaration while using the current syntax is the best, performance and semantic-wise. It requires us (ppxlib maintainers) to maintain a very specific exception for that purpose only so I'd suggest the following course of action:

  1. We add this feature to ppxlib as a hidden or deprecated, made ppx_import only API entry
  2. We make the ppx_import ppxlib port use that feature and release it
  3. We add the support for the above suggested syntax in ppx_import, this should be a few more lines of ppxlib registration code so nothing hard to maintain on your side.
  4. We deprecate the use of the old syntax, with a clear time schedule for its removal
  5. We remove the old syntax support in ppx_import in a major release
  6. We remove the corresponding feature in ppxlib in a major release

I think this solution is satisfying for everyone so let's settle on this. We don't have to rush the deprecation cycle but I think it's important to do move on with the syntax migration once the port is done and released. I'll be happy to help on the ppx_import side if that allows us to get rid of any ad-hoc weirdness in ppxlib.

gasche commented 3 years ago

The last thing is not really a downside, but more of a theoretical remark about that approach: taking out the machinery of a PPX rewriter, in particular a whole file transformation, for a transformation that could be done once and for all with a command as simple as sed 's/type\(.*\)\[%import:\([^]]*\)\]/type%import\1\2/g' isn't ideal.

In my experience, this is not how things work in practice. The following things will happen:

All of this can be dealt with, but it's not a one-line sed script. It never is. I would happily take the performance hit of a mostly-useless extra pass (only for users who decided to use ppx_import in the first place, none of which ever complained to us about any compile-time performance issue) instead of having to deal with all of this.

pitag-ha commented 3 years ago

unless we specifically think of keeping an import extension registered on type expressions to show a proper recommendation to switch to the new syntax.

That sounds like a very good idea to me if we go down the road of the syntax change (which I still think we should)! That could also simply be implemented as a context-free rule.

maintainers of projects using ppx_import will not hear about the script anyway

If we implement your suggestion of keeping an import extension for the old syntax to show an error message about how to switch to the new syntax, I think that shouldn't be a problem, should it? We could write an update script, point to the script in the README and point to the README in the error reporting for the old syntax.

most online material (READMEs, blog posts, etc.) will not be updated and still use the wrong syntax forever

If I'm not missing anything, the only READMEs affected should be other PPX. Is that right? I've just grepped quickly through the ppx-universe and the only places affected should be ppxlib's HISTORY.md and ppx_optcomp's README. I'd be happy to submit the two PRs to fix that. About the blog posts: I think think it's a well-known fact that blog posts can get out-of-date. And even if people follow the out-of-date blog post suggestion of the old syntax, they would get the new syntax suggested in the error message with your trick.

they will jump from OCaml 4.08 to OCaml 5.04 and everything will break at once for them

That's a good point. But to be fair, if you jump from OCaml 4.08 to OCaml 5.04, you probably expect quite some breakages. And again, there would be an error message making clear how to fix it.

the script is inadequate because it doesn't handle all the other cases (multiline declarations, and t = [%import ..])

Again, good point! My one-liner suggestion has been a bit too naive. Still, I'm sure we could figure out a quite simple script that would work. I also know that @NathanReb and some folks have been talking about a tool using a dune promote workflow to help updating to a new major version of some project (I think actually in the context of dune, but we could see if we could also use it here).

ejgallego commented 3 years ago

Thanks folks for all the comments and work on this issue, while I tend to be in the more conservative side, and I acknowledge the issues @gasche does point out just above, I think in this case the breakage is justified, as making the ppx_import syntax compositional will save us much time in the future, at the cost of the migration.

For OPAM that's not super bad as only 10 projects depend on ppx_import, and a few of them I'm involved with .

  • the script is inadequate because it doesn't handle all the other cases (multiline declarations, and t = [%import ..])

True, on the other hand for the few remaining problems, manual fix seems straightforward in this case, so not a super huge deal IMHO.

  • most online material (READMEs, blog posts, etc.) will not be updated and still use the wrong syntax forever

Indeed a big problem, tho there is not so much material covering ppx_import as in other cases, so impact is more limited.

maintainers of projects using ppx_import will not hear about the script anyway, because they are doing updates at their own rythm and any effort to warn them of the transition (on Discuss, whatever) will miss most of them; we could notify them individually on opam, but that is work and people use ppx_import in one-off scripts that are not on opam.

I'd expect that this could be handled kinda okish, for the 1.x series, a warning would appear, so they would eventually notice it; as for the 2.x series, indeed the opam warning and release notes should do it; I'd say it is expected that the maintainers do realize that a 1.x -> 2.x transition may impact them, so they could read the CHANGES file.

they will jump from OCaml 4.08 to OCaml 5.04 and everything will break at once for them.

I'd expect them to be able to debug the breakage in the event of such a jump, wouldn't they?

unless we specifically think of keeping an import extension registered on type expressions to show a proper recommendation to switch to the new syntax.

I'd do indeed do this, in Coq we have done something similar [keeping parsing entries for removed stuff, so the new syntax can be recommended in an error message] and worked well.

But it is true that at some point we removed the error entries, and got some users doing jumps and not getting the recommendation. But I'd say that we can keep it for a while with ppx_import.

pitag-ha commented 3 years ago

@ejgallego , awesome!

@NathanReb is currently working on adding a virtual extension node to ppxlib for the temporal solution for a ppx_import.1.x release. Once that's done, we can finish this port and start working on the long-term solution by adding the new syntax and deprecating the old one.

tatchi commented 3 years ago

Thanks for all the comments and the great feedback.

@NathanReb is currently working on adding a virtual extension node to ppxlib for the temporal solution for a ppx_import.1.x release. Once that's done, we can finish this port and start working on the long-term solution by adding the new syntax and deprecating the old one.

Happy to try implementing that solution once @NathanReb finishes his work 😁

ejgallego commented 3 years ago

Great!

@gasche , any further thoughts on the tradeoff between the compat breakage vs making the syntax compositional?

I think your general points about ppx design are still valid, however it seems to me that ppx_import actually doesn't need them, so my pref is for the syntax update.

gasche commented 3 years ago

I don't have a strong preference, and you are a heavier user of ppx_import than I am, so I am happy to follow your intuition. In any case, I think there is no debate that the proposed syntax is slightly better. The main point of discussion was how to manage the transition (make ppxlib more general to support the patterns we need, or hard-code a specific transition tailored for ppx_import), and this is a choice for ppxlib people rather than ppx_import people. So: yes, please go ahead!

pitag-ha commented 3 years ago

Happy to try implementing that solution once @NathanReb finishes his work :grin:

@tatchi, are you still down for that? @NathanReb has now implemented the new function Extension.__declare_ppx_import necessary to finish this port. He has also given quite some detail about how to use it and what to keep in mind when doing so on the corresponding PR description.

So if you'd like to, you could finish the port now! For now (until we cut the next release), you could just pin ppxlib to our main branch and work with the dev version by adding something like

pin-depends: [
  [
     "ppxlib.0.23.0"
     "git+https://github.com/ocaml-ppx/ppxlib#main"
  ]
]

to ppx_import's opam file (just temporarily of course. it would have to be removed before merging this).

What do you think?

tatchi commented 3 years ago

Happy to try implementing that solution once @NathanReb finishes his work 😁

@tatchi, are you still down for that? @NathanReb has now implemented the new function Extension.__declare_ppx_import necessary to finish this port. He has also given quite some detail about how to use it and what to keep in mind when doing so on the corresponding PR description.

So if you'd like to, you could finish the port now! For now (until we cut the next release), you could just pin ppxlib to our main branch and work with the dev version by adding something like

pin-depends: [
  [
     "ppxlib.0.23.0"
     "git+https://github.com/ocaml-ppx/ppxlib#main"
  ]
]

to ppx_import's opam file (just temporarily of course. it would have to be removed before merging this).

What do you think?

Hi @pitag-ha, thanks for notifying me. I'm still down for it! Things are not very fresh in my memory though, it will probably take me some time to re-read the thread and understand what was the issue/proposed solution 😆

Anyway, I'm definitely happy to give it a shot 🤗

pitag-ha commented 3 years ago

Sure! And please just ask if you have any question :)

tatchi commented 3 years ago

I rebased the branch to bring in https://github.com/ocaml-ppx/ppx_import/pull/61 (the failing test) and modified the code to use the new ppx_import syntax. The relevant commit is https://github.com/ocaml-ppx/ppx_import/pull/54/commits/23fd6a8b8ff7c7f2dccad08dcc013e4c3501de6b

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 https://github.com/ocaml-ppx/ppxlib/pull/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).

NathanReb commented 3 years ago

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 commented 3 years ago

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: https://github.com/ocaml-ppx/ppxlib/pull/271#issue-696125992 (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 commented 2 years ago

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 :crossed_fingers: !