Skip to content

Fix issues with overlapping argument name and default value, using alias and default value together #5989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Feb 8, 2023
Merged

Fix issues with overlapping argument name and default value, using alias and default value together #5989

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c673fc8
test result diff
mununki Feb 7, 2023
a415dbe
add tests
mununki Feb 7, 2023
9916513
fix issue overlapping argument with default value
mununki Feb 7, 2023
6abbae7
fix build error with Pexp_field
mununki Feb 8, 2023
875834c
relocate tests
mununki Feb 8, 2023
ec49a4b
fix test
mununki Feb 8, 2023
edcf127
fix handling default value
mununki Feb 8, 2023
6c0a0bb
fix tests
mununki Feb 8, 2023
ba92bfa
update changelog
mununki Feb 8, 2023
d36ce45
fix tests
mununki Feb 8, 2023
91ead84
fix tests
mununki Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions jscomp/build_tests/react_ppx/src/recursive_component_test.bs.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions jscomp/test/alias_default_value_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';


function Alias_default_value_test$C0(props) {
var b = props.b;
var b$1 = b !== undefined ? b : 2;
var a = props.a;
var a$1 = a !== undefined ? a : 2;
return a$1 + b$1 | 0;
}

var C0 = {
a: 1,
make: Alias_default_value_test$C0
};

function Alias_default_value_test$C1(props) {
var foo = props.foo;
if (foo !== undefined) {
return foo;
} else {
return "";
}
}

var C1 = {
make: Alias_default_value_test$C1
};

exports.C0 = C0;
exports.C1 = C1;
/* No side effect */
18 changes: 18 additions & 0 deletions jscomp/test/alias_default_value_test.res
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
@@bs.config({
flags: ["-bs-jsx", "4"],
})

module C0 = {
let a = 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this line. as it's not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it 6c0a0bb

@react.component
let make = (~a=2, ~b=a * 2) => {
React.int(a + b)
}
}

module C1 = {
@react.component
let make = (~foo as bar="") => {
React.string(bar)
}
}
3 changes: 2 additions & 1 deletion jscomp/test/build.ninja
View file
Open in desktop

Large diffs are not rendered by default.

61 changes: 39 additions & 22 deletions res_syntax/src/reactjs_jsx_v4.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1037,26 +1037,37 @@ let transformStructureItem ~config mapper item =
(argToType ~newtypes ~typeConstraints)
[] namedArgList
in
let namedArgWithDefaultValueList =
List.filter_map argWithDefaultValue namedArgList
let vbMatch (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))
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"
Expand Down Expand Up @@ -1184,11 +1195,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
Expand Down
5 changes: 5 additions & 0 deletions res_syntax/tests/ppx/react/aliasProps.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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="") => React.string(bar)
}
10 changes: 10 additions & 0 deletions res_syntax/tests/ppx/react/defaultValueProp.res
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module C0 = {
let a = 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this line, as it's not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it 6c0a0bb

@react.component
let make = (~a=2, ~b=a*2) => React.int(a + b)
}

module C1 = {
@react.component
let make = (~a=2, ~b) => React.int(a + b)
}
29 changes: 25 additions & 4 deletions res_syntax/tests/ppx/react/expected/aliasProps.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 _ = props.priority

React.string(text)
}
Expand All @@ -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 p = props.priority

React.string(p ++ text)
}
Expand All @@ -37,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 => ""
}

React.string(bar)
}
let make = {
let \"AliasProps$C2" = (props: props<_>) => make(props)

\"AliasProps$C2"
}
}
43 changes: 43 additions & 0 deletions res_syntax/tests/ppx/react/expected/defaultValueProp.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the error message I'm getting "The module or file props can't be found." this is probably a path A.foo where A happens to be props and not a field access, which it should be.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field access is Pexp_field.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Correct.

| Some(b) => b
| None => a * 2
Copy link
Collaborator

@cristianoc cristianoc Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you copy paste this, you'll get a a not found.
The oder should be: first let a then b.
Also, it should not be let ... and but let ... let or it won't compile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, a should refer to the outside scope of make function. The original code is this:

@react.component
let make = (~a=2, ~b=a*2) => <div />

So, a in (a* 2) is referring to out of this make function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite tricky than I guess. It has to be affected by the order, but it should not.

let make = (props) => {
  let a = 2 // 1
  let b = a * 2 // <- a should be reference to the outside of make function, but it refers to 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We transform the optional labelled argument to nolabel argument props. So there no where to handle the default value that is the root cause of this issue, I guess.

}
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"
}
}

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
}

React.int(a + b)
}
let make = {
let \"DefaultValueProp$C1" = (props: props<_>) => make(props)

\"DefaultValueProp$C1"
}
}
8 changes: 4 additions & 4 deletions res_syntax/tests/ppx/react/expected/uncurriedProps.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => (. ()) => ()
}
Expand All @@ -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 => (. _, _, _) => ()
}
Expand Down