Skip to content

Commit 06b68e8

Browse files
Hans Halversonfacebook-github-bot
Hans Halverson
authored and
facebook-github-bot
committed
Intern comments in JSX expression containers
Summary: Intern leading and trailing comments around JSX expression containers. Leading and trailing comments can appear adjacent to JSX expression containers in JSX attributes. Note that the comments inside the expression container will be attached to the inner expression: ``` <div attribute=/* L JSX expr */ { /* L expr */ expr /* T expr */ } /* T JSX expr */ /> ``` JSX expression containers that appears as children of a JSX element cannot have leading or trailing comments. Note that trailing comments of JSX expression containers in JSX attributes could also be attached to the next JSX attribute name. This will be fixed in the future once we have an API for eating portions of comments, as this same problem appears for the trailing comments on statements. Reviewed By: pieterv Differential Revision: D20316862 fbshipit-source-id: 96e849a4bd6a9ef8438c431d346d8bfe40d64219
1 parent 8256bda commit 06b68e8

File tree

10 files changed

+186
-51
lines changed

10 files changed

+186
-51
lines changed

‎src/parser/estree_translator.ml‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1538,7 +1538,7 @@ with type t = Impl.t = struct
15381538
| ExpressionContainer (loc, expr) -> jsx_expression_container (loc, expr))
15391539
and jsx_spread_attribute (loc, { JSX.SpreadAttribute.argument }) =
15401540
node "JSXSpreadAttribute" loc [("argument", expression argument)]
1541-
and jsx_expression_container (loc, { JSX.ExpressionContainer.expression = expr }) =
1541+
and jsx_expression_container (loc, { JSX.ExpressionContainer.expression = expr; comments }) =
15421542
let expression =
15431543
match expr with
15441544
| JSX.ExpressionContainer.Expression expr -> expression expr
@@ -1551,9 +1551,9 @@ with type t = Impl.t = struct
15511551
_end = { loc._end with column = loc._end.column - 1 };
15521552
}
15531553
in
1554-
node "JSXEmptyExpression" empty_loc []
1554+
node ?comments "JSXEmptyExpression" empty_loc []
15551555
in
1556-
node "JSXExpressionContainer" loc [("expression", expression)]
1556+
node ?comments "JSXExpressionContainer" loc [("expression", expression)]
15571557
and jsx_text (loc, { JSX.Text.value; raw }) =
15581558
node "JSXText" loc [("value", string value); ("raw", string raw)]
15591559
and jsx_member_expression (loc, { JSX.MemberExpression._object; property }) =

‎src/parser/flow_ast.ml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1239,7 +1239,10 @@ and JSX : sig
12391239
end
12401240

12411241
module ExpressionContainer : sig
1242-
type ('M, 'T) t = { expression: ('M, 'T) expression }
1242+
type ('M, 'T) t = {
1243+
expression: ('M, 'T) expression;
1244+
comments: ('M, unit) Syntax.t option;
1245+
}
12431246

12441247
and ('M, 'T) expression =
12451248
| Expression of ('M, 'T) Expression.t

‎src/parser/jsx_parser.ml‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,24 +27,26 @@ module JSX (Parse : Parser_common.PARSER) = struct
2727
Eat.pop_lex_mode env;
2828
attr
2929

30-
let expression_container' env =
31-
let expression =
32-
if Peek.token env = T_RCURLY then
33-
JSX.ExpressionContainer.EmptyExpression
34-
else
35-
JSX.ExpressionContainer.Expression (Parse.expression env)
36-
in
37-
{ JSX.ExpressionContainer.expression }
30+
let expression_container_contents env =
31+
if Peek.token env = T_RCURLY then
32+
JSX.ExpressionContainer.EmptyExpression
33+
else
34+
JSX.ExpressionContainer.Expression (Parse.expression env)
3835

3936
let expression_container env =
4037
Eat.push_lex_mode env Lex_mode.NORMAL;
4138
let container =
4239
with_loc
4340
(fun env ->
41+
let leading = Peek.comments env in
4442
Expect.token env T_LCURLY;
45-
let container = expression_container' env in
43+
let expression = expression_container_contents env in
4644
Expect.token env T_RCURLY;
47-
container)
45+
let trailing = Peek.comments env in
46+
{
47+
JSX.ExpressionContainer.expression;
48+
comments = Flow_ast_utils.mk_comments_opt ~leading ~trailing ();
49+
})
4850
env
4951
in
5052
Eat.pop_lex_mode env;
@@ -63,8 +65,8 @@ module JSX (Parse : Parser_common.PARSER) = struct
6365
let expr = Parse.assignment env in
6466
JSX.SpreadChild expr
6567
| _ ->
66-
let container = expression_container' env in
67-
JSX.ExpressionContainer container
68+
let expression = expression_container_contents env in
69+
JSX.ExpressionContainer { JSX.ExpressionContainer.expression; comments = None }
6870
in
6971
Expect.token env T_RCURLY;
7072
result)

‎src/parser/test/flow/comment_interning/jsx_element.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,5 @@
99
<div /* 5.1 L JSX id */ name /* 5.2 JSX id */ />;
1010

1111
<div /* 6.1 L JSX id */ name /* 6.2 T JSX id */="test" />;
12+
13+
<div name=/* 7.1 L JSX expr */ {/* 7.2 L num */ 1 /* 7.3 T num */} /* 7.4 T JSX expr */ />;

‎src/parser/test/flow/comment_interning/jsx_element.tree.json‎

Lines changed: 108 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"type":"Program",
3-
"loc":{"source":null,"start":{"line":1,"column":24},"end":{"line":11,"column":58}},
4-
"range":[24,459],
3+
"loc":{"source":null,"start":{"line":1,"column":24},"end":{"line":13,"column":91}},
4+
"range":[24,552],
55
"body":[
66
{
77
"type":"ExpressionStatement",
@@ -408,6 +408,88 @@
408408
"children":[]
409409
},
410410
"directive":null
411+
},
412+
{
413+
"type":"ExpressionStatement",
414+
"loc":{"source":null,"start":{"line":13,"column":0},"end":{"line":13,"column":91}},
415+
"range":[461,552],
416+
"expression":{
417+
"type":"JSXElement",
418+
"loc":{"source":null,"start":{"line":13,"column":0},"end":{"line":13,"column":90}},
419+
"range":[461,551],
420+
"openingElement":{
421+
"type":"JSXOpeningElement",
422+
"loc":{"source":null,"start":{"line":13,"column":0},"end":{"line":13,"column":90}},
423+
"range":[461,551],
424+
"name":{
425+
"type":"JSXIdentifier",
426+
"loc":{"source":null,"start":{"line":13,"column":1},"end":{"line":13,"column":4}},
427+
"range":[462,465],
428+
"name":"div"
429+
},
430+
"attributes":[
431+
{
432+
"type":"JSXAttribute",
433+
"loc":{"source":null,"start":{"line":13,"column":5},"end":{"line":13,"column":66}},
434+
"range":[466,527],
435+
"name":{
436+
"type":"JSXIdentifier",
437+
"loc":{"source":null,"start":{"line":13,"column":5},"end":{"line":13,"column":9}},
438+
"range":[466,470],
439+
"name":"name"
440+
},
441+
"value":{
442+
"type":"JSXExpressionContainer",
443+
"trailingComments":[
444+
{
445+
"type":"Block",
446+
"loc":{"source":null,"start":{"line":13,"column":67},"end":{"line":13,"column":87}},
447+
"range":[528,548],
448+
"value":" 7.4 T JSX expr "
449+
}
450+
],
451+
"leadingComments":[
452+
{
453+
"type":"Block",
454+
"loc":{"source":null,"start":{"line":13,"column":10},"end":{"line":13,"column":30}},
455+
"range":[471,491],
456+
"value":" 7.1 L JSX expr "
457+
}
458+
],
459+
"loc":{"source":null,"start":{"line":13,"column":31},"end":{"line":13,"column":66}},
460+
"range":[492,527],
461+
"expression":{
462+
"type":"Literal",
463+
"trailingComments":[
464+
{
465+
"type":"Block",
466+
"loc":{"source":null,"start":{"line":13,"column":50},"end":{"line":13,"column":65}},
467+
"range":[511,526],
468+
"value":" 7.3 T num "
469+
}
470+
],
471+
"leadingComments":[
472+
{
473+
"type":"Block",
474+
"loc":{"source":null,"start":{"line":13,"column":32},"end":{"line":13,"column":47}},
475+
"range":[493,508],
476+
"value":" 7.2 L num "
477+
}
478+
],
479+
"loc":{"source":null,"start":{"line":13,"column":48},"end":{"line":13,"column":49}},
480+
"range":[509,510],
481+
"value":1,
482+
"raw":"1"
483+
}
484+
}
485+
}
486+
],
487+
"selfClosing":true
488+
},
489+
"closingElement":null,
490+
"children":[]
491+
},
492+
"directive":null
411493
}
412494
],
413495
"comments":[
@@ -518,6 +600,30 @@
518600
"loc":{"source":null,"start":{"line":11,"column":29},"end":{"line":11,"column":47}},
519601
"range":[430,448],
520602
"value":" 6.2 T JSX id "
603+
},
604+
{
605+
"type":"Block",
606+
"loc":{"source":null,"start":{"line":13,"column":10},"end":{"line":13,"column":30}},
607+
"range":[471,491],
608+
"value":" 7.1 L JSX expr "
609+
},
610+
{
611+
"type":"Block",
612+
"loc":{"source":null,"start":{"line":13,"column":32},"end":{"line":13,"column":47}},
613+
"range":[493,508],
614+
"value":" 7.2 L num "
615+
},
616+
{
617+
"type":"Block",
618+
"loc":{"source":null,"start":{"line":13,"column":50},"end":{"line":13,"column":65}},
619+
"range":[511,526],
620+
"value":" 7.3 T num "
621+
},
622+
{
623+
"type":"Block",
624+
"loc":{"source":null,"start":{"line":13,"column":67},"end":{"line":13,"column":87}},
625+
"range":[528,548],
626+
"value":" 7.4 T JSX expr "
521627
}
522628
]
523629
}

‎src/parser_utils/flow_ast_differ.ml‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1218,8 +1218,8 @@ let program
12181218
| (Some (Ast.JSX.Attribute.Literal (loc, lit1)), Some (Ast.JSX.Attribute.Literal (_, lit2)))
12191219
->
12201220
diff_if_changed (literal loc) lit1 lit2 |> Base.Option.return
1221-
| (Some (ExpressionContainer (_, expr1)), Some (ExpressionContainer (_, expr2))) ->
1222-
diff_if_changed_ret_opt jsx_expression expr1 expr2
1221+
| (Some (ExpressionContainer (loc, expr1)), Some (ExpressionContainer (_, expr2))) ->
1222+
diff_if_changed_ret_opt (jsx_expression loc) expr1 expr2
12231223
| _ -> None
12241224
in
12251225
join_diff_list [name_diff; value_diff]
@@ -1238,24 +1238,29 @@ let program
12381238
| (Fragment frag1, Fragment frag2) ->
12391239
diff_if_changed_ret_opt (jsx_fragment old_loc) frag1 frag2
12401240
| (ExpressionContainer expr1, ExpressionContainer expr2) ->
1241-
diff_if_changed_ret_opt jsx_expression expr1 expr2
1241+
diff_if_changed_ret_opt (jsx_expression old_loc) expr1 expr2
12421242
| (SpreadChild expr1, SpreadChild expr2) ->
12431243
diff_if_changed expression expr1 expr2 |> Base.Option.return
12441244
| (Text _, Text _) -> None
12451245
| _ -> None
12461246
in
12471247
Base.Option.value changes ~default:[(old_loc, Replace (JSXChild child1, JSXChild child2))]
12481248
and jsx_expression
1249+
(loc : Loc.t)
12491250
(jsx_expr1 : (Loc.t, Loc.t) Ast.JSX.ExpressionContainer.t)
12501251
(jsx_expr2 : (Loc.t, Loc.t) Ast.JSX.ExpressionContainer.t) : node change list option =
12511252
let open Ast.JSX in
1252-
let { ExpressionContainer.expression = expr1 } = jsx_expr1 in
1253-
let { ExpressionContainer.expression = expr2 } = jsx_expr2 in
1254-
match (expr1, expr2) with
1255-
| (ExpressionContainer.Expression expr1', ExpressionContainer.Expression expr2') ->
1256-
Some (diff_if_changed expression expr1' expr2')
1257-
| (ExpressionContainer.EmptyExpression, ExpressionContainer.EmptyExpression) -> Some []
1258-
| _ -> None
1253+
let { ExpressionContainer.expression = expr1; comments = comments1 } = jsx_expr1 in
1254+
let { ExpressionContainer.expression = expr2; comments = comments2 } = jsx_expr2 in
1255+
let comments_diff = syntax_opt loc comments1 comments2 in
1256+
let expression_diff =
1257+
match (expr1, expr2) with
1258+
| (ExpressionContainer.Expression expr1', ExpressionContainer.Expression expr2') ->
1259+
Some (diff_if_changed expression expr1' expr2')
1260+
| (ExpressionContainer.EmptyExpression, ExpressionContainer.EmptyExpression) -> Some []
1261+
| _ -> None
1262+
in
1263+
join_diff_list [expression_diff; comments_diff]
12591264
and assignment
12601265
(loc : Loc.t)
12611266
(assn1 : (Loc.t, Loc.t) Ast.Expression.Assignment.t)

‎src/parser_utils/flow_ast_mapper.ml‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,11 +1294,20 @@ class ['loc] mapper =
12941294

12951295
method jsx_expression _loc (jsx_expr : ('loc, 'loc) Ast.JSX.ExpressionContainer.t) =
12961296
let open Ast.JSX.ExpressionContainer in
1297-
let { expression } = jsx_expr in
1297+
let { expression; comments } = jsx_expr in
1298+
let comments' = this#syntax_opt comments in
12981299
match expression with
12991300
| Expression expr ->
1300-
id this#expression expr jsx_expr (fun expr -> { expression = Expression expr })
1301-
| EmptyExpression -> jsx_expr
1301+
let expr' = this#expression expr in
1302+
if expr == expr' && comments == comments' then
1303+
jsx_expr
1304+
else
1305+
{ expression = Expression expr'; comments = comments' }
1306+
| EmptyExpression ->
1307+
if comments == comments' then
1308+
jsx_expr
1309+
else
1310+
{ expression = EmptyExpression; comments = comments' }
13021311

13031312
method jsx_name (name : ('loc, 'loc) Ast.JSX.name) =
13041313
let open Ast.JSX in

‎src/parser_utils/flow_polymorphic_ast_mapper.ml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,13 +1072,14 @@ class virtual ['M, 'T, 'N, 'U] mapper =
10721072
method jsx_expression (jsx_expr : ('M, 'T) Ast.JSX.ExpressionContainer.t)
10731073
: ('N, 'U) Ast.JSX.ExpressionContainer.t =
10741074
let open Ast.JSX.ExpressionContainer in
1075-
let { expression } = jsx_expr in
1075+
let { expression; comments } = jsx_expr in
10761076
let expression' =
10771077
match expression with
10781078
| Expression expr -> Expression (this#expression expr)
10791079
| EmptyExpression -> EmptyExpression
10801080
in
1081-
{ expression = expression' }
1081+
let comments' = Base.Option.map ~f:this#syntax comments in
1082+
{ expression = expression'; comments = comments' }
10821083

10831084
method jsx_name (name : ('M, 'T) Ast.JSX.name) : ('N, 'U) Ast.JSX.name =
10841085
let open Ast.JSX in

‎src/parser_utils/output/js_layout_generator.ml‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1987,17 +1987,18 @@ and jsx_member_expression (loc, { Ast.JSX.MemberExpression._object; property })
19871987
jsx_identifier property;
19881988
] )
19891989

1990-
and jsx_expression_container { Ast.JSX.ExpressionContainer.expression = expr } =
1991-
fuse
1992-
[
1993-
Atom "{";
1994-
begin
1995-
match expr with
1996-
| Ast.JSX.ExpressionContainer.Expression expr -> expression expr
1997-
| Ast.JSX.ExpressionContainer.EmptyExpression -> Empty
1998-
end;
1999-
Atom "}";
2000-
]
1990+
and jsx_expression_container loc { Ast.JSX.ExpressionContainer.expression = expr; comments } =
1991+
layout_node_with_comments_opt loc comments
1992+
@@ fuse
1993+
[
1994+
Atom "{";
1995+
begin
1996+
match expr with
1997+
| Ast.JSX.ExpressionContainer.Expression expr -> expression expr
1998+
| Ast.JSX.ExpressionContainer.EmptyExpression -> Empty
1999+
end;
2000+
Atom "}";
2001+
]
20012002

20022003
and jsx_attribute (loc, { Ast.JSX.Attribute.name; value }) =
20032004
let module A = Ast.JSX.Attribute in
@@ -2020,7 +2021,7 @@ and jsx_attribute (loc, { Ast.JSX.Attribute.name; value }) =
20202021
match v with
20212022
| A.Literal (loc, lit) -> source_location_with_comments (loc, literal lit)
20222023
| A.ExpressionContainer (loc, express) ->
2023-
source_location_with_comments (loc, jsx_expression_container express)
2024+
source_location_with_comments (loc, jsx_expression_container loc express)
20242025
end;
20252026
]
20262027
| None -> flat_ugly_space (* TODO we shouldn't do this for the last attr *)
@@ -2119,7 +2120,7 @@ and jsx_child (loc, child) =
21192120
match child with
21202121
| Ast.JSX.Element elem -> Some (loc, jsx_element loc elem)
21212122
| Ast.JSX.Fragment frag -> Some (loc, jsx_fragment loc frag)
2122-
| Ast.JSX.ExpressionContainer express -> Some (loc, jsx_expression_container express)
2123+
| Ast.JSX.ExpressionContainer express -> Some (loc, jsx_expression_container loc express)
21232124
| Ast.JSX.SpreadChild expr -> Some (loc, fuse [Atom "{..."; expression expr; Atom "}"])
21242125
| Ast.JSX.Text { Ast.JSX.Text.raw; _ } ->
21252126
begin

‎src/typing/statement.ml‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5953,14 +5953,19 @@ and jsx_mk_props cx reason c name attributes children =
59535953
| Some
59545954
(Attribute.ExpressionContainer
59555955
( ec_loc,
5956-
{ ExpressionContainer.expression = ExpressionContainer.Expression (loc, e) }
5957-
)) ->
5956+
{
5957+
ExpressionContainer.expression = ExpressionContainer.Expression (loc, e);
5958+
comments;
5959+
} )) ->
59585960
let (((_, t), _) as e) = expression cx (loc, e) in
59595961
( t,
59605962
Some
59615963
(Attribute.ExpressionContainer
59625964
( (ec_loc, t),
5963-
{ ExpressionContainer.expression = ExpressionContainer.Expression e } )) )
5965+
{
5966+
ExpressionContainer.expression = ExpressionContainer.Expression e;
5967+
comments;
5968+
} )) )
59645969
(* <element name={} /> *)
59655970
| Some (Attribute.ExpressionContainer _ as ec) ->
59665971
let t = EmptyT.at attr_loc |> with_trust bogus_trust in
@@ -6153,15 +6158,16 @@ and jsx_body cx (loc, child) =
61536158
(Some (UnresolvedArg t), (loc, Fragment f))
61546159
| ExpressionContainer ec ->
61556160
ExpressionContainer.(
6156-
let { expression = ex } = ec in
6161+
let { expression = ex; ExpressionContainer.comments } = ec in
61576162
let (unresolved_param, ex) =
61586163
match ex with
61596164
| Expression e ->
61606165
let (((_, t), _) as e) = expression cx e in
61616166
(Some (UnresolvedArg t), Expression e)
61626167
| EmptyExpression -> (None, EmptyExpression)
61636168
in
6164-
(unresolved_param, (loc, ExpressionContainer { expression = ex })))
6169+
( unresolved_param,
6170+
(loc, ExpressionContainer { expression = ex; ExpressionContainer.comments }) ))
61656171
| SpreadChild expr ->
61666172
let (((_, t), _) as e) = expression cx expr in
61676173
(Some (UnresolvedSpreadArg t), (loc, SpreadChild e))

0 commit comments

Comments
 (0)