Skip to content
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

Closed
vicuna opened this issue Sep 26, 2014 · 8 comments
Closed

Backport fix for #5904 into 4.02.1 #6582

vicuna opened this issue Sep 26, 2014 · 8 comments
Assignees
Labels
Milestone

Comments

@vicuna
Copy link

vicuna commented Sep 26, 2014

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:

  1. sedlex (Use "cookies" from 4.02.1 to make named regexps work in toplevel ocaml-community/sedlex#10) that implements named regular expressions as `let' structure items
  2. ppx_deriving (https://github.com/whitequark/ppx_deriving), which currently uses Dynlink to load deriving plugins rather than hook into findlib's #require machinery. As ppx_deriving will migrate from Dynlink in immediate future, lack of toplevel cookies is a major problem for it and it would be great if it would not be broken in toplevel until 4.03 (i.e. a year or so from now).

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.

@vicuna
Copy link
Author

vicuna commented Sep 27, 2014

Comment author: @damiendoligez

While I'm aware that 4.02.1 is a bugfix-only release

That's not really the criterion. The main criteria are:

  1. backward-compatibility: must not break anything that works in 4.02.0 (except of course things that rely on bugs).
  2. safety: must have extremely low risk of breaking any "mainstream" feature and very low risk of breaking anything else.
  3. importance: the bug or missing feature must have significant impact on our user base.

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.

@vicuna
Copy link
Author

vicuna commented Sep 28, 2014

Comment author: @whitequark

I believe all three criteria are satisfied.

  1. I've tested the patch against testsuites of all ppx code in existence, and it all works. I also tested the features it introduces.
  2. The patch doesn't touch anything except ppx rewriting code, and as said, ppx works with it.
  3. I believe that the impact is significant. We're trying to make ppx_deriving a replacement for type_conv for 4.02, and it would be great if it wouldn't be broken in toplevel until 4.03 (a year away).

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?

@vicuna
Copy link
Author

vicuna commented Sep 28, 2014

Comment author: @whitequark

Oh, not just for type_conv--the ocsigen team is also interested in replacing deriving with ppx_deriving for eliom.

@vicuna
Copy link
Author

vicuna commented Sep 28, 2014

Comment author: @damiendoligez

OK, as far as I'm concerned this is good enough.

@vicuna
Copy link
Author

vicuna commented Sep 29, 2014

Comment author: @whitequark

Alain, can you commit the backport?

@vicuna
Copy link
Author

vicuna commented Oct 1, 2014

Comment author: @alainfrisch

Committed to 4.02 (rev 15411).

Further, this is not a breaking change; all software that worked with 4.02.0 will work with this backport as well.

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?

@vicuna
Copy link
Author

vicuna commented Oct 1, 2014

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.

@vicuna
Copy link
Author

vicuna commented Oct 2, 2014

Comment author: @alainfrisch

Ok, I've made the new argument optional (default = true). Committed to trunk (15414) and 4.02 (15415).

@vicuna vicuna closed this as completed Dec 7, 2016
@vicuna vicuna added this to the 4.02.1 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
avsm pushed a commit to avsm/ocaml that referenced this issue Jun 21, 2020
avsm pushed a commit to avsm/ocaml that referenced this issue Jun 21, 2020
…herry-picked from trunk, commit 15414.)

git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.02@15415 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants