AST cleanup: first class expression and patterns for records with opt… #7192
Conversation
cristianoc
force-pushed
the
cleanup-optional-exp-pat
branch
from
58fd6c5 to
7a42482
Compare
| @@ -83,7 +83,8 @@ and pattern_desc = | |||
| See {!Types.row_desc} for an explanation of the last parameter. | |||
| *) | |||
| | Tpat_record of | |||
| (Longident.t loc * label_description * pattern) list * closed_flag | |||
| (Longident.t loc * label_description * pattern * bool (* optional *)) list | |||
There was a problem hiding this comment.
optional pattern now explicit
There was a problem hiding this comment.
For cases like this, as the number of elements in the payload grows, do you think it would make sense to move to inline records at some point?
There was a problem hiding this comment.
Definitely.
Keeping things self contained for this PR.
| fields: | ||
| (Types.label_description | ||
| * record_label_definition | ||
| * bool (* optional *)) |
There was a problem hiding this comment.
optional record field now explicit
| @@ -183,7 +183,8 @@ and pattern_desc = | |||
| (* `A (None) | |||
| `A P (Some P) | |||
| *) | |||
| | Ppat_record of (Longident.t loc * pattern) list * closed_flag | |||
| | Ppat_record of | |||
| (Longident.t loc * pattern * bool (* optional *)) list * closed_flag | |||
There was a problem hiding this comment.
optional pattern now explicit
| @@ -260,7 +261,9 @@ and expression_desc = | |||
| (* `A (None) | |||
| `A E (Some E) | |||
| *) | |||
| | Pexp_record of (Longident.t loc * expression) list * expression option | |||
| | Pexp_record of | |||
| (Longident.t loc * expression * bool (* optional *)) list | |||
There was a problem hiding this comment.
optional record field now explicit
| @@ -261,7 +261,8 @@ let pattern sub pat = | |||
| | Tpat_record (list, closed) -> | |||
| Ppat_record | |||
| ( List.map | |||
| (fun (lid, _, pat) -> (map_loc sub lid, sub.pat sub pat)) | |||
| (fun (lid, _, pat, opt) -> | |||
| (map_loc sub lid, sub.pat sub pat, opt)) | |||
There was a problem hiding this comment.
This is where the extra field in typed ASTs is used: to compile back to untyped ASTs.
Before, this was passed down in attributes.
The nest phase, translation to lambda, does not need this information.
| @@ -83,7 +83,8 @@ and pattern_desc = | |||
| See {!Types.row_desc} for an explanation of the last parameter. | |||
| *) | |||
| | Tpat_record of | |||
| (Longident.t loc * label_description * pattern) list * closed_flag | |||
| (Longident.t loc * label_description * pattern * bool (* optional *)) list | |||
There was a problem hiding this comment.
For cases like this, as the number of elements in the payload grows, do you think it would make sense to move to inline records at some point?
| @@ -100,5 +100,6 @@ type ttt = {x: int, y?: string} | |||
| let optParen = {x: 3, y: ?(someBool ? Some("") : None)} | |||
| let optParen = {x: 3, y: ?(3 + 4)} | |||
| let optParen = {x: 3, y: ?foo(bar)} | |||
| let optParen = {x: 3, y: ?foo->bar} | |||
| let optParen = {x: 3, y: ?(foo->bar)} | |||
There was a problem hiding this comment.
Is the output change intentional?
There was a problem hiding this comment.
It was random before.
We can make a choice now.
Before it was adding parens to 3+4 because it would see a printable attribute (res.optional was considered printable).
Now I have explicitly made binary operators adding parens.
I can special case -> if without parens is preferable.
Any thoughts?
There was a problem hiding this comment.
One possibility is to treat it just like any other unary prefix operator, so have the same rules as - e.
There was a problem hiding this comment.
Personally I actually like it better with parens, and if that avoids special casing -> here, all the better.
…ional fields.
Extend record patterns and expressions in untyped and typed AST with a per-field boolean. To indicate
x : ? e.This was previously represented as an attribute
res.optional.