-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add extract_as attribute. #3466
Conversation
fa1b61c
to
ec07497
Compare
ec07497
to
3307f3c
Compare
That looks pretty cool, I'm thinking: this kind of lightweight OCaml "FFI" directly in the compiler F* sources would be kind of awesome, right? 😃 |
To be clear: this attribute does not replace F* by OCaml. It replaces F* by some other F* code. This is mainly useful when you have a type with extra structure that you want to forget during extraction; in Pulse we have the For backend-specific implementations, you'd want a different attribute. Maybe something like |
@mtzguido Do you have any thoughts on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Gabriel, this looks good, just wondering about that true
boolean.
More high-level I was wondering if we could have implemented this by using Syntax.Visit.visit_tm
to just replace each fvar by its extract_as just before extracting, but that will not interact well with normalization. If we did extraction into OCaml incrementally, and did inlining over the OCaml AST, I think that would work and be easier, but that's for another day : ).
let fixup_sigelt_extract_as se = | ||
match se.sigel, find_map se.sigattrs N.is_extract_as_attr with | ||
| Sig_let {lids; lbs=(_, [lb])}, Some impl -> | ||
{se with sigel = Sig_let {lids; lbs=(true, [{lb with lbdef = impl}])}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The true
is interesting here, so we always mark it recursive? Can it really be so since the term that is the argument to extract_as
cannot mention the name of this sigelt (without pack_fv trickery)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case it's not a big deal, but maybe worth a comment in the attribute's doc and/or here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned offline, the implementation can indeed be recursive. This is for example the case with recursive Pulse functions.
I have added a comment to both the attribute and the fixup function here.
| Tm_uinst({n=Tm_fvar fv}, [u]), [_; (t, _)] | ||
when Syntax.fv_eq_lid fv PC.extract_as_lid -> | ||
Some t | ||
| Tm_fvar fv, [t, _] when Syntax.fv_eq_lid fv PC.extract_as_lid -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal but probably better to avoid these back-and-forth changes within a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going by the contribution guidelines. We should probably update them now.
25b2242
to
2afb172
Compare
I rebased this btw. If we add a comment about the recursive bit being set then I'm happy merging. Also: check-world run https://github.com/FStarLang/FStar/actions/runs/11058160723 |
2afb172
to
b887b0f
Compare
I definitely would like to see |
To quote the documentation for the attribute: