Jsx ast #7286

nojaf · 2025-02-08T18:46:37Z

This is a part of #7283.
I'm introducing Pexp_jsx_fragment to represent fragment syntax <></>.

I found it insightful to just try it out and see what code changes are necessary.

In short a fragment is now parsed as:

        expression 
          attribute  "JSX"
            []
          Pexp_construct "::" 
          Some
            expression 
              Pexp_tuple
              [
                expression 
                  attribute  "JSX"
                    []
                  Pexp_apply
                  expression 
                    Pexp_ident "SectionHeader.createElement" 
                  [
                    <arg>
                    Labelled "children"
                      expression 
                        Pexp_construct "::" 
                        Some
                          expression 
                            Pexp_tuple
                            [
                              expression 
                                attribute  "res.braces"
                                  []
                                Pexp_apply
                                expression 
                                  Pexp_ident "React.string" 
                                [
                                  <arg>
                                  Nolabel
                                    expression 
                                      Pexp_constant PConst_string ("abc",Some "*j")
                                ]
                              expression 
                                Pexp_construct "[]" 
                                None
                            ]
                    <arg>
                    Nolabel
                      expression 
                        Pexp_construct "()" 
                        None
                  ]
                expression 
                  Pexp_construct "[]" 
                  None
              ]

after this change it becomes:

        expression 
          Pexp_jsx_fragment          [
            expression 
              attribute  "JSX"
                []
              Pexp_apply
              expression 
                Pexp_ident "SectionHeader.createElement" 
              [
                <arg>
                Labelled "children"
                  expression 
                    Pexp_construct "::" 
                    Some
                      expression 
                        Pexp_tuple
                        [
                          expression 
                            attribute  "res.braces"
                              []
                            Pexp_apply
                            expression 
                              Pexp_ident "React.string" 
                            [
                              <arg>
                              Nolabel
                                expression 
                                  Pexp_constant PConst_string ("abc",Some "*j")
                            ]
                          expression 
                            Pexp_construct "[]" 
                            None
                        ]
                <arg>
                Nolabel
                  expression 
                    Pexp_construct "()" 
                    None
              ]
          ]

I'll add some comment to relevant changes.

nojaf · 2025-02-08T18:48:10Z

compiler/syntax/src/res_core.ml


 (*
 * jsx-fragment ::=
 *  | <> </>
 *  | <> jsx-children </>
 *)
-and parse_jsx_fragment p =
+and parse_jsx_fragment start_pos p =


I prefer ranges to be accurate. The location should start at the opening < token.

nojaf · 2025-02-08T18:50:55Z

compiler/syntax/src/res_printer.ml

  Doc.group
    (Doc.concat
       [
+         line_sep;


@shulhi this part isn't 100% correct.
I'm not familiar enough with this part of the codebase.

let z1 = <> <SectionHeader> {React.string("abc")} </SectionHeader> </>

will be formatted to

let z1 = <> <SectionHeader> {React.string("abc")} </SectionHeader> </>

so I'm missing some indent here. Not sure how that part works.
Would love to hear your thoughts on this.

@nojaf I checked, the indentation is handled here,

rescript/compiler/syntax/src/res_printer.ml

Lines 2085 to 2103 in 9da9b19

let should_indent =

match opt_braces with

| Some _ -> false

| _ -> (

ParsetreeViewer.is_binary_expression expr

||

match vb.pvb_expr with

| {

pexp_attributes = [({Location.txt = "res.ternary"}, _)];

pexp_desc = Pexp_ifthenelse (if_expr, _, _);

} ->

ParsetreeViewer.is_binary_expression if_expr

|| ParsetreeViewer.has_attributes if_expr.pexp_attributes

| {pexp_desc = Pexp_newtype _} -> false

| {pexp_attributes = [({Location.txt = "res.taggedTemplate"}, _)]} ->

false

| e ->

ParsetreeViewer.has_attributes e.pexp_attributes

|| ParsetreeViewer.is_array_access e)

You might need to handle the case for Pexp_jsx_fragment here.

Thanks, the old code will instead have matched ParsetreeViewer.has_attributes e.pexp_attributes. Indentation now works as before.

nojaf · 2025-02-08T18:51:45Z

tests/analysis_tests/tests/src/expected/Completion.res.txt

@@ -1000,7 +1000,7 @@ Path Objects.Rec.

 Complete src/Completion.res 120:7
 posCursor:[120:7] posNoWhite:[120:6] Found expr:[119:11->123:1]
-posCursor:[120:7] posNoWhite:[120:6] Found expr:[120:5->122:5]
+posCursor:[120:7] posNoWhite:[120:6] Found expr:[120:5->123:0]


Changes like this are to be expected as the range of a fragment spans from <> till end </>.

nojaf · 2025-02-08T18:53:35Z

tests/analysis_tests/tests/src/expected/Fragment.res.txt

 posCursor:[9:56] posNoWhite:[9:55] Found expr:[9:13->9:66]
 JSX <SectionHeader:[9:13->9:26] > _children:9:26
-posCursor:[9:56] posNoWhite:[9:55] Found expr:__ghost__[9:10->9:67]


This ghost expression is a part of the AstHelper.make_list_expression result.
It is no longer present in the new AST, but it also didn't serve any purpose.
I believe it is okay that this test is slightly different.
In the end the result didn't change.

nojaf · 2025-02-08T18:56:56Z

compiler/ml/parsetree.ml

+  (* [%id] *)
+  (* . *)
+  (* represents <> foo </> , the entire range is stored in the expression , we keep track of >, children and </ *)
+  | Pexp_jsx_fragment of


The reason the store the > and </ token is this edge case:
https://rescript-lang.org/try?version=v12.0.0-alpha.8&module=esmodule&code=DYUwLgBA+hC8ECgCQAeCB6AVBAzmATgIYB2A5iBJuhAHzJIDeASiIQMZgB0e+AlmQAoARAFsQACyEBKAL7IU1LBEIiIASQh9S4yFVpA

If we ever want to restore comments I suppose we need the proper anchors.

compiler/syntax/src/res_core.ml

compiler/syntax/src/res_comments_table.ml

tests/syntax_tests/data/printer/expr/expected/braced.res.txt

tests/syntax_tests/data/printer/comments/expected/jsx.res.txt

Map child expressions Initial mapping of Pexp_jsx_fragment to 0 Correct location in mapping Update analysis for jsx_fragment Remove unused code Print something for ml print Commit invalid test results for reference Try improve printing Correct fragment range, try and print comments Indent jsx Process comments from children inside fragment Attach comments to fragment tags Fix comment Improve comment formatting Print single element on same line Update comment WIP: Debug More debugging Works Fix some jsx printing Fix the test Clean up Update tests with location changes

nojaf · 2025-02-21T09:17:06Z

Thank you @shulhi for fixing all the remaining formatter problem of the fragment node!
The fragment node is now in a good state.
@cristianoc could you review this PR before I continue to introduce a new node for jsx_element.

cristianoc

Left some comments.
Just checking: is the conversion complete in that the representation of fragments is completely moved over to the new representation?

cristianoc · 2025-02-21T17:31:37Z

analysis/src/CompletionFrontEnd.ml

@@ -1232,7 +1232,8 @@ let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor
                         then ValueOrField
                         else Value);
                    }))
-        | Pexp_construct ({txt = Lident ("::" | "()")}, _) ->
+        | Pexp_construct ({txt = Lident ("::" | "()")}, _) | Pexp_jsx_fragment _


Are :: and () still used?

Yes, for JSX tag.

cristianoc · 2025-02-21T17:33:56Z

compiler/ml/ast_mapper_to0.ml

@@ -407,6 +407,18 @@ module E = struct
    | Pexp_open (ovf, lid, e) ->
      open_ ~loc ~attrs ovf (map_loc sub lid) (sub.expr sub e)
    | Pexp_extension x -> extension ~loc ~attrs (sub.extension sub x)
+    | Pexp_jsx_fragment (o, xs, c) ->


There should be a corresponding change in ast_mapper_from0 that inverts the conversion.
Also, some extension of the test cases in tests/tools_tests/ppx/TestPpx.res to check that back and forth conversion works fine.

nojaf · 2025-02-22T09:44:47Z

Left some comments. Just checking: is the conversion complete in that the representation of fragments is completely moved over to the new representation?

Yes, that is the idea. The parser only produces jsx_fragment and not the old representation.
Besides the ast_mapper_from0 I believe everything is dealt with.

nojaf commented Feb 8, 2025

View reviewed changes

nojaf commented Feb 10, 2025

View reviewed changes

compiler/syntax/src/res_core.ml Show resolved Hide resolved

nojaf commented Feb 13, 2025

View reviewed changes

compiler/syntax/src/res_comments_table.ml Outdated Show resolved Hide resolved

nojaf commented Feb 14, 2025

View reviewed changes

tests/syntax_tests/data/printer/expr/expected/braced.res.txt Show resolved Hide resolved

nojaf commented Feb 14, 2025

View reviewed changes

tests/syntax_tests/data/printer/comments/expected/jsx.res.txt Outdated Show resolved Hide resolved

nojaf force-pushed the jsx-ast branch from 9451cd2 to db7eee6 Compare February 21, 2025 09:15

cristianoc reviewed Feb 21, 2025

View reviewed changes

nojaf added 2 commits February 24, 2025 13:38

Initial mapping from0

28aa287

Format code

b57f2df

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jsx ast #7286

Jsx ast #7286

nojaf commented Feb 8, 2025

nojaf Feb 8, 2025

nojaf Feb 8, 2025

shulhi Feb 11, 2025 •

edited

Loading

nojaf Feb 11, 2025

nojaf Feb 8, 2025

nojaf Feb 8, 2025

nojaf Feb 8, 2025

nojaf commented Feb 21, 2025

cristianoc left a comment

cristianoc Feb 21, 2025

shulhi Feb 24, 2025

cristianoc Feb 21, 2025

nojaf commented Feb 22, 2025

	let should_indent =
	match opt_braces with
	\| Some _ -> false
	\| _ -> (
	ParsetreeViewer.is_binary_expression expr
	\|\|
	match vb.pvb_expr with
	\| {
	pexp_attributes = [({Location.txt = "res.ternary"}, _)];
	pexp_desc = Pexp_ifthenelse (if_expr, _, _);
	} ->
	ParsetreeViewer.is_binary_expression if_expr
	\|\| ParsetreeViewer.has_attributes if_expr.pexp_attributes
	\| {pexp_desc = Pexp_newtype _} -> false
	\| {pexp_attributes = [({Location.txt = "res.taggedTemplate"}, _)]} ->
	false
	\| e ->
	ParsetreeViewer.has_attributes e.pexp_attributes
	\|\| ParsetreeViewer.is_array_access e)

Jsx ast #7286

Are you sure you want to change the base?

Jsx ast #7286

Conversation

nojaf commented Feb 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shulhi Feb 11, 2025 • edited Loading

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

nojaf commented Feb 21, 2025

cristianoc left a 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

nojaf commented Feb 22, 2025

shulhi Feb 11, 2025 •

edited

Loading