Improve code generated for default arguments in JSX V4 (#6041)

* Improve code generated for default arguments in JSX V4

This
```res
@react.component
let foo = (~x, ~y=3+x, ?z) => ...
```

was generating

```res
({props: props<_, _, _>) => {
  let x = props.x
  let y = switch props.y {
  | None => 3 + x
  | Some(y) => y
  }
  let z = props.z
...
```

now it generates instead
```
({x, ?y, ?z}: props<_, _, _>) => {
  let x = x
  let y = switch y {
  | None => 3 + x
  | Some(y) => y
  }
  let z = z
...
```

the different in generated code is that bindings such as `let x = props.x` are not present in the `js` output.

* Don't emit code for `as`.

* Fix issue with `as module(...)`

Functions of the form `(~comp as module(Comp: Comp))=>` are represented internally as a `Pexp_constraint`, which makes them look like `~comp : t`.
Now special-case this pattern so it's treated just like the other forms of `as ...`.

Fixes #5976

Author: cristianoc

CHANGELOG.md

@@ -76,6 +76,8 @@ These are only breaking changes for unformatted code.
 - 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
 - Fix issue where generic compare on `float` values would be different from the compare for type `float` https://github.com/rescript-lang/rescript-compiler/pull/6042
+- Improve code generated for default arguments in JSX V4 https://github.com/rescript-lang/rescript-compiler/pull/6041
+- Fix issue with JSX V4 props of the form `~p as module(...)` https://github.com/rescript-lang/rescript-compiler/pull/6041
 
 #### :nail_care: Polish
 

docs/JSXV4.md

@@ -264,16 +264,16 @@ let make = React.forwardRef({
 ### Transformation for Component Definition
 
 ```rescript
-@react.component (~x, ~y=3+x, ?z) => body
+@react.component (~x, ~y=3+x, ~z=?) => body
 ```
 
 is transformed to
 
 ```rescript
 type props<'x, 'y, 'z> = {x: 'x, y?: 'y, z?: 'z}
 
-({x, y, z}: props<_>) => {
-  let y = switch props.y {
+({x, ?y, ?z}: props<_, _, _>) => {
+  let y = switch y {
   | None => 3 + x
   | Some(y) => y
   }

jscomp/test/alias_default_value_test.js

@@ -2,9 +2,9 @@
 
 
 function Alias_default_value_test$C0(props) {
+  var b = props.b;
   var a = props.a;
   var a$1 = a !== undefined ? a : 2;
-  var b = props.b;
   var b$1 = b !== undefined ? b : (a$1 << 1);
   return a$1 + b$1 | 0;
 }
@@ -14,9 +14,9 @@ var C0 = {
 };
 
 function Alias_default_value_test$C1(props) {
-  var foo = props.foo;
-  if (foo !== undefined) {
-    return foo;
+  var bar = props.foo;
+  if (bar !== undefined) {
+    return bar;
   } else {
     return "";
   }
@@ -26,6 +26,51 @@ var C1 = {
   make: Alias_default_value_test$C1
 };
 
+function Alias_default_value_test$C2(props) {
+  var a = props.a;
+  var bar = props.foo;
+  var bar$1 = bar !== undefined ? bar : "";
+  var a$1 = a !== undefined ? a : bar$1;
+  return bar$1 + a$1 + props.b;
+}
+
+var C2 = {
+  make: Alias_default_value_test$C2
+};
+
+function Alias_default_value_test$C3(props) {
+  var text = props.text;
+  if (text !== undefined) {
+    return text;
+  } else {
+    return "Test";
+  }
+}
+
+var C3 = {
+  make: Alias_default_value_test$C3
+};
+
+function Alias_default_value_test$C4(props) {
+  return props.a;
+}
+
+var C4 = {
+  make: Alias_default_value_test$C4
+};
+
+function Alias_default_value_test$C6(props) {
+  return props.comp.xx;
+}
+
+var C6 = {
+  make: Alias_default_value_test$C6
+};
+
 exports.C0 = C0;
 exports.C1 = C1;
+exports.C2 = C2;
+exports.C3 = C3;
+exports.C4 = C4;
+exports.C6 = C6;
 /* No side effect */

jscomp/test/alias_default_value_test.res

@@ -15,3 +15,31 @@ module C1 = {
     React.string(bar)
   }
 }
+
+module C2 = {
+  @react.component
+  let make = (~foo as bar="", ~a=bar, ~b) => {
+    React.string(bar ++ a ++ b)
+  }
+}
+
+module C3 = {
+  @react.component
+  let make = (~priority as _, ~text="Test") => React.string(text)
+}
+
+module C4 = {
+  @react.component
+  let make = (~a as b, ~x=true) => b
+}
+
+module C6 = {
+  module type Comp = {
+    let xx : int
+    @react.component
+    let make: unit => React.element
+  }
+
+  @react.component
+  let make = (~comp as module(Comp: Comp), ~x as (a, b)) => Comp.xx
+}

res_syntax/src/reactjs_jsx_v4.ml

@@ -640,6 +640,7 @@ let rec recursivelyTransformNamedArgsForMake expr args newtypes coreType =
     in
     let type_ =
       match pattern with
+      | {ppat_desc = Ppat_constraint (_, {ptyp_desc = Ptyp_package _})} -> None
       | {ppat_desc = Ppat_constraint (_, type_)} -> Some type_
       | _ -> None
     in
@@ -843,39 +844,34 @@ let modifiedBinding ~bindingLoc ~bindingPatLoc ~fnName binding =
   in
   (wrapExpressionWithBinding wrapExpression, hasForwardRef, expression)
 
-let vbMatch (name, default, _, alias, loc, _) =
+let vbMatch ~expr (name, default, _, alias, loc, _) =
   let label = getLabel name in
   match default with
   | Some default ->
-    Vb.mk
-      (Pat.var (Location.mkloc alias loc))
-      (Exp.match_
-         (Exp.field
-            (Exp.ident {txt = Lident "props"; loc = Location.none})
-            (Location.mknoloc @@ Lident label))
-         [
-           Exp.case
-             (Pat.construct
-                (Location.mknoloc @@ Lident "Some")
-                (Some (Pat.var (Location.mknoloc label))))
-             (Exp.ident (Location.mknoloc @@ Lident label));
-           Exp.case
-             (Pat.construct (Location.mknoloc @@ Lident "None") None)
-             default;
-         ])
-  | None ->
-    Vb.mk
-      (Pat.var (Location.mkloc alias loc))
-      (Exp.field
-         (Exp.ident {txt = Lident "props"; loc = Location.none})
-         (Location.mknoloc @@ Lident label))
+    let value_binding =
+      Vb.mk
+        (Pat.var (Location.mkloc alias loc))
+        (Exp.match_
+           (Exp.ident {txt = Lident alias; loc = Location.none})
+           [
+             Exp.case
+               (Pat.construct
+                  (Location.mknoloc @@ Lident "Some")
+                  (Some (Pat.var (Location.mknoloc label))))
+               (Exp.ident (Location.mknoloc @@ Lident label));
+             Exp.case
+               (Pat.construct (Location.mknoloc @@ Lident "None") None)
+               default;
+           ])
+    in
+    Exp.let_ Nonrecursive [value_binding] expr
+  | None -> expr
 
 let vbMatchExpr namedArgList expr =
   let rec aux namedArgList =
     match namedArgList with
     | [] -> expr
-    | [namedArg] -> Exp.let_ Nonrecursive [vbMatch namedArg] expr
-    | namedArg :: rest -> Exp.let_ Nonrecursive [vbMatch namedArg] (aux rest)
+    | namedArg :: rest -> vbMatch namedArg ~expr:(aux rest)
   in
   aux (List.rev namedArgList)
 
@@ -970,11 +966,10 @@ let mapBinding ~config ~emptyLoc ~pstr_loc ~fileName ~recFlag binding =
     in
     let rec stripConstraintUnpack ~label pattern =
       match pattern with
+      | {ppat_desc = Ppat_constraint (_, {ptyp_desc = Ptyp_package _})} ->
+        pattern
       | {ppat_desc = Ppat_constraint (pattern, _)} ->
         stripConstraintUnpack ~label pattern
-      | {ppat_desc = Ppat_unpack _; ppat_loc} ->
-        (* remove unpack e.g. model: module(T) *)
-        Pat.var ~loc:ppat_loc {txt = label; loc = ppat_loc}
       | _ -> pattern
     in
     let rec returnedExpression patternsWithLabel patternsWithNolabel
@@ -1043,11 +1038,6 @@ let mapBinding ~config ~emptyLoc ~pstr_loc ~fileName ~recFlag binding =
       | [] -> Pat.any ()
       | _ -> Pat.record (List.rev patternsWithLabel) Open
     in
-    let recordPattern =
-      if hasDefaultValue namedArgList then
-        Pat.var {txt = "props"; loc = emptyLoc}
-      else recordPattern
-    in
     let expression =
       Exp.fun_ Nolabel None
         (Pat.constraint_ recordPattern

res_syntax/tests/ppx/react/aliasProps.res

@@ -14,3 +14,30 @@ module C2 = {
   @react.component
   let make = (~foo as bar="") => React.string(bar)
 }
+
+module C3 = {
+  @react.component
+  let make = (~foo as bar="", ~a=bar, ~b) => {
+    React.string(bar ++ a ++ b)
+  }
+}
+
+module C4 = {
+  @react.component
+  let make = (~a as b, ~x=true) => <div> b </div>
+}
+
+module C5 = {
+  @react.component
+  let make = (~a as (x, y), ~z=3) => x + y + z
+}
+
+module C6 = {
+  module type Comp = {
+    @react.component
+    let make: unit => React.element
+  }
+
+  @react.component
+  let make = (~comp as module(Comp: Comp), ~x as (a, b)) => <Comp />
+}

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

@@ -3,9 +3,8 @@
 module C0 = {
   type props<'priority, 'text> = {priority: 'priority, text?: 'text}
 
-  let make = (props: props<_, _>) => {
-    let _ = props.priority
-    let text = switch props.text {
+  let make = ({priority: _, ?text, _}: props<_, _>) => {
+    let text = switch text {
     | Some(text) => text
     | None => "Test"
     }
@@ -22,9 +21,8 @@ module C0 = {
 module C1 = {
   type props<'priority, 'text> = {priority: 'priority, text?: 'text}
 
-  let make = (props: props<_, _>) => {
-    let p = props.priority
-    let text = switch props.text {
+  let make = ({priority: p, ?text, _}: props<_, _>) => {
+    let text = switch text {
     | Some(text) => text
     | None => "Test"
     }
@@ -41,8 +39,8 @@ module C1 = {
 module C2 = {
   type props<'foo> = {foo?: 'foo}
 
-  let make = (props: props<_>) => {
-    let bar = switch props.foo {
+  let make = ({foo: ?bar, _}: props<_>) => {
+    let bar = switch bar {
     | Some(foo) => foo
     | None => ""
     }
@@ -55,3 +53,79 @@ module C2 = {
     \"AliasProps$C2"
   }
 }
+
+module C3 = {
+  type props<'foo, 'a, 'b> = {foo?: 'foo, a?: 'a, b: 'b}
+
+  let make = ({foo: ?bar, ?a, b, _}: props<_, _, _>) => {
+    let bar = switch bar {
+    | Some(foo) => foo
+    | None => ""
+    }
+    let a = switch a {
+    | Some(a) => a
+    | None => bar
+    }
+
+    {
+      React.string(bar ++ a ++ b)
+    }
+  }
+  let make = {
+    let \"AliasProps$C3" = (props: props<_>) => make(props)
+
+    \"AliasProps$C3"
+  }
+}
+
+module C4 = {
+  type props<'a, 'x> = {a: 'a, x?: 'x}
+
+  let make = ({a: b, ?x, _}: props<_, _>) => {
+    let x = switch x {
+    | Some(x) => x
+    | None => true
+    }
+
+    ReactDOM.jsx("div", {children: ?ReactDOM.someElement(b)})
+  }
+  let make = {
+    let \"AliasProps$C4" = (props: props<_>) => make(props)
+
+    \"AliasProps$C4"
+  }
+}
+
+module C5 = {
+  type props<'a, 'z> = {a: 'a, z?: 'z}
+
+  let make = ({a: (x, y), ?z, _}: props<_, _>) => {
+    let z = switch z {
+    | Some(z) => z
+    | None => 3
+    }
+
+    x + y + z
+  }
+  let make = {
+    let \"AliasProps$C5" = (props: props<_>) => make(props)
+
+    \"AliasProps$C5"
+  }
+}
+
+module C6 = {
+  module type Comp = {
+    type props = {}
+
+    let make: React.componentLike<props, React.element>
+  }
+  type props<'comp, 'x> = {comp: 'comp, x: 'x}
+
+  let make = ({comp: module(Comp: Comp), x: (a, b), _}: props<_, _>) => React.jsx(Comp.make, {})
+  let make = {
+    let \"AliasProps$C6" = (props: props<_>) => make(props)
+
+    \"AliasProps$C6"
+  }
+}