Fix issue with JSX V4 when components are nested

Fixes #5975

The issue is due to a combination of:
1) when a component let make = body is transformed, first the body is transformed, then when transformStructureItem4 is called, the body is transformed again
2) @react.component is left after the PPX transformation

Because of this, in the case of nested components, the transformed inner components is transformed again by transformStructureItem4, but it should not as it's been desugared already.

It would be enough to change either one, to avoid the error with nested components, but this PR handles both aspects.

Author: cristianoc

CHANGELOG.md

@@ -74,6 +74,7 @@ These are only breaking changes for unformatted code.
 - Support `@gentype.import` as an alias to `@genType.import` in the compiler https://github.com/rescript-lang/rescript-compiler/pull/6020
 - Fix issue with integer overflow check https://github.com/rescript-lang/rescript-compiler/pull/6028
 - Fix issue with JSX V4 and newtype https://github.com/rescript-lang/rescript-compiler/pull/6029
+- Fix issue with JSX V4 when components are nested https://github.com/rescript-lang/rescript-compiler/pull/6031
 
 #### :nail_care: Polish
 

res_syntax/src/reactjs_jsx_ppx.ml

@@ -127,8 +127,8 @@ let getMapper ~config =
           | Pstr_attribute attr -> processConfigAttribute attr config
           | _ -> ());
           let item = default_mapper.structure_item mapper item in
-          if config.version = 3 then transformStructureItem3 mapper item
-          else if config.version = 4 then transformStructureItem4 mapper item
+          if config.version = 3 then transformStructureItem3 item
+          else if config.version = 4 then transformStructureItem4 item
           else [item])
         items
       |> List.flatten

res_syntax/src/reactjs_jsx_v3.ml

@@ -446,8 +446,7 @@ let jsxMapper ~config =
       args
   in
 
-  let rec recursivelyTransformNamedArgsForMake mapper expr args newtypes =
-    let expr = mapper.expr mapper expr in
+  let rec recursivelyTransformNamedArgsForMake expr args newtypes =
     match expr.pexp_desc with
     (* TODO: make this show up with a loc. *)
     | Pexp_fun (Labelled "key", _, _, _) | Pexp_fun (Optional "key", _, _, _) ->
@@ -494,7 +493,7 @@ let jsxMapper ~config =
         | _ -> None
       in
 
-      recursivelyTransformNamedArgsForMake mapper expression
+      recursivelyTransformNamedArgsForMake expression
         ((arg, default, pattern, alias, pattern.ppat_loc, type_) :: args)
         newtypes
     | Pexp_fun
@@ -517,10 +516,9 @@ let jsxMapper ~config =
         "React: react.component refs only support plain arguments and type \
          annotations."
     | Pexp_newtype (label, expression) ->
-      recursivelyTransformNamedArgsForMake mapper expression args
-        (label :: newtypes)
+      recursivelyTransformNamedArgsForMake expression args (label :: newtypes)
     | Pexp_constraint (expression, _typ) ->
-      recursivelyTransformNamedArgsForMake mapper expression args newtypes
+      recursivelyTransformNamedArgsForMake expression args newtypes
     | _ -> (args, newtypes, None)
   in
 
@@ -586,7 +584,7 @@ let jsxMapper ~config =
   in
 
   let nestedModules = ref [] in
-  let transformStructureItem mapper item =
+  let transformStructureItem item =
     match item with
     (* external *)
     | {
@@ -825,7 +823,7 @@ let jsxMapper ~config =
           let props = getPropsAttr payload in
           (* do stuff here! *)
           let namedArgList, newtypes, forwardRef =
-            recursivelyTransformNamedArgsForMake mapper
+            recursivelyTransformNamedArgsForMake
               (modifiedBindingOld binding)
               [] []
           in

res_syntax/src/reactjs_jsx_v4.ml

@@ -598,9 +598,7 @@ let transformLowercaseCall3 ~config mapper jsxExprLoc callExprLoc attrs
          })
       args
 
-let rec recursivelyTransformNamedArgsForMake mapper expr args newtypes coreType
-    =
-  let expr = mapper.expr mapper expr in
+let rec recursivelyTransformNamedArgsForMake expr args newtypes coreType =
   match expr.pexp_desc with
   (* TODO: make this show up with a loc. *)
   | Pexp_fun (Labelled "key", _, _, _) | Pexp_fun (Optional "key", _, _, _) ->
@@ -647,7 +645,7 @@ let rec recursivelyTransformNamedArgsForMake mapper expr args newtypes coreType
       | _ -> None
     in
 
-    recursivelyTransformNamedArgsForMake mapper expression
+    recursivelyTransformNamedArgsForMake expression
       ((arg, default, pattern, alias, pattern.ppat_loc, type_) :: args)
       newtypes coreType
   | Pexp_fun
@@ -680,10 +678,10 @@ let rec recursivelyTransformNamedArgsForMake mapper expr args newtypes coreType
       "React: react.component refs only support plain arguments and type \
        annotations."
   | Pexp_newtype (label, expression) ->
-    recursivelyTransformNamedArgsForMake mapper expression args
-      (label :: newtypes) coreType
+    recursivelyTransformNamedArgsForMake expression args (label :: newtypes)
+      coreType
   | Pexp_constraint (expression, coreType) ->
-    recursivelyTransformNamedArgsForMake mapper expression args newtypes
+    recursivelyTransformNamedArgsForMake expression args newtypes
       (Some coreType)
   | _ -> (args, newtypes, coreType)
 
@@ -724,7 +722,7 @@ let check_string_int_attribute_iter =
 
   {Ast_iterator.default_iterator with attribute}
 
-let transformStructureItem ~config mapper item =
+let transformStructureItem ~config item =
   match item with
   (* external *)
   | {
@@ -848,6 +846,8 @@ let transformStructureItem ~config mapper item =
               binding with
               pvb_pat = {binding.pvb_pat with ppat_loc = emptyLoc};
               pvb_loc = emptyLoc;
+              pvb_attributes =
+                binding.pvb_attributes |> List.filter otherAttrsPure;
             }
           in
           let fnName = getFnName binding.pvb_pat in
@@ -891,8 +891,7 @@ let transformStructureItem ~config mapper item =
           let modifiedBinding binding =
             let hasApplication = ref false in
             let wrapExpressionWithBinding expressionFn expression =
-              Vb.mk ~loc:bindingLoc
-                ~attrs:(List.filter otherAttrsPure binding.pvb_attributes)
+              Vb.mk ~loc:bindingLoc ~attrs:binding.pvb_attributes
                 (Pat.var ~loc:bindingPatLoc {loc = bindingPatLoc; txt = fnName})
                 (expressionFn expression)
             in
@@ -1000,7 +999,7 @@ let transformStructureItem ~config mapper item =
           in
           (* do stuff here! *)
           let namedArgList, newtypes, _typeConstraints =
-            recursivelyTransformNamedArgsForMake mapper
+            recursivelyTransformNamedArgsForMake
               (modifiedBindingOld binding)
               [] [] None
           in

res_syntax/tests/ppx/react/expected/aliasProps.res.txt

@@ -3,7 +3,6 @@
 module C0 = {
   type props<'priority, 'text> = {priority: 'priority, text?: 'text}
 
-  @react.component
   let make = (props: props<_, _>) => {
     let _ = props.priority
     let text = switch props.text {
@@ -23,7 +22,6 @@ module C0 = {
 module C1 = {
   type props<'priority, 'text> = {priority: 'priority, text?: 'text}
 
-  @react.component
   let make = (props: props<_, _>) => {
     let p = props.priority
     let text = switch props.text {
@@ -43,7 +41,6 @@ module C1 = {
 module C2 = {
   type props<'foo> = {foo?: 'foo}
 
-  @react.component
   let make = (props: props<_>) => {
     let bar = switch props.foo {
     | Some(foo) => foo

res_syntax/tests/ppx/react/expected/commentAtTop.res.txt

@@ -1,6 +1,5 @@
 type props<'msg> = {msg: 'msg} // test React JSX file
 
-@react.component
 let make = ({msg, _}: props<_>) => {
   ReactDOM.jsx("div", {children: ?ReactDOM.someElement({msg->React.string})})
 }

res_syntax/tests/ppx/react/expected/defaultValueProp.res.txt

@@ -1,6 +1,5 @@
 module C0 = {
   type props<'a, 'b> = {a?: 'a, b?: 'b}
-  @react.component
   let make = (props: props<_, _>) => {
     let a = switch props.a {
     | Some(a) => a
@@ -22,7 +21,6 @@ module C0 = {
 module C1 = {
   type props<'a, 'b> = {a?: 'a, b: 'b}
 
-  @react.component
   let make = (props: props<_, _>) => {
     let a = switch props.a {
     | Some(a) => a

res_syntax/tests/ppx/react/expected/fileLevelConfig.res.txt

@@ -20,7 +20,6 @@ module V3 = {
 module V4C = {
   type props<'msg> = {msg: 'msg}
 
-  @react.component
   let make = ({msg, _}: props<_>) => {
     ReactDOM.createDOMElementVariadic("div", [{msg->React.string}])
   }
@@ -36,7 +35,6 @@ module V4C = {
 module V4A = {
   type props<'msg> = {msg: 'msg}
 
-  @react.component
   let make = ({msg, _}: props<_>) => {
     ReactDOM.jsx("div", {children: ?ReactDOM.someElement({msg->React.string})})
   }

res_syntax/tests/ppx/react/expected/firstClassModules.res.txt

@@ -64,7 +64,6 @@ module Select = {
     items: 'items,
   }
 
-  @react.component
   let make = (
     type a key,
     {model, selected, onChange, items, _}: props<

res_syntax/tests/ppx/react/expected/forwardRef.res.txt

@@ -75,7 +75,6 @@ module V4C = {
       ref?: 'ref,
     }
 
-    @react.component
     let make = (
       {?className, children, _}: props<_, _, ReactRef.currentDomRef>,
       ref: Js.Nullable.t<ReactRef.currentDomRef>,
@@ -103,7 +102,6 @@ module V4C = {
   }
   type props = {}
 
-  @react.component
   let make = (_: props) => {
     let input = React.useRef(Js.Nullable.null)
 
@@ -132,7 +130,6 @@ module V4CUncurried = {
       ref?: 'ref,
     }
 
-    @react.component
     let make = (
       {?className, children, _}: props<_, _, ReactRef.currentDomRef>,
       ref: Js.Nullable.t<ReactRef.currentDomRef>,
@@ -160,7 +157,6 @@ module V4CUncurried = {
   }
   type props = {}
 
-  @react.component
   let make = (_: props) => {
     let input = React.useRef(Js.Nullable.null)
 
@@ -191,7 +187,6 @@ module V4A = {
       ref?: 'ref,
     }
 
-    @react.component
     let make = ({?className, children, _}: props<_, _, ReactDOM.Ref.currentDomRef>, ref) =>
       ReactDOM.jsxs(
         "div",
@@ -217,7 +212,6 @@ module V4A = {
   }
   type props = {}
 
-  @react.component
   let make = (_: props) => {
     let input = React.useRef(Js.Nullable.null)
 
@@ -245,7 +239,6 @@ module V4AUncurried = {
       ref?: 'ref,
     }
 
-    @react.component
     let make = ({?className, children, _}: props<_, _, ReactDOM.Ref.currentDomRef>, ref) =>
       ReactDOM.jsxs(
         "div",
@@ -271,7 +264,6 @@ module V4AUncurried = {
   }
   type props = {}
 
-  @react.component
   let make = (_: props) => {
     let input = React.useRef(Js.Nullable.null)
 

res_syntax/tests/ppx/react/expected/interface.res.txt

@@ -1,6 +1,6 @@
 module A = {
   type props<'x> = {x: 'x}
-  @react.component let make = ({x, _}: props<_>) => React.string(x)
+  let make = ({x, _}: props<_>) => React.string(x)
   let make = {
     let \"Interface$A" = (props: props<_>) => make(props)
     \"Interface$A"
@@ -10,7 +10,7 @@ module A = {
 module NoProps = {
   type props = {}
 
-  @react.component let make = (_: props) => ReactDOM.jsx("div", {})
+  let make = (_: props) => ReactDOM.jsx("div", {})
   let make = {
     let \"Interface$NoProps" = props => make(props)
 

res_syntax/tests/ppx/react/expected/interfaceWithRef.res.txt

@@ -1,5 +1,4 @@
 type props<'x, 'ref> = {x: 'x, ref?: 'ref}
-@react.component
 let make = (
   {x, _}: props<string, ReactDOM.Ref.currentDomRef>,
   ref: Js.Nullable.t<ReactDOM.Ref.currentDomRef>,

res_syntax/tests/ppx/react/expected/mangleKeyword.res.txt

@@ -22,7 +22,6 @@ let c31 = React.createElement(C31.make, C31.makeProps(~_open="x", ()))
 module C4C0 = {
   type props<'T_open, 'T_type> = {@as("open") _open: 'T_open, @as("type") _type: 'T_type}
 
-  @react.component
   let make = ({@as("open") _open, @as("type") _type, _}: props<_, string>) => React.string(_open)
   let make = {
     let \"MangleKeyword$C4C0" = (props: props<_>) => make(props)
@@ -44,7 +43,6 @@ let c4c1 = React.createElement(C4C1.make, {_open: "x", _type: "t"})
 module C4A0 = {
   type props<'T_open, 'T_type> = {@as("open") _open: 'T_open, @as("type") _type: 'T_type}
 
-  @react.component
   let make = ({@as("open") _open, @as("type") _type, _}: props<_, string>) => React.string(_open)
   let make = {
     let \"MangleKeyword$C4A0" = (props: props<_>) => make(props)

res_syntax/tests/ppx/react/expected/nested.res.txt

@@ -0,0 +1,21 @@
+module Outer = {
+  type props = {}
+  let make = (_: props) => {
+    module Inner = {
+      type props = {}
+
+      let make = (_: props) => ReactDOM.jsx("div", {})
+      let make = {
+        let \"Nested$Outer" = props => make(props)
+
+        \"Nested$Outer"
+      }
+    }
+
+    React.jsx(Inner.make, {})
+  }
+  let make = {
+    let \"Nested$Outer" = props => make(props)
+    \"Nested$Outer"
+  }
+}

res_syntax/tests/ppx/react/expected/newtype.res.txt

@@ -26,7 +26,6 @@ module V3 = {
 module V4C = {
   type props<'a, 'b, 'c> = {a: 'a, b: 'b, c: 'c}
 
-  @react.component
   let make = (type a, {a, b, c, _}: props<a, array<option<[#Foo(a)]>>, 'a>) =>
     ReactDOM.createDOMElementVariadic("div", [])
   let make = {
@@ -41,7 +40,6 @@ module V4C = {
 module V4A = {
   type props<'a, 'b, 'c> = {a: 'a, b: 'b, c: 'c}
 
-  @react.component
   let make = (type a, {a, b, c, _}: props<a, array<option<[#Foo(a)]>>, 'a>) =>
     ReactDOM.jsx("div", {})
   let make = {
@@ -54,7 +52,6 @@ module V4A = {
 module V4A1 = {
   type props<'a, 'b, 'c> = {a: 'a, b: 'b, c: 'c}
 
-  @react.component
   let make = (type x y, {a, b, c, _}: props<x, array<y>, 'a>) => ReactDOM.jsx("div", {})
   let make = {
     let \"Newtype$V4A1" = (props: props<_>) => make(props)
@@ -70,7 +67,6 @@ module type T = {
 module V4A2 = {
   type props<'foo> = {foo: 'foo}
 
-  @react.component
   let make = (type a, {foo, _}: props<module(T with type t = a)>) => {
     module T = unpack(foo)
     ReactDOM.jsx("div", {})
@@ -85,7 +81,6 @@ module V4A2 = {
 module V4A3 = {
   type props<'foo> = {foo: 'foo}
 
-  @react.component
   let make = (type a, {foo, _}: props<_>) => {
     module T = unpack(foo: T with type t = a)
     foo