From 1e0f307735865461410867018f7236ae80e999a6 Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 01:23:27 +0900 Subject: [PATCH 1/8] Fix issue overlapping argument with default value --- cli/reactjs_jsx_v4.ml | 58 ++++++++++++------- tests/ppx/react/defaultValueProp.res | 9 +++ tests/ppx/react/expected/aliasProps.res.txt | 10 ++-- .../react/expected/defaultValueProp.res.txt | 40 +++++++++++++ .../ppx/react/expected/uncurriedProps.res.txt | 8 +-- 5 files changed, 95 insertions(+), 30 deletions(-) create mode 100644 tests/ppx/react/defaultValueProp.res create mode 100644 tests/ppx/react/expected/defaultValueProp.res.txt diff --git a/cli/reactjs_jsx_v4.ml b/cli/reactjs_jsx_v4.ml index b4c9ed61..cad85ec8 100644 --- a/cli/reactjs_jsx_v4.ml +++ b/cli/reactjs_jsx_v4.ml @@ -729,10 +729,10 @@ let argToType ~newtypes ~(typeConstraints : core_type option) types :: types | _ -> types -let argWithDefaultValue (name, default, _, _, _, _) = - match default with - | Some default when isOptional name -> Some (getLabel name, default) - | _ -> None +let hasDefaultValue nameArgList = + nameArgList + |> List.exists (fun (name, default, _, _, _, _) -> + Option.is_some default && isOptional name) let argToConcreteType types (name, attrs, loc, type_) = match name with @@ -1016,26 +1016,34 @@ let transformStructureItem ~config mapper item = (argToType ~newtypes ~typeConstraints) [] namedArgList in - let namedArgWithDefaultValueList = - List.filter_map argWithDefaultValue namedArgList + let vbMatch (name, default, _, _, _, _) = + let label = getLabel name in + match default with + | Some default -> + Vb.mk + (Pat.var (Location.mknoloc label)) + (Exp.match_ + (Exp.ident + {txt = Ldot (Lident "props", label); 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; + ]) + | None -> + Vb.mk + (Pat.var (Location.mknoloc label)) + (Exp.ident @@ Location.mknoloc @@ Ldot (Lident "props", label)) in - let vbMatch (label, default) = - Vb.mk - (Pat.var (Location.mknoloc label)) - (Exp.match_ - (Exp.ident {txt = Lident label; 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; - ]) + let vbMatchList = + if hasDefaultValue namedArgList then List.map vbMatch namedArgList + else [] in - let vbMatchList = List.map vbMatch namedArgWithDefaultValueList in (* type props = { ... } *) let propsRecordType = makePropsRecordType ~coreTypeOfAttr ~typVarsOfCoreType "props" @@ -1163,11 +1171,17 @@ let transformStructureItem ~config mapper item = (fun expr (_, pattern) -> Exp.fun_ Nolabel None pattern expr) expression patternsWithNolabel in + (* ({a, b, _}: props<'a, 'b>) *) let recordPattern = match patternsWithLabel with | [] -> 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 diff --git a/tests/ppx/react/defaultValueProp.res b/tests/ppx/react/defaultValueProp.res new file mode 100644 index 00000000..8a42dad5 --- /dev/null +++ b/tests/ppx/react/defaultValueProp.res @@ -0,0 +1,9 @@ +module C0 = { + @react.component + let make = (~a=2, ~b=a*2) =>
+} + +module C1 = { + @react.component + let make = (~a=2, ~b) =>
+} diff --git a/tests/ppx/react/expected/aliasProps.res.txt b/tests/ppx/react/expected/aliasProps.res.txt index 721846d1..b6e2b1ca 100644 --- a/tests/ppx/react/expected/aliasProps.res.txt +++ b/tests/ppx/react/expected/aliasProps.res.txt @@ -4,11 +4,12 @@ module C0 = { type props<'priority, 'text> = {priority: 'priority, text?: 'text} @react.component - let make = ({priority: _, ?text, _}: props<'priority, 'text>) => { - let text = switch text { + let make = (props: props<'priority, 'text>) => { + let text = switch props.text { | Some(text) => text | None => "Test" } + and priority = props.priority React.string(text) } @@ -23,11 +24,12 @@ module C1 = { type props<'priority, 'text> = {priority: 'priority, text?: 'text} @react.component - let make = ({priority: p, ?text, _}: props<'priority, 'text>) => { - let text = switch text { + let make = (props: props<'priority, 'text>) => { + let text = switch props.text { | Some(text) => text | None => "Test" } + and priority = props.priority React.string(p ++ text) } diff --git a/tests/ppx/react/expected/defaultValueProp.res.txt b/tests/ppx/react/expected/defaultValueProp.res.txt new file mode 100644 index 00000000..bff14f94 --- /dev/null +++ b/tests/ppx/react/expected/defaultValueProp.res.txt @@ -0,0 +1,40 @@ +module C0 = { + type props<'a, 'b> = {a?: 'a, b?: 'b} + @react.component + let make = (props: props<'a, 'b>) => { + let b = switch props.b { + | Some(b) => b + | None => a * 2 + } + and a = switch props.a { + | Some(a) => a + | None => 2 + } + + ReactDOM.jsx("div", {}) + } + let make = { + let \"DefaultValueProp$C0" = (props: props<_>) => make(props) + \"DefaultValueProp$C0" + } +} + +module C1 = { + type props<'a, 'b> = {a?: 'a, b: 'b} + + @react.component + let make = (props: props<'a, 'b>) => { + let b = props.b + and a = switch props.a { + | Some(a) => a + | None => 2 + } + + ReactDOM.jsx("div", {}) + } + let make = { + let \"DefaultValueProp$C1" = (props: props<_>) => make(props) + + \"DefaultValueProp$C1" + } +} diff --git a/tests/ppx/react/expected/uncurriedProps.res.txt b/tests/ppx/react/expected/uncurriedProps.res.txt index cf50e101..2d00031d 100644 --- a/tests/ppx/react/expected/uncurriedProps.res.txt +++ b/tests/ppx/react/expected/uncurriedProps.res.txt @@ -2,8 +2,8 @@ type props<'a> = {a?: 'a} @react.component -let make = ({?a, _}: props<(. unit) => unit>) => { - let a = switch a { +let make = (props: props<(. unit) => unit>) => { + let a = switch props.a { | Some(a) => a | None => (. ()) => () } @@ -30,8 +30,8 @@ module Foo = { type props<'callback> = {callback?: 'callback} @react.component - let make = ({?callback, _}: props<(. string, bool, bool) => unit>) => { - let callback = switch callback { + let make = (props: props<(. string, bool, bool) => unit>) => { + let callback = switch props.callback { | Some(callback) => callback | None => (. _, _, _) => () } From 6f6928407ceefc7100511b6b6150ff8ee25a879b Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 01:31:38 +0900 Subject: [PATCH 2/8] add loc to pattern --- cli/reactjs_jsx_v4.ml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/reactjs_jsx_v4.ml b/cli/reactjs_jsx_v4.ml index cad85ec8..73384740 100644 --- a/cli/reactjs_jsx_v4.ml +++ b/cli/reactjs_jsx_v4.ml @@ -1016,12 +1016,12 @@ let transformStructureItem ~config mapper item = (argToType ~newtypes ~typeConstraints) [] namedArgList in - let vbMatch (name, default, _, _, _, _) = + let vbMatch (name, default, {ppat_loc}, _, _, _) = let label = getLabel name in match default with | Some default -> Vb.mk - (Pat.var (Location.mknoloc label)) + (Pat.var (Location.mkloc label ppat_loc)) (Exp.match_ (Exp.ident {txt = Ldot (Lident "props", label); loc = Location.none}) @@ -1037,7 +1037,7 @@ let transformStructureItem ~config mapper item = ]) | None -> Vb.mk - (Pat.var (Location.mknoloc label)) + (Pat.var (Location.mkloc label ppat_loc)) (Exp.ident @@ Location.mknoloc @@ Ldot (Lident "props", label)) in let vbMatchList = From 74e71b16c7f21e7d2cf4f97b167b8bc019b452e1 Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 02:16:33 +0900 Subject: [PATCH 3/8] fix alias props with default value --- cli/reactjs_jsx_v4.ml | 6 +++--- tests/ppx/react/aliasProps.res | 5 +++++ tests/ppx/react/expected/aliasProps.res.txt | 23 +++++++++++++++++++-- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/cli/reactjs_jsx_v4.ml b/cli/reactjs_jsx_v4.ml index 73384740..ceafab4f 100644 --- a/cli/reactjs_jsx_v4.ml +++ b/cli/reactjs_jsx_v4.ml @@ -1016,12 +1016,12 @@ let transformStructureItem ~config mapper item = (argToType ~newtypes ~typeConstraints) [] namedArgList in - let vbMatch (name, default, {ppat_loc}, _, _, _) = + let vbMatch (name, default, _, alias, loc, _) = let label = getLabel name in match default with | Some default -> Vb.mk - (Pat.var (Location.mkloc label ppat_loc)) + (Pat.var (Location.mkloc alias loc)) (Exp.match_ (Exp.ident {txt = Ldot (Lident "props", label); loc = Location.none}) @@ -1037,7 +1037,7 @@ let transformStructureItem ~config mapper item = ]) | None -> Vb.mk - (Pat.var (Location.mkloc label ppat_loc)) + (Pat.var (Location.mkloc alias loc)) (Exp.ident @@ Location.mknoloc @@ Ldot (Lident "props", label)) in let vbMatchList = diff --git a/tests/ppx/react/aliasProps.res b/tests/ppx/react/aliasProps.res index 71734f50..4feda2e8 100644 --- a/tests/ppx/react/aliasProps.res +++ b/tests/ppx/react/aliasProps.res @@ -9,3 +9,8 @@ module C1 = { @react.component let make = (~priority as p, ~text="Test") => React.string(p ++ text) } + +module C2 = { + @react.component + let make = (~foo as bar="") =>
+} \ No newline at end of file diff --git a/tests/ppx/react/expected/aliasProps.res.txt b/tests/ppx/react/expected/aliasProps.res.txt index b6e2b1ca..c9e3136a 100644 --- a/tests/ppx/react/expected/aliasProps.res.txt +++ b/tests/ppx/react/expected/aliasProps.res.txt @@ -9,7 +9,7 @@ module C0 = { | Some(text) => text | None => "Test" } - and priority = props.priority + and _ = props.priority React.string(text) } @@ -29,7 +29,7 @@ module C1 = { | Some(text) => text | None => "Test" } - and priority = props.priority + and p = props.priority React.string(p ++ text) } @@ -39,3 +39,22 @@ module C1 = { \"AliasProps$C1" } } + +module C2 = { + type props<'foo> = {foo?: 'foo} + + @react.component + let make = (props: props<'foo>) => { + let bar = switch props.foo { + | Some(foo) => foo + | None => "" + } + + ReactDOM.jsx("div", {}) + } + let make = { + let \"AliasProps$C2" = (props: props<_>) => make(props) + + \"AliasProps$C2" + } +} From a229095a132a629ed5b9109eb7c9b821dbf60826 Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 02:24:44 +0900 Subject: [PATCH 4/8] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2717505f..e1a901c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,8 @@ - Fix issue where uncurried functions were incorrectly converting the type of a prop given as a default value to curried https://github.com/rescript-lang/syntax/pull/731 - Fix issue with printing async functions with locally abstract types https://github.com/rescript-lang/syntax/pull/732 - Fix support for recursive components in JSX V4 https://github.com/rescript-lang/syntax/pull/733 +- Fix issue with overlapping labelled argument with default value https://github.com/rescript-lang/syntax/pull/734 +- Fix issue with using alias and default value together https://github.com/rescript-lang/syntax/pull/734 #### :eyeglasses: Spec Compliance From d0fe971394053e5a27fb317bc8a3be9d448dc04a Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 11:13:44 +0900 Subject: [PATCH 5/8] fix build error with Pexp_field --- cli/reactjs_jsx_v4.ml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cli/reactjs_jsx_v4.ml b/cli/reactjs_jsx_v4.ml index ceafab4f..2b3401c7 100644 --- a/cli/reactjs_jsx_v4.ml +++ b/cli/reactjs_jsx_v4.ml @@ -1023,8 +1023,9 @@ let transformStructureItem ~config mapper item = Vb.mk (Pat.var (Location.mkloc alias loc)) (Exp.match_ - (Exp.ident - {txt = Ldot (Lident "props", label); loc = Location.none}) + (Exp.field + (Exp.ident {txt = Lident "props"; loc = Location.none}) + (Location.mknoloc @@ Lident label)) [ Exp.case (Pat.construct @@ -1038,7 +1039,9 @@ let transformStructureItem ~config mapper item = | None -> Vb.mk (Pat.var (Location.mkloc alias loc)) - (Exp.ident @@ Location.mknoloc @@ Ldot (Lident "props", label)) + (Exp.field + (Exp.ident {txt = Lident "props"; loc = Location.none}) + (Location.mknoloc @@ Lident label)) in let vbMatchList = if hasDefaultValue namedArgList then List.map vbMatch namedArgList From 6a01cabf1b8f82a0e70ede6ec02ada45efaf9249 Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 11:15:35 +0900 Subject: [PATCH 6/8] fix tests --- tests/ppx/react/aliasProps.res | 4 ++-- tests/ppx/react/defaultValueProp.res | 5 +++-- tests/ppx/react/expected/aliasProps.res.txt | 2 +- tests/ppx/react/expected/defaultValueProp.res.txt | 7 +++++-- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/ppx/react/aliasProps.res b/tests/ppx/react/aliasProps.res index 4feda2e8..c6951703 100644 --- a/tests/ppx/react/aliasProps.res +++ b/tests/ppx/react/aliasProps.res @@ -12,5 +12,5 @@ module C1 = { module C2 = { @react.component - let make = (~foo as bar="") =>
-} \ No newline at end of file + let make = (~foo as bar="") => React.string(bar) +} diff --git a/tests/ppx/react/defaultValueProp.res b/tests/ppx/react/defaultValueProp.res index 8a42dad5..d9f1cc85 100644 --- a/tests/ppx/react/defaultValueProp.res +++ b/tests/ppx/react/defaultValueProp.res @@ -1,9 +1,10 @@ module C0 = { + let a = 1 @react.component - let make = (~a=2, ~b=a*2) =>
+ let make = (~a=2, ~b=a*2) => React.int(a + b) } module C1 = { @react.component - let make = (~a=2, ~b) =>
+ let make = (~a=2, ~b) => React.int(a + b) } diff --git a/tests/ppx/react/expected/aliasProps.res.txt b/tests/ppx/react/expected/aliasProps.res.txt index c9e3136a..319f6a83 100644 --- a/tests/ppx/react/expected/aliasProps.res.txt +++ b/tests/ppx/react/expected/aliasProps.res.txt @@ -50,7 +50,7 @@ module C2 = { | None => "" } - ReactDOM.jsx("div", {}) + React.string(bar) } let make = { let \"AliasProps$C2" = (props: props<_>) => make(props) diff --git a/tests/ppx/react/expected/defaultValueProp.res.txt b/tests/ppx/react/expected/defaultValueProp.res.txt index bff14f94..d0bbe310 100644 --- a/tests/ppx/react/expected/defaultValueProp.res.txt +++ b/tests/ppx/react/expected/defaultValueProp.res.txt @@ -1,5 +1,7 @@ module C0 = { + let a = 1 type props<'a, 'b> = {a?: 'a, b?: 'b} + @react.component let make = (props: props<'a, 'b>) => { let b = switch props.b { @@ -11,10 +13,11 @@ module C0 = { | None => 2 } - ReactDOM.jsx("div", {}) + React.int(a + b) } let make = { let \"DefaultValueProp$C0" = (props: props<_>) => make(props) + \"DefaultValueProp$C0" } } @@ -30,7 +33,7 @@ module C1 = { | None => 2 } - ReactDOM.jsx("div", {}) + React.int(a + b) } let make = { let \"DefaultValueProp$C1" = (props: props<_>) => make(props) From 0acade4334978fde98108f29f5390b9a4c53f1b9 Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 22:49:15 +0900 Subject: [PATCH 7/8] fix handling default value of props --- cli/reactjs_jsx_v4.ml | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/cli/reactjs_jsx_v4.ml b/cli/reactjs_jsx_v4.ml index 2b3401c7..c53a467d 100644 --- a/cli/reactjs_jsx_v4.ml +++ b/cli/reactjs_jsx_v4.ml @@ -1043,9 +1043,15 @@ let transformStructureItem ~config mapper item = (Exp.ident {txt = Lident "props"; loc = Location.none}) (Location.mknoloc @@ Lident label)) in - let vbMatchList = - if hasDefaultValue namedArgList then List.map vbMatch namedArgList - else [] + 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) + in + aux (List.rev namedArgList) in (* type props = { ... } *) let propsRecordType = @@ -1165,8 +1171,9 @@ let transformStructureItem ~config mapper item = in (* add pattern matching for optional prop value *) let expression = - if List.length vbMatchList = 0 then expression - else Exp.let_ Nonrecursive vbMatchList expression + if hasDefaultValue namedArgList then + vbMatchExpr namedArgList expression + else expression in (* (ref) => expr *) let expression = From 5319f73628bcc228cd3174fede857fea2314a950 Mon Sep 17 00:00:00 2001 From: Woonki Moon Date: Wed, 8 Feb 2023 22:49:21 +0900 Subject: [PATCH 8/8] fix tests --- tests/ppx/react/defaultValueProp.res | 1 - tests/ppx/react/expected/aliasProps.res.txt | 4 ++-- tests/ppx/react/expected/defaultValueProp.res.txt | 15 ++++++--------- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/ppx/react/defaultValueProp.res b/tests/ppx/react/defaultValueProp.res index d9f1cc85..7764dd8a 100644 --- a/tests/ppx/react/defaultValueProp.res +++ b/tests/ppx/react/defaultValueProp.res @@ -1,5 +1,4 @@ module C0 = { - let a = 1 @react.component let make = (~a=2, ~b=a*2) => React.int(a + b) } diff --git a/tests/ppx/react/expected/aliasProps.res.txt b/tests/ppx/react/expected/aliasProps.res.txt index 319f6a83..ece2f660 100644 --- a/tests/ppx/react/expected/aliasProps.res.txt +++ b/tests/ppx/react/expected/aliasProps.res.txt @@ -5,11 +5,11 @@ module C0 = { @react.component let make = (props: props<'priority, 'text>) => { + let _ = props.priority let text = switch props.text { | Some(text) => text | None => "Test" } - and _ = props.priority React.string(text) } @@ -25,11 +25,11 @@ module C1 = { @react.component let make = (props: props<'priority, 'text>) => { + let p = props.priority let text = switch props.text { | Some(text) => text | None => "Test" } - and p = props.priority React.string(p ++ text) } diff --git a/tests/ppx/react/expected/defaultValueProp.res.txt b/tests/ppx/react/expected/defaultValueProp.res.txt index d0bbe310..dc8f002c 100644 --- a/tests/ppx/react/expected/defaultValueProp.res.txt +++ b/tests/ppx/react/expected/defaultValueProp.res.txt @@ -1,23 +1,20 @@ module C0 = { - let a = 1 type props<'a, 'b> = {a?: 'a, b?: 'b} - @react.component let make = (props: props<'a, 'b>) => { + let a = switch props.a { + | Some(a) => a + | None => 2 + } let b = switch props.b { | Some(b) => b | None => a * 2 } - and a = switch props.a { - | Some(a) => a - | None => 2 - } React.int(a + b) } let make = { let \"DefaultValueProp$C0" = (props: props<_>) => make(props) - \"DefaultValueProp$C0" } } @@ -27,11 +24,11 @@ module C1 = { @react.component let make = (props: props<'a, 'b>) => { - let b = props.b - and a = switch props.a { + let a = switch props.a { | Some(a) => a | None => 2 } + let b = props.b React.int(a + b) }