Oxcaml: Support for modes - #1454
Conversation
Leonidas-from-XIV
left a comment
Leonidas-from-XIV
left a comment
There was a problem hiding this comment.
Looks nice, the logic determining what to leave out is actually more readable than the description makes it sound IMHO :)
| | [ (arg, _) ] -> TypeExpr.Arrow (None, arg, res) | ||
| TypeExpr.Arrow | ||
| (None, (Tuple (List.map (fun (x, _mods) -> None, x) args), []), (res, [])) | ||
| | [ (arg, _) ] -> TypeExpr.Arrow (None, (arg, []), (res, [])) |
There was a problem hiding this comment.
I wonder whether it wouldn't potentially make sense to make this into a record type (with some usefully named helper constructor functions) because the empty lists everywhere make it a bit verbose.
| let read_label = Cmi.read_label | ||
|
|
||
| let rec read_core_type env container ctyp = | ||
| read_core_type_modal env Cmi.legacy_modes container ctyp |
There was a problem hiding this comment.
Why does this always read legacy_modes?
There was a problem hiding this comment.
This argument is the base value for the accumulator for the "currently implicit modes" (such that we don't need to display them). It gets updated when traversing an arrow type such that the argument' modes carry over implicit modes for the output (otherwise we would display the redundant @ local described in the PR). In OCaml legacy_modes is just ():unit since we don't make use of it :)
| | Any | ||
| | Alias of t * string | ||
| | Arrow of label option * t * t | ||
| | Arrow of label option * (t * Modes.t) * (t * Modes.t) |
There was a problem hiding this comment.
As mentioned above maybe it would be better to have a with_mode record type of sorts here.
| val mode_both : int @ local -> int @ local | ||
| (** Modes on both argument and return. *) | ||
|
|
||
| val mode_multi : string @ local once -> string @ local unique |
There was a problem hiding this comment.
What if the modes are flipped?
There was a problem hiding this comment.
Flipped as in string @ once local -> string @ unique local? I'll add a test to show that similarly to the OxCaml compiler printer for the toplevel or inferred interfaces, the modes order is normalized by Cmi.tree_of_modes predefined order :)
There was a problem hiding this comment.
Yep, that's exactly what I meant :)
| let type_ = | ||
| match Cmi.read_type_expr env expr.exp_type with | ||
| | Arrow (_, _, t) -> t | ||
| | Arrow (_, _, (t, _)) -> t |
There was a problem hiding this comment.
Is there anything subtle here? Can the first argument have any mode that we're throwing away?
panglesd
left a comment
panglesd
left a comment
There was a problem hiding this comment.
I will need a bit more time to review the loading, since it is touching the internals of oxcaml (eg I'm not sure what zap_to_legacy is) but the rest is solid, and very well tested! Thanks!
| | _, _ -> Some modes.portability | ||
| in | ||
| let diff = Mode.Alloc.Const.diff modes Mode.Alloc.Const.legacy in | ||
| let diff = { diff with forkable; yielding; contention; portability } in |
There was a problem hiding this comment.
Maybe it would be nice to fully destruct diff here: It would make a compiler error if a new axis is introduced as a field in the record, warning us that there is a line to add in the list below.
There was a problem hiding this comment.
I realize only now this is a copy of an oxcaml function, but I still think we could modify it to be warned when a new field is added.
There was a problem hiding this comment.
I'm not sure to see where this module comes from. Maybe a leftover from a previous organization where things were grouped into modules?
| arg.ca_type, read_modalities Immutable arg.ca_modalities | ||
|
|
||
| let tree_of_modes (modes : Mode.Alloc.Const.t) : string list = | ||
| (* Same as the OxCaml's [Printtyp.tree_of_modes]: axes whose value is legacy |
There was a problem hiding this comment.
In today's OxCaml's main, tree_of_modes has been moved to Out_type. Maybe we could avoid naming the module containing the function?
| (* Same as the OxCaml's [Printtyp.tree_of_modes]: axes whose value is legacy | |
| (* Same as the OxCaml's [tree_of_modes]: axes whose value is legacy |
| let curried_acc modes arg_mode = | ||
| Ctype.curry_mode modes (Mode.Alloc.zap_to_legacy arg_mode) | ||
|
|
||
| let mode_is_implied modes res_mode = |
There was a problem hiding this comment.
modes could be named implied_mode, if I understand correctly. And I think the same in other functions would make them easier to read!
4 participants
This PR adds support for rendering OxCaml modes. Most of the complexity comes from the heuristics copied from the OxCaml compiler to omit modes which are either legacy, implied by other axes, or inferred from the context (for arrow types, e.g.
a @ local -> b -> c -> dis internally represented asa @ local -> (b -> (c -> d @ local) @ local) @ localbecause a partial application of the first argumenta @ localmust return alocalclosure (otherwise the local value could escape its scope)... but since the@ localexpansion is unreadable, we display the short form as it implies the long one)Fixes #1417