-
Notifications
You must be signed in to change notification settings - Fork 42
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
Generative labels #1169
base: master
Are you sure you want to change the base?
Generative labels #1169
Conversation
allow better printing
This reverts commit 49a9a61.
This reverts commit 5150615.
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
Co-authored-by: Daniel Hillerström <[email protected]>
This commit fixes a regression introduced by the previous commit, wherein numeric labels wouldn't get looked up correctly in `Label.{Map,Set}`. This commit also generalises `fresh` such that it can be used locally within a binding context. Furthermore, it changes the syntax such that it no longer introduces encloses a list of bindings, but instead uses the standard block-structured scope of Links.
The feature, as implemented by this patch, does not interact well with some of Links' other features, e.g. modules. However that shouldn't be a blocker. Another thing, after doing some thinking os that I would like to repurpose |
It is worth remarking that this patch further widens the gap between type-inferable expressions and type-ascribable expressions. For instance,
The programmer can never manually ascribe the effect-type to the above function, because the label
A similar problem also exists in SML with local datatype definitions, e.g. - fn () => let datatype t = Unit in Unit end;;
val it = fn : unit -> ?.t |
The |
Of course we can. The question is whether we should. |
Well... it's true that we could in theory (with a bit of care to disambiguate syntax) choose to overload Here's the relevant parser code for supporting
This is indeed a hack... and arguably bad language design. But I could imagine a more systematic way of allowing certain suitably contextually-distinguished keywords (e.g. those that must appear before an open brace character such as |
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 general, I'm having troubles trying to understand how exactly the feature is supposed to work, even after reading the original PR and trying to understand what may have changed in this version. I would appreciate an up-to-date description.
In particular, I'm interested in understanding how the removal of local labels from types and datatype definitions is supposed to work (erase_local_labels_from_tycon
and erase_local_labels_from_type
in Typesugar
).
Also, do we have an idea if this negatively interacts with Remy-style rows once we actually implement them?
(function | ||
| Types.Present t -> | ||
extract t | ||
| _ -> assert false) | ||
(* | Types.Present t -> *) |
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.
should this be here?
@@ -18,9 +18,9 @@ type tyvar = Quantifier.t | |||
type tyarg = Types.type_arg | |||
[@@deriving show] | |||
|
|||
type name_set = Utility.stringset | |||
type name_set = Label.Set.t |
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.
Are all of these really labels? For example, name_map
is used by the XmlNode
constructor, and I'm not sure if we consider the names in their to be "labels".
type t = | ||
| Local of local | ||
| Global of global | ||
| Number of int |
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.
After some offline discussion with @dhil it seems like Number
is just a special case of Global
, with a more efficient internal representation. I think we should therefore rename Number
to Global_number
and make the latter and Global
behave equivalently outside of this module. Further, of_int
should be make_global_of_int
instead.
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 the naming can be better here too:
type t =
| Bound of ...
| Textual of ...
| Number of ...
let bind_labels context ls = {context with label_env = Label.Env.bind_labels ls context.label_env} | ||
(* let unbind_labels context ls = {context with label_env = Label.Env.unbind_labels ls context.label_env} *) | ||
|
||
(* let extend = Types.extend_typing_environment *) |
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.
debugging code?
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 am not sure. But it can be deleted.
} ; | ||
Some (`Mutual (qs, tygroup)) | ||
|
||
(* let erase_local_labels labels decls ctx = *) |
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.
leftover code
| _ -> dt | ||
in | ||
try Some (e dt) | ||
with CannotErase -> None |
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.
using exceptions for control flow?
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.
Yes.
@@ -4898,12 +5127,27 @@ and type_binding : context -> binding -> binding * context * Usage.t = | |||
let () = unify pos ~handle:Gripers.bind_exp | |||
(pos_and_typ e, no_pos Types.unit_type) in | |||
Exp (erase e), empty_context, usages e | |||
| FreshLabel ls -> |
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 don't understand what's happening here. It seems that at this point, some local labels ls
are brought into scope, but then we try to remove occurences of these labels from types in context
, which is the typing context before ls
are brought into scope?
Is similar logic necessary for when labels are brought into scope using the fresh
keyword in, say, a function definition? I assume the code here is only run for standalone fresh
declarations, right?
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 there is a bug here actually. The labels are bought into to scope, but I think they should be removed when the block is exited. For this to work correctly, I think we need to track the "level" (i.e. block nesting depth) that the labels were introduced at.
This PR is a rebase of #1148 on top of the latest changes to
master
-- in particular those made to effect handlers.This PR also generalises the previous work the following ways:
fresh
declarations can be made in any binding contexts (just likefun
,var
, etc.)fresh
does no longer equipped with an explicit scope, wherein the fresh labels are defined, instead it now follows the standard block-structured lexical scope of Links.This PR also fixes a regression for session exceptions introduced by one of the previous patches, which made a change to the operation syntax at the type level. However, this change was never propagated properly, as a result session exceptions continued to use the reinterpretation of the function arrow.