-
Notifications
You must be signed in to change notification settings - Fork 18
Uncase removal pass #191
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
Uncase removal pass #191
Conversation
|
I am assuming the CI is using version 4.10-4.12 ? Will note that for the future. (Maybe 5.0 given it states in the README to get ocaml 5.0 or higher) |
|
@rossberg: would you know why the CI would stack overflow? I have ran the tests locally and they seem to be fine. |
|
I have no idea in particular, but usually that's because some algorithm ends up in an infinite loop. |
|
FWIW, sometimes not all tests are run properly unless you do a make clean. |
Hmm thats what I thought initially, it might just be that I didn't |
Doesn't have to be now, but probably it'll improve our lives down the road if the name includes the name of the construction that's being projected out |
Fair enough, though for the sake of this PR and #195 I'll change it in a future PR. Though in the cases that it is actually used on at the moment, the constructors are empty strings. But who knows how this will be used later. |
Ah ok, I wasn’t aware of that. What’s the source syntax for that? What comes after the |
|
An example of this is: Which has source syntax as: After the |
|
Thanks! |
|
Perhaps the simplest example of an anonymous constructor being generated would be something like |
|
@nomeata I guess you can review this one then! Would you like me to fix the merge conflicts first though? |
|
Not needed, I can look at the diff. But I need to point out that I am fundamentally not a good reviewer, because I am usually too optimistic and too trusting… |
nomeata
left a comment
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.
Looks good enough to me
| if has_one_case then normal else | ||
| (* extra handling in case that it has more than one case *) | ||
| let extra_arg = ExpA (VarE (fresh_name $ at) $$ at % user_typ) $ at in | ||
| let new_bind = ExpB (fresh_name $ at, user_typ) $ at in | ||
| let opt_type = IterT (no_name_tupt, Opt) $ at in | ||
| let none_exp = OptE (None) $$ at % no_name_tupt in | ||
| let opt_tup = OptE (Some new_tup) $$ at % opt_type in | ||
| let clause' = DefD (List.map make_bind params @ new_binds, List.map make_arg params @ [new_arg], opt_tup, []) $ at in | ||
| let extra_clause = DefD (List.map make_bind params @ new_binds @ [new_bind], List.map make_arg params @ [extra_arg], none_exp, []) $ at in | ||
| DecD ((proj_prefix ^ id.it ^ "_" ^ Int.to_string idx) $ id.at, new_params, opt_type, [clause'; extra_clause]) |
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.
Have you considered not writing this code, and instead add a partial annotation when there is more than one case, and leave the opt-transformation to the totality pass?
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.
Haven't considered it, (or maybe I have, and I can't remember). It would make it cleaner yes.
Only issue with adding a partial annotation is making more dependencies between passes, which we kind of want to avoid, as there are already many dependencies.
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 think dependencies are ok where they compose nicely or avoid duplicating logic. But it's not a lot of code, so fine as is indeed.
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.
Makes sense. I'll leave it for now, and once we have a bigger picture on all the dependencies, I'll look into improving this.
| let clause = DefD (List.map make_bind params @ new_binds, List.map make_arg params @ [new_arg], new_tup, []) $ at in | ||
| let normal = DecD ((proj_prefix ^ id.it ^ "_" ^ Int.to_string idx) $ id.at, new_params, no_name_tupt, [clause]) in | ||
|
|
||
| if has_one_case then normal else |
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.
Just a nit, but (unless you go via the partial hint anyways), can you if has_one_case earlier, and indent, so that those bindings that are only relevant in the then-case are really only in the the then case?
let make_func …
let …
if has_one_case then
let …
DecD …
else
let …
DecD …
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.
Fair enough! I'll modify this while fixing the merge conflicts
This transformation focuses on transforming uncase expressions into explicit projection functions.
This is achieved through the following steps:
to a list of mixops.
the projection functions accordingly for each mixop in the list. If there is more than one case,
the projection function returns the case tuple wrapped in an optional type.
to a function call.
This pass works with/without dependent types. It will simply add the dependent type parameters
to the projection function whenver necessary.
As an example,
given the following type:
where A and B are case constructors, and v c are types.
Assume we have uncase expressions somewhere in our script (with x being a variable of type foo):
(x!A).0 and (x!B).1This is transformed into:
with uncase expressions being transformed into:
(the($proj_foo_0(x))).0 and (the($proj_foo_1(x))).1Names were specifically chosen here for simplicity.
This pass is an attempt to make uncase semantics more general, even though its use case is only limited to single case types.