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

Safe-to-marshall annotation #7448

Open
vicuna opened this issue Jan 3, 2017 · 9 comments
Open

Safe-to-marshall annotation #7448

vicuna opened this issue Jan 3, 2017 · 9 comments

Comments

@vicuna
Copy link

vicuna commented Jan 3, 2017

Original bug ID: 7448
Reporter: @Chris00
Status: acknowledged (set by @damiendoligez on 2017-02-27T14:48:43Z)
Resolution: open
Priority: normal
Severity: feature
Platform: x86_64
OS: GNU/Linux
OS Version: Debian testing
Version: 4.04.0
Category: typing
Monitored by: @gasche @ygrek @hcarty

Bug description

In the spirit of the annotation ocaml.unboxed and ocaml.immediate, it would be useful to add an annotation ocaml.safe_marshal that would check that a value is safe to marshal. (By default, closures would not be allowed but the optional payload may indicate that we allow them.) Indeed, especially when using libraries themselves relying on other libraries... it is brittle to assert by digging the code that a value can be marshalled without raising an exception—and this stands badly against changes in those libraries.

@github-actions
Copy link

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
@Drup
Copy link
Contributor

Drup commented May 9, 2020

How would such annotation work on abstract types ? It seems like it would be incomplete by nature, making it not very useful.

@gasche
Copy link
Member

gasche commented May 9, 2020

I believe that the proposal is precisely to be able to add this annotation on an abstract type in a signature, like ocaml.immediate, to properly deal with abstraction boundaries.

@github-actions github-actions bot removed the Stale label Jun 10, 2020
@github-actions
Copy link

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
Copy link

github-actions bot commented Jul 1, 2022

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 Jul 1, 2022
@gasche
Copy link
Member

gasche commented Jul 1, 2022

We are in the process of revamping the implementation of immediate, see #10041. I think that other "type properties" like the one proposed here would be easier to add after that work is complete.

@github-actions github-actions bot removed the Stale label Jul 4, 2022
@github-actions
Copy link

github-actions bot commented Jul 7, 2023

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 Jul 7, 2023
@Octachron
Copy link
Member

@gasche , do you still think we should revisit the issue now that we have revamped the handling of immediate?

@gasche
Copy link
Member

gasche commented Jan 24, 2024

I think that we actually haven't revamped the handling of immediates, none of the PR attempting to do it have been merged -- for a mix of not-so-good and some good reasons.

If someone wants to implement the current feature, I think that this is a reasonable thing to do without waiting for any other change.

(what follows is a brain dump of what I remember on this question)

The question is where in the typechecker structures we want to store kind information on abstract types. #10041 was suggesting that the current way we are doing it is not the best one, and proposing a different approach. I also needed to answer this question in my own (still unmerged) work on constructor unboxing, and this is the same question here. The Jane Street fork also touches this for unboxed types layouts, and I hoped that we could all converge to a single good way to do this, merge it upstream (independently of those features), with a record type at the right place that we can grow it independently without too much conflicts.

But then the Jane Street people realized that they weren't sure which approach was best, and (if I understand correctly) they went back from what #10041 proposed to something closer to what we currently have upstream -- I'm not sure. But we still don't have this record type to which anyone can just easily add this information, so there is no obvious way to implement it without conflicting with everyone else that has unmerged changes doing something like this.

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

5 participants