Skip to content

Pass existing location to children prop #7540

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.

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
- Fix `index out of bounds` exception thrown in rare cases by `rescript-editor-analysis.exe codeAction` command. https://github.com/rescript-lang/rescript/pull/7523
- Don't produce duplicate type definitions for recursive types on hover. https://github.com/rescript-lang/rescript/pull/7524
- Prop punning when types don't match results in I/O error: _none_: No such file or directory. https://github.com/rescript-lang/rescript/pull/7533
- Pass location to children prop in jsx ppx. https://github.com/rescript-lang/rescript/pull/7540

#### :nail_care: Polish

Expand Down
20 changes: 13 additions & 7 deletions compiler/syntax/src/jsx_v4.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1208,7 +1208,7 @@ let append_children_prop (config : Jsx_common.jsx_config) mapper
| "react" -> Lident "ReactDOM"
| _generic -> module_access_name config "Elements"
in
Exp.apply
Exp.apply ~loc:child.pexp_loc
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this change is responsible for the changes in the dead code analysis. But this change does not seem necessary to fix the location issue in error messages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have an explanation as to why this would happen. More non-dummy locations should lead to more value dependencies found in the dead code analysis, which just traverses the typed ast.
Unless, somehow, this change affects the locations produced during type inference.

(Exp.ident
{txt = Ldot (element_binding, "someElement"); loc = Location.none})
[(Nolabel, mapper.expr mapper child)]
Expand All @@ -1220,18 +1220,24 @@ let append_children_prop (config : Jsx_common.jsx_config) mapper
in
props
@ [
JSXPropValue ({txt = "children"; loc = Location.none}, is_optional, expr);
JSXPropValue
({txt = "children"; loc = child.pexp_loc}, is_optional, expr);
]
| JSXChildrenItems xs ->
| JSXChildrenItems (head :: _ as xs) ->
let loc =
match List.rev xs with
| [] -> head.pexp_loc
| lastChild :: _ ->
{head.pexp_loc with loc_end = lastChild.pexp_loc.loc_end}
in
(* this is a hack to support react components that introspect into their children *)
props
@ [
JSXPropValue
( {txt = "children"; loc = Location.none},
( {txt = "children"; loc},
false,
Exp.apply
(Exp.ident
{txt = module_access_name config "array"; loc = Location.none})
Exp.apply ~loc
(Exp.ident {txt = module_access_name config "array"; loc})
[(Nolabel, Exp.array (List.map (mapper.expr mapper) xs))] );
]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,6 @@
addValueReference ComponentAsProp.res:12:24 --> ComponentAsProp.res:12:13
addValueReference ComponentAsProp.res:13:16 --> React.res:3:0
addValueReference ComponentAsProp.res:11:14 --> ComponentAsProp.res:6:34
addValueReference ComponentAsProp.res:9:6 --> ComponentAsProp.res:6:12
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zth, any thoughts on whether this is problematic or not?

addValueReference ComponentAsProp.res:10:6 --> ComponentAsProp.res:6:20
addValueReference ComponentAsProp.res:12:24 --> ComponentAsProp.res:12:13
addValueReference ComponentAsProp.res:13:16 --> React.res:3:0
addValueReference ComponentAsProp.res:11:14 --> ComponentAsProp.res:6:34
addValueReference ComponentAsProp.res:9:6 --> ComponentAsProp.res:6:12
addValueReference ComponentAsProp.res:10:6 --> ComponentAsProp.res:6:20
addValueReference ComponentAsProp.res:12:24 --> ComponentAsProp.res:12:13
addValueReference ComponentAsProp.res:13:16 --> React.res:3:0
addValueReference ComponentAsProp.res:11:14 --> ComponentAsProp.res:6:34
addValueReference ComponentAsProp.res:9:6 --> ComponentAsProp.res:6:12
addValueReference ComponentAsProp.res:10:6 --> ComponentAsProp.res:6:20
addValueReference ComponentAsProp.res:12:24 --> ComponentAsProp.res:12:13
addValueReference ComponentAsProp.res:13:16 --> React.res:3:0
addValueReference ComponentAsProp.res:11:14 --> ComponentAsProp.res:6:34
addTypeReference _none_:1:-1 --> ComponentAsProp.res:6:12
addTypeReference _none_:1:-1 --> ComponentAsProp.res:6:20
addTypeReference _none_:1:-1 --> ComponentAsProp.res:6:34
Expand Down Expand Up @@ -422,52 +407,17 @@
addValueReference Hooks.res:10:29 --> Hooks.res:4:12
addValueReference Hooks.res:10:75 --> Hooks.res:5:7
addValueReference Hooks.res:9:7 --> React.res:7:0
addTypeReference Hooks.res:10:29 --> Hooks.res:1:16
addValueReference Hooks.res:10:29 --> Hooks.res:4:12
addValueReference Hooks.res:10:75 --> Hooks.res:5:7
addValueReference Hooks.res:9:7 --> React.res:7:0
addValueReference Hooks.res:13:54 --> React.res:7:0
addValueReference Hooks.res:13:54 --> React.res:7:0
addValueReference Hooks.res:13:40 --> Hooks.res:5:7
addValueReference Hooks.res:13:26 --> Hooks.res:5:14
addValueReference Hooks.res:14:5 --> ImportHooks.res:13:0
addValueReference Hooks.res:15:7 --> React.res:7:0
addValueReference Hooks.res:15:32 --> React.res:7:0
addValueReference Hooks.res:15:7 --> React.res:7:0
addValueReference Hooks.res:15:32 --> React.res:7:0
addValueReference Hooks.res:14:76 --> Hooks.res:14:57
addValueReference Hooks.res:14:63 --> React.res:7:0
addValueReference Hooks.res:17:5 --> ImportHookDefault.res:6:0
addValueReference Hooks.res:19:7 --> React.res:7:0
addValueReference Hooks.res:19:32 --> React.res:7:0
addValueReference Hooks.res:19:7 --> React.res:7:0
addValueReference Hooks.res:19:32 --> React.res:7:0
addValueReference Hooks.res:18:74 --> Hooks.res:18:55
addValueReference Hooks.res:18:61 --> React.res:7:0
addTypeReference Hooks.res:10:29 --> Hooks.res:1:16
addValueReference Hooks.res:10:29 --> Hooks.res:4:12
addValueReference Hooks.res:10:75 --> Hooks.res:5:7
addValueReference Hooks.res:9:7 --> React.res:7:0
addTypeReference Hooks.res:10:29 --> Hooks.res:1:16
addValueReference Hooks.res:10:29 --> Hooks.res:4:12
addValueReference Hooks.res:10:75 --> Hooks.res:5:7
addValueReference Hooks.res:9:7 --> React.res:7:0
addValueReference Hooks.res:13:54 --> React.res:7:0
addValueReference Hooks.res:13:54 --> React.res:7:0
addValueReference Hooks.res:13:40 --> Hooks.res:5:7
addValueReference Hooks.res:13:26 --> Hooks.res:5:14
addValueReference Hooks.res:14:5 --> ImportHooks.res:13:0
addValueReference Hooks.res:15:7 --> React.res:7:0
addValueReference Hooks.res:15:32 --> React.res:7:0
addValueReference Hooks.res:15:7 --> React.res:7:0
addValueReference Hooks.res:15:32 --> React.res:7:0
addValueReference Hooks.res:14:76 --> Hooks.res:14:57
addValueReference Hooks.res:14:63 --> React.res:7:0
addValueReference Hooks.res:17:5 --> ImportHookDefault.res:6:0
addValueReference Hooks.res:19:7 --> React.res:7:0
addValueReference Hooks.res:19:32 --> React.res:7:0
addValueReference Hooks.res:19:7 --> React.res:7:0
addValueReference Hooks.res:19:32 --> React.res:7:0
addValueReference Hooks.res:18:74 --> Hooks.res:18:55
addValueReference Hooks.res:18:61 --> React.res:7:0
addTypeReference _none_:1:-1 --> Hooks.res:4:12
Expand All @@ -478,21 +428,14 @@
addTypeReference Hooks.res:29:66 --> Hooks.res:1:16
addValueReference Hooks.res:29:66 --> Hooks.res:29:14
addValueReference Hooks.res:29:34 --> React.res:7:0
addTypeReference Hooks.res:29:66 --> Hooks.res:1:16
addValueReference Hooks.res:29:66 --> Hooks.res:29:14
addValueReference Hooks.res:29:34 --> React.res:7:0
addTypeReference _none_:1:-1 --> Hooks.res:29:14
addRecordLabelDeclaration vehicle Hooks.res:33:16 path:+Hooks.Inner.Inner2.props
addRecordLabelDeclaration vehicle Hooks.res:33:16 path:+Hooks.Inner.Inner2.props
addTypeReference Hooks.res:33:68 --> Hooks.res:1:16
addValueReference Hooks.res:33:68 --> Hooks.res:33:16
addValueReference Hooks.res:33:36 --> React.res:7:0
addTypeReference Hooks.res:33:68 --> Hooks.res:1:16
addValueReference Hooks.res:33:68 --> Hooks.res:33:16
addValueReference Hooks.res:33:36 --> React.res:7:0
addTypeReference _none_:1:-1 --> Hooks.res:33:16
addValueReference Hooks.res:39:25 --> React.res:3:0
addValueReference Hooks.res:39:25 --> React.res:3:0
addTypeReference Hooks.res:47:2 --> Hooks.res:1:16
addValueReference Hooks.res:45:4 --> Hooks.res:45:31
addTypeReference Hooks.res:47:14 --> Hooks.res:1:16
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

We've found a bug for you!
/.../fixtures/missing_required_prop.res:28:7-37

26 ┆ let make = () => {
27 ┆ <Wrapper>
28 ┆ <div> {""->React.string} </div>
29 ┆ </Wrapper>
30 ┆ }

This JSX component does not accept child elements. It has no children prop <Wrapper />
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

We've found a bug for you!
/.../fixtures/missing_required_prop_when_children.res:31:7-32:37

29 ┆ let make = () => {
30 ┆ <Wrapper>
31 ┆ <button> {"yo"->React.string} </button>
32 ┆  <div> {""->React.string} </div>
33 ┆ </Wrapper>
34 ┆ }

The component <Wrapper /> is missing these required props:
value
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

We've found a bug for you!
/.../fixtures/missing_required_prop_when_single_child.res:28:7-37

26 ┆ let make = () => {
27 ┆ <Wrapper>
28 ┆ <div> {""->React.string} </div>
29 ┆ </Wrapper>
30 ┆ }

The component <Wrapper /> is missing these required props:
value
31 changes: 31 additions & 0 deletions tests/build_tests/super_errors/fixtures/missing_required_prop.res
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
module React = {
type element = Jsx.element
@val external null: element = "null"
type componentLike<'props, 'return> = Jsx.componentLike<'props, 'return>
type component<'props> = Jsx.component<'props>
external component: componentLike<'props, element> => component<'props> = "%identity"
@module("react/jsx-runtime")
external jsx: (component<'props>, 'props) => element = "jsx"
external string: string => element = "%identity"
}
module ReactDOM = {
external someElement: React.element => option<React.element> = "%identity"
@module("react/jsx-runtime")
external jsx: (string, JsxDOM.domProps) => Jsx.element = "jsx"
}

module Wrapper = {
@react.component
let make = (~value: 'value) => {
<div> {React.null} </div>
}
}

module SomeComponent = {
@react.component
let make = () => {
<Wrapper>
<div> {""->React.string} </div>
</Wrapper>
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module React = {
type element = Jsx.element
@val external null: element = "null"
type componentLike<'props, 'return> = Jsx.componentLike<'props, 'return>
type component<'props> = Jsx.component<'props>
external component: componentLike<'props, element> => component<'props> = "%identity"
@module("react/jsx-runtime")
external jsx: (component<'props>, 'props) => element = "jsx"
@module("react/jsx-runtime")
external jsxs: (component<'props>, 'props) => element = "jsxs"
external string: string => element = "%identity"
external array: array<element> => element = "%identity"
}
module ReactDOM = {
external someElement: React.element => option<React.element> = "%identity"
@module("react/jsx-runtime")
external jsx: (string, JsxDOM.domProps) => Jsx.element = "jsx"
}

module Wrapper = {
@react.component
let make = (~value: 'value, ~children: React.element) => {
<div> {children} </div>
}
}

module SomeComponent = {
@react.component
let make = () => {
<Wrapper>
<button> {"yo"->React.string} </button>
<div> {""->React.string} </div>
</Wrapper>
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
module React = {
type element = Jsx.element
@val external null: element = "null"
type componentLike<'props, 'return> = Jsx.componentLike<'props, 'return>
type component<'props> = Jsx.component<'props>
external component: componentLike<'props, element> => component<'props> = "%identity"
@module("react/jsx-runtime")
external jsx: (component<'props>, 'props) => element = "jsx"
external string: string => element = "%identity"
}
module ReactDOM = {
external someElement: React.element => option<React.element> = "%identity"
@module("react/jsx-runtime")
external jsx: (string, JsxDOM.domProps) => Jsx.element = "jsx"
}

module Wrapper = {
@react.component
let make = (~value: 'value, ~children: React.element) => {
<div> {children} </div>
}
}

module SomeComponent = {
@react.component
let make = () => {
<Wrapper>
<div> {""->React.string} </div>
</Wrapper>
}
}
14 changes: 2 additions & 12 deletions tests/syntax_tests/data/ast-mapping/expected/JSXFragments.res.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
let empty = React.jsx(React.jsxFragment, {})

let fragmentWithBracedExpresssion = React.jsx(
React.jsxFragment,
{
children: {React.int(1 + 2)},
},
)
let fragmentWithBracedExpresssion = React.jsx(React.jsxFragment, {children: {React.int(1 + 2)}})

let fragmentWithJSXElements = React.jsxs(
React.jsxFragment,
Expand All @@ -23,12 +18,7 @@ let nestedFragments = React.jsxs(
children: React.array([
ReactDOM.jsx("h1", {children: ?ReactDOM.someElement({React.string("Hi")})}),
ReactDOM.jsx("p", {children: ?ReactDOM.someElement({React.string("Hello")})}),
React.jsx(
React.jsxFragment,
{
children: {React.string("Bye")},
},
),
React.jsx(React.jsxFragment, {children: {React.string("Bye")}}),
]),
},
)
21 changes: 3 additions & 18 deletions tests/syntax_tests/data/ppx/react/expected/fragment.res.txt
Original file line number Diff line number Diff line change
@@ -1,29 +1,14 @@
@@jsxConfig({version: 4})

let _ = React.jsx(React.jsxFragment, {})
let _ = React.jsx(
React.jsxFragment,
{
children: ReactDOM.jsx("div", {}),
},
)
let _ = React.jsx(React.jsxFragment, {children: ReactDOM.jsx("div", {})})
let _ = React.jsxs(
React.jsxFragment,
{children: React.array([ReactDOM.jsx("div", {}), ReactDOM.jsx("div", {})])},
)
let _ = React.jsx(
React.jsxFragment,
{
children: React.jsx(React.jsxFragment, {}),
},
)
let _ = React.jsx(React.jsxFragment, {children: React.jsx(React.jsxFragment, {})})
let _ = React.jsx(Z.make, {})
let _ = React.jsx(
Z.make,
{
children: ReactDOM.jsx("div", {}),
},
)
let _ = React.jsx(Z.make, {children: ReactDOM.jsx("div", {})})
let _ = React.jsx(Z.make, {a: "a", children: ReactDOM.jsx("div", {})})
let _ = React.jsxs(
Z.make,
Expand Down