-
Notifications
You must be signed in to change notification settings - Fork 177
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
Switch to ppxlib #807
Switch to ppxlib #807
Conversation
I'm not familiar with the ppx (and especially with the ppx implementation), I'll review the MR next week, but I might have to ask for additional review. |
At the very least, I suggest we wait for the new ppxlib to be released, which is probably going to break compat one way or the other. And yeah, if we switch to ppxlib, might as well try to use their fancy hook system... |
Waiting for the next release of |
Seems there won't be any release of |
There is currently no release of ppxlib compatible with both omp2 and OCaml 4.12. The latest release is compatible with OCaml 4.12 but requires omp2. This makes packages relying on both lwt_ppx and some other ppxlib-based ppxs, uninstallable when using the soon to come OCaml 4.12. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This looks correct up to some nits.
Could you apply this patch to get opam pin/development installation working again? Related: aantron/bisect_ppx#327. This should also (help) fix the CI.
diff --git a/lwt.opam b/lwt.opam
index 277dea245..0fe007bfd 100644
--- a/lwt.opam
+++ b/lwt.opam
@@ -28,7 +28,8 @@ depends: [
"result" # result is needed as long as Lwt supports OCaml 4.02.
"seq" # seq is needed as long as Lwt supports OCaml < 4.07.0.
- "bisect_ppx" {dev & >= "2.0.0"}
+ # Until https://github.com/aantron/bisect_ppx/pull/327.
+ # "bisect_ppx" {dev & >= "2.0.0"}
"ocamlfind" {dev & >= "1.7.3-1"}
]
src/ppx/dune
Outdated
@@ -13,10 +13,10 @@ let () = Jbuild_plugin.V1.send @@ {| | |||
(public_name lwt_ppx) | |||
(synopsis "Lwt PPX syntax extension") | |||
(modules ppx_lwt) | |||
(libraries compiler-libs.common ocaml-migrate-parsetree ppx_tools_versioned) | |||
(libraries compiler-libs.common ppxlib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT we are now using ocaml-compiler-libs.common
(e.g. new line 268).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/ppx/ppx_lwt.ml
Outdated
if !warned then | ||
default_mapper.structure mapper structure | ||
method! structure = begin fun structure -> | ||
if !warned then super#structure structure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reduce formatting changes like here, where the change on old line 268 also wrongly appears to affect old line 267. This applies to many of the other diff hunks also, parts of which have changed only in indentation or splitting of tokens across lines. The formatting in this file is not great, but we should change it in separate work because this change is a bit scary and keeping it focused will be much easier for review, and for debuging later, if necessary, if this ends up as someone's git bisect
endpoint :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand it very well, sorry about that. I hope I fixed all superfluous changes.
8c3dd48
to
2c73ec1
Compare
I tried to fix the various problems and I rebased the PR on master. |
eea6dd9
to
33d2db7
Compare
Thanks! Note that I've adjusted the CI accordingly. |
No problem, thanks for the careful review! |
This is my shot at converting the
lwt_ppx
toppxlib
instead ofppx_tools_versioned
, which is going to be deprecated. This new version passes all tests performed after adune runtest
invocation, I only see this warning:which is I believe is expected.
I tried to stay as close as possible to the original code, by sticking to a whole-module transformation. An alternative might have been to resort to
ppxlib
's context-free rules which are preferable but maybe not expressive enough here, I'm not sure.One recurrent difference with the original code is that I think there is no equivalent of the
[@metaloc ]
attribute inppxlib.metaquot
which forced me to introduce all quotations with alet loc in ...
prelude.I'd be surprised if there were nothing to change to this patch, so if you think it can lead to something, I'd be happy to work on it further.