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
Backport fix for #5904 into 4.02.1 #6582
Comments
Comment author: @damiendoligez
That's not really the criterion. The main criteria are:
Of course, "mainstream" is a fuzzy concept, but I don't consider ppx to be mainstream yet because it's a new feature, so I'm willing to accept more risk of breaking it. For example, if you were touching the parser or the type-checker, I'd only accept a zero-risk patch (basically, a carefully reviewed one-liner). That said, the size of the patch makes me nervous. Has it been reviewed? Thoroughly tested in trunk, maybe? Someone please convince me that it's safe enough to include in 4.02.1 with very little time left for testing. |
Comment author: @whitequark I believe all three criteria are satisfied.
The diff looks scarier than it is, a good part of it is just moving code around. As described, I have tested the trunk with all ppx-related code I'm aware of. What kind of review and testing would convince you? |
Comment author: @whitequark Oh, not just for type_conv--the ocsigen team is also interested in replacing deriving with ppx_deriving for eliom. |
Comment author: @damiendoligez OK, as far as I'm concerned this is good enough. |
Comment author: @whitequark Alain, can you commit the backport? |
Comment author: @alainfrisch Committed to 4.02 (rev 15411).
Something could break, because of the extra non-optional parameters added to Pparse.apply_rewriters (e.g. in the rewriter you added recently to ppx_tools). Do you think it is worth making this parameter optional? |
Comment author: @whitequark Alain, right, I completely forgot about it -- utop indeed breaks with this patch. I think ~recover should be made optional. This will also make rewriter from ppx_tools build on both 4.02 and 4.02.1 &c. I also think it should be optional from the API standpoint--the dominant use case is rewriter hosting tools, this API should be geared for that. |
Comment author: @alainfrisch Ok, I've made the new argument optional (default = true). Committed to trunk (15414) and 4.02 (15415). |
git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.02@15411 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
…herry-picked from trunk, commit 15414.) git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.02@15415 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
Original bug ID: 6582
Reporter: @whitequark
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2016-12-07T10:36:44Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.1+dev
Target version: 4.02.1+dev
Fixed in version: 4.02.1+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @gasche @diml @hcarty
Bug description
Relevant commit: 44c2066
Currently, when used in the toplevel, ppx rewriters cannot keep any state between phrases. This interacts badly with the assumptions users have about toplevel; in particular, it means that the exact same code executed in toplevel and compiled in batch mode will behave in different ways.
There are already several use cases for this feature:
While I'm aware that 4.02.1 is a bugfix-only release, this problem arguably qualifies as a bug. Similar problems have been fixed in the past even when 4.02.0 was in feature-freeze mode:
Further, this is not a breaking change; all software that worked with 4.02.0 will work with this backport as well.
The text was updated successfully, but these errors were encountered: