JSX PPX: add Jsx.element return constraint #7939
Conversation
cknitt
force-pushed
the
return-jsx-element
branch
from
5bf383c to
64dc1a2
Compare
cknitt
force-pushed
the
return-jsx-element
branch
from
64dc1a2 to
80b9809
Compare
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
| This has type: [1;31mint[0m | ||
| But it's expected to have type: [1;33mJsx.element[0m | ||
|
|
||
| In JSX, all content must be JSX elements. You can convert int to a JSX element with [1;33mReact.int[0m. No newline at end of file |
There was a problem hiding this comment.
The error messages could be further improved for this special case.
Leaving that to @zth. 🙂
| | Nonrecursive -> | ||
| Some | ||
| (make_new_binding ~loc:empty_loc ~full_module_name modified_binding) | ||
| (make_new_binding ~loc:binding_loc ~full_module_name modified_binding) |
There was a problem hiding this comment.
Not sure about this change that Codex made or about the loc handling in general.
/cc @zth
There was a problem hiding this comment.
I'd be wary of changing locs from empty to something else. Since it's used by the editor tooling. But maybe there's a reason for this change? Could you ask Codex?
There was a problem hiding this comment.
Codex says:
That change happened when I added the Jsx.element return constraint in compiler/syntax/src/jsx_v4.ml:919. The new constraint uses the loc you pass into make_new_binding, and with the old empty_loc everything was reported as coming from “line 1, char 0” of the file. Switching to binding.pvb_loc makes the generated wrapper carry the component’s real location, so any type error (or merlin focus) now points back to the actual let make definition instead of the dummy top-of-file location.
I do not see any effect of this change in the tests. When I revert it, tests still give the same result.
| {"contents": {"kind": "markdown", "value": "```rescript\nstring\n```"}} | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nJsx.element\n```\n\n---\n\n```\n \n```\n```rescript\ntype Jsx.element\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx.res%22%2C25%2C0%5D)\n"}} | ||
|
|
||
| Hover src/Hover.res 42:15 | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nstring\n```"}} | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nJsx.element\n```\n\n---\n\n```\n \n```\n```rescript\ntype Jsx.element\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx.res%22%2C25%2C0%5D)\n"}} |
There was a problem hiding this comment.
Hmmm, maybe not
@react.component
let make = (~name) => React.string(name)
// ^hov
module C2 = {
@react.component
let make2 = (~name: string) => React.string(name)
// ^hov
}
| Hover src/Jsx2.resi 1:4 | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nprops<string>\n```\n\n---\n\n```\n \n```\n```rescript\ntype props<'first> = {first: 'first}\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx2.resi%22%2C0%2C0%5D)\n"}} | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nJsx.element\n```\n\n---\n\n```\n \n```\n```rescript\ntype Jsx.element\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx.res%22%2C25%2C0%5D)\n"}} |
There was a problem hiding this comment.
Same here, is this expected? If so, why?
| module M4: { | ||
| @react.component | ||
| let make: (~first: string, ~fun: string=?, ~second: string=?) => React.element | ||
| let make: (~first: string, ~fun: string=?, ~second: string=?) => Jsx.element |
There was a problem hiding this comment.
Interesting change. I would assume we'd still want it to be React.element?
There was a problem hiding this comment.
As the return type of all Jsx component functions is now constrained to Jsx.element, it makes sense that that's also what turns up when generating the interface.
Fixes #7722