-
Notifications
You must be signed in to change notification settings - Fork 103
Add support for 5.4 labeled tuples #607
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
base: main
Are you sure you want to change the base?
Conversation
75b5ad1 to
fa03fea
Compare
Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
Signed-off-by: Nathan Rebours <[email protected]>
ed2f409 to
7fa47ad
Compare
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.
Overall I think the approach is good here @NathanReb. A few remarks before reviewing more closely:
- Why did you go for this particular encoding? I'm a little worried that over time, the complexity of all of these encodings might be a bit much to handle, did you consider other ways? For example, serialising to a string or something?
- If I'm reading this correctly, the only way for new users to easily match on these features is via
Ast_pattern? For example, I'm maintaining ppx_deriving_yaml which does the usual direct pattern-matching on nodes (sometimes via metaquot). What would it look like to add support for labeled tuples there (or a different example of a real-world deriver)? Perhaps some docs would be helpful for users as I think Ast_pattern is not widely used at the moment.
let expand_expr expr =
match expr with
| { pexp_desc = Pexp_record x; _ } -> ...
| { pexp_desc = Pexp_tuple l; _ } -> handle_regular_tuple l
| _ -> unsupported_payload_error ()To add a case for labeled_tuples you'd add the following: let expand_expr expr =
match expr with
| { pexp_desc = Pexp_record x; _ } -> ...
| { pexp_desc = Pexp_tuple l; _ } -> handle_regular tuple l
| { pexp_loc; _ } ->
(match Ast_pattern.(parse_res (pexp_labeled_tuple __)) pexp_loc expr (fun x -> x) with
| Ok l -> handle_labeled_tuple l
| Error _ -> unsupported_payload_error ())I think we could smooth things out by exposing a couple of helpers in Ast_pattern such as a |
|
Thanks Nathan, even that small example would be great in some documentation. Given this is, until our next major release bump, supposed to be used by authors I think we need to make it easy for ppx authors to onboard :) |
This adds support for 5.4 labeled tuples and comes with different things:
Ast_builderandAst_patternhave 3 new functions each to build/destruct encoded labeled tuple types, expressions and patterns.The actual handling of the extension point is done in a single module in astlib:
Encoding_504. It provides constructor/destructor based on the 5.3 AST (used in the migration code) and on the 5.2 AST (used in Ast_builder/Ast_pattern).To make sure they don't desync in the future, I functorized the encoding/decoding. This required writing quite a bit of boiler plate and therefore would only advise this when we actually want to provide a stable API to handle such encoded nodes and would not bother otherwise.
E.g., when adding support for
Bivarianttype parameters, I will only provide encoding/decoding functions based on the 5.3 AST to be used in the migration code.