Zero alloc annotation for OxCaml#1422
Zero alloc annotation for OxCaml#1422Leonidas-from-XIV wants to merge 8 commits intoocaml:masterfrom
Conversation
art-w
left a comment
There was a problem hiding this comment.
It looks good! I have a minor nitpick, mostly to be safe because I don't know if the additional O.cut could break existing docs. Do you prefer addressing the remaining TODOs for the cmti in a separate PR?
| let name, _, _ = attribute_unpack attr in | ||
| match String.equal name "zero_alloc" with | ||
| | true -> Some Lang.Value.Zero_alloc | ||
| | false -> None |
There was a problem hiding this comment.
The documentation mentions that it's possible to add some option to the zero_alloc, e.g. [@@zero_alloc strict] or assume or opt etc. This doesn't look critical since strict/assume shouldn't be relevant for end-users consuming the documentation, but maybe opt is important?
There was a problem hiding this comment.
Good idea! I've looked into how the OxCaml toplevel prints it and it looks like this:
| Implementation | Inferred interface |
|---|---|
[@zero_alloc assume] |
[@@zero_alloc] |
[@zero_alloc strict] |
[@@zero_alloc strict] |
[@zero_alloc opt] |
[@@zero_alloc opt] |
I will add these.
panglesd
left a comment
There was a problem hiding this comment.
Thanks!
I've left a nitpick, a comment about payload support and one about not loading attributes in OxCaml.
Some small comments that probably require no change in this PR:
- In the oxcaml doc, they use
val[@zero_alloc] f : trather thanval f : t [@@zero_alloc]as in this PR (they are different syntax for the same thing). So maybe oxcaml users are more familiar with the former? But keeping as is is fine to me! - There is already some logic to extract special attributes (eg
@alertand@deprecated). However, they are rendered differently (using odoc@-tags). It's a pity to have two ways to handle attributes, but forzero_alloc(and other oxcaml-specific attributes to come), I think I prefer your approach.
| module Zero_alloc : sig | ||
| type t = Assume | Opt | Strict | ||
| end | ||
| type attr = Zero_alloc of Zero_alloc.t |
There was a problem hiding this comment.
According to the docs, the only allowed payloads are opt, strict and arity <n>.
If I understand correctly "assume" does not correspond to the default. (Assume in a module type would be: do not check that the implementation do not allocate, but use that information in uses outside.)
Moreover, it seems possible to use multiple (valid) payloads.
So I think we have two options in terms of type representation: the "more correct" one
| module Zero_alloc : sig | |
| type t = Assume | Opt | Strict | |
| end | |
| type attr = Zero_alloc of Zero_alloc.t | |
| module Zero_alloc : sig | |
| type t = {opt : unit option ; strict: unit option; arity: int option } | |
| end | |
| type attr = Zero_alloc of Zero_alloc.t |
or the one that uses the fact that it has already been validated by the oxcaml compiler:
| module Zero_alloc : sig | |
| type t = Assume | Opt | Strict | |
| end | |
| type attr = Zero_alloc of Zero_alloc.t | |
| module Zero_alloc : sig | |
| type t = Arity of n | Opt | Strict | |
| end | |
| type attr = Zero_alloc of Zero_alloc.t |
Both are fine to me! And the second one is probably less work.
Here are some examples of zero_alloc uses in module types to "validate" the oxcaml doc:
# module type T = sig val[@zero_alloc] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc] end
(** The error says the accepted attributes {e for implementation}, not interface, unfortunately *)
# module type T = sig val[@zero_alloc gloubli-boulga] f : int -> int -> int end ;;
Warning 47 [attribute-payload]: illegal payload for attribute 'zero_alloc'.
It must be either 'assume', 'assume_unless_opt', 'strict', 'opt', 'opt strict', 'assume strict', 'assume never_returns_normally', 'assume never_returns_normally strict', 'assume error', 'ignore', 'arity <int_constant>', 'custom_error_message <string_constant>' or empty
(** assume is not allowed *)
# module type T = sig val[@zero_alloc assume] f : int -> int -> int end ;;
Error: zero_alloc assume attributes are not supported in signatures
(** strict, opt, and arity <n> (and combinations) are allowed *)
# module type T = sig val[@zero_alloc strict] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc strict] end
# module type T = sig val[@zero_alloc opt] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc opt] end
# module type T = sig val[@zero_alloc arity 1] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc arity 1] end
# module type T = sig val[@zero_alloc strict opt] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc strict opt] end
# module type T = sig val[@zero_alloc strict arity 2] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc strict] end
# module type T = sig val[@zero_alloc arity 1 opt strict] f : int -> int -> int end ;;
module type T =
sig val f : int -> int -> int [@@zero_alloc strict opt arity 1] end
# module type T = sig val[@zero_alloc] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc] end
# module type T = sig val[@zero_alloc] f : int -> int -> int end ;;
module type T = sig val f : int -> int -> int [@@zero_alloc] end| | { Location.txt = name; loc }, attr_payload -> (name, attr_payload, loc) | ||
| #endif | ||
|
|
||
| let ident (pexp_desc : Parsetree.expression_desc) = |
There was a problem hiding this comment.
Extreme nitpick: I find the name slightly too generic. Maybe "ident_of_expression"?
Very good point. I mostly did it this way as this is how the oxcaml toplevel prints it. So I assumed this is the way to go, but I will at least add a test-case to make sure that the |
We received feedback that the later form looks nicer for documentation :) |
This PR aims to take the zero alloc annotation from the source and retain it in the parsed output.
Currently this PR supports the
[@@zero_alloc]annotation on signatures. Next step would be to support the[@zero_alloc]annotation on bindings; not sure if it would make sense to add it in this PR or another one.This is my first PR to Odoc so excuse me for being a bit lost, I'll happily fix up/change stuff.
cc @art-w