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

Don't treat unboxed constructors as having statically-known size in the let-rec check #1593

Merged
merged 4 commits into from Feb 1, 2018

Conversation

Projects
None yet
3 participants
@yallop
Copy link
Member

yallop commented Feb 1, 2018

Fixes PR7717.

Here's @chambart's test case

type a = A of b [@@unboxed]
and b = X of a | Y

let rec a = A (if Sys.opaque_identity true then X a else Y)

Previously this would pass the let rec well-formedness check, compile, and crash when a was examined.

This PR changes the code to special-case unboxed variants and records so that they're no longer considered to have statically-known size. After this PR the rhs of the let rec binding is considered dynamic and the program is rejected.

@gasche

gasche approved these changes Feb 1, 2018

Copy link
Member

gasche left a comment

The change looks correct to me -- the description wasn't clear on the fact that not all unboxed constructors are dynamically-sized, it depends on the classification of their argument.

@yallop yallop force-pushed the yallop:dynamic-unboxed-letrec branch from 088cd3b to fc9b811 Feb 1, 2018

@chambart

This comment has been minimized.

Copy link
Contributor

chambart commented Feb 1, 2018

This looks good to me. Maybe you may just add a test of something like

type r = A of r list [@@unboxed];;
let rec a = A [a];;

Which is and should still be accepted.

@yallop

This comment has been minimized.

Copy link
Member Author

yallop commented Feb 1, 2018

@chambart: I've added the test in 51ed89b.

@chambart

This comment has been minimized.

Copy link
Contributor

chambart commented Feb 1, 2018

Thanks a lot, I merge.

@chambart chambart merged commit 9aae798 into ocaml:trunk Feb 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yallop yallop deleted the yallop:dynamic-unboxed-letrec branch Feb 1, 2018

@yallop

This comment has been minimized.

Copy link
Member Author

yallop commented Feb 6, 2018

@gasche: since @chambart's example produces a runtime crash (segmentation fault), this fix may be worth considering for inclusion in a patch release.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.