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

optional parameters and non generalizable type variables #6899

Closed
vicuna opened this issue Jun 10, 2015 · 8 comments
Closed

optional parameters and non generalizable type variables #6899

vicuna opened this issue Jun 10, 2015 · 8 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Jun 10, 2015

Original bug ID: 6899
Reporter: @trefis
Assigned to: @garrigue
Status: closed (set by @xavierleroy on 2017-02-16T14:15:11Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.03.0+dev / +beta1
Target version: 4.03.0+dev / +beta1
Fixed in version: 4.03.0+dev / +beta1
Category: typing

Bug description

With trunk programs using optional parameter sometimes fail to type check.
Here is a smallest reproducible example I have:


$ cat test.ml
module Option : sig
type 'a t = 'a option
val is_some : 'a t -> bool
end = struct
type 'a t = 'a option
let is_some = function
| None -> false
| Some _ -> true
end

let test ?x () = Option.is_some x

$ ocamlc test.ml
File "test.ml", line 11, characters 9-33:
Error: The type of this expression, ?x: -> unit -> bool,
contains type variables that cannot be generalized


N.B. this only happens with trunk, 4.02 is fine; it doesn't happen in the toplevel.

There are two problems here (though probably a single root):

1/ The type of [x] contains variables that cannot be generalized.
This is probably related to #5663 and the consequent changes.

2/ The type of ?x is not printed, instead we get .
Meaning: printtyp doesn't know [x] is an option.

File attachments

@vicuna
Copy link
Author

vicuna commented Jun 10, 2015

Comment author: @trefis

Hmmm, the example can actually be cleaned.
New version:

type 'a t = 'a option
let is_some : 'a t -> bool = function
| None -> false
| Some _ -> true

let test ?x () = is_some x

@vicuna
Copy link
Author

vicuna commented Jun 10, 2015

Comment author: @trefis

Disregard the attached patch, the condition is the wrong way around and doesn't fix anything once changed.
(Context: I was trying to make sense of ebbf345 , at some point my example wasn't rejected anymore, I thought I had a fix).

@vicuna
Copy link
Author

vicuna commented Jun 22, 2015

Comment author: @trefis

I made a github pull request for this, it is at #204 .

As mentioned: the GPR doesn't attempt to fix the printing problem.

@vicuna
Copy link
Author

vicuna commented Jun 22, 2015

Comment author: @garrigue

Thanks for finding a solution.
This does not explain what the problem is, but I expect it to be a problem with levels in the type of the expression after generalization; one should never use the type of an expression partially generalized. So yes, your solution seems to be right.

I'm willing to merge it, but I have a question: how do I get a patch from your pull request?
The bridge between svn and github only works in one direction...

@vicuna
Copy link
Author

vicuna commented Jun 23, 2015

Comment author: @trefis

I expect it to be a problem with levels in the type of the expression after generalization; one should never use the type of an expression partially generalized

Yes, exactly.

I'm willing to merge it, but I have a question: how do I get a patch from your pull request?

I just attached the patch here, it is pr6899.patch

@vicuna
Copy link
Author

vicuna commented Jun 23, 2015

Comment author: @trefis

And git decided to add colors to the diff... The proper patch is pr6899-color=never.patch ...
Sorry.

@vicuna
Copy link
Author

vicuna commented Jun 23, 2015

Comment author: @trefis

Thank you for merging and adding a test!
I noticed that as a side effect of my patches, code like the following:

include struct
  let foo `Test = ()
  let wrap f `Test = f
  let bar = wrap ()
end

are now correctly rejected, while they weren't previously.
pr6899_more_tests.patch adds a regression test to ensure this particular code is rejected (as well as another more straightforward bad code).
Do you mind merging that as well?

Note: The piece of code given in example here is also accepted on 4.02.2. I don't know if people are considering doing another minor release at some point, but if yes then the patches should probably be backported on that branch.

@vicuna
Copy link
Author

vicuna commented Jun 29, 2015

Comment author: @garrigue

Merged the patch at revision 16184 and the extra tests at revision 16190.

@vicuna vicuna closed this as completed Feb 16, 2017
@vicuna vicuna added the typing label Mar 14, 2019
@vicuna vicuna added this to the 4.03.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants