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 :)
| 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?
There was a problem hiding this comment.
This part of the code is destructuring the self -> t from the method type, and as far as I know there's no syntax to annotate methods with modes/modalities. The modes within the arrow (e.g. method foo: string @ local -> ... are in t = string @ local -> ... so there are not lost. Nevertheless I added an explicit check to detect if that ever changes :)
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!
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