Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

fix: Avoid tail comments inside tag from being eaten #664

Merged
merged 10 commits into from
Sep 30, 2022
Merged
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
- Fix location issue in error messages with JSX V4 where the multiple props types are defined https://github.com/rescript-lang/syntax/pull/655
- Fix location issue in make function in JSX V4 that breaks dead code elimination https://github.com/rescript-lang/syntax/pull/660
- Fix parsing (hence pretty printing) of expressions with underscore `_` and comments.
- Fix printing of comments inside JSX tag https://github.com/rescript-lang/syntax/pull/664
- Fix issue where formatter erases tail comments inside JSX tag https://github.com/rescript-lang/syntax/issues/663

## ReScript 10.0

Expand Down
41 changes: 38 additions & 3 deletions src/res_comments_table.ml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module Comment = Res_comment
module Doc = Res_doc
module ParsetreeViewer = Res_parsetree_viewer

type t = {
leading: (Location.t, Comment.t list) Hashtbl.t;
Expand Down Expand Up @@ -1310,9 +1311,43 @@ and walkExpression expr t comments =
walkExpression callExpr t inside;
after)
in
let afterExpr, rest = partitionAdjacentTrailing callExpr.pexp_loc after in
attach t.trailing callExpr.pexp_loc afterExpr;
walkList (arguments |> List.map (fun (_, e) -> ExprArgument e)) t rest
if ParsetreeViewer.isJsxExpression expr then (
let props =
arguments
|> List.filter (fun (label, _) ->
match label with
| Asttypes.Labelled "children" -> false
| Asttypes.Nolabel -> false
| _ -> true)
in
let maybeChildren =
arguments
|> List.find_opt (fun (label, _) ->
label = Asttypes.Labelled "children")
in
match maybeChildren with
(* There is no need to deal with this situation as the children cannot be NONE *)
| None -> ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not walking the props at all?
What happens if you have props with comments and no children?

Copy link
Contributor Author

@ah-yu ah-yu Sep 30, 2022

Choose a reason for hiding this comment

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

Would you give me a hint under what circumstances this would happen? I tested some cases and found children were always there

If there are no children elements inside, there will be Pexp_construct "[]" in the AST

Labelled "children"
         expression (test.res[2,1+2]..[2,1+3])
           Pexp_construct "[]" (test.res[2,1+2]..[2,1+3]) ghost
           None

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment saying just that.

| Some (_, children) ->
let leading, inside, _ = partitionByLoc after children.pexp_loc in
if props = [] then
(* All comments inside a tag are trailing comments of the tag if there are no props
<A
// comment
// comment
/>
*)
let afterExpr, _ =
Copy link
Contributor

Choose a reason for hiding this comment

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

What happen to the second argument, _ in this case?
Why can it be ignored?

Copy link
Contributor Author

@ah-yu ah-yu Sep 30, 2022

Choose a reason for hiding this comment

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

Because it is an empty list.

<A
// comment
// comment
>
</A>

If there are no props, all comments after A are trailing comments of A

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment saying that.

partitionAdjacentTrailing callExpr.pexp_loc after
in
attach t.trailing callExpr.pexp_loc afterExpr
else
walkList (props |> List.map (fun (_, e) -> ExprArgument e)) t leading;
walkExpression children t inside)
else
let afterExpr, rest = partitionAdjacentTrailing callExpr.pexp_loc after in
attach t.trailing callExpr.pexp_loc afterExpr;
walkList (arguments |> List.map (fun (_, e) -> ExprArgument e)) t rest
| Pexp_fun (_, _, _, _) | Pexp_newtype _ -> (
let _, parameters, returnExpr = funExpr expr in
let comments =
Expand Down
13 changes: 4 additions & 9 deletions src/res_printer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -4039,20 +4039,15 @@ and printJsxExpression ~customLayout lident args cmtTbl =
{
Parsetree.pexp_desc =
Pexp_construct ({txt = Longident.Lident "[]"}, None);
pexp_loc = loc;
} ->
if isSelfClosing then
Doc.concat
[Doc.line; printComments (Doc.text "/>") cmtTbl loc]
else
Doc.concat [Doc.softLine; printComments Doc.nil cmtTbl loc]
| _ -> Doc.nil);
}
when isSelfClosing ->
Doc.concat [Doc.line; Doc.text "/>"]
| _ -> Doc.concat [Doc.softLine; Doc.greaterThan]);
]);
(if isSelfClosing then Doc.nil
else
Doc.concat
[
Doc.greaterThan;
(if hasChildren then printChildren children
else
match children with
Expand Down
3 changes: 2 additions & 1 deletion tests/conversion/reason/expected/bracedJsx.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ let make = () => {
<div
className=Styles.terminal
onClick={event => (event->ReactEvent.Mouse.target)["querySelector"]("input")["focus"]()}
ref={containerRef->ReactDOMRe.Ref.domRef}>
ref={containerRef->ReactDOMRe.Ref.domRef}
>
{state.history
->Array.mapWithIndex((index, item) =>
<div key={j`$index`} className=Styles.line>
Expand Down
15 changes: 14 additions & 1 deletion tests/printer/comments/expected/jsx.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module Cite = {

<A
value=""
// Comment
// Comment
/>

<A /* comment */ />
Expand All @@ -18,6 +18,19 @@ module Cite = {
// Comment
</A>

<A
// comment1
// comment 2
/>

<A
// comment1
value=""
// comment2
>
<B /* comment3 */ />
</A>

<div>
// Must not jump inside braces
{React.string("Hello, World!")}
Expand Down
13 changes: 13 additions & 0 deletions tests/printer/comments/jsx.res
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,19 @@ module Cite = {
// Comment
</A>

<A
// comment1
// comment 2
/>

<A
// comment1
value=""
// comment2
>
<B /* comment3 */ />
</A>

<div>
// Must not jump inside braces
{React.string("Hello, World!")}
Expand Down
3 changes: 2 additions & 1 deletion tests/printer/expr/expected/jsx.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ let avatarSection =
onMouseLeave={_ => setHoveringAdmin(false)}
onClick={_e => {
stopImpersonating(csrfToken)
}}>
}}
>
<Avatar user={viewer} size={45} />
</div>
: React.nullElement}
Expand Down
6 changes: 4 additions & 2 deletions tests/printer/other/expected/signaturePicker.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ let make = () => {
<div className="pr-2 font-bold text-gray-400 text-lg"> {"Signature"->string} </div>
<select
id="country"
className="transition duration-150 ease-in-out sm:text-sm sm:leading-5 border-none font-bold text-2xl text-gray-600 bg-transparent">
className="transition duration-150 ease-in-out sm:text-sm sm:leading-5 border-none font-bold text-2xl text-gray-600 bg-transparent"
>
{options
->Belt.List.map(option =>
<option key={option->TimeSignature.toString}>
Expand All @@ -47,7 +48,8 @@ let make = () => {
fill="none"
strokeLinecap="round"
strokeLinejoin="round"
className="text-gray-400">
className="text-gray-400"
>
<polyline points="6 9 12 15 18 9" />
</svg>
</label>
Expand Down