Skip to content
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

Jump to bottom

Parse list and list spread in JSX #1972

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Parse list and list spread in JSX #1972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions formatTest/typeCheckedTests/expected_output/jsx.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,3 +553,12 @@ let div = (~onClick, ~children, ()) => ();
<div onClick=onClickHandler>
<> "foobar" </>
</div>;

/* https://github.com/facebook/reason/issues/1467 */
<Foo> 1 2 </Foo>;

<Foo> 1 2 3 4 </Foo>;

<Foo> <> 1 2 3 4 </> </Foo>;

<Foo> <> 1 2 3 </> </Foo>;
9 changes: 9 additions & 0 deletions formatTest/typeCheckedTests/input/jsx.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,3 +436,12 @@ let onClickHandler = () => ();
let div = (~onClick, ~children, ()) => ();

<div onClick=onClickHandler> <> "foobar" </> </div>;

/* https://github.com/facebook/reason/issues/1467 */
<Foo> ...[1, 2] </Foo>;

<Foo> [1, 2] [3,4] </Foo>;

<Foo> <> [1, 2] [3,4] </> </Foo>;

<Foo> <> ...[1, 2, 3] </> </Foo>;
8 changes: 5 additions & 3 deletions formatTest/unit_tests/expected_output/jsx.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,13 +330,15 @@ let x = foo /></ bar;
)
/>;

switch (foo) {
| `Variant => <Component />
};

/* https://github.com/facebook/reason/issues/2028 */
<Foo bar=M.[] />;

<Foo bar=M.[]> M.[] </Foo>;

<Foo bar=M.[]> ...M.[] </Foo>;

switch (foo) {
| `Variant => <Component />
};
<Foo> 1 2 other </Foo>;
8 changes: 5 additions & 3 deletions formatTest/unit_tests/input/jsx.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,13 +227,15 @@ let x = foo /></ bar;
))
/>;

switch(foo) {
| `Variant =><Component />
};

/* https://github.com/facebook/reason/issues/2028 */
<Foo bar=M.[]></Foo>;

<Foo bar=M.[]> M.[] </Foo>;

<Foo bar=M.[]> ...M.[] </Foo>;

switch(foo) {
| `Variant =><Component />
};
<Foo> ...[[1,2] , other] </Foo>;
8,957 changes: 4,615 additions & 4,342 deletions src/reason-parser/reason_parser.messages.checked-in
View file
Open in desktop

Large diffs are not rendered by default.

65 changes: 59 additions & 6 deletions src/reason-parser/reason_parser.mly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,12 @@ let package_type_of_module_type pmty =
| _ ->
err pmty.pmty_loc
"only module type identifier and 'with type' constraints are supported"

let mklist lst startp endp =
let seq, ext_opt = lst in
let loc = mklocation startp endp in
make_real_exp (mktailexp_extension loc seq ext_opt)

%}


Expand Down Expand Up @@ -2751,12 +2757,30 @@ jsx_start_tag_and_args_without_leading_less:
(jsx_component lident $2, lident) }
;

jsx_expr_list:
LBRACKET expr_comma_seq_extension RBRACKET
{ mklist $2 $startpos($2) $endpos($2) }

jsx_children_including_list:
| simple_expr_no_call { $1 }
| jsx_expr_list { $1 }

jsx:
| LESSGREATER simple_expr_no_call* LESSSLASHGREATER
{ let loc = mklocation $symbolstartpos $endpos in
let body = mktailexp_extension loc $2 None in
makeFrag loc body
}
| LESSGREATER jsx_expr_list+ LESSSLASHGREATER
{ let loc = mklocation $symbolstartpos $endpos in
let body = mktailexp_extension loc $2 None in
makeFrag loc body
}
| LESSGREATER DOTDOTDOT jsx_children_including_list LESSSLASHGREATER
{ let loc = mklocation $symbolstartpos $endpos in
let body = $3 (*mktailexp_extension loc $3 None*) in
makeFrag loc body
}
| jsx_start_tag_and_args SLASHGREATER
{ let (component, _) = $1 in
let loc = mklocation $symbolstartpos $endpos in
Expand All @@ -2777,7 +2801,18 @@ jsx:
(Nolabel, mkexp_constructor_unit loc loc)
] loc
}
| jsx_start_tag_and_args GREATER DOTDOTDOT simple_expr_no_call LESSSLASHIDENTGREATER
| jsx_start_tag_and_args GREATER jsx_expr_list+ LESSSLASHIDENTGREATER
{ let (component, start) = $1 in
let loc = mklocation $symbolstartpos $endpos in
(* TODO: Make this tag check simply a warning *)
let endName = Longident.parse $4 in
let _ = ensureTagsAreEqual start endName loc in
component [
(Labelled "children", mktailexp_extension loc $3 None);
(Nolabel, mkexp_constructor_unit loc loc)
] loc
}
| jsx_start_tag_and_args GREATER DOTDOTDOT jsx_children_including_list LESSSLASHIDENTGREATER
(* <Foo> ...bar </Foo> or <Foo> ...((a) => 1) </Foo> *)
{ let (component, start) = $1 in
let loc = mklocation $symbolstartpos $endpos in
Expand All @@ -2798,6 +2833,16 @@ jsx_without_leading_less:
let body = mktailexp_extension loc $2 None in
makeFrag loc body
}
| GREATER jsx_expr_list+ LESSSLASHGREATER
{ let loc = mklocation $symbolstartpos $endpos in
let body = mktailexp_extension loc $2 None in
makeFrag loc body
}
| GREATER DOTDOTDOT jsx_children_including_list LESSSLASHGREATER
{ let loc = mklocation $symbolstartpos $endpos in
let body = $3 (*mktailexp_extension loc $3 None*) in
makeFrag loc body
}
| jsx_start_tag_and_args_without_leading_less SLASHGREATER {
let (component, _) = $1 in
let loc = mklocation $symbolstartpos $endpos in
Expand All @@ -2818,7 +2863,18 @@ jsx_without_leading_less:
(Nolabel, mkexp_constructor_unit loc loc)
] loc
}
| jsx_start_tag_and_args_without_leading_less GREATER DOTDOTDOT simple_expr_no_call LESSSLASHIDENTGREATER {
| jsx_start_tag_and_args_without_leading_less GREATER jsx_expr_list+ LESSSLASHIDENTGREATER
{ let (component, start) = $1 in
let loc = mklocation $symbolstartpos $endpos in
(* TODO: Make this tag check simply a warning *)
let endName = Longident.parse $4 in
let _ = ensureTagsAreEqual start endName loc in
component [
(Labelled "children", mktailexp_extension loc $3 None);
(Nolabel, mkexp_constructor_unit loc loc)
] loc
}
| jsx_start_tag_and_args_without_leading_less GREATER DOTDOTDOT jsx_children_including_list LESSSLASHIDENTGREATER {
let (component, start) = $1 in
let loc = mklocation $symbolstartpos $endpos in
(* TODO: Make this tag check simply a warning *)
Expand Down Expand Up @@ -3189,10 +3245,7 @@ simple_expr_call:
{ let (body, args) = $1 in
(body, List.rev_append $2 args) }
| LBRACKET expr_comma_seq_extension RBRACKET
{ let seq, ext_opt = $2 in
let loc = mklocation $startpos($2) $endpos($2) in
(make_real_exp (mktailexp_extension loc seq ext_opt), [])
}
{ (mklist $2 $startpos($2) $endpos($2), []) }
| simple_expr_template_constructor { ($1, []) }
;

Expand Down
4 changes: 2 additions & 2 deletions src/reason-parser/reason_pprint_ast.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5975,9 +5975,9 @@ let printer = object(self:'self)
| ({txt="JSX"; loc}, PStr []) :: _ ->
begin match self#simplest_expression x with
| Some r -> self#formatChildren remaining (r :: processedRev)
| None -> self#formatChildren (remaining @ children) processedRev
| None -> self#formatChildren (children @ remaining) processedRev
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 has been broken forever 🙈

Copy link
Member

Choose a reason for hiding this comment

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

What's this again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Printing of fragments. we wanna print the fragment's children before the fragment's siblings.

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e. depth first vs. breadth first

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 test:

<div> <> ...[ [1,2], [3,4] ] </> </div>

was getting formatted to

<div> <> 1 3 2 4 </> </div>

i.e. 1 3 2 4 instead of 1 2 3 4 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above, I think I expect <div> <> [1,2] [3,4] </> </div> here. Am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I'm not sure. need someone else to weigh in

end
| _ -> self#formatChildren (remaining @ children) processedRev
| _ -> self#formatChildren (children @ remaining) processedRev
end
| {pexp_desc = Pexp_apply(expr, l); pexp_attributes} :: remaining ->
self#formatChildren remaining (self#simplifyUnparseExpr ~wrap:("{", "}") (List.hd children) :: processedRev)
Expand Down