Remove parser modes - #8610
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba681ec574
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8610 +/- ##
==========================================
+ Coverage 77.32% 77.48% +0.15%
==========================================
Files 467 468 +1
Lines 63313 63619 +306
==========================================
+ Hits 48957 49295 +338
+ Misses 14356 14324 -32
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b374049de4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf4a97d54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | Pexp_tuple _ -> true | ||
| | _ -> false) | ||
| ~tuple:(fun ~loc args -> Ast_helper0.Exp.tuple ~loc args) | ||
| ~loc ~attrs args |
There was a problem hiding this comment.
When #Pair(a, b) is converted for an external PPX, this passes the whole variant location to the synthesized v0 Pexp_tuple, so its span starts at #Pair rather than at the opening parenthesis as parser-produced v0 payloads did. PPXs that inspect payload locations or issue payload diagnostics therefore receive an unexpectedly broad span; the analogous pattern branch has the same problem. Preserve the argument-list/parentheses span when synthesizing these tuples.
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
| when List.exists | ||
| (fun (payload_exp : Parsetree.expression) -> | ||
| loc_has_cursor payload_exp.pexp_loc | ||
| || Completion_expressions.is_expr_hole payload_exp | ||
| && loc_has_cursor pexp_loc) |
There was a problem hiding this comment.
When the cursor is after a comma or in whitespace between constructor arguments, such as Three("", |[]), none of the now-separate payload expression locations contains it, so this guard no longer records a constructor and signature help disappears. Previously the tuple payload spanned the complete parenthesized argument list. Check the outer constructor/argument-list extent and infer the adjacent argument index; the pattern guard below needs the same treatment.
Useful? React with 👍 / 👎.
cristianoc
left a comment
cristianoc
left a comment
There was a problem hiding this comment.
what happens in the bridge should stat in the bridge: ast should be restored and any encoding trick should be gone and not leaked in to the type checker
| | [] -> (None, attrs) | ||
| | [arg] when is_tuple arg -> (Some arg, add_constructor_tuple_arg_attr attrs) | ||
| | [arg] -> (Some arg, attrs) | ||
| | args -> (Some (tuple ~loc args), add_constructor_args_attr attrs) |
There was a problem hiding this comment.
[suggestion] _res.constructor_args (and _res.constructor_tuple_arg) already restore the current argument-list AST across frozen v0: several arguments become a tuple plus this marker, a single tuple argument gets the other marker, and from0 splits or keeps accordingly.
Unmarked v0 C(tuple) is still one payload. It should decode as [Pexp_tuple …] with no extra metadata. Instead, decode_args stamps _res.legacy_constructor_payload and typecore/printer unpack it using cstr_arity — the old tuple-as-args heuristic, leaked out of the bridge.
from0 cannot know arity, and it does not need to. _res.constructor_args / explicit_arity already mean “split”; unmarked means “one argument.” Typecore should only see the restored list. Dropping the legacy attr is enough: unmarked Pair(tuple) is an arity mismatch, explicit_arity still typechecks as two args, and Unary((int, int)) still typechecks.
There was a problem hiding this comment.
I agree that removing this would simplify typecore and the printer. The complication is fresh nodes generated by existing PPXs: in the frozen v0 AST, Some(Pexp_tuple [a; b]) historically represented both multiple constructor arguments and a single tuple payload, with type checking resolving the ambiguity.
Our bridge markers preserve round-tripped nodes, but existing PPXs don’t necessarily add them—or explicit_arity—when constructing new nodes. Treating every unmarked tuple as one argument would therefore break previously working PPXs.
There was a problem hiding this comment.
I agree that removing this would simplify typecore and the printer. The complication is fresh nodes generated by existing PPXs: in the frozen v0 AST,
Some(Pexp_tuple [a; b])historically represented both multiple constructor arguments and a single tuple payload, with type checking resolving the ambiguity.Our bridge markers preserve round-tripped nodes, but existing PPXs don’t necessarily add them—or
explicit_arity—when constructing new nodes. Treating every unmarked tuple as one argument would therefore break previously working PPXs.
I don't think there are any PPXs that even look at, let alone construct, any expressions.
There was a problem hiding this comment.
OK sury does, so that's a good example to investigate
There was a problem hiding this comment.
Investigated sury-ppx. It generates multi-argument constructor applications with no marker at all:
$ bsc -ppx node_modules/sury-ppx/bin -bs-ast -o Test.ast Test.res # @schema type t = A(int, string) | B(int)
$ bsc -dparsetree Test.ast 2>&1 | grep -c explicit_arity
0The dump contains Pexp_construct "A" -> Some (Pexp_tuple [...]), and the only attribute in all 173 lines is the schema one on the type declaration. It compiles today purely because of constr.cstr_arity > 1 in type_construct. So the compatibility concern is real: @schema on any variant with a multi-argument constructor would stop compiling. (sury-ppx is what rewatch/testrepo/packages/with-ppx uses.)
That said, I don't think the objection and the compatibility requirement are actually in conflict. They only collide because the type checker was made strict.
#8606 solved the same class of problem for string literals: the node carries source and semantic side by side, the printer reads one, the type checker reads the other, and the mode disappears without any valid program changing meaning. The analogue here is: the parsetree carries the syntactic argument list (which this PR gives it), and the type checker derives the semantic one from it. The disanalogy is that escape decoding is context-free while arity resolution needs the environment - so the semantic half cannot be stored in the node, it has to be computed in type_construct, right after Constructor.disambiguate. That is the only difference, and it is a few lines.
I prototyped it on top of cbf4a97:
- deletes
_res.legacy_constructor_payloadentirely - the attribute, the stamping infrom0, both removal helpers, bothres_printerunpacking sites, and thefilter_parsing_attrsentry - adds
constructor_args_of_exp_payload/constructor_args_of_pat_payloadintypecore.ml, used only at the two sites that already know the arity
Net -54 lines against this PR; against base (bc382a4) it is +1354 -1080 vs this PR's +1407 -1079. Every cleanup in the description survives: parser modes gone, for_printer gone, the Pexp_tuple [Pexp_tuple _] signal gone from the printer, arity explicit in the parsetree, and explicit_arity still deleted from builtin_attributes with its only four remaining references inside ast_mapper_from0. The one item it declines is "removes type-checker logic that inferred constructor arity by inspecting tuple nodes" - it names that logic and confines it to two functions instead of deleting it. Base already had exactly this rule inline at both sites; the prototype gives it a name and adds the direction the parser used to handle.
| master | this PR | prototype | |
|---|---|---|---|
Pair((1, 2)) |
OK | arity error | OK |
Some(1, 2) : option<(int,int)> |
OK | arity error | OK |
pattern Pair((u, v)) |
OK | arity error | OK |
pattern Some(u, v) |
OK | arity error | OK |
legacy PPX, unmarked v0 C(tuple) |
OK | OK (via marker) | OK, no marker |
Also checked: Pair(1), Pair(1,2,3), U(1,2) on arity 1 and pattern Pair(u) are still rejected with byte-identical messages to master; formatter output is identical to master and idempotent on a file mixing all four spellings plus poly variants; test_syntax.sh and ROUNDTRIP_TEST=1 both pass; ocamlformat clean. ounit is 257/259 - the two failures are fresh_ast0_constructor_tuple_defers_arity_to_typechecker, which asserts has_attr "_res.legacy_constructor_payload" literally, and ..._reprints_without_internal_metadata, which is a cosmetic difference on the post-PPX printing path only.
Two other things that came up while testing:
The marker is visible in -dsource. On this branch, compiling a PPX-produced multi-argument constructor prints:
let x = ((Pair (1, 2))[@_res.legacy_constructor_payload ])
It is stripped during typing and filtered by the .res printer, but it does reach user-facing output.
The breaking change is bidirectional and the changelog documents one direction. The entry covers Some(x, y), but Pair((1, 2)) on type pair = Pair(int, int) - one argument, arity two - also errors now, as does the pattern form. That is a materially larger migration surface than described.
If the strictness is wanted on its own merits, that seems reasonable to me - Some(x, y) quietly meaning Some((x, y)) is a genuine wart. But then it should be its own PR with its own migration story, at minimum an error message that suggests the nested parens (typecore has both the arguments and the arity right there). Bundling it here makes it look like removing the parser modes forces a breaking change, and it does not.
Prototype patch (apply on top of cbf4a97)
diff --git a/compiler/ml/ast_mapper_from0.ml b/compiler/ml/ast_mapper_from0.ml
index 4bc5009d5..373ce8321 100644
--- a/compiler/ml/ast_mapper_from0.ml
+++ b/compiler/ml/ast_mapper_from0.ml
@@ -166,7 +166,6 @@ let map_loc sub {loc; txt} = {loc = sub.location sub loc; txt}
let record_rest_attr_name = "_res.record_rest"
let constructor_args_attr_name = "_res.constructor_args"
let constructor_tuple_arg_attr_name = "_res.constructor_tuple_arg"
-let legacy_constructor_payload_attr_name = "_res.legacy_constructor_payload"
let has_explicit_arity_attr (attrs : Pt.attributes) =
List.exists
@@ -190,17 +189,15 @@ let remove_constructor_args_attr attrs =
let remove_constructor_tuple_arg_attr attrs =
remove_internal_marker_attr ~name:constructor_tuple_arg_attr_name attrs
-let add_legacy_constructor_payload_attr attrs =
- (Location.mknoloc legacy_constructor_payload_attr_name, Pt.PStr []) :: attrs
-
-let decode_args ~map ~tuple_args ~split_tuple ~known_tuple_arg = function
- | None -> ([], false)
+(* An unmarked v0 [C(tuple)] is one argument here. A payload that a legacy PPX
+ built as a tuple standing for several arguments is renormalized in Typecore,
+ which is the first place that knows the constructor's arity. *)
+let decode_args ~map ~tuple_args ~split_tuple = function
+ | None -> []
| Some arg -> (
match tuple_args arg with
- | Some args when split_tuple -> (List.map map args, false)
- | Some _ when known_tuple_arg -> ([map arg], false)
- | Some _ -> ([map arg], true)
- | None -> ([map arg], false))
+ | Some args when split_tuple -> List.map map args
+ | _ -> [map arg])
let record_rest_of_pattern (rest : Pt.pattern) =
match rest.Pt.ppat_desc with
@@ -887,10 +884,8 @@ module E = struct
| Pexp_construct (lid, arg) -> (
let lid1 = map_loc sub lid in
let has_constructor_args, attrs = remove_constructor_args_attr attrs in
- let has_constructor_tuple_arg, attrs =
- remove_constructor_tuple_arg_attr attrs
- in
- let args, has_legacy_constructor_payload =
+ let _, attrs = remove_constructor_tuple_arg_attr attrs in
+ let args =
decode_args ~map:(sub.expr sub)
~tuple_args:(fun arg ->
match arg.pexp_desc with
@@ -900,12 +895,7 @@ module E = struct
(has_constructor_args
|| has_explicit_arity_attr attrs
|| lid.txt = Longident.Lident "::")
- ~known_tuple_arg:has_constructor_tuple_arg arg
- in
- let attrs =
- if has_legacy_constructor_payload then
- add_legacy_constructor_payload_attr attrs
- else attrs
+ arg
in
let exp1 = construct ~loc ~attrs lid1 args in
match lid.txt with
@@ -966,17 +956,14 @@ module E = struct
| _ -> exp1)
| Pexp_variant (lab, arg) ->
let has_constructor_args, attrs = remove_constructor_args_attr attrs in
- let has_constructor_tuple_arg, attrs =
- remove_constructor_tuple_arg_attr attrs
- in
- let args, _ =
+ let _, attrs = remove_constructor_tuple_arg_attr attrs in
+ let args =
decode_args ~map:(sub.expr sub)
~tuple_args:(fun arg ->
match arg.pexp_desc with
| Pexp_tuple args -> Some args
| _ -> None)
- ~split_tuple:has_constructor_args
- ~known_tuple_arg:has_constructor_tuple_arg arg
+ ~split_tuple:has_constructor_args arg
in
variant ~loc ~attrs lab args
| Pexp_record (l, eo) ->
@@ -1147,10 +1134,8 @@ module P = struct
| Ppat_tuple pl -> tuple ~loc ~attrs (List.map (sub.pat sub) pl)
| Ppat_construct (l, arg) ->
let has_constructor_args, attrs = remove_constructor_args_attr attrs in
- let has_constructor_tuple_arg, attrs =
- remove_constructor_tuple_arg_attr attrs
- in
- let args, has_legacy_constructor_payload =
+ let _, attrs = remove_constructor_tuple_arg_attr attrs in
+ let args =
decode_args ~map:(sub.pat sub)
~tuple_args:(fun arg ->
match arg.ppat_desc with
@@ -1160,27 +1145,19 @@ module P = struct
(has_constructor_args
|| has_explicit_arity_attr attrs
|| l.txt = Longident.Lident "::")
- ~known_tuple_arg:has_constructor_tuple_arg arg
- in
- let attrs =
- if has_legacy_constructor_payload then
- add_legacy_constructor_payload_attr attrs
- else attrs
+ arg
in
construct ~loc ~attrs (map_loc sub l) args
| Ppat_variant (l, arg) ->
let has_constructor_args, attrs = remove_constructor_args_attr attrs in
- let has_constructor_tuple_arg, attrs =
- remove_constructor_tuple_arg_attr attrs
- in
- let args, _ =
+ let _, attrs = remove_constructor_tuple_arg_attr attrs in
+ let args =
decode_args ~map:(sub.pat sub)
~tuple_args:(fun arg ->
match arg.ppat_desc with
| Ppat_tuple args -> Some args
| _ -> None)
- ~split_tuple:has_constructor_args
- ~known_tuple_arg:has_constructor_tuple_arg arg
+ ~split_tuple:has_constructor_args arg
in
variant ~loc ~attrs l args
| Ppat_record (lpl, cf) ->
diff --git a/compiler/ml/typecore.ml b/compiler/ml/typecore.ml
index e22249bbc..5a4ac679b 100644
--- a/compiler/ml/typecore.ml
+++ b/compiler/ml/typecore.ml
@@ -1217,17 +1217,31 @@ exception Need_backtrack
Unification may update the typing environment. *)
(* constrs <> None => called from parmatch: backtrack on or-patterns
explode > 0 => explode Ppat_any for gadts *)
-let legacy_constructor_payload_attr_name = "_res.legacy_constructor_payload"
-
-let remove_legacy_constructor_payload_attr attrs =
- let rec loop rev_attrs = function
- | ({Location.txt; _}, PStr []) :: attrs
- when txt = legacy_constructor_payload_attr_name ->
- (true, List.rev_append rev_attrs attrs)
- | attr :: attrs -> loop (attr :: rev_attrs) attrs
- | [] -> (false, List.rev rev_attrs)
- in
- loop [] attrs
+(* A constructor payload in the parsetree is the argument list as it was
+ spelled: [C(a, b)] is two arguments and [C((a, b))] is one tuple argument.
+ Both spellings denote the same value, so the semantic argument list is
+ derived here, where the declared arity is known - the parser and the AST0
+ bridge cannot know it. Only these two functions read the syntactic list;
+ everything downstream sees the semantic one. *)
+let constructor_args_of_exp_payload ~arity (sargs : Parsetree.expression list) :
+ Parsetree.expression list =
+ match sargs with
+ | [{pexp_desc = Pexp_tuple args}] when arity > 1 -> args
+ | {pexp_loc = first_loc} :: (_ :: _ as rest) when arity = 1 ->
+ let last = List.nth rest (List.length rest - 1) in
+ let loc = Location.{first_loc with loc_end = last.pexp_loc.loc_end} in
+ [{pexp_desc = Pexp_tuple sargs; pexp_loc = loc; pexp_attributes = []}]
+ | sargs -> sargs
+
+let constructor_args_of_pat_payload ~arity (sargs : Parsetree.pattern list) :
+ Parsetree.pattern list =
+ match sargs with
+ | [{ppat_desc = Ppat_tuple args}] when arity > 1 -> args
+ | {ppat_loc = first_loc} :: (_ :: _ as rest) when arity = 1 ->
+ let last = List.nth rest (List.length rest - 1) in
+ let loc = Location.{first_loc with loc_end = last.ppat_loc.loc_end} in
+ [{ppat_desc = Ppat_tuple sargs; ppat_loc = loc; ppat_attributes = []}]
+ | sargs -> sargs
let rec type_pat ~constrs ~labels ~no_existentials ~mode ~explode ~env sp
expected_ty k =
@@ -1402,10 +1416,6 @@ and type_pat_aux ~constrs ~labels ~no_existentials ~mode ~explode ~env sp
pat_env = !env;
})
| Ppat_construct (lid, sargs) ->
- let has_legacy_constructor_payload, ppat_attributes =
- remove_legacy_constructor_payload_attr sp.ppat_attributes
- in
- let sp = {sp with ppat_attributes} in
let opath =
try
let p0, p, _ = extract_concrete_variant !env expected_ty in
@@ -1440,10 +1450,7 @@ and type_pat_aux ~constrs ~labels ~no_existentials ~mode ~explode ~env sp
correct head *)
if constr.cstr_generalized then unify_head_only loc !env expected_ty constr;
let sargs =
- match sargs with
- | [{ppat_desc = Ppat_tuple sargs}]
- when has_legacy_constructor_payload && constr.cstr_arity > 1 ->
- sargs
+ match constructor_args_of_pat_payload ~arity:constr.cstr_arity sargs with
| [({ppat_desc = Ppat_any} as sp)] when constr.cstr_arity <> 1 ->
if constr.cstr_arity = 0 then
Location.prerr_warning sp.ppat_loc
@@ -4429,9 +4436,6 @@ and type_application ~context total_app env funct (sargs : sargs) :
Apply_non_function (expand_head env funct.exp_type) )))
and type_construct ~context env loc lid sargs ty_expected attrs =
- let has_legacy_constructor_payload, attrs =
- remove_legacy_constructor_payload_attr attrs
- in
let opath =
try
let p0, p, _ = extract_concrete_variant env ty_expected in
@@ -4447,13 +4451,7 @@ and type_construct ~context env loc lid sargs ty_expected attrs =
Env.mark_constructor Env.Positive env (Longident.last lid.txt) constr;
Builtin_attributes.check_deprecated loc constr.cstr_attributes
constr.cstr_name;
- let sargs =
- match sargs with
- | [{pexp_desc = Pexp_tuple sargs}]
- when has_legacy_constructor_payload && constr.cstr_arity > 1 ->
- sargs
- | sargs -> sargs
- in
+ let sargs = constructor_args_of_exp_payload ~arity:constr.cstr_arity sargs in
if List.length sargs <> constr.cstr_arity then
raise
(Error
diff --git a/compiler/syntax/src/res_parsetree_viewer.ml b/compiler/syntax/src/res_parsetree_viewer.ml
index 796b107c3..9acb810e6 100644
--- a/compiler/syntax/src/res_parsetree_viewer.ml
+++ b/compiler/syntax/src/res_parsetree_viewer.ml
@@ -227,8 +227,7 @@ let filter_parsing_attrs attrs =
Location.txt =
( "res.braces" | "ns.braces" | "res.iflet" | "res.ternary"
| "res.await" | "res.patVariantSpread" | "res.dictPattern"
- | "res.dictSpread" | "res.inlineRecordDefinition"
- | "_res.legacy_constructor_payload" );
+ | "res.dictSpread" | "res.inlineRecordDefinition" );
},
_ ) ->
false
diff --git a/compiler/syntax/src/res_printer.ml b/compiler/syntax/src/res_printer.ml
index 0133555b5..a868811c1 100644
--- a/compiler/syntax/src/res_printer.ml
+++ b/compiler/syntax/src/res_printer.ml
@@ -2617,16 +2617,6 @@ and print_extension ~state ~at_module_lvl (string_loc, payload) cmt_tbl =
in
Doc.group (Doc.concat [ext_name; print_payload ~state payload cmt_tbl])
-and remove_legacy_constructor_payload_attr attrs =
- let rec loop rev_attrs = function
- | ({Location.txt = "_res.legacy_constructor_payload"}, Parsetree.PStr [])
- :: attrs ->
- (true, List.rev_append rev_attrs attrs)
- | attr :: attrs -> loop (attr :: rev_attrs) attrs
- | [] -> (false, List.rev rev_attrs)
- in
- loop [] attrs
-
and print_pattern_args ~state (patterns : Parsetree.pattern list) cmt_tbl =
match patterns with
| [] -> Doc.nil
@@ -2671,15 +2661,6 @@ and print_pattern_args ~state (patterns : Parsetree.pattern list) cmt_tbl =
]
and print_pattern ~state (p : Parsetree.pattern) cmt_tbl =
- let has_legacy_constructor_payload, ppat_attributes =
- remove_legacy_constructor_payload_attr p.ppat_attributes
- in
- let p =
- match (has_legacy_constructor_payload, p.ppat_desc) with
- | true, Ppat_construct (constr, [{ppat_desc = Ppat_tuple args}]) ->
- {p with ppat_desc = Ppat_construct (constr, args); ppat_attributes}
- | _ -> {p with ppat_attributes}
- in
let pattern_without_attributes =
match p.ppat_desc with
| Ppat_any -> Doc.text "_"
@@ -3191,15 +3172,6 @@ and print_object_get_doc ~state ~expr_loc parent_expr
Doc.group (Doc.concat [parent_doc; Doc.lbracket; member; Doc.rbracket])
and print_expression ~state (e : Parsetree.expression) cmt_tbl =
- let has_legacy_constructor_payload, pexp_attributes =
- remove_legacy_constructor_payload_attr e.pexp_attributes
- in
- let e =
- match (has_legacy_constructor_payload, e.pexp_desc) with
- | true, Pexp_construct (constr, [{pexp_desc = Pexp_tuple args}]) ->
- {e with pexp_desc = Pexp_construct (constr, args); pexp_attributes}
- | _ -> {e with pexp_attributes}
- in
let print_arrow e =
let async, parameters, return_expr = Parsetree_viewer.fun_expr e in
let attrs_on_arrow = e.pexp_attributes inReproducing the legacy-PPX row without installing a PPX
A stand-in for a PPX that builds its own constructor node: blank the bridge's markers in the marshalled v0 file, byte for byte, so the layout is untouched and from0 sees an unmarked Pexp_construct(C, Some(Pexp_tuple [...])) - exactly what sury-ppx emits.
cat > legacy_ppx.sh <<'EOF'
#!/bin/sh
perl -pe 's/_res\.constructor_args/_res.xxxxxxxxxxxxxxxx/g; s/_res\.constructor_tuple_arg/_res.yyyyyyyyyyyyyyyyyyyy/g' < "$1" > "$2"
EOF
chmod +x legacy_ppx.sh
printf 'type pair = Pair(int, int)\nlet x = Pair(1, 2)\n' > P1.res
bsc -nopervasives -w -a -ppx ./legacy_ppx.sh P1.res-nopervasives avoids needing a stdlib built against this branch's bumped cmi_magic_number.
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Apply the normalization approach proposed by @cristianoc in PR rescript-lang#8610. Keep source argument lists for printing and resolve semantic grouping after constructor disambiguation, without legacy PPX marker handling in the type checker or printer. Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
cbf4a97 to
a6d5edc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6d5edc346
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
cknitt
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc3063c7aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Christoph Knittel <ck@cca.io>
Signed-off-by: Christoph Knittel <ck@cca.io>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ed6059854
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ~split_tuple: | ||
| (has_constructor_args | ||
| || has_explicit_arity_attr attrs | ||
| || lid.txt = Longident.Lident "::") |
There was a problem hiding this comment.
If an existing v0 PPX returns a canonical :: node whose Pexp_tuple payload carries attributes, this unconditional list split retains only the tuple's children and silently drops pexp_attributes; mapping the result back to v0 then produces an unattributed tuple. This changes the frozen wire shape and can break chained PPXs that communicate through payload attributes, so attributed list payloads need an explicit preservation path (the analogous pattern branch has the same issue).
AGENTS.md reference: AGENTS.md:L37-L39
Useful? React with 👍 / 👎.
Signed-off-by: Christoph Knittel <ck@cca.io>
2 participants
Motivation
The parser previously had two modes:
These produced different trees for printing and type checking. The printer needed to preserve constructor argument grouping and parentheses, while the type-checker representation collapsed those distinctions.
The modes also handled differences in string-literal representation. [#8606](#8606) unified that representation and was the first step toward removing the modes. Constructor argument grouping was the main remaining obstacle.
This PR replaces the two representations with one source-preserving parsetree, while retaining existing source and PPX compatibility.
Source syntax and semantic arity
Constructor expressions and patterns now carry located argument lists:
The same approach applies to polymorphic variant expressions, patterns, and type payload groups.
These lists describe what was written, not the constructor’s declared arity. For example, omitting locations:
The printer can preserve either spelling. Once the type checker resolves an ordinary constructor’s declaration, it normalizes the arguments accordingly: unpacking a tuple for a multi-argument constructor or packing multiple syntactic arguments into a single tuple payload.
Existing spellings such as
Some(x, y)andSome((x, y))therefore remain supported. This does not introduce stricter arity rules or require source migration.Normalization happens within the existing type-checking flow, without an additional AST pass. Ordinary constructors continue into the typedtree as semantic argument lists; polymorphic variants retain their optional single-payload representation. No typedtree or later-layer representation changes are needed.
Argument locations
Argument lists carry their own source span, including the surrounding parentheses. Consumers no longer need to reconstruct that span from individual arguments or encode it in attributes.
This simplifies printing and comment handling and fixes constructor signature help appearing in whitespace or comments between the constructor name and its arguments. Signature help now uses the argument-list boundaries.
Cleanup
The unified representation removes:
ParseForTypeCheckerandDefaultfor_printerparameters and the developer-only-typecheckerparser flagIt also consolidates constructor argument parsing and printing, makes AST helpers accept located argument lists directly, and confines legacy explicit-arity attribute handling to the AST0 compatibility boundary.
PPX compatibility
The frozen
parsetree0representation remains unchanged.The bridge encodes multiple syntactic arguments as a tuple with internal
_res.constructor_argsmetadata. Conversion back consumes that metadata and restores the argument list. A single tuple argument needs no marker.Unmarked tuples generated by existing PPXs remain supported: their semantic grouping is resolved during type checking. Legacy
explicit_arityandocaml.explicit_arityattributes are handled at the bridge boundary.Multi-argument locations survive through the encoded tuple. Where AST0 cannot represent a separate outer argument-list span for a single payload, conversion falls back to the payload’s location.
Testing
Coverage includes constructor and polymorphic variant expressions, patterns, and type payloads; tuple grouping compatibility; arity diagnostics; AST0 round trips and legacy PPX output; argument locations and parser recovery; printing and comment preservation; signature-help boundaries; and end-to-end compilation and runtime behavior.