Skip to content

Add loc to each props in JSX4 #5937

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

Merged
merged 11 commits into from
Jan 14, 2023
Merged
Show file tree
Hide file tree
Changes from 8 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
4 changes: 2 additions & 2 deletions jscomp/ml/typedecl.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1844,11 +1844,11 @@ let tys_of_constr_args = function

let report_error ppf = function
| Repeated_parameter ->
fprintf ppf "A type parameter occurs several times"
fprintf ppf "A type parameter occurs several times. If this is a component, check for duplicated props."
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is not needed anymore right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, I will revert it.

| Duplicate_constructor s ->
fprintf ppf "Two constructors are named %s" s
| Duplicate_label s ->
fprintf ppf "Two labels are named %s" s
fprintf ppf "Two labels are named %s. If this is a component, check for duplicated props." s
| Recursive_abbrev s ->
fprintf ppf "The type abbreviation %s is cyclic" s
| Cycle_in_def (s, ty) ->
Expand Down
74 changes: 50 additions & 24 deletions res_syntax/src/reactjs_jsx_v4.ml
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ let recordFromProps ~loc ~removeKey callArguments =
(* let make = ({id, name, children}: props<'id, 'name, 'children>) *)
let makePropsTypeParamsTvar namedTypeList =
namedTypeList
|> List.filter_map (fun (_isOptional, label, _, _interiorType) ->
|> List.filter_map (fun (_isOptional, label, _, loc, _interiorType) ->
if label = "key" then None
else Some (Typ.var @@ safeTypeFromValue (Labelled label)))
else Some (Typ.var ~loc @@ safeTypeFromValue (Labelled label)))

let stripOption coreType =
match coreType with
Expand All @@ -268,7 +268,7 @@ let stripJsNullable coreType =
let makePropsTypeParams ?(stripExplicitOption = false)
?(stripExplicitJsNullableOfRef = false) namedTypeList =
namedTypeList
|> List.filter_map (fun (isOptional, label, _, interiorType) ->
|> List.filter_map (fun (isOptional, label, _, loc, interiorType) ->
if label = "key" then None
(* TODO: Worth thinking how about "ref_" or "_ref" usages *)
else if label = "ref" then
Expand All @@ -277,7 +277,7 @@ let makePropsTypeParams ?(stripExplicitOption = false)
For example, if JSX ppx is used for React Native, type would be different.
*)
match interiorType with
| {ptyp_desc = Ptyp_var "ref"} -> Some (refType Location.none)
| {ptyp_desc = Ptyp_var "ref"} -> Some (refType loc)
| _ ->
(* Strip explicit Js.Nullable.t in case of forwardRef *)
if stripExplicitJsNullableOfRef then stripJsNullable interiorType
Expand All @@ -287,21 +287,39 @@ let makePropsTypeParams ?(stripExplicitOption = false)
else if isOptional && stripExplicitOption then stripOption interiorType
else Some interiorType)

let makeLabelDecls ~loc namedTypeList =
namedTypeList
|> List.map (fun (isOptional, label, attrs, interiorType) ->
if label = "key" then
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc}
interiorType
else if isOptional then
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc}
(Typ.var @@ safeTypeFromValue @@ Labelled label)
else
Type.field ~loc ~attrs {txt = label; loc}
(Typ.var @@ safeTypeFromValue @@ Labelled label))
let makeLabelDecls namedTypeList =
let rec mem_fst x = function
| [] -> false
| (a, _) :: l -> compare a (fst x) = 0 || mem_fst x l
in
let rec duplicated = function
| [] -> None
| hd :: tl -> if mem_fst hd tl then Some hd else duplicated tl
in
let duplicatedProp =
(* Find the duplicated prop from the back *)
namedTypeList |> List.rev
Copy link
Member

Choose a reason for hiding this comment

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

Now we always have to do an additional List.rev and List.map even if there are no duplicated labels at all, in which case we have another List.map below.

Could this be done in a more efficient way, avoiding additional List allocations in the standard case (no duplicate props)?
Maybe it would be better

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elavorate your thought? Do you mean that finding the duplicated prop without List.rev first and then List.rev if there is duplicated one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this?

  1. check if there is duplicated without List.map or List.exists
  2. if there is, then List.map and List.rev

Copy link
Member

Choose a reason for hiding this comment

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

Yes, something like that, it would be ideal if we could perform the check without creating any additional lists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, something like that, it would be ideal if we could perform the check without creating any additional lists.

Agree! I'll work on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for being late, I'm stuck with tasks from company 🥲

How does it look? 10421cd Looking a bit verbose, but I think it is avoiding the list relocation before checking for duplicated props.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me! Thanks a lot!

|> List.map (fun (_, label, _, loc, _) -> (label, loc))
|> duplicated
in
match duplicatedProp with
| Some (label, loc) ->
React_jsx_common.raiseError ~loc "JSX: found the duplicated prop `%s`" label
| None ->
namedTypeList
|> List.map (fun (isOptional, label, attrs, loc, interiorType) ->
if label = "key" then
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc}
interiorType
else if isOptional then
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc}
(Typ.var @@ safeTypeFromValue @@ Labelled label)
else
Type.field ~loc ~attrs {txt = label; loc}
(Typ.var @@ safeTypeFromValue @@ Labelled label))

let makeTypeDecls propsName loc namedTypeList =
let labelDeclList = makeLabelDecls ~loc namedTypeList in
let labelDeclList = makeLabelDecls namedTypeList in
(* 'id, 'className, ... *)
let params =
makePropsTypeParamsTvar namedTypeList
Expand Down Expand Up @@ -702,17 +720,22 @@ let argToType ~newtypes ~(typeConstraints : core_type option) types
in
match (type_, name, default) with
| Some type_, name, _ when isOptional name ->
(true, getLabel name, attrs, {type_ with ptyp_attributes = optionalAttrs})
( true,
getLabel name,
attrs,
loc,
{type_ with ptyp_attributes = optionalAttrs} )
:: types
| Some type_, name, _ -> (false, getLabel name, attrs, type_) :: types
| Some type_, name, _ -> (false, getLabel name, attrs, loc, type_) :: types
| None, name, _ when isOptional name ->
( true,
getLabel name,
attrs,
loc,
Typ.var ~loc ~attrs:optionalAttrs (safeTypeFromValue name) )
:: types
| None, name, _ when isLabelled name ->
(false, getLabel name, attrs, Typ.var ~loc (safeTypeFromValue name))
(false, getLabel name, attrs, loc, Typ.var ~loc (safeTypeFromValue name))
:: types
| _ -> types

Expand All @@ -721,10 +744,12 @@ let argWithDefaultValue (name, default, _, _, _, _) =
| Some default when isOptional name -> Some (getLabel name, default)
| _ -> None

let argToConcreteType types (name, attrs, _loc, type_) =
let argToConcreteType types (name, attrs, loc, type_) =
match name with
| name when isLabelled name -> (false, getLabel name, attrs, type_) :: types
| name when isOptional name -> (true, getLabel name, attrs, type_) :: types
| name when isLabelled name ->
(false, getLabel name, attrs, loc, type_) :: types
| name when isOptional name ->
(true, getLabel name, attrs, loc, type_) :: types
| _ -> types

let check_string_int_attribute_iter =
Expand Down Expand Up @@ -1306,7 +1331,8 @@ let transformSignatureItem ~config _mapper item =
makePropsRecordTypeSig ~coreTypeOfAttr ~typVarsOfCoreType "props"
psig_loc
((* If there is Nolabel arg, regard the type as ref in forwardRef *)
(if !hasForwardRef then [(true, "ref", [], refType Location.none)]
(if !hasForwardRef then
[(true, "ref", [], Location.none, refType Location.none)]
else [])
@ namedTypeList)
in
Expand Down
10 changes: 2 additions & 8 deletions res_syntax/tests/ppx/react/expected/aliasProps.res.txt
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
@@jsxConfig({version: 4, mode: "automatic"})

module C0 = {
type props<'priority, 'text> = {
priority: 'priority,
text?: 'text,
}
type props<'priority, 'text> = {priority: 'priority, text?: 'text}
Copy link
Member Author

Choose a reason for hiding this comment

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

Wonder why this PR change leads to remove the trailing comma.

Copy link
Member

Choose a reason for hiding this comment

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

Probably formatted differently now because of the loc changes.


@react.component
let make = ({priority: _, ?text, _}: props<'priority, 'text>) => {
Expand All @@ -23,10 +20,7 @@ module C0 = {
}

module C1 = {
type props<'priority, 'text> = {
priority: 'priority,
text?: 'text,
}
type props<'priority, 'text> = {priority: 'priority, text?: 'text}

@react.component
let make = ({priority: p, ?text, _}: props<'priority, 'text>) => {
Expand Down
4 changes: 1 addition & 3 deletions res_syntax/tests/ppx/react/expected/commentAtTop.res.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
type props<'msg> = { // test React JSX file
msg: 'msg,
}
type props<'msg> = {msg: 'msg} // test React JSX file

@react.component
let make = ({msg, _}: props<'msg>) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@ let t = React.createElement(Foo.component, Foo.componentProps(~a=1, ~b={"1"}, ()
@@jsxConfig({version: 4, mode: "classic"})

module Foo = {
type props<'a, 'b> = {
a: 'a,
b: 'b,
}
type props<'a, 'b> = {a: 'a, b: 'b}

@module("Foo")
external component: React.componentLike<props<int, string>, React.element> = "component"
Expand All @@ -27,10 +24,7 @@ let t = React.createElement(Foo.component, {a: 1, b: "1"})
@@jsxConfig({version: 4, mode: "automatic"})

module Foo = {
type props<'a, 'b> = {
a: 'a,
b: 'b,
}
type props<'a, 'b> = {a: 'a, b: 'b}

@module("Foo")
external component: React.componentLike<props<int, string>, React.element> = "component"
Expand Down
8 changes: 2 additions & 6 deletions res_syntax/tests/ppx/react/expected/fileLevelConfig.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ module V3 = {
@@jsxConfig({version: 4, mode: "classic"})

module V4C = {
type props<'msg> = {
msg: 'msg,
}
type props<'msg> = {msg: 'msg}

@react.component
let make = ({msg, _}: props<'msg>) => {
Expand All @@ -36,9 +34,7 @@ module V4C = {
@@jsxConfig({version: 4, mode: "automatic"})

module V4A = {
type props<'msg> = {
msg: 'msg,
}
type props<'msg> = {msg: 'msg}

@react.component
let make = ({msg, _}: props<'msg>) => {
Expand Down
4 changes: 1 addition & 3 deletions res_syntax/tests/ppx/react/expected/interface.res.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
module A = {
type props<'x> = {
x: 'x,
}
type props<'x> = {x: 'x}
@react.component let make = ({x, _}: props<'x>) => React.string(x)
let make = {
let \"Interface$A" = (props: props<_>) => make(props)
Expand Down
4 changes: 1 addition & 3 deletions res_syntax/tests/ppx/react/expected/interface.resi.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
module A: {
type props<'x> = {
x: 'x,
}
type props<'x> = {x: 'x}
let make: React.componentLike<props<string>, React.element>
}

Expand Down
5 changes: 1 addition & 4 deletions res_syntax/tests/ppx/react/expected/interfaceWithRef.res.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
type props<'x, 'ref> = {
x: 'x,
ref?: 'ref,
}
type props<'x, 'ref> = {x: 'x, ref?: 'ref}
@react.component
let make = (
{x, _}: props<string, ReactDOM.Ref.currentDomRef>,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,2 @@
type props<'x, 'ref> = {
x: 'x,
ref?: 'ref,
}
type props<'x, 'ref> = {x: 'x, ref?: 'ref}
let make: React.componentLike<props<string, ReactDOM.Ref.currentDomRef>, React.element>
20 changes: 4 additions & 16 deletions res_syntax/tests/ppx/react/expected/mangleKeyword.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@ let c31 = React.createElement(C31.make, C31.makeProps(~_open="x", ()))
@@jsxConfig({version: 4, mode: "classic"})

module C4C0 = {
type props<'T_open, 'T_type> = {
@as("open") _open: 'T_open,
@as("type") _type: 'T_type,
}
type props<'T_open, 'T_type> = {@as("open") _open: 'T_open, @as("type") _type: 'T_type}

@react.component
let make = ({@as("open") _open, @as("type") _type, _}: props<'T_open, string>) =>
Expand All @@ -35,10 +32,7 @@ module C4C0 = {
}
}
module C4C1 = {
type props<'T_open, 'T_type> = {
@as("open") _open: 'T_open,
@as("type") _type: 'T_type,
}
type props<'T_open, 'T_type> = {@as("open") _open: 'T_open, @as("type") _type: 'T_type}

external make: @as("open") React.componentLike<props<string, string>, React.element> = "default"
}
Expand All @@ -49,10 +43,7 @@ let c4c1 = React.createElement(C4C1.make, {_open: "x", _type: "t"})
@@jsxConfig({version: 4, mode: "automatic"})

module C4A0 = {
type props<'T_open, 'T_type> = {
@as("open") _open: 'T_open,
@as("type") _type: 'T_type,
}
type props<'T_open, 'T_type> = {@as("open") _open: 'T_open, @as("type") _type: 'T_type}

@react.component
let make = ({@as("open") _open, @as("type") _type, _}: props<'T_open, string>) =>
Expand All @@ -64,10 +55,7 @@ module C4A0 = {
}
}
module C4A1 = {
type props<'T_open, 'T_type> = {
@as("open") _open: 'T_open,
@as("type") _type: 'T_type,
}
type props<'T_open, 'T_type> = {@as("open") _open: 'T_open, @as("type") _type: 'T_type}

external make: @as("open") React.componentLike<props<string, string>, React.element> = "default"
}
Expand Down
12 changes: 2 additions & 10 deletions res_syntax/tests/ppx/react/expected/newtype.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ module V3 = {
@@jsxConfig({version: 4, mode: "classic"})

module V4C = {
type props<'a, 'b, 'c> = {
a: 'a,
b: 'b,
c: 'c,
}
type props<'a, 'b, 'c> = {a: 'a, b: 'b, c: 'c}

@react.component
let make = ({a, b, c, _}: props<'\"type-a", array<option<[#Foo('\"type-a")]>>, 'a>) =>
Expand All @@ -43,11 +39,7 @@ module V4C = {
@@jsxConfig({version: 4, mode: "automatic"})

module V4A = {
type props<'a, 'b, 'c> = {
a: 'a,
b: 'b,
c: 'c,
}
type props<'a, 'b, 'c> = {a: 'a, b: 'b, c: 'c}

@react.component
let make = ({a, b, c, _}: props<'\"type-a", array<option<[#Foo('\"type-a")]>>, 'a>) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ module User = {
type t = {firstName: string, lastName: string}

let format = user => "Dr." ++ user.lastName
type props<'doctor> = {
doctor: 'doctor,
}
type props<'doctor> = {doctor: 'doctor}

@react.component
let make = ({doctor, _}: props<'doctor>) => {
Expand Down
9 changes: 2 additions & 7 deletions res_syntax/tests/ppx/react/expected/removedKeyProp.res.txt
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
@@jsxConfig({version: 4, mode: "classic"})

module Foo = {
type props<'x, 'y> = {
x: 'x,
y: 'y,
}
type props<'x, 'y> = {x: 'x, y: 'y}

@react.component let make = ({x, y, _}: props<'x, 'y>) => React.string(x ++ y)
let make = {
Expand All @@ -15,9 +12,7 @@ module Foo = {
}

module HasChildren = {
type props<'children> = {
children: 'children,
}
type props<'children> = {children: 'children}

@react.component
let make = ({children, _}: props<'children>) => ReactDOM.createElement(React.fragment, [children])
Expand Down
10 changes: 2 additions & 8 deletions res_syntax/tests/ppx/react/expected/topLevel.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@ module V3 = {
@@jsxConfig({version: 4, mode: "classic"})

module V4C = {
type props<'a, 'b> = {
a: 'a,
b: 'b,
}
type props<'a, 'b> = {a: 'a, b: 'b}

@react.component
let make = ({a, b, _}: props<'a, 'b>) => {
Expand All @@ -42,10 +39,7 @@ module V4C = {
@@jsxConfig({version: 4, mode: "automatic"})

module V4A = {
type props<'a, 'b> = {
a: 'a,
b: 'b,
}
type props<'a, 'b> = {a: 'a, b: 'b}

@react.component
let make = ({a, b, _}: props<'a, 'b>) => {
Expand Down
Loading