From 4f4d9d79db6151dc995fee6c62c8eaf73c2fbba9 Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 17 Oct 2026 07:57:18 +0000 Subject: [PATCH 1/2] Represent JSX component annotations with a parsetree node `@react.component`, `@jsx.component` and their `componentWithProps` forms on a `let`, an `external` or a signature item become `pvb_jsx_component` / `pval_jsx_component`: the kind (labelled arguments, optionally sharing a props type given as `(: t)`, or a props record), the spelling, and the attribute's location. `Ast_helper.Vb.mk` and `Val.mk` take the annotation out of the attributes, as `Type.field` does for `@as`, and `Vb.binding_attributes` / `Val.description_attributes` put it back where it was written for the printers, the mappers and iterators, the v0 PPX bridge and the typed tree. The JSX transform reads the node instead of scanning attribute names. An annotation whose payload the transform does not understand, such as `@react.component("x")`, stays an attribute and is now reported; it used to be accepted with the payload ignored. Example (rescript): module C = { @react.component("Button") let make = (~label) => React.string(label) } Diagnostic (text): @react.component takes no payload, or the props type to share, as in @react.component(: props). Review: Why a field on the binding, not a structure item of its own? The annotation is written on a `let`, an `external` or a signature `let`, and the JSX transform expands it in place. A field keeps that shape, and bindings the transform leaves alone (local `let`s, `componentWithProps` on a signature item, everything when JSX is off) keep their meaning without special cases. Review: Why does the typed tree get the attribute back? Nothing downstream reads the node, but `.cmi` attributes and tools that read typed attributes saw `@react.component` wherever the JSX transform did not consume it. Restoring the written attributes keeps that, and with it diagnostics that print those declarations. Review: Where does the attribute go on the v0 wire? At its source position among the other attributes. PPX output carries no positions, so everything ties; the annotation then goes first, where it is usually written. A PPX that writes it after another attribute sees the order swapped after a round trip; `@as` has the same limitation. Review: Do duplicates behave as before? Yes. The first `@react.component`, or failing that the first `componentWithProps`, becomes the node, the precedence the transform always applied. The rest stay attributes: bindings drop them as before, externals and signature items count them for the existing "Only one JSX component call" and `componentWithProps` external errors. Area: syntax printer frontend typechecker analysis tests Output: unchanged, the test suites' generated JS is byte-identical; 232 generated programs compile to identical JS without JSX, and with JSX 4 except the 45 with an unsupported payload, which now fail with the error above Formatting: unchanged, base and branch res_parser print 232 generated and comment-heavy files identically, plain and through the v0 AST Formats: ast cmi cmt PPX: unchanged wire: PPXs see the attributes at their source positions; ast-mapping fixture JsxComponents.res Editor: unchanged: iterators see the annotation as an attribute, and Utils.is_jsx_component reads the node Errors: none, because the new messages are inline `raise_errorf` calls, which the catalog's scope leaves out Tests: +tests/syntax_tests/data/ast-mapping/JsxComponents.res +tests/build_tests/super_errors/fixtures/react_component_invalid_payload.res +tests/build_tests/super_errors/fixtures/react_componentWithProps_invalid_payload.res ~tests/ounit_tests/ounit_ast_mapper0_tests.ml Measured: differential harness, base vs branch bsc -bs-jsx 4: 187 of 232 programs identical, 45 differ by the new payload error; without JSX: 232 of 232 identical PR: #8884 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014hcyfimX2ss8qj5MZwdMdP Signed-off-by: Christoph Knittel --- analysis/src/dump_ast.ml | 2 +- analysis/src/utils.ml | 7 +- changes/jsx-component-node.internal.md | 1 + changes/jsx-component-payload-error.polish.md | 1 + compiler/ext/config.ml | 8 +- compiler/frontend/ast_external_mk.ml | 1 + .../frontend/ast_tuple_pattern_flatten.ml | 30 ++- compiler/frontend/bs_builtin_ppx.ml | 11 ++ compiler/ml/ast_helper.ml | 86 +++++++++ compiler/ml/ast_helper.mli | 21 +++ compiler/ml/ast_iterator.ml | 12 +- compiler/ml/ast_mapper.ml | 13 +- compiler/ml/ast_mapper_to0.ml | 13 +- compiler/ml/parsetree.ml | 28 +++ compiler/ml/pprintast.ml | 9 +- compiler/ml/printast.ml | 4 +- compiler/ml/typecore.ml | 3 +- compiler/ml/typedecl.ml | 10 +- compiler/syntax/src/jsx_common.ml | 36 ++-- compiler/syntax/src/jsx_v4.ml | 64 +++++-- compiler/syntax/src/res_printer.ml | 5 +- ...nentWithProps_invalid_payload.res.expected | 11 ++ ...act_component_invalid_payload.res.expected | 10 + ...act_componentWithProps_invalid_payload.res | 6 + .../react_component_invalid_payload.res | 4 + tests/ounit_tests/ounit_ast_mapper0_tests.ml | 171 ++++++++++++++++++ .../data/ast-mapping/JsxComponents.res | 42 +++++ .../expected/JsxComponents.res.txt | 88 +++++++++ 28 files changed, 625 insertions(+), 72 deletions(-) create mode 100644 changes/jsx-component-node.internal.md create mode 100644 changes/jsx-component-payload-error.polish.md create mode 100644 tests/build_tests/super_errors/expected/react_componentWithProps_invalid_payload.res.expected create mode 100644 tests/build_tests/super_errors/expected/react_component_invalid_payload.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/react_componentWithProps_invalid_payload.res create mode 100644 tests/build_tests/super_errors/fixtures/react_component_invalid_payload.res create mode 100644 tests/syntax_tests/data/ast-mapping/JsxComponents.res create mode 100644 tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt diff --git a/analysis/src/dump_ast.ml b/analysis/src/dump_ast.ml index 53437c830e3..ae837885965 100644 --- a/analysis/src/dump_ast.ml +++ b/analysis/src/dump_ast.ml @@ -367,7 +367,7 @@ let print_value_binding value ~pos ~indentation = ^ ". " ^ print_core_type pvc_type ~pos in - print_attributes value.Parsetree.pvb_attributes + print_attributes (Ast_helper.Vb.binding_attributes value) ^ "value" ^ ":\n" ^ add_indentation (indentation + 1) ^ (value.pvb_pat |> print_pattern ~pos ~indentation:(indentation + 1)) diff --git a/analysis/src/utils.ml b/analysis/src/utils.ml index 68734918570..42f460c92b2 100644 --- a/analysis/src/utils.ml +++ b/analysis/src/utils.ml @@ -161,10 +161,9 @@ let rec unwrap_if_option (t : Types.type_expr) = | _ -> t let is_jsx_component (vb : Parsetree.value_binding) = - vb.pvb_attributes - |> List.exists (function - | {Location.txt = "react.component" | "jsx.component"}, _payload -> true - | _ -> false) + match vb.pvb_jsx_component with + | Some {pjc_kind = Pjc_labelled _} -> true + | Some {pjc_kind = Pjc_with_props} | None -> false let check_name name ~prefix ~exact = if exact then name = prefix else starts_with name prefix diff --git a/changes/jsx-component-node.internal.md b/changes/jsx-component-node.internal.md new file mode 100644 index 00000000000..7673ae521ef --- /dev/null +++ b/changes/jsx-component-node.internal.md @@ -0,0 +1 @@ +- Represent `@react.component`, `@jsx.component` and their `componentWithProps` forms with a dedicated parsetree node. https://github.com/rescript-lang/rescript/pull/8884 diff --git a/changes/jsx-component-payload-error.polish.md b/changes/jsx-component-payload-error.polish.md new file mode 100644 index 00000000000..e48f5d757c4 --- /dev/null +++ b/changes/jsx-component-payload-error.polish.md @@ -0,0 +1 @@ +- Report a JSX component annotation with an unsupported payload, such as `@react.component("x")`, instead of ignoring the payload. https://github.com/rescript-lang/rescript/pull/8884 diff --git a/compiler/ext/config.ml b/compiler/ext/config.ml index 863cd6353e4..8b9658a611b 100644 --- a/compiler/ext/config.ml +++ b/compiler/ext/config.ml @@ -1,10 +1,10 @@ -let cmi_magic_number = "Caml1999I037" +let cmi_magic_number = "Caml1999I038" (* Magic numbers for marshaled values of the *current* parsetree, whose layout changes across compiler versions. *) -and ast_impl_magic_number = "ResImpl01316" +and ast_impl_magic_number = "ResImpl01317" -and ast_intf_magic_number = "ResIntf01316" +and ast_intf_magic_number = "ResIntf01317" (* Magic numbers of the frozen Parsetree0 (OCaml 4.06) layout used on the external-PPX wire. They must never be written in front of a @@ -13,7 +13,7 @@ and ast0_impl_magic_number = "Caml1999M022" and ast0_intf_magic_number = "Caml1999N022" -and cmt_magic_number = "Caml1999T044" +and cmt_magic_number = "Caml1999T045" let get_load_path () = (Compiler_request_state.current ()).load_path let set_load_path path = (Compiler_request_state.current ()).load_path <- path diff --git a/compiler/frontend/ast_external_mk.ml b/compiler/frontend/ast_external_mk.ml index 6614ae86986..8cb76a4c09a 100644 --- a/compiler/frontend/ast_external_mk.ml +++ b/compiler/frontend/ast_external_mk.ml @@ -36,6 +36,7 @@ let local_external_apply loc ?(pval_attributes = []) pval_type; pval_loc = loc; pval_prim = Some pval_prim; + pval_jsx_component = None; pval_attributes; }; ], diff --git a/compiler/frontend/ast_tuple_pattern_flatten.ml b/compiler/frontend/ast_tuple_pattern_flatten.ml index 88e2c6d20f4..40f814c423c 100644 --- a/compiler/frontend/ast_tuple_pattern_flatten.ml +++ b/compiler/frontend/ast_tuple_pattern_flatten.ml @@ -60,10 +60,18 @@ let flattern_tuple_pattern_vb (self : Ast_mapper.mapper) in let pvb_expr = self.expr self vb.pvb_expr in let pvb_attributes = self.attributes self vb.pvb_attributes in + let pvb_jsx_component = vb.pvb_jsx_component in let pvb_expr_desc = (Ast_payload.unwrap_braces pvb_expr).pexp_desc in match (pvb_constraint, pvb_pat.ppat_desc, pvb_expr_desc) with | Some _, _, _ -> - {pvb_pat; pvb_expr; pvb_constraint; pvb_loc = vb.pvb_loc; pvb_attributes} + { + pvb_pat; + pvb_expr; + pvb_constraint; + pvb_jsx_component; + pvb_loc = vb.pvb_loc; + pvb_attributes; + } :: acc | None, Ppat_tuple xs, _ when List.for_all is_simple_pattern xs -> ( match Ast_open_cxt.destruct_open_tuple pvb_expr [] with @@ -77,12 +85,20 @@ let flattern_tuple_pattern_vb (self : Ast_mapper.mapper) pvb_pat = pat; pvb_expr = Ast_open_cxt.restore_exp exp wholes; pvb_constraint = None; + pvb_jsx_component; pvb_attributes; pvb_loc = vb.pvb_loc; } :: acc) | _ -> - {pvb_pat; pvb_expr; pvb_constraint; pvb_loc = vb.pvb_loc; pvb_attributes} + { + pvb_pat; + pvb_expr; + pvb_constraint; + pvb_jsx_component; + pvb_loc = vb.pvb_loc; + pvb_attributes; + } :: acc) | None, Ppat_record (_, _, Some rest), Pexp_pack {pmod_desc = Pmod_ident _} -> Location.raise_errorf ~loc:rest.rest_loc @@ -99,6 +115,7 @@ let flattern_tuple_pattern_vb (self : Ast_mapper.mapper) Ast_helper.Exp.ident ~loc:lid.loc {lid with txt = Ldot (id.txt, s)}; pvb_constraint = None; + pvb_jsx_component = None; pvb_attributes = []; pvb_loc = pat.ppat_loc; } @@ -106,7 +123,14 @@ let flattern_tuple_pattern_vb (self : Ast_mapper.mapper) Location.raise_errorf ~loc:lid.loc "Not supported pattern match on modules") | _ -> - {pvb_pat; pvb_expr; pvb_constraint; pvb_loc = vb.pvb_loc; pvb_attributes} + { + pvb_pat; + pvb_expr; + pvb_constraint; + pvb_jsx_component; + pvb_loc = vb.pvb_loc; + pvb_attributes; + } :: acc let value_bindings_mapper (self : Ast_mapper.mapper) diff --git a/compiler/frontend/bs_builtin_ppx.ml b/compiler/frontend/bs_builtin_ppx.ml index de147ec53cf..b35556e7cca 100644 --- a/compiler/frontend/bs_builtin_ppx.ml +++ b/compiler/frontend/bs_builtin_ppx.ml @@ -493,6 +493,7 @@ let signature_item_mapper (self : mapper) (sigi : Parsetree.signature_item) : { value_desc with pval_prim = Some (Ast_external_mk.inline_string semantic); + pval_jsx_component = None; pval_attributes = []; }; } @@ -506,6 +507,7 @@ let signature_item_mapper (self : mapper) (sigi : Parsetree.signature_item) : { value_desc with pval_prim = Some (Ast_external_mk.inline_int s); + pval_jsx_component = None; pval_attributes = []; }; } @@ -518,6 +520,7 @@ let signature_item_mapper (self : mapper) (sigi : Parsetree.signature_item) : { value_desc with pval_prim = Some (Ast_external_mk.inline_bigint s); + pval_jsx_component = None; pval_attributes = []; }; } @@ -530,6 +533,7 @@ let signature_item_mapper (self : mapper) (sigi : Parsetree.signature_item) : { value_desc with pval_prim = Some (Ast_external_mk.inline_float s); + pval_jsx_component = None; pval_attributes = []; }; } @@ -543,6 +547,7 @@ let signature_item_mapper (self : mapper) (sigi : Parsetree.signature_item) : { value_desc with pval_prim = Some (Ast_external_mk.inline_bool (txt = "true")); + pval_jsx_component = None; pval_attributes = []; }; } @@ -586,6 +591,7 @@ let structure_item_mapper (self : mapper) (str : Parsetree.structure_item) : pvb_pat = {ppat_desc = Ppat_var pval_name} as pvb_pat; pvb_expr; pvb_constraint = None; + pvb_jsx_component; pvb_attributes; pvb_loc; }; @@ -615,6 +621,7 @@ let structure_item_mapper (self : mapper) (str : Parsetree.structure_item) : pval_name; pval_type = Ast_literal.type_string (); pval_loc = pvb_loc; + pval_jsx_component = None; pval_attributes = []; pval_prim = Some (Ast_external_mk.inline_string semantic); }; @@ -630,6 +637,7 @@ let structure_item_mapper (self : mapper) (str : Parsetree.structure_item) : pval_name; pval_type = Ast_literal.type_int (); pval_loc = pvb_loc; + pval_jsx_component = None; pval_attributes = []; pval_prim = Some (Ast_external_mk.inline_int s); }; @@ -644,6 +652,7 @@ let structure_item_mapper (self : mapper) (str : Parsetree.structure_item) : pval_name; pval_type = Ast_literal.type_float; pval_loc = pvb_loc; + pval_jsx_component = None; pval_attributes = []; pval_prim = Some (Ast_external_mk.inline_float s); }; @@ -660,6 +669,7 @@ let structure_item_mapper (self : mapper) (str : Parsetree.structure_item) : pval_name; pval_type = Ast_literal.type_bool (); pval_loc = pvb_loc; + pval_jsx_component = None; pval_attributes = []; pval_prim = Some (Ast_external_mk.inline_bool (txt = "true")); }; @@ -675,6 +685,7 @@ let structure_item_mapper (self : mapper) (str : Parsetree.structure_item) : pvb_pat; pvb_expr; pvb_constraint = None; + pvb_jsx_component; pvb_attributes; pvb_loc; }; diff --git a/compiler/ml/ast_helper.ml b/compiler/ml/ast_helper.ml index 8931123e97b..1ee19e0cb9a 100644 --- a/compiler/ml/ast_helper.ml +++ b/compiler/ml/ast_helper.ml @@ -321,15 +321,96 @@ module Str = struct let attribute ?loc a = mk ?loc (Pstr_attribute a) end +(* [@react.component] and its variants are taken out of the attributes when a + value binding or value description is built, so every producer - the + parser, a ppx, the frozen AST bridge - agrees on what is a component. *) +module Jsx_component = struct + let of_attribute (({txt; loc}, payload) : attribute) = + let component pjc_kind pjc_namespace = + Some {pjc_kind; pjc_namespace; pjc_loc = loc} + in + match (txt, payload) with + | "react.component", PStr [] -> component (Pjc_labelled None) Pjc_react + | "react.component", PTyp typ -> + component (Pjc_labelled (Some typ)) Pjc_react + | "jsx.component", PStr [] -> component (Pjc_labelled None) Pjc_jsx + | "jsx.component", PTyp typ -> component (Pjc_labelled (Some typ)) Pjc_jsx + | "react.componentWithProps", PStr [] -> component Pjc_with_props Pjc_react + | "jsx.componentWithProps", PStr [] -> component Pjc_with_props Pjc_jsx + | _ -> None + + let to_attribute {pjc_kind; pjc_namespace; pjc_loc} : attribute = + let namespace = + match pjc_namespace with + | Pjc_react -> "react" + | Pjc_jsx -> "jsx" + in + match pjc_kind with + | Pjc_labelled None -> + ({txt = namespace ^ ".component"; loc = pjc_loc}, PStr []) + | Pjc_labelled (Some typ) -> + ({txt = namespace ^ ".component"; loc = pjc_loc}, PTyp typ) + | Pjc_with_props -> + ({txt = namespace ^ ".componentWithProps"; loc = pjc_loc}, PStr []) + + (* The first [@react.component], or failing that the first + [@react.componentWithProps]: the JSX transform has always given them + this precedence. A later one, or one whose payload is not understood, is + left in the attributes for the JSX transform to deal with, so that + formatting does not drop it. *) + let take attrs = + let take_first is_kind = + let rec go seen = function + | [] -> (None, attrs) + | attr :: rest -> ( + match of_attribute attr with + | Some component when is_kind component.pjc_kind -> + (Some component, List.rev_append seen rest) + | _ -> go (attr :: seen) rest) + in + go [] attrs + in + match + take_first (function + | Pjc_labelled _ -> true + | Pjc_with_props -> false) + with + | (Some _, _) as found -> found + | None, _ -> + take_first (function + | Pjc_with_props -> true + | Pjc_labelled _ -> false) + + (* Inverse of [take], for printers and the frozen AST bridge. Attributes + print in source order, so the annotation goes back where it was written. + Ppx output has no source position, so all of it ties on [Location.none]; + the annotation then goes first among the tied, where it is usually + written. *) + let put_back component attrs = + match component with + | None -> attrs + | Some component -> + let written_earlier (({loc}, _) : attribute) = + loc.loc_start.pos_cnum < component.pjc_loc.loc_start.pos_cnum + in + let earlier, later = List.partition written_earlier attrs in + earlier @ (to_attribute component :: later) +end + module Val = struct let mk ?(loc = !default_loc) ?(attrs = []) ?prim name typ = + let jsx_component, attrs = Jsx_component.take attrs in { pval_name = name; pval_type = typ; + pval_jsx_component = jsx_component; pval_attributes = attrs; pval_loc = loc; pval_prim = prim; } + + let description_attributes vd = + Jsx_component.put_back vd.pval_jsx_component vd.pval_attributes end module Md = struct @@ -364,13 +445,18 @@ end module Vb = struct let mk ?(loc = !default_loc) ?(attrs = []) ?constraint_ pat expr = + let jsx_component, attrs = Jsx_component.take attrs in { pvb_pat = pat; pvb_expr = expr; pvb_constraint = constraint_; + pvb_jsx_component = jsx_component; pvb_attributes = attrs; pvb_loc = loc; } + + let binding_attributes vb = + Jsx_component.put_back vb.pvb_jsx_component vb.pvb_attributes end module Type = struct diff --git a/compiler/ml/ast_helper.mli b/compiler/ml/ast_helper.mli index 19d502945e1..6cadf757a66 100644 --- a/compiler/ml/ast_helper.mli +++ b/compiler/ml/ast_helper.mli @@ -276,6 +276,15 @@ module Exp : sig Location.t -> expression list -> expression option -> expression end +(** JSX component annotations *) +module Jsx_component : sig + val of_attribute : attribute -> jsx_component option + (** The component an attribute such as [@react.component] denotes, if its + payload is one the JSX transform understands. *) + + val to_attribute : jsx_component -> attribute +end + (** Value declarations *) module Val : sig val mk : @@ -285,6 +294,12 @@ module Val : sig str -> core_type -> value_description + (** A [@react.component] annotation in [attrs] is taken out and becomes + [pval_jsx_component]. *) + + val description_attributes : value_description -> attrs + (** The attributes as written, with the [@react.component] annotation put + back. For printers, which must reproduce the source. *) end (** Type declarations *) @@ -486,4 +501,10 @@ module Vb : sig pattern -> expression -> value_binding + (** A [@react.component] annotation in [attrs] is taken out and becomes + [pvb_jsx_component]. *) + + val binding_attributes : value_binding -> attrs + (** The attributes as written, with the [@react.component] annotation put + back. For printers, which must reproduce the source. *) end diff --git a/compiler/ml/ast_iterator.ml b/compiler/ml/ast_iterator.ml index a8dfb9d1991..965c5d5bd77 100644 --- a/compiler/ml/ast_iterator.ml +++ b/compiler/ml/ast_iterator.ml @@ -513,12 +513,12 @@ let default_iterator = type_extension = T.iter_type_extension; extension_constructor = T.iter_extension_constructor; value_description = - (fun this - {pval_name; pval_type; pval_prim = _; pval_loc; pval_attributes} - -> + (fun this ({pval_name; pval_type; pval_prim = _; pval_loc} as vd) -> iter_loc this pval_name; this.typ this pval_type; - this.attributes this pval_attributes; + (* An iterator sees the component annotation as the attribute it was + written as. *) + this.attributes this (Ast_helper.Val.description_attributes vd); this.location this pval_loc); pat = P.iter; expr = E.iter; @@ -556,7 +556,7 @@ let default_iterator = this.location this pincl_loc; this.attributes this pincl_attributes); value_binding = - (fun this {pvb_pat; pvb_expr; pvb_constraint; pvb_attributes; pvb_loc} -> + (fun this ({pvb_pat; pvb_expr; pvb_constraint; pvb_loc} as vb) -> this.pat this pvb_pat; Option.iter (fun {pvc_newtypes; pvc_type} -> @@ -565,7 +565,7 @@ let default_iterator = pvb_constraint; this.expr this pvb_expr; this.location this pvb_loc; - this.attributes this pvb_attributes); + this.attributes this (Ast_helper.Vb.binding_attributes vb)); constructor_declaration = (fun this {pcd_name; pcd_args; pcd_res; pcd_loc; pcd_attributes} -> iter_loc this pcd_name; diff --git a/compiler/ml/ast_mapper.ml b/compiler/ml/ast_mapper.ml index ad378652250..c689750dc08 100644 --- a/compiler/ml/ast_mapper.ml +++ b/compiler/ml/ast_mapper.ml @@ -524,9 +524,12 @@ let default_mapper = type_extension = T.map_type_extension; extension_constructor = T.map_extension_constructor; value_description = - (fun this {pval_name; pval_type; pval_prim; pval_loc; pval_attributes} -> + (fun this ({pval_name; pval_type; pval_prim; pval_loc} as vd) -> + (* The component annotation goes back into the attributes so that a + mapper sees [@react.component] where it has always been, and + [Val.mk] takes it out again. *) Val.mk (map_loc this pval_name) (this.typ this pval_type) - ~attrs:(this.attributes this pval_attributes) + ~attrs:(this.attributes this (Val.description_attributes vd)) ~loc:(this.location this pval_loc) ?prim:pval_prim); pat = P.map; @@ -567,7 +570,7 @@ let default_mapper = ~loc:(this.location this pincl_loc) ~attrs:(this.attributes this pincl_attributes)); value_binding = - (fun this {pvb_pat; pvb_expr; pvb_constraint; pvb_attributes; pvb_loc} -> + (fun this ({pvb_pat; pvb_expr; pvb_constraint; pvb_loc} as vb) -> let pvb_pat = this.pat this pvb_pat in let constraint_ = Option.map @@ -579,9 +582,11 @@ let default_mapper = pvb_constraint in let pvb_expr = this.expr this pvb_expr in + (* As for value descriptions, the component annotation goes back into + the attributes and [Vb.mk] takes it out again. *) Vb.mk pvb_pat pvb_expr ?constraint_ ~loc:(this.location this pvb_loc) - ~attrs:(this.attributes this pvb_attributes)); + ~attrs:(this.attributes this (Vb.binding_attributes vb))); constructor_declaration = (fun this ({pcd_name; pcd_args; pcd_res; pcd_loc} as cd) -> Type.constructor (map_loc this pcd_name) diff --git a/compiler/ml/ast_mapper_to0.ml b/compiler/ml/ast_mapper_to0.ml index e5e8112580a..fa4171b42c1 100644 --- a/compiler/ml/ast_mapper_to0.ml +++ b/compiler/ml/ast_mapper_to0.ml @@ -988,7 +988,7 @@ let default_mapper = type_extension = T.map_type_extension; extension_constructor = T.map_extension_constructor; value_description = - (fun this {pval_name; pval_type; pval_prim; pval_loc; pval_attributes} -> + (fun this ({pval_name; pval_type; pval_prim; pval_loc} as vd) -> let prim = match pval_prim with | None -> [] @@ -1001,8 +1001,12 @@ let default_mapper = "External declarations already processed for compilation cannot \ be converted for an external PPX" in + (* v0 has no component node: PPXs see the [@react.component] + attribute, and [Ast_helper.Val.mk] takes it out again on the way + back. *) Val.mk (map_loc this pval_name) (this.typ this pval_type) - ~attrs:(this.attributes this pval_attributes) + ~attrs: + (this.attributes this (Ast_helper.Val.description_attributes vd)) ~loc:(this.location this pval_loc) ~prim); pat = P.map; @@ -1043,7 +1047,7 @@ let default_mapper = ~loc:(this.location this pincl_loc) ~attrs:(this.attributes this pincl_attributes)); value_binding = - (fun this {pvb_pat; pvb_expr; pvb_constraint; pvb_attributes; pvb_loc} -> + (fun this ({pvb_pat; pvb_expr; pvb_constraint; pvb_loc} as vb) -> let loc = this.location this pvb_loc in let pvb_pat, pvb_expr = match pvb_constraint with @@ -1069,7 +1073,8 @@ let default_mapper = in (pat, expr) in - Vb.mk pvb_pat pvb_expr ~loc ~attrs:(this.attributes this pvb_attributes)); + Vb.mk pvb_pat pvb_expr ~loc + ~attrs:(this.attributes this (Ast_helper.Vb.binding_attributes vb))); constructor_declaration = (fun this ({pcd_name; pcd_args; pcd_res; pcd_loc} as cd) -> Type.constructor (map_loc this pcd_name) diff --git a/compiler/ml/parsetree.ml b/compiler/ml/parsetree.ml index bd865de669e..08cf688c296 100644 --- a/compiler/ml/parsetree.ml +++ b/compiler/ml/parsetree.ml @@ -523,10 +523,35 @@ and value_description = { pval_name: string loc; pval_type: core_type; pval_prim: primitive_repr option; + pval_jsx_component: jsx_component option; + (* The [@react.component] annotation, taken out of the attributes when + the description is built. *) pval_attributes: attributes; (* ... [@@id1] [@@id2] *) pval_loc: Location.t; } +(* A JSX component definition: [@react.component], [@jsx.component] or their + [componentWithProps] forms on a [let], an [external] or a signature item. + The attribute is the surface syntax; this is what it means. The JSX + transform reads it, and the printer puts the attribute back where it was + written. *) +and jsx_component = { + pjc_kind: jsx_component_kind; + pjc_namespace: jsx_component_namespace; + pjc_loc: Location.t; (* the attribute's own location *) +} + +and jsx_component_kind = + | Pjc_labelled of core_type option + (* [@react.component]: the props are the labelled arguments. + [@react.component(: t)] makes them share the props type [t]. *) + | Pjc_with_props +(* [@react.componentWithProps]: the function takes a props record *) + +(* How the attribute was spelled. Both mean the same; the configured JSX + module decides the runtime. *) +and jsx_component_namespace = Pjc_react | Pjc_jsx + (* val x: T (prim = None) external x: T = "s" (prim = Some _) @@ -821,6 +846,9 @@ and value_binding = { pvb_pat: pattern; pvb_expr: expression; pvb_constraint: value_constraint option; + pvb_jsx_component: jsx_component option; + (* The [@react.component] annotation, taken out of the attributes when + the binding is built. *) pvb_attributes: attributes; pvb_loc: Location.t; } diff --git a/compiler/ml/pprintast.ml b/compiler/ml/pprintast.ml index 00bb5711e1c..21f027e475c 100644 --- a/compiler/ml/pprintast.ml +++ b/compiler/ml/pprintast.ml @@ -1052,7 +1052,8 @@ and signature_item ctxt f x : unit = | Psig_value vd -> let intro = if vd.pval_prim = None then "val" else "external" in pp f "@[<2>%s@ %a@ :@ %a@]%a" intro protect_ident vd.pval_name.txt - (value_description ctxt) vd (item_attributes ctxt) vd.pval_attributes + (value_description ctxt) vd (item_attributes ctxt) + (Ast_helper.Val.description_attributes vd) | Psig_typext te -> type_extension ctxt f te | Psig_exception ed -> exception_declaration ctxt f ed | Psig_module @@ -1239,7 +1240,8 @@ and binding ctxt f {pvb_pat = p; pvb_expr = x; pvb_constraint; _} = and bindings ctxt f (rf, l) = let binding kwd rf f x = pp f "@[<2>%s %a%a@]%a" kwd rec_flag rf (binding ctxt) x - (item_attributes ctxt) x.pvb_attributes + (item_attributes ctxt) + (Ast_helper.Vb.binding_attributes x) in match l with | [] -> () @@ -1296,7 +1298,8 @@ and structure_item ctxt f x = md (item_attributes ctxt) attrs | Pstr_primitive vd -> pp f "@[external@ %a@ :@ %a@]%a" protect_ident vd.pval_name.txt - (value_description ctxt) vd (item_attributes ctxt) vd.pval_attributes + (value_description ctxt) vd (item_attributes ctxt) + (Ast_helper.Val.description_attributes vd) | Pstr_include incl -> pp f "@[include@ %a@]%a" (module_expr ctxt) incl.pincl_mod (item_attributes ctxt) incl.pincl_attributes diff --git a/compiler/ml/printast.ml b/compiler/ml/printast.ml index 574b3c7bb39..87c89d269c4 100644 --- a/compiler/ml/printast.ml +++ b/compiler/ml/printast.ml @@ -493,7 +493,7 @@ and jsx_props i ppf xs = and value_description i ppf x = line i ppf "value_description %a %a\n" fmt_string_loc x.pval_name fmt_location x.pval_loc; - attributes i ppf x.pval_attributes; + attributes i ppf (Ast_helper.Val.description_attributes x); core_type (i + 1) ppf x.pval_type; match x.pval_prim with | None -> () @@ -809,7 +809,7 @@ and case i ppf {pc_bar; pc_lhs; pc_guard; pc_rhs} = and value_binding i ppf x = line i ppf "\n"; - attributes (i + 1) ppf x.pvb_attributes; + attributes (i + 1) ppf (Ast_helper.Vb.binding_attributes x); pattern (i + 1) ppf x.pvb_pat; (match x.pvb_constraint with | None -> () diff --git a/compiler/ml/typecore.ml b/compiler/ml/typecore.ml index 38f4e491c1c..ad522c84116 100644 --- a/compiler/ml/typecore.ml +++ b/compiler/ml/typecore.ml @@ -5026,7 +5026,8 @@ and type_let ~context ?(check = fun s -> Warnings.Unused_var s) { vb_pat = p; vb_expr = e; - vb_attributes = pvb.pvb_attributes; + (* As written, like a value description's in [Typedecl]. *) + vb_attributes = Ast_helper.Vb.binding_attributes pvb; vb_loc = pvb.pvb_loc; }) l spat_sexp_list diff --git a/compiler/ml/typedecl.ml b/compiler/ml/typedecl.ml index 230147cf6ab..2ad0b295f35 100644 --- a/compiler/ml/typedecl.ml +++ b/compiler/ml/typedecl.ml @@ -1892,6 +1892,10 @@ let parse_arity env _core_type ty = let transl_value_decl env loc valdecl = let cty = Typetexp.transl_type_scheme env valdecl.pval_type in let ty = cty.ctyp_type in + (* The typed tree keeps the attributes as written: only the JSX transform + reads the component annotation, and a declaration it leaves alone keeps + the attribute, as it always has. *) + let attributes = Ast_helper.Val.description_attributes valdecl in let v = match valdecl.pval_prim with | None when Env.is_in_signature env -> @@ -1899,7 +1903,7 @@ let transl_value_decl env loc valdecl = val_type = ty; val_kind = Val_reg; Types.val_loc = loc; - val_attributes = valdecl.pval_attributes; + val_attributes = attributes; } | None -> (* unreachable: `pval_prim = None` outside a signature can only arise @@ -1921,7 +1925,7 @@ let transl_value_decl env loc valdecl = val_type = ty; val_kind = Val_prim prim; Types.val_loc = loc; - val_attributes = valdecl.pval_attributes; + val_attributes = attributes; } in let id, newenv = @@ -1936,7 +1940,7 @@ let transl_value_decl env loc valdecl = val_val = v; val_prim = valdecl.pval_prim; val_loc = valdecl.pval_loc; - val_attributes = valdecl.pval_attributes; + val_attributes = attributes; } in (desc, newenv) diff --git a/compiler/syntax/src/jsx_common.ml b/compiler/syntax/src/jsx_common.ml index a48cf11bc97..9ac51f84708 100644 --- a/compiler/syntax/src/jsx_common.ml +++ b/compiler/syntax/src/jsx_common.ml @@ -8,27 +8,33 @@ type jsx_config = { mutable has_component: bool; } -(* Helper method to look up the [@react.component] attribute *) -let has_attr (loc, _) = - match loc.txt with +(* [@react.component] and [@react.componentWithProps] that are still among the + attributes: [Ast_helper] takes the first one into the binding's or + description's component node and leaves any other. *) +let is_component_attr (({txt}, _) : attribute) = + match txt with | "react.component" | "jsx.component" -> true | _ -> false -let has_attr_with_props (loc, _) = - match loc.txt with +let is_component_with_props_attr (({txt}, _) : attribute) = + match txt with | "react.componentWithProps" | "jsx.componentWithProps" -> true | _ -> false -(* Iterate over the attributes and try to find the [@react.component] attribute *) -let has_attr_on_binding pred {pvb_attributes} = - List.find_opt pred pvb_attributes <> None - -let core_type_of_attrs attributes = - List.find_map - (fun ({txt}, payload) -> - match (txt, payload) with - | ("react.component" | "jsx.component"), PTyp core_type -> Some core_type - | _ -> None) +(* A component attribute with a payload [Ast_helper] cannot read stays an + attribute, so formatting keeps it; reject it here instead of silently + ignoring the payload. *) +let check_component_payloads attributes = + List.iter + (fun ((({txt; loc}, _) : attribute) as attr) -> + if Ast_helper.Jsx_component.of_attribute attr = None then + if is_component_attr attr then + Location.raise_errorf ~loc + "@@%s takes no payload, or the props type to share, as in @@%s(: \ + props)." + txt txt + else if is_component_with_props_attr attr then + Location.raise_errorf ~loc "@@%s takes no payload." txt) attributes let typ_vars_of_core_type {ptyp_desc} = diff --git a/compiler/syntax/src/jsx_v4.ml b/compiler/syntax/src/jsx_v4.ml index 4b6d01e5490..2d488bd46a2 100644 --- a/compiler/syntax/src/jsx_v4.ml +++ b/compiler/syntax/src/jsx_v4.ml @@ -571,7 +571,9 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = {expr with pexp_desc = Pexp_sequence (first, constrain_jsx_return second)} | _ -> jsx_element_constraint expr in - if Jsx_common.has_attr_on_binding Jsx_common.has_attr binding then ( + Jsx_common.check_component_payloads binding.pvb_attributes; + match binding.pvb_jsx_component with + | Some {pjc_kind = Pjc_labelled core_type_of_attr} -> check_multiple_components ~config ~loc:pstr_loc; let binding_newtypes, binding_core_type = match binding.pvb_constraint with @@ -579,9 +581,6 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = | Some {pvc_newtypes; pvc_type} -> (List.rev_map (fun name -> (name, [])) pvc_newtypes, Some pvc_type) in - let core_type_of_attr = - Jsx_common.core_type_of_attrs binding.pvb_attributes - in let typ_vars_of_core_type = core_type_of_attr |> Option.map Jsx_common.typ_vars_of_core_type @@ -595,6 +594,7 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = pvb_pat = {binding.pvb_pat with ppat_loc = empty_loc}; pvb_constraint = None; pvb_loc = empty_loc; + pvb_jsx_component = None; pvb_attributes = binding.pvb_attributes |> List.filter other_attrs_pure; } in @@ -818,12 +818,12 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = } in let new_binding = Some (binding_wrapper full_expression) in - (Some props_record_type, binding, new_binding)) - else if Jsx_common.has_attr_on_binding Jsx_common.has_attr_with_props binding - then + (Some props_record_type, binding, new_binding) + | Some {pjc_kind = Pjc_with_props} -> let modified_binding = { binding with + pvb_jsx_component = None; pvb_attributes = binding.pvb_attributes |> List.filter other_attrs_pure; } in @@ -925,11 +925,12 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = ( None, { binding with + pvb_jsx_component = None; pvb_attributes = binding.pvb_attributes |> List.filter other_attrs_pure; pvb_expr = binding_expr |> constrain_jsx_return; }, new_binding ) - else (None, binding, None) + | None -> (None, binding, None) let rec collect_prop_types types {ptyp_desc} = match ptyp_desc with @@ -955,6 +956,24 @@ let rec collect_prop_types types {ptyp_desc} = go types params | _ -> types +(* How many [@react.component] and [@react.componentWithProps] annotations a + value description carries: the one in its component node, and any left in + its attributes. *) +let count_component_annotations {pval_jsx_component; pval_attributes} = + let count is_attr = List.length (List.filter is_attr pval_attributes) in + let labelled, with_props = + match pval_jsx_component with + | Some {pjc_kind = Pjc_labelled _} -> (1, 0) + | Some {pjc_kind = Pjc_with_props} -> (0, 1) + | None -> (0, 0) + in + ( labelled + count Jsx_common.is_component_attr, + with_props + count Jsx_common.is_component_with_props_attr ) + +let core_type_of_component = function + | Some {pjc_kind = Pjc_labelled core_type} -> core_type + | Some {pjc_kind = Pjc_with_props} | None -> None + let transform_structure_item ~config item = match item with (* external *) @@ -963,12 +982,10 @@ let transform_structure_item ~config item = pstr_desc = Pstr_primitive ({pval_attributes; pval_type} as value_description); } as pstr -> ( - match - ( List.filter Jsx_common.has_attr pval_attributes, - List.filter Jsx_common.has_attr_with_props pval_attributes ) - with - | [], [] -> [item] - | _, [_] -> + Jsx_common.check_component_payloads pval_attributes; + match count_component_annotations value_description with + | 0, 0 -> [item] + | _, 1 -> Jsx_common.raise_error ~loc:pstr_loc "Components cannot be defined as externals when using \ @react.componentWithProps.\n\n\ @@ -977,11 +994,13 @@ let transform_structure_item ~config item = use the type React.component instead.\n\ Alternatively, use @react.component for an external definition with \ labeled arguments." - | [_], [] -> + | 1, 0 -> check_multiple_components ~config ~loc:pstr_loc; check_string_int_attribute_iter.structure_item check_string_int_attribute_iter item; - let core_type_of_attr = Jsx_common.core_type_of_attrs pval_attributes in + let core_type_of_attr = + core_type_of_component value_description.pval_jsx_component + in let typ_vars_of_core_type = core_type_of_attr |> Option.map Jsx_common.typ_vars_of_core_type @@ -1018,6 +1037,7 @@ let transform_structure_item ~config item = { value_description with pval_type = {pval_type with ptyp_desc = new_external_type}; + pval_jsx_component = None; pval_attributes = List.filter other_attrs_pure pval_attributes; }; } @@ -1069,13 +1089,16 @@ let transform_signature_item ~config item = psig_loc; psig_desc = Psig_value ({pval_attributes; pval_type} as psig_desc); } as psig -> ( - match List.filter Jsx_common.has_attr pval_attributes with - | [] -> [item] - | [_] -> + Jsx_common.check_component_payloads pval_attributes; + match count_component_annotations psig_desc with + | 0, _ -> [item] + | 1, _ -> check_multiple_components ~config ~loc:psig_loc; check_string_int_attribute_iter.signature_item check_string_int_attribute_iter item; - let core_type_of_attr = Jsx_common.core_type_of_attrs pval_attributes in + let core_type_of_attr = + core_type_of_component psig_desc.pval_jsx_component + in let typ_vars_of_core_type = core_type_of_attr |> Option.map Jsx_common.typ_vars_of_core_type @@ -1112,6 +1135,7 @@ let transform_signature_item ~config item = { psig_desc with pval_type = {pval_type with ptyp_desc = new_external_type}; + pval_jsx_component = None; pval_attributes = List.filter other_attrs_pure pval_attributes; }; } diff --git a/compiler/syntax/src/res_printer.ml b/compiler/syntax/src/res_printer.ml index 910d699fec0..232bb9e93ba 100644 --- a/compiler/syntax/src/res_printer.ml +++ b/compiler/syntax/src/res_printer.ml @@ -1312,7 +1312,8 @@ and print_value_description ~state value_description cmt_tbl = in let attrs = print_attributes ~state ~loc:value_description.pval_name.loc - value_description.pval_attributes cmt_tbl + (Ast_helper.Val.description_attributes value_description) + cmt_tbl in let header = if is_external then "external " else "let " in let inline_record_definitions = @@ -2450,7 +2451,7 @@ and print_value_binding ~state ~rec_flag (vb : Parsetree.value_binding) cmt_tbl i = let has_unwrap = ref false in let attrs = - vb.pvb_attributes + Ast_helper.Vb.binding_attributes vb |> List.filter_map (function | {Asttypes.txt = "let.unwrap"}, _ -> has_unwrap := true; diff --git a/tests/build_tests/super_errors/expected/react_componentWithProps_invalid_payload.res.expected b/tests/build_tests/super_errors/expected/react_componentWithProps_invalid_payload.res.expected new file mode 100644 index 00000000000..e3d44b03fca --- /dev/null +++ b/tests/build_tests/super_errors/expected/react_componentWithProps_invalid_payload.res.expected @@ -0,0 +1,11 @@ + + We've found a bug for you! + /.../fixtures/react_componentWithProps_invalid_payload.res:4:3-27 + + 2 │ + 3 │ module C = { + 4 │ @react.componentWithProps(: props) + 5 │ let make = (props: props) => React.string(props.label) + 6 │ } + + @react.componentWithProps takes no payload. \ No newline at end of file diff --git a/tests/build_tests/super_errors/expected/react_component_invalid_payload.res.expected b/tests/build_tests/super_errors/expected/react_component_invalid_payload.res.expected new file mode 100644 index 00000000000..b49901daa1f --- /dev/null +++ b/tests/build_tests/super_errors/expected/react_component_invalid_payload.res.expected @@ -0,0 +1,10 @@ + + We've found a bug for you! + /.../fixtures/react_component_invalid_payload.res:2:3-18 + + 1 │ module C = { + 2 │ @react.component("Button") + 3 │ let make = (~label) => React.string(label) + 4 │ } + + @react.component takes no payload, or the props type to share, as in @react.component(: props). \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/react_componentWithProps_invalid_payload.res b/tests/build_tests/super_errors/fixtures/react_componentWithProps_invalid_payload.res new file mode 100644 index 00000000000..6423d6be30c --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/react_componentWithProps_invalid_payload.res @@ -0,0 +1,6 @@ +type props = {label: string} + +module C = { + @react.componentWithProps(: props) + let make = (props: props) => React.string(props.label) +} diff --git a/tests/build_tests/super_errors/fixtures/react_component_invalid_payload.res b/tests/build_tests/super_errors/fixtures/react_component_invalid_payload.res new file mode 100644 index 00000000000..2249c13a596 --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/react_component_invalid_payload.res @@ -0,0 +1,4 @@ +module C = { + @react.component("Button") + let make = (~label) => React.string(label) +} diff --git a/tests/ounit_tests/ounit_ast_mapper0_tests.ml b/tests/ounit_tests/ounit_ast_mapper0_tests.ml index f0ebbb5b864..12b63d205bd 100644 --- a/tests/ounit_tests/ounit_ast_mapper0_tests.ml +++ b/tests/ounit_tests/ounit_ast_mapper0_tests.ml @@ -797,6 +797,164 @@ let map_value_binding0 vb = let to_value_binding0 vb = Ast_mapper_to0.default_mapper.value_binding Ast_mapper_to0.default_mapper vb +let attr_names attrs = + List.map (fun (({txt} : string Asttypes.loc), _) -> txt) attrs + +let make_binding ?(attrs = []) () = + Ast_helper.Vb.mk ~loc ~attrs + (Ast_helper.Pat.var ~loc (Location.mknoloc "make")) + (Ast_helper.Exp.ident ~loc (Location.mknoloc (Longident.Lident "x"))) + +let props_type = + Ast_helper.Typ.constr ~loc (Location.mknoloc (Longident.Lident "props")) [] + +let test_jsx_component_becomes_the_binding_node _ = + let vb = + make_binding ~attrs:[attr "react.component" (Parsetree.PTyp props_type)] () + in + (match vb.pvb_jsx_component with + | Some {pjc_kind = Pjc_labelled (Some _); pjc_namespace = Pjc_react} -> () + | _ -> assert_failure "Expected @react.component(: props) as the node"); + OUnit.assert_equal ~printer:(String.concat ", ") [] + (attr_names vb.pvb_attributes); + let vb0 = to_value_binding0 vb in + match vb0.pvb_attributes with + | [ + ( {txt = "react.component"}, + PTyp {ptyp_desc = Ptyp_constr ({txt = Lident "props"}, [])} ); + ] -> + () + | _ -> assert_failure "Expected @react.component(: props) on the ast0 wire" + +(* The first [@react.component] wins over an earlier + [@react.componentWithProps], as it always has in the JSX transform; the + others stay attributes. *) +let test_jsx_component_precedence _ = + let vb = + make_binding + ~attrs: + [ + attr "react.componentWithProps" (Parsetree.PStr []); + attr "jsx.component" (Parsetree.PStr []); + attr "react.component" (Parsetree.PStr []); + ] + () + in + (match vb.pvb_jsx_component with + | Some {pjc_kind = Pjc_labelled None; pjc_namespace = Pjc_jsx} -> () + | _ -> assert_failure "Expected @jsx.component as the node"); + OUnit.assert_equal ~printer:(String.concat ", ") + ["react.componentWithProps"; "react.component"] + (attr_names vb.pvb_attributes) + +(* A payload the JSX transform does not understand is not dropped: the + attribute stays, for the transform to reject and the printer to keep. *) +let test_jsx_component_with_unknown_payload_stays_an_attribute _ = + let payload = + Parsetree.PStr + [ + Ast_helper.Str.eval + (Ast_helper.Exp.constant + (Pconst_string (String_literal.string_from_semantic "x"))); + ] + in + let vd = + Ast_helper.Val.mk ~loc + ~attrs: + [ + attr "react.component" payload; + attr "react.componentWithProps" (Parsetree.PTyp props_type); + ] + (Location.mknoloc "make") props_type + in + OUnit.assert_bool "Expected no node" (vd.pval_jsx_component = None); + OUnit.assert_equal ~printer:(String.concat ", ") + ["react.component"; "react.componentWithProps"] + (attr_names vd.pval_attributes) + +let test_jsx_component_keeps_its_place_on_the_wire _ = + let vb = + make_binding + ~attrs: + [ + ( located_string ~loc:(source_loc 10 20) "deprecated", + Parsetree.PStr [] ); + ( located_string ~loc:(source_loc 30 40) "react.componentWithProps", + Parsetree.PStr [] ); + (located_string ~loc:(source_loc 50 60) "genType", Parsetree.PStr []); + ] + () + in + OUnit.assert_equal ~printer:(String.concat ", ") + ["deprecated"; "react.componentWithProps"; "genType"] + (attr_names (to_value_binding0 vb).pvb_attributes) + +(* Ppx output carries no source position, so every attribute ties. The + annotation goes back first among them, where it is usually written. *) +let test_jsx_component_from_ppx_goes_first_on_the_wire _ = + let vb0 = + Ast_helper0.Vb.mk ~loc + ~attrs: + [ + (Location.mknoloc "jsx.component", Parsetree0.PStr []); + (Location.mknoloc "genType", Parsetree0.PStr []); + ] + (Ast_helper0.Pat.var ~loc (Location.mknoloc "make")) + (Ast_helper0.Exp.ident ~loc (Location.mknoloc (Longident.Lident "x"))) + in + let vb = map_value_binding0 vb0 in + (match vb.pvb_jsx_component with + | Some {pjc_kind = Pjc_labelled None; pjc_namespace = Pjc_jsx} -> () + | _ -> assert_failure "Expected the ppx's @jsx.component as the node"); + OUnit.assert_equal ~printer:(String.concat ", ") ["genType"] + (attr_names vb.pvb_attributes); + OUnit.assert_equal ~printer:(String.concat ", ") + ["jsx.component"; "genType"] + (attr_names (to_value_binding0 vb).pvb_attributes) + +(* A mapper sees the attribute and [Vb.mk] takes it out again, so a mapper that + only relocates keeps the component. *) +let test_jsx_component_survives_the_mapper _ = + let vb = + make_binding ~attrs:[attr "react.componentWithProps" (Parsetree.PStr [])] () + in + let mapped = + Ast_mapper.default_mapper.value_binding Ast_mapper.default_mapper vb + in + (match mapped.pvb_jsx_component with + | Some {pjc_kind = Pjc_with_props; pjc_namespace = Pjc_react} -> () + | _ -> assert_failure "Expected the mapped binding to keep the node"); + OUnit.assert_equal ~printer:(String.concat ", ") [] + (attr_names mapped.pvb_attributes) + +let test_jsx_component_on_value_descriptions_through_ast0 _ = + let vd = + Ast_helper.Val.mk ~loc + ~attrs: + [ + attr "module" (Parsetree.PStr []); + attr "react.component" (Parsetree.PStr []); + ] + ~prim:(Prim_name "default") (Location.mknoloc "make") props_type + in + (match vd.pval_jsx_component with + | Some {pjc_kind = Pjc_labelled None} -> () + | _ -> assert_failure "Expected @react.component as the node"); + let vd0 = + Ast_mapper_to0.default_mapper.value_description + Ast_mapper_to0.default_mapper vd + in + OUnit.assert_equal ~printer:(String.concat ", ") + ["react.component"; "module"] + (attr_names vd0.pval_attributes); + let back = + Ast_mapper_from0.default_mapper.value_description + Ast_mapper_from0.default_mapper vd0 + in + OUnit.assert_bool "Expected the node back" (back.pval_jsx_component <> None); + OUnit.assert_equal ~printer:(String.concat ", ") ["module"] + (attr_names back.pval_attributes) + let test_value_constraint_roundtrips_through_ast0 _ = let newtype = Location.mknoloc "a" in let typ = @@ -2355,6 +2513,19 @@ let suites = >:: test_field_runtime_name_keeps_ppx_order_on_the_wire; "constructor_runtime_tag_reaches_ast0_as_an_attribute" >:: test_constructor_runtime_tag_reaches_ast0_as_an_attribute; + "jsx_component_becomes_the_binding_node" + >:: test_jsx_component_becomes_the_binding_node; + "jsx_component_precedence" >:: test_jsx_component_precedence; + "jsx_component_with_unknown_payload_stays_an_attribute" + >:: test_jsx_component_with_unknown_payload_stays_an_attribute; + "jsx_component_keeps_its_place_on_the_wire" + >:: test_jsx_component_keeps_its_place_on_the_wire; + "jsx_component_from_ppx_goes_first_on_the_wire" + >:: test_jsx_component_from_ppx_goes_first_on_the_wire; + "jsx_component_survives_the_mapper" + >:: test_jsx_component_survives_the_mapper; + "jsx_component_on_value_descriptions_through_ast0" + >:: test_jsx_component_on_value_descriptions_through_ast0; "ppx_context_on_the_wire" >:: test_ppx_context_on_the_wire; "fixed_argument_roundtrips_through_ast0" >:: test_fixed_argument_roundtrips_through_ast0; diff --git a/tests/syntax_tests/data/ast-mapping/JsxComponents.res b/tests/syntax_tests/data/ast-mapping/JsxComponents.res new file mode 100644 index 00000000000..5d7cdfc2905 --- /dev/null +++ b/tests/syntax_tests/data/ast-mapping/JsxComponents.res @@ -0,0 +1,42 @@ +module A = { + @react.component + let make = (~x) => React.string(x) +} + +module B = { + /** Doc */ + @deprecated @jsx.component(: sharedProps) @genType + let make = (~x) => React.string(x) +} + +module C = { + @react.componentWithProps + let make = (props: props) => React.string(props.x) +} + +module D = { + @jsx.componentWithProps @react.component + let make = (~x) => React.string(x) +} + +module E = { + @react.component @module("x") + external make: (~x: string) => React.element = "default" +} + +module F: { + @react.component + let make: (~x: string) => React.element + @jsx.componentWithProps + let other: props => React.element +} = { + @react.component + let make = (~x) => React.string(x) + let other = props => React.string(props.x) +} + +let f = () => { + @react.component + let make = (~x) => React.string(x) + make +} diff --git a/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt b/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt new file mode 100644 index 00000000000..b9a236b2076 --- /dev/null +++ b/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt @@ -0,0 +1,88 @@ +module A = { + @res.jsxComponentProps + type props<'x> = { + x: 'x, + } + let make = ({x, _}: props<_>): React.element => React.string(x) + let make = React.component({ + let \"JsxComponents$A" = (props: props<_>) => make(props) + \"JsxComponents$A" + }) +} + +module B = { + type props = sharedProps + + /** Doc */ + @deprecated @genType let make = ({x, _}: props): React.element => React.string(x) + /** Doc */ + @deprecated @genType + let make = React.component({ + let \"JsxComponents$B" = props => make(props) + + \"JsxComponents$B" + }) +} + +module C = { + let make = (props: props): React.element => React.string(props.x) + let make = React.component({ + let \"JsxComponents$C" = (props: props): React.element => make(props) + \"JsxComponents$C" + }) +} + +module D = { + @res.jsxComponentProps + type props<'x> = { + x: 'x, + } + + let make = ({x, _}: props<_>): React.element => React.string(x) + let make = React.component({ + let \"JsxComponents$D" = (props: props<_>) => make(props) + + \"JsxComponents$D" + }) +} + +module E = { + @res.jsxComponentProps @live + type props<'x> = { + x: 'x, + } + + @module("x") + external make: React.component> = "default" +} + +module F: { + @res.jsxComponentProps + type props<'x> = { + x: 'x, + } + + let make: React.component> + @jsx.componentWithProps + let other: props => React.element +} = { + @res.jsxComponentProps + type props<'x> = { + x: 'x, + } + + let make = ({x, _}: props<_>): React.element => React.string(x) + let make = React.component({ + let \"JsxComponents$F" = (props: props<_>) => make(props) + + \"JsxComponents$F" + }) + + let other = props => React.string(props.x) +} + +let f = () => { + @react.component + let make = (~x) => React.string(x) + make +} From ce53d0d03986c0263a9f34407cedaa7cb2eb7e0d Mon Sep 17 00:00:00 2001 From: Christoph Knittel Date: Sat, 17 Oct 2026 08:57:26 +0000 Subject: [PATCH 2/2] Reject a second JSX component annotation on a definition A definition with two component annotations, such as `@react.component @react.componentWithProps let make = ...`, is now an error. On `let` bindings the second annotation used to be dropped silently; on externals and signature items it was already an error, with messages that depended on the combination. With no annotation left over after the node takes one, the JSX transform no longer filters attributes or counts annotations: externals and signature items match on the node directly. Example (rescript): module C = { @react.component @react.componentWithProps let make = (~label) => React.string(label) } Diagnostic (text): A definition can have only one JSX component annotation. Breaking: a `let` with two component annotations stops compiling; delete the extra one. The one that took effect is the first `@react.component`, or failing that the first `componentWithProps`. Review: Which diagnostics change on externals and signature items? "Only one JSX component call can exist on a component at one time", and the `componentWithProps` external error where `@react.component` was also present, become the new message, located at the second annotation. Area: syntax tests Output: unchanged, except that the 45 generated programs with two annotations now fail with the error above; 17 of them compiled before Formatting: unchanged, base and branch res_parser print 232 generated files identically Formats: none PPX: unchanged Editor: unchanged Errors: none, because the message is an inline `raise_errorf` call, which the catalog's scope leaves out Tests: +tests/build_tests/super_errors/fixtures/react_component_duplicate_annotation.res ~tests/syntax_tests/data/ast-mapping/JsxComponents.res Measured: differential harness, base vs branch bsc -bs-jsx 4: 142 of 232 programs identical, 45 differ by the payload error, 45 by this one; without JSX: 232 of 232 identical PR: #8884 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014hcyfimX2ss8qj5MZwdMdP Signed-off-by: Christoph Knittel --- changes/jsx-component-payload-error.polish.md | 2 +- compiler/ml/ast_helper.ml | 2 +- compiler/syntax/src/jsx_common.ml | 45 +++++------ compiler/syntax/src/jsx_v4.ml | 76 ++++--------------- ...omponent_duplicate_annotation.res.expected | 10 +++ .../react_component_duplicate_annotation.res | 4 + .../data/ast-mapping/JsxComponents.res | 4 +- .../expected/JsxComponents.res.txt | 10 +-- 8 files changed, 56 insertions(+), 97 deletions(-) create mode 100644 tests/build_tests/super_errors/expected/react_component_duplicate_annotation.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/react_component_duplicate_annotation.res diff --git a/changes/jsx-component-payload-error.polish.md b/changes/jsx-component-payload-error.polish.md index e48f5d757c4..ccbfb2918a9 100644 --- a/changes/jsx-component-payload-error.polish.md +++ b/changes/jsx-component-payload-error.polish.md @@ -1 +1 @@ -- Report a JSX component annotation with an unsupported payload, such as `@react.component("x")`, instead of ignoring the payload. https://github.com/rescript-lang/rescript/pull/8884 +- Report a JSX component annotation with an unsupported payload, such as `@react.component("x")`, or a second one on the same definition, instead of ignoring it. https://github.com/rescript-lang/rescript/pull/8884 diff --git a/compiler/ml/ast_helper.ml b/compiler/ml/ast_helper.ml index 1ee19e0cb9a..c1f04ea7d63 100644 --- a/compiler/ml/ast_helper.ml +++ b/compiler/ml/ast_helper.ml @@ -356,7 +356,7 @@ module Jsx_component = struct (* The first [@react.component], or failing that the first [@react.componentWithProps]: the JSX transform has always given them this precedence. A later one, or one whose payload is not understood, is - left in the attributes for the JSX transform to deal with, so that + left in the attributes for the JSX transform to reject, so that formatting does not drop it. *) let take attrs = let take_first is_kind = diff --git a/compiler/syntax/src/jsx_common.ml b/compiler/syntax/src/jsx_common.ml index 9ac51f84708..7d6dd48450c 100644 --- a/compiler/syntax/src/jsx_common.ml +++ b/compiler/syntax/src/jsx_common.ml @@ -8,33 +8,30 @@ type jsx_config = { mutable has_component: bool; } -(* [@react.component] and [@react.componentWithProps] that are still among the - attributes: [Ast_helper] takes the first one into the binding's or - description's component node and leaves any other. *) -let is_component_attr (({txt}, _) : attribute) = - match txt with - | "react.component" | "jsx.component" -> true - | _ -> false - -let is_component_with_props_attr (({txt}, _) : attribute) = - match txt with - | "react.componentWithProps" | "jsx.componentWithProps" -> true - | _ -> false - -(* A component attribute with a payload [Ast_helper] cannot read stays an - attribute, so formatting keeps it; reject it here instead of silently - ignoring the payload. *) -let check_component_payloads attributes = +(* [Ast_helper] takes the first [@react.component], or failing that the first + [@react.componentWithProps], into the binding's or description's component + node. One still among the attributes is a second annotation, or one with a + payload the node cannot hold; it stays an attribute so that formatting keeps + it, and the JSX transform rejects it here. *) +let reject_leftover_component_attrs attributes = List.iter (fun ((({txt; loc}, _) : attribute) as attr) -> - if Ast_helper.Jsx_component.of_attribute attr = None then - if is_component_attr attr then + match txt with + | "react.component" | "jsx.component" | "react.componentWithProps" + | "jsx.componentWithProps" -> ( + match Ast_helper.Jsx_component.of_attribute attr with + | Some _ -> Location.raise_errorf ~loc - "@@%s takes no payload, or the props type to share, as in @@%s(: \ - props)." - txt txt - else if is_component_with_props_attr attr then - Location.raise_errorf ~loc "@@%s takes no payload." txt) + "A definition can have only one JSX component annotation." + | None -> ( + match txt with + | "react.component" | "jsx.component" -> + Location.raise_errorf ~loc + "@@%s takes no payload, or the props type to share, as in @@%s(: \ + props)." + txt txt + | _ -> Location.raise_errorf ~loc "@@%s takes no payload." txt)) + | _ -> ()) attributes let typ_vars_of_core_type {ptyp_desc} = diff --git a/compiler/syntax/src/jsx_v4.ml b/compiler/syntax/src/jsx_v4.ml index 2d488bd46a2..c36d55c754e 100644 --- a/compiler/syntax/src/jsx_v4.ml +++ b/compiler/syntax/src/jsx_v4.ml @@ -55,14 +55,6 @@ let jsx_component_expr config ~loc expr = (Exp.ident ~loc {loc; txt = module_access_name config "component"}) [(Nolabel, expr)] -(* Helper method to filter out any attribute that isn't [@react.component] *) -let other_attrs_pure (loc, _) = - match loc.txt with - | "react.component" | "jsx.component" | "react.componentWithProps" - | "jsx.componentWithProps" -> - false - | _ -> true - (* Finds the name of the variable the binding is assigned to, otherwise raises Invalid_argument *) let rec get_fn_name binding = match binding with @@ -571,7 +563,7 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = {expr with pexp_desc = Pexp_sequence (first, constrain_jsx_return second)} | _ -> jsx_element_constraint expr in - Jsx_common.check_component_payloads binding.pvb_attributes; + Jsx_common.reject_leftover_component_attrs binding.pvb_attributes; match binding.pvb_jsx_component with | Some {pjc_kind = Pjc_labelled core_type_of_attr} -> check_multiple_components ~config ~loc:pstr_loc; @@ -595,7 +587,6 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = pvb_constraint = None; pvb_loc = empty_loc; pvb_jsx_component = None; - pvb_attributes = binding.pvb_attributes |> List.filter other_attrs_pure; } in let fn_name = get_fn_name binding.pvb_pat in @@ -820,13 +811,7 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = let new_binding = Some (binding_wrapper full_expression) in (Some props_record_type, binding, new_binding) | Some {pjc_kind = Pjc_with_props} -> - let modified_binding = - { - binding with - pvb_jsx_component = None; - pvb_attributes = binding.pvb_attributes |> List.filter other_attrs_pure; - } - in + let modified_binding = {binding with pvb_jsx_component = None} in let fn_name = get_fn_name modified_binding.pvb_pat in let full_module_name = make_module_name file_name config.nested_modules fn_name @@ -926,7 +911,6 @@ let map_binding ~config ~empty_loc ~pstr_loc ~file_name binding = { binding with pvb_jsx_component = None; - pvb_attributes = binding.pvb_attributes |> List.filter other_attrs_pure; pvb_expr = binding_expr |> constrain_jsx_return; }, new_binding ) @@ -956,24 +940,6 @@ let rec collect_prop_types types {ptyp_desc} = go types params | _ -> types -(* How many [@react.component] and [@react.componentWithProps] annotations a - value description carries: the one in its component node, and any left in - its attributes. *) -let count_component_annotations {pval_jsx_component; pval_attributes} = - let count is_attr = List.length (List.filter is_attr pval_attributes) in - let labelled, with_props = - match pval_jsx_component with - | Some {pjc_kind = Pjc_labelled _} -> (1, 0) - | Some {pjc_kind = Pjc_with_props} -> (0, 1) - | None -> (0, 0) - in - ( labelled + count Jsx_common.is_component_attr, - with_props + count Jsx_common.is_component_with_props_attr ) - -let core_type_of_component = function - | Some {pjc_kind = Pjc_labelled core_type} -> core_type - | Some {pjc_kind = Pjc_with_props} | None -> None - let transform_structure_item ~config item = match item with (* external *) @@ -982,10 +948,10 @@ let transform_structure_item ~config item = pstr_desc = Pstr_primitive ({pval_attributes; pval_type} as value_description); } as pstr -> ( - Jsx_common.check_component_payloads pval_attributes; - match count_component_annotations value_description with - | 0, 0 -> [item] - | _, 1 -> + Jsx_common.reject_leftover_component_attrs pval_attributes; + match value_description.pval_jsx_component with + | None -> [item] + | Some {pjc_kind = Pjc_with_props} -> Jsx_common.raise_error ~loc:pstr_loc "Components cannot be defined as externals when using \ @react.componentWithProps.\n\n\ @@ -994,13 +960,10 @@ let transform_structure_item ~config item = use the type React.component instead.\n\ Alternatively, use @react.component for an external definition with \ labeled arguments." - | 1, 0 -> + | Some {pjc_kind = Pjc_labelled core_type_of_attr} -> check_multiple_components ~config ~loc:pstr_loc; check_string_int_attribute_iter.structure_item check_string_int_attribute_iter item; - let core_type_of_attr = - core_type_of_component value_description.pval_jsx_component - in let typ_vars_of_core_type = core_type_of_attr |> Option.map Jsx_common.typ_vars_of_core_type @@ -1038,14 +1001,10 @@ let transform_structure_item ~config item = value_description with pval_type = {pval_type with ptyp_desc = new_external_type}; pval_jsx_component = None; - pval_attributes = List.filter other_attrs_pure pval_attributes; }; } in - [props_record_type; new_structure] - | _ -> - Jsx_common.raise_error ~loc:pstr_loc - "Only one JSX component call can exist on a component at one time") + [props_record_type; new_structure]) (* let component = ... *) | {pstr_loc; pstr_desc = Pstr_value (rec_flag, value_bindings)} -> ( let file_name = filename_from_loc pstr_loc in @@ -1089,16 +1048,15 @@ let transform_signature_item ~config item = psig_loc; psig_desc = Psig_value ({pval_attributes; pval_type} as psig_desc); } as psig -> ( - Jsx_common.check_component_payloads pval_attributes; - match count_component_annotations psig_desc with - | 0, _ -> [item] - | 1, _ -> + Jsx_common.reject_leftover_component_attrs pval_attributes; + match psig_desc.pval_jsx_component with + (* [@react.componentWithProps] on a signature item has always been left + alone. *) + | None | Some {pjc_kind = Pjc_with_props} -> [item] + | Some {pjc_kind = Pjc_labelled core_type_of_attr} -> check_multiple_components ~config ~loc:psig_loc; check_string_int_attribute_iter.signature_item check_string_int_attribute_iter item; - let core_type_of_attr = - core_type_of_component psig_desc.pval_jsx_component - in let typ_vars_of_core_type = core_type_of_attr |> Option.map Jsx_common.typ_vars_of_core_type @@ -1136,14 +1094,10 @@ let transform_signature_item ~config item = psig_desc with pval_type = {pval_type with ptyp_desc = new_external_type}; pval_jsx_component = None; - pval_attributes = List.filter other_attrs_pure pval_attributes; }; } in - [props_record_type; new_structure] - | _ -> - Jsx_common.raise_error ~loc:psig_loc - "Only one JSX component call can exist on a component at one time") + [props_record_type; new_structure]) | _ -> [item] (* There appear to be slightly different rules of transformation whether the component is upper-, lowercase or a fragment *) diff --git a/tests/build_tests/super_errors/expected/react_component_duplicate_annotation.res.expected b/tests/build_tests/super_errors/expected/react_component_duplicate_annotation.res.expected new file mode 100644 index 00000000000..92d18d281da --- /dev/null +++ b/tests/build_tests/super_errors/expected/react_component_duplicate_annotation.res.expected @@ -0,0 +1,10 @@ + + We've found a bug for you! + /.../fixtures/react_component_duplicate_annotation.res:2:20-44 + + 1 │ module C = { + 2 │ @react.component @react.componentWithProps + 3 │ let make = (~label) => React.string(label) + 4 │ } + + A definition can have only one JSX component annotation. \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/react_component_duplicate_annotation.res b/tests/build_tests/super_errors/fixtures/react_component_duplicate_annotation.res new file mode 100644 index 00000000000..014104471f4 --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/react_component_duplicate_annotation.res @@ -0,0 +1,4 @@ +module C = { + @react.component @react.componentWithProps + let make = (~label) => React.string(label) +} diff --git a/tests/syntax_tests/data/ast-mapping/JsxComponents.res b/tests/syntax_tests/data/ast-mapping/JsxComponents.res index 5d7cdfc2905..abc6c1f1425 100644 --- a/tests/syntax_tests/data/ast-mapping/JsxComponents.res +++ b/tests/syntax_tests/data/ast-mapping/JsxComponents.res @@ -15,8 +15,8 @@ module C = { } module D = { - @jsx.componentWithProps @react.component - let make = (~x) => React.string(x) + @jsx.componentWithProps + let make = (props: props) => React.string(props.x) } module E = { diff --git a/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt b/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt index b9a236b2076..b463e41a075 100644 --- a/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt +++ b/tests/syntax_tests/data/ast-mapping/expected/JsxComponents.res.txt @@ -33,15 +33,9 @@ module C = { } module D = { - @res.jsxComponentProps - type props<'x> = { - x: 'x, - } - - let make = ({x, _}: props<_>): React.element => React.string(x) + let make = (props: props): React.element => React.string(props.x) let make = React.component({ - let \"JsxComponents$D" = (props: props<_>) => make(props) - + let \"JsxComponents$D" = (props: props): React.element => make(props) \"JsxComponents$D" }) }