Add loc to each props in JSX4 #5937

mununki · 2023-01-10T08:55:51Z

This PR fixes the #5936. Now it shows the error with loc.

The message is not a bit kind yet, but the typechecker would not distinguish whether it is a type parameter of props type or not.

mununki · 2023-01-10T13:08:26Z

Maybe error msg with hint would be more helpful. 288e508 like https://github.com/rescript-lang/rescript-compiler/blob/d78f5c746e73ba57b4952913509b3f0f70c00236/jscomp/ml/typecore.ml#L3703

mununki · 2023-01-10T22:25:53Z

res_syntax/tests/ppx/react/expected/aliasProps.res.txt

-    priority: 'priority,
-    text?: 'text,
-  }
+  type props<'priority, 'text> = {priority: 'priority, text?: 'text}


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

Probably formatted differently now because of the loc changes.

mununki · 2023-01-11T01:06:19Z

Now, it shows error like this:

jscomp/ml/typedecl.ml

cknitt · 2023-01-11T06:56:20Z

res_syntax/tests/ppx/react/expected/aliasProps.res.txt

-    priority: 'priority,
-    text?: 'text,
-  }
+  type props<'priority, 'text> = {priority: 'priority, text?: 'text}


Probably formatted differently now because of the loc changes.

cknitt · 2023-01-11T07:00:34Z

I tested it and it works fine, thanks a lot!

I was wondering if it would be feasible to check for duplicate labels and emit an error directly in the PPX where there is still more information available than in the type checker. Then we could have an even better error message, something like Duplicate label "a".

Co-authored-by: Christoph Knittel <[email protected]>

mununki · 2023-01-11T15:55:01Z

Now it raises the error in ppx when there are duplicates. b825937

mununki · 2023-01-11T16:00:30Z

A bit polishing error msg

cknitt · 2023-01-11T20:38:34Z

res_syntax/src/reactjs_jsx_v4.ml

+  in
+  let duplicatedProp =
+    (* Find the duplicated prop from the back *)
+    namedTypeList |> List.rev


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

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?

Maybe this?

check if there is duplicated without List.map or List.exists

if there is, then List.map and List.rev

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

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.

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.

Looks good to me! Thanks a lot!

cknitt · 2023-01-13T14:23:38Z

I think it would be good to backport this one to 10.1 - what do you think @mununki @cristianoc?

mununki · 2023-01-13T14:23:42Z

Also backport to syntax repo for v10.1.1?

cknitt · 2023-01-13T14:27:18Z

Ah, and a CHANGELOG entry would be great. 🙂

cristianoc · 2023-01-13T14:35:37Z

jscomp/ml/typedecl.ml

@@ -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."


This change is not needed anymore right?

Correct, I will revert it.

cristianoc · 2023-01-13T14:39:12Z

res_syntax/src/reactjs_jsx_v4.ml

+  in
+  let rec duplicated = function
+    | [] -> None
+    | hd :: tl -> if mem_label hd tl then Some hd else duplicated tl


If you throw an error right here, then you don't need anything else below.
So a checkDuplicateLabel function that returns unit.

For perf, there are cases with several hundred props, in ReScript-react. This is quadratic, and happens only once when the big type is declared, but does not affect using the function. So I think it's OK.

cristianoc · 2023-01-13T14:40:33Z

res_syntax/src/reactjs_jsx_v4.ml

+    | Some (_, label, _, loc, _) ->
+      React_jsx_common.raiseError ~loc "JSX: found the duplicated prop `%s`"
+        label
+    | None ->


This logic is a bit unclear. If one throws an error above as soon as a duplicate is found, then this logic is not needed at all.

The logics after duplicated fn are to look for the duplicated prop in backward. If we get a duplicated prop's label and loc, then we can show the error msg pointing the last one.
Do you think it is enough to show the first duplicated prop as error msg?

I think showing the first duplicated one is ok.

Otherwise, just reverse then iterate.

How does it look? b381364

Looks great thanks.

cristianoc · 2023-01-13T14:41:34Z

res_syntax/src/reactjs_jsx_v4.ml

+let makeLabelDecls namedTypeList =
+  let rec mem_label ((_, la, _, _, _) as x) = function
+    | [] -> false
+    | (_, lb, _, _, _) :: l -> compare lb la = 0 || mem_label x l


Nit: lb = la is cleaner than using compare

Also, it's way faster if one annotated that it's a string

cristianoc

Looks good to go.

mununki mentioned this pull request Jan 10, 2023

JSX4: Unhelpful error message in case of duplicate prop #5936

Closed

cristianoc requested a review from cknitt January 10, 2023 09:42

mununki force-pushed the add-loc-to-props branch from e4afca8 to c8a8d4a Compare January 10, 2023 14:57

cknitt force-pushed the add-loc-to-props branch from e60842f to 2ae270c Compare January 10, 2023 19:06

mununki added 4 commits January 10, 2023 20:54

add loc to each props in JSX4

f0507b7

more informative error msg

588acf8

add a period into error msg

13d148a

add test

81f3dfd

cknitt force-pushed the add-loc-to-props branch from 2ae270c to 81f3dfd Compare January 10, 2023 19:54

mununki commented Jan 10, 2023

View reviewed changes

cknitt reviewed Jan 11, 2023

View reviewed changes

mununki and others added 3 commits January 11, 2023 18:51

Update jscomp/ml/typedecl.ml

12a59aa

Co-authored-by: Christoph Knittel <[email protected]>

Update jscomp/ml/typedecl.ml

ced073d

Co-authored-by: Christoph Knittel <[email protected]>

find the duplicated props in ppx JSX4

b825937

better error msg

41382f7

cknitt reviewed Jan 11, 2023

View reviewed changes

avoid list relocation

10421cd

cknitt approved these changes Jan 13, 2023

View reviewed changes

cristianoc reviewed Jan 13, 2023

View reviewed changes

mununki added 2 commits January 14, 2023 17:26

revert error msg

b19e8c9

clean up

b381364

cristianoc approved these changes Jan 14, 2023

View reviewed changes

mununki merged commit 705b1c9 into master Jan 14, 2023

mununki deleted the add-loc-to-props branch January 14, 2023 10:04

This was referenced Jan 14, 2023

Add loc to each props in JSX4 rescript-lang/syntax#728

Merged

Update changelog #5945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add loc to each props in JSX4 #5937

Add loc to each props in JSX4 #5937

mununki commented Jan 10, 2023

mununki commented Jan 10, 2023

mununki Jan 10, 2023

cknitt Jan 11, 2023

mununki commented Jan 11, 2023

cknitt Jan 11, 2023

cknitt commented Jan 11, 2023

mununki commented Jan 11, 2023

mununki commented Jan 11, 2023

cknitt Jan 11, 2023

mununki Jan 11, 2023

mununki Jan 12, 2023

cknitt Jan 12, 2023

mununki Jan 12, 2023

mununki Jan 12, 2023

cknitt Jan 13, 2023

cknitt commented Jan 13, 2023

mununki commented Jan 13, 2023

cknitt commented Jan 13, 2023

cristianoc Jan 13, 2023

mununki Jan 13, 2023

cristianoc Jan 13, 2023

cristianoc Jan 13, 2023

mununki Jan 13, 2023

cristianoc Jan 13, 2023

cristianoc Jan 13, 2023

mununki Jan 14, 2023

cristianoc Jan 14, 2023

cristianoc Jan 13, 2023

cristianoc Jan 13, 2023

cristianoc left a comment

Add loc to each props in JSX4 #5937

Add loc to each props in JSX4 #5937

Conversation

mununki commented Jan 10, 2023

mununki commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki commented Jan 11, 2023

Choose a reason for hiding this comment

cknitt commented Jan 11, 2023

mununki commented Jan 11, 2023

mununki commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cknitt commented Jan 13, 2023

mununki commented Jan 13, 2023

cknitt commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment