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

Inflexibility of unboxed types in recursive declarations #7364

Closed
vicuna opened this issue Sep 21, 2016 · 10 comments
Closed

Inflexibility of unboxed types in recursive declarations #7364

vicuna opened this issue Sep 21, 2016 · 10 comments
Assignees
Labels

Comments

@vicuna
Copy link

@vicuna vicuna commented Sep 21, 2016

Original bug ID: 7364
Reporter: @gasche
Assigned to: @damiendoligez
Status: assigned (set by @mshinwell on 2016-12-07T16:54:05Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.04.0 +dev / +beta1 / +beta2
Category: typing
Has duplicate: #7532 #7774
Monitored by: sweeks @yallop

Bug description

Markus Mottl remarked that unboxed types safeguards against floating points are not computed precisely enough in the case of mutually recursive type declarations:

#606 (comment)

His example is natural, it suggests that fixing this limitation before the release could be very beneficial.

Steps to reproduce

type ('a, 'kind) tree =
  | Root : { mutable value : 'a; mutable rank : int } -> ('a, [ `root ]) tree
  | Inner : { mutable parent : 'a node } -> ('a, [ `inner ]) tree

and 'a node = Node : ('a, _) tree -> 'a node  [@@ocaml.unboxed]

type 'a t = ('a, [ `inner ]) tree
@vicuna
Copy link
Author

@vicuna vicuna commented Sep 21, 2016

Comment author: @alainfrisch

The criterion to decide float/non-float is not really specified, and there will certainly be other similar cases (e.g. with recursive modules). This can be improved over time (e.g. by dropping the unboxed float array hack?), but doing it in a rush so close from the release date does not seem a good idea to me, especially considering that the current implementation for the criterion is already quite complex.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 26, 2016

Comment author: @damiendoligez

I agree with Alain. This probably needs a fixpoint computation on type declarations, which I won't do in a hurry.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 26, 2016

Comment author: @gasche

Ok, I updated the Changelog to emphasize this limitation.

@vicuna
Copy link
Author

@vicuna vicuna commented Feb 23, 2017

Comment author: @gasche

I believe that this is a fairly problematic restriction to the feature, in the sense that it does not make sense from a language design point of view. I understand that it is not on anyone's high-priority list, but by the time it gets pushed to such a list it will be too late, your users will be relying on the feature and want to support OCaml versions that are broken in this way, so people will design ugly work-around.

Is there an actual step we can take to make sure that this is at least ready for 4.06? I have three ideas:

  1. Give Gabriel a permanent research position.
  2. Entice Jeremy Yallop into solving the problem.
  3. Find a student/hobbyist with free time that would be willing to work on a demanding-work, moderate-risk, nice-reward programming task for the OCaml compiler.

If we decide to go for (3), I can write a project description (I think that it would be around two months of work for someone not super-familiar with the codebase, if we include the integration time), and publicize the demand on the caml-list for example.

@vicuna
Copy link
Author

@vicuna vicuna commented Oct 15, 2017

Comment author: @xavierleroy

  1. Give Gabriel a permanent research position.

Achievement unlocked!

@vicuna
Copy link
Author

@vicuna vicuna commented Nov 29, 2017

Comment author: @gasche

So I'm currently advising a student intern (Simon Colin) working on precisely this issue (better understanding the [@@unboxed] safety check, and extending it to mutually-recursive definitions), and I hope that he will have something to show around January/February.

(If someone else wants to work on this before that time, please get in touch with me first.)

@vicuna
Copy link
Author

@vicuna vicuna commented Apr 14, 2018

Comment author: @gasche

Rodolphe has a simpler (but less well-justified) example of overly restrictive behavior of the current implementation:

type 'a t = {name : string; data : 'a}
and any = Any : 'a t -> any [@@ocaml.unboxed]

and I gave a link in its duplicate MPR to Simon's github repository:

https://github.com/SimonColin/ocaml-unboxed-check-project

Basically the current status is that the researchy part is done, but there is no compiler patch yet. I was hoping to write it together with Simon, but if that doesn't work out I'll try to write it in time for 4.08.

@vicuna
Copy link
Author

@vicuna vicuna commented Dec 11, 2018

Comment author: @gasche

With Rodolphe Lepigre, we implemented Simon's internship as a Pull Request:

#2188

@github-actions
Copy link

@github-actions github-actions bot commented May 9, 2020

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 9, 2020
@yallop
Copy link
Member

@yallop yallop commented May 9, 2020

I think that @rlepigre, @gasche and @SimonColin have thoroughly fixed this in #2188, which is now merged.

The code in the bug report is now accepted in trunk:

# type ('a, 'kind) tree =
  | Root : { mutable value : 'a; mutable rank : int } -> ('a, [ `root ]) tree
  | Inner : { mutable parent : 'a node } -> ('a, [ `inner ]) tree
and 'a node = Node : ('a, _) tree -> 'a node  [@@ocaml.unboxed] ;;

type ('a, 'kind) tree =
    Root : { mutable value : 'a; mutable rank : int;
    } -> ('a, [ `root ]) tree
  | Inner : { mutable parent : 'a node; } -> ('a, [ `inner ]) tree
and 'a node = Node : ('a, 'b) tree -> 'a node [@@unboxed]

# type 'a t = ('a, [ `inner ]) tree;;

type 'a t = ('a, [ `inner ]) tree
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.