Remove parser modes by encoding constructor arity explicitly - #8610
Remove parser modes by encoding constructor arity explicitly#8610cknitt wants to merge 28 commits into
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.45% +0.13%
==========================================
Files 467 468 +1
Lines 63313 63570 +257
==========================================
+ Hits 48957 49241 +284
+ Misses 14356 14329 -27
🚀 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".
| let nested = | ||
| match List.rev nested with | ||
| | Completable.NTupleItem {item_num} :: rest -> | ||
| [ | ||
| Completable.NVariantPayload | ||
| {constructor_name = Longident.last constructor_lid.txt; item_num}; | ||
| ] | ||
| @ rest | ||
| | nested -> | ||
| [ | ||
| Completable.NVariantPayload | ||
| { | ||
| constructor_name = Longident.last constructor_lid.txt; | ||
| item_num = 0; | ||
| }; | ||
| ] | ||
| @ nested | ||
| Completable.NVariantPayload | ||
| {constructor_name = Longident.last constructor_lid.txt; item_num} | ||
| :: List.rev nested |
There was a problem hiding this comment.
For a multi-argument constructor written with the supported tuple spelling, such as type t = Pair(int, {name: string}); let _ = Pair((1, {na|})), args contains one tuple expression, so this always prefixes NVariantPayload(item_num=0) and retains the nested NTupleItem(item_num=1). The type checker normalizes that tuple to the two constructor arguments, meaning completion instead tries to descend into tuple item 1 of the first int argument and loses record-field completion; it should resolve the constructor arity and map the tuple elements to constructor payload indices. The analogous constructor-pattern traversal has the same mismatch.
Useful? React with 👍 / 👎.
| | Some i -> i) | ||
| | `ConstructorPat (_, {ppat_desc = Ppat_record (fields, _, _rest)}) | ||
| -> ( | ||
| | `ConstructorExpr (_, [_]) -> 0 |
There was a problem hiding this comment.
When a binary constructor uses the still-supported parenthesized-tuple spelling, for example Pair((1, 2|)), the syntax-preserving AST has one top-level item even though type checking normalizes it to two constructor arguments. This branch therefore reports parameter 0 regardless of which tuple element contains the cursor, whereas the previous tuple-payload handling selected parameter 1 here; use the resolved TupleArg arity to inspect the tuple's child locations. The corresponding single-item pattern branch below has the same regression.
Useful? React with 👍 / 👎.
2 participants
Motivation
The ReScript parser previously supported two modes:
These modes allowed the parser to produce different trees depending on whether its output was intended for the type checker or the printer.
In particular, the type-checker representation collapsed the distinction between multiple constructor arguments and a tuple passed as a single constructor argument. The printer needed a different, source-preserving representation so it could retain the extra parentheses.
The parser mode was also used for differences in string-literal representation. #8606 normalized string literals across the compiler and was the first step toward removing the mode entirely. Constructor arity was the main remaining reason for it.
This change
Constructor arity is now represented explicitly in the parsetree:
The same approach is used for polymorphic variant expressions, patterns, and type payload groups.
For example:
Their applications have unambiguous parsetree representations:
The type checker can therefore compare the number of syntactic arguments directly with the constructor's declared arity.
This also means that a constructor with one tuple payload must use the corresponding nested parentheses:
Writing
Some(x, y)now correctly reports an arity mismatch.Alignment with subsequent compiler layers
The new representation is more closely aligned with subsequent compiler layers. Nominal constructors are already represented as argument lists in the typedtree, so the type checker no longer needs to infer their arity by inspecting tuple nodes.
Polymorphic variants continue to be normalized to their semantic single-payload representation when entering the type checker. Their source-level arity remains explicit in the parsetree for printing and tooling.
There is therefore no need to change the typedtree or later compiler representations.
Cleanup enabled by the new representation
With both source shape and constructor arity available in one parsetree, this PR removes:
ParseForTypeCheckerandDefaultfrom the parserfor_printerargument throughout parser APIs and their callers-typecheckerparser flagIt also:
ocaml.explicit_arityhandling to the AST0 compatibility boundaryPPX compatibility
The frozen
parsetree0representation remains unchanged.When converting multiple constructor arguments to AST0, the bridge encodes them as a tuple and attaches internal
_res.constructor_argsmetadata. Conversion back uses that metadata to restore the argument list and removes the internal attribute before returning to the current parsetree.This preserves the distinction between multiple arguments and a single tuple argument across PPX round trips without requiring changes to the frozen PPX-facing AST.
Breaking change
Code that relied on the previous ambiguity must add explicit tuple parentheses.
For example:
must become:
when the constructor has one tuple payload.
Testing
The change includes coverage for: