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

Do something with *predef*.option #6611

Closed
vicuna opened this issue Oct 12, 2014 · 3 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Oct 12, 2014

Original bug ID: 6611
Reporter: @whitequark
Assigned to: @garrigue
Status: closed (set by @xavierleroy on 2016-12-07T10:47:00Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.02.2+dev / +rc1
Fixed in version: 4.03.0+dev / +beta1
Category: ~DO NOT USE (was: OCaml general)
Related to: #6367
Monitored by: @gasche @diml

Bug description

Currently, the types of all optional arguments in function signatures must be wrapped in an internal "predef option" type, which can be done with this function:

let wrap_predef_option typ =
  let predef_option = mknoloc (Ldot (Lident "*predef*", "option")) in
  Typ.constr predef_option [typ]

This is extremely unintuitive. The compiler will normally show types not wrapped in "predef option" as "" and complain that the function does not match its signature. In fact, it cost me an hour of debugging before I accidentally looked at Pprintast and noticed an assertion that checks for "predef option".

There are two ways to fix this that I can see.

  1. Proper way. Remove the notion of "predef option" from Parsetree entirely. "?" at the start of the label already carries all the necessary information.

  2. Workaround. Make Typ.arrow look at label and automatically wrap the type in "predef option" if it starts with "?".

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Oct 13, 2014

Comment author: @alainfrisch

I'd go for 1), which is technically very simple: just move the code which adds the predef.option wrapper from parser.mly to typetexp.ml/typeclass.ml (in the cases for Ptyp_arrow/Pcty_arrow). But this cannot be done lightly, since it would break for instance camlp4/camlp5. One more backward compatible change would be to add the wrapper only if it is not already present in the Parsetree.

I've added a patch which does that (untested), but this seems too risky to go into 4.02.1.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 17, 2014

Comment author: @damiendoligez

The patch looks good, but please add a comment in wrap_option_arg explaining the backward-compatibility workaround.

Also, before closing this PR we will need to push the information to the camlp4 and camlp5 developers.

@vicuna

This comment has been minimized.

Copy link
Author

commented Dec 22, 2014

Comment author: @garrigue

I committed this in trunk, at revision 15738, but without the compatibility provision, since I have just exhauced 6367 which is incompatible at the type level anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.