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.

compiler/syntax/src/res_printer.ml

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?

analysis/src/CompletionFrontEnd.ml

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.

Alright, I think the new tests from #7318 prove this now.

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.

shulhi · 2025-02-27T22:02:15Z

tests/tools_tests/ppx/TestPpx.res

@@ -61,3 +61,7 @@ let eq2 = 3 === 3

 let test = async () => 12
 let f = async () => (await test()) + 1
+
+module Fragments = {
+  let f1 = <> </>


This should be <></>, no? (without the space).

Sounds reasonable, although I do believe this is the current behavior. (Playground)

nojaf · 2025-03-03T09:17:00Z

@cristianoc, anything else you can think of for jsx_fragment?
If not, we can start looking at other jsx syntax.

cristianoc · 2025-03-03T09:40:14Z

@cristianoc, anything else you can think of for jsx_fragment? If not, we can start looking at other jsx syntax.

This could be ready to go. Is the absence of changes in ast-mapping what tells us that the back and forth conversion is just as before adding the fragment node?

cristianoc · 2025-03-03T09:41:41Z

Would you add a changelog too? Assuming this change will go ahead, and the rest of the JSX investigated in a separate PR.

nojaf · 2025-03-03T09:57:46Z

This could be ready to go. Is the absence of changes in ast-mapping what tells us that the back and forth conversion is just as before adding the fragment node?

Yes, I understand. Today, I made some necessary changes to ensure the tests remained consistent.

Would you add a changelog too? Assuming this change will go ahead, and the rest of the JSX investigated in a separate PR.

I would recommend investigating this in the same PR. There isn't much benefit to having just the fragments part. To avoid the risk of not completing a second PR, I would suggest keeping it as one.

That being said, I would like to achieve a sense of completion for the fragments before taking on more changes.

cristianoc · 2025-03-03T13:40:54Z

Sure continuing jsx sounds good.

nojaf · 2025-03-31T15:11:59Z

Alright, I think I’ve figured out all the printing details!

This is ready for review!
Some things to note:

There may be some minor differences with the formatter; I expect a few minor formatter bugs to arise once we release this to the public.
The AST mapping works well but may not always have accurate ranges after conversion. Some information is lost when transitioning from new -> old -> new. However, I believe that is acceptable.
Naming can be challenging; I’m open to renaming types if you think I missed the mark.
Thank you for taking the time to review this; I appreciate it, and I know it’s quite a bit!

cristianoc

Looks great! That was some work!!
Left some minor comments on the implementation.
The first order of business is that compilation is not affected, and that seems fine.
Second, the changes in printing. They look good in tests. Perhaps worth trying on an existing large project to see what the differences are in the field. @cknitt any candidates?

cristianoc · 2025-04-01T08:30:29Z

analysis/src/CompletionFrontEnd.ml

-        | Pexp_apply {funct = {pexp_desc = Pexp_ident compName}; args}
-          when Res_parsetree_viewer.is_jsx_expression expr ->
+        | Pexp_jsx_element
+            ( Jsx_unary_element


thoughts about pros/cons of unifying the jsx ast nodes into one?
In this case, perhaps one could streamline the code a bit more, but don't know about the rest

I believe combining these is beneficial, although it may not be a game changer. If you don't need to handle JSX, you can avoid a single branch in your match. However, if you do need to handle them, you'll need to address all of them.

compiler/ml/typecore.ml

compiler/syntax/src/res_core.ml

cristianoc · 2025-04-01T08:38:18Z

compiler/syntax/src/res_parens.ml

@@ -383,7 +382,7 @@ let jsx_child_expr expr =
         ( Pexp_ident _ | Pexp_constant _ | Pexp_field _ | Pexp_construct _
         | Pexp_variant _ | Pexp_array _ | Pexp_pack _ | Pexp_record _
         | Pexp_extension _ | Pexp_letmodule _ | Pexp_letexception _
-         | Pexp_open _ | Pexp_sequence _ | Pexp_let _ );
+         | Pexp_open _ | Pexp_sequence _ | Pexp_let _ | Pexp_jsx_element _ );


double checking: only 1 of the 3 jsx ast nodes here is correct?

This captures all three nodes, so what exactly do you mean here?

cristianoc · 2025-04-01T08:41:38Z

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

-Resolved opens 1 Stdlib
-Path Comp.make
-{"contents": {"kind": "markdown", "value": "```rescript\nint\n```"}}
+{"contents": {"kind": "markdown", "value": "```rescript\nComp.props<int>\n```\n\n---\n\n```\n \n```\n```rescript\ntype Comp.props<'age> = {age: 'age}\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx2.res%22%2C157%2C2%5D)\n"}}


was this incorrect before?

Not really, but after these changes there was type information for this hover, where previously there wasn't. I suspect this is the case because of more accurate ranges in the node now.

cristianoc · 2025-04-01T08:46:33Z

Forgot to ask: this seems complete (minus some leftover commented code in a couple of files). Anything known to be missing / less confident about?

nojaf · 2025-04-01T09:15:12Z

Forgot to ask: this seems complete (minus some leftover commented code in a couple of files). Anything known to be missing / less confident about?

So like I said printer bugs with comments might be a thing.
I did notice that some blank lines can disappear after formatting, will look into that.

I believe I undid, #7269

And, I'm not sure if anyone is doing something with ppx with this, if so, might be interesting to hear if that still works for them.

…en printing

nojaf · 2025-04-01T12:02:07Z

Alright, I found a solution to the missing blank lines, similar to how #7269 introduced it.

This is ready for some testing on real world projects.

zth

Looks great, absolutely fantastic job! 🎉 ⭐ 👏

zth · 2025-04-02T07:38:44Z

analysis/src/CompletionFrontEnd.ml

+                     jsx_container_element_children =
+                       JSXChildrenSpreading _ | JSXChildrenItems (_ :: _);
+                   }) ->
+              (* This is a weird edge case where there is no closing tag but there are children *)


<div>{React.string("hello")

Is that an example of such a case?

zth · 2025-04-02T07:41:20Z

compiler/ml/ast_mapper_from0.ml

+  let try_map_jsx_prop (sub : mapper) (lbl : Asttypes.Noloc.arg_label)
+      (e : expression) : Parsetree.jsx_prop option =
+    match (lbl, e) with
+    | Asttypes.Noloc.Labelled "_spreadProps", expr ->


Should the constant _spreadProps be defined somewhere central perhaps, instead of being inlined? Does it matter?

It is used 4 times, so seems like worth doing, do you have any place in mind?

If there's a place where logic around JSX prop spreading is located, that's probably a good place.

Hmm, it is used in the mapping from and to and in Completion.jsx:

I don't think this really does anything though for completion.

zth · 2025-04-02T07:41:54Z

compiler/ml/ast_mapper_to0.ml

zth · 2025-04-02T07:42:23Z

compiler/ml/parsetree.ml

+}
+
+and jsx_unary_element = {
+  (* jsx_unary_element_opening: Lexing.position; *)


Remove these leftover comments...?

zth · 2025-04-02T07:43:13Z

compiler/ml/parsetree.ml

+   *   |  lident
+   *   | ?lident
+   *)
+  | JSXPropPunning of (* optional *) bool * (* name *) string loc


Not that important, but sometimes I wonder if we should just default to inline records instead of tuples.

zth · 2025-04-02T07:43:29Z

compiler/ml/parsetree.ml

+and jsx_closing_container_tag = {
+  (* </ *)
+  jsx_closing_container_tag_start: Lexing.position;
+  (* name *)
+  jsx_closing_container_tag_name: Longident.t loc;
+  (* > *)
+  jsx_closing_container_tag_end: Lexing.position;
+}


Great comments!

zth · 2025-04-02T07:43:50Z

compiler/ml/pprintast.ml

Very cool to get this in the actual printer now.

zth · 2025-04-02T07:48:22Z

tests/syntax_tests/data/parsing/errors/expressions/expected/implementation.res.txt

@@ -12,7 +12,6 @@

 module InstallerDownload =
  struct
-    let make [arity:1]() = ((div ~children:[] ())[@res.braces ][@JSX ])
-      [@@react.component ]
+    let make [arity:1]() = ((<div />)[@res.braces ])[@@react.component ]


cknitt · 2025-04-02T07:59:41Z

This is ready for some testing on real world projects.

Very nice work! I can do some testing on real-world projects later today or tomorrow.

shulhi · 2025-04-03T04:31:35Z

analysis/src/CompletionFrontEnd.ml

+                {
+                  jsx_container_element_tag_name_start = compName;
+                  jsx_container_element_props = props;
+                } ) ->
          inJsxContext := true;


I'm curious why do we need inJsxContext flag since we are already in JSX branch? Maybe we can document this decision somewhere in the code.

This is used in CompletionBackEnd to autocomplete ->React.string when dotting into a string. CPField and CPPipe do have a comment for it.

cknitt · 2025-04-03T15:17:46Z

This is awesome! 🎉 Just tested it against a large project (that also uses a PPX) with no issues whatsoever.

Everything compiled fine and the JS output was unchanged compared to the alpha.11 release.

Reformatting the sources led to minor changes regarding fragments and comments that I all consider improvements:

Before:

let renderWithParens = el => <>
  {t(nbsp ++ "(")}
  el
  {t(")")}
</>

After:

let renderWithParens = el =>
  <>
    {t(nbsp ++ "(")}
    el
    {t(")")}
  </>

Before:

    <input
      disabled
      className="hidden"
      id=/* accept=".*" */
      "fileUploadButton"
      type_="file"
      multiple=false
      onChange
      onClick

After:

    <input
      disabled
      className="hidden"
      id=/* accept=".*" */ "fileUploadButton"
      type_="file"
      multiple=false
      onChange
      onClick

cristianoc

Ready to go!

nojaf · 2025-04-03T17:10:54Z

Thanks for testing @cknitt!

There seems to be a bit of inconsistency between fragment and container tag: example

I made a change to address the fragment example you posted.
As for the input I'm considering this an improvement. Debatable of course but I'm gonna leave it at that.

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

shulhi reviewed Feb 27, 2025

View reviewed changes

nojaf added 4 commits March 3, 2025 07:55

Merge branch 'master' into jsx-ast

32161a3

Fix jsx fragment mapping

d714829

Remove fragment

e27b339

Update test output

92657ee

nojaf added 8 commits March 6, 2025 11:55

Introducing Pexp_jsx_unary_element & Pexp_jsx_container_element

441de6e

Refactor fragment transformation.

bf1b411

Initial transform of Pexp_jsx_unary_element in automatic mode

5fe5f12

Make props for unary element in automatic mode.

9251609

Initial custom component unary tag

1aaa686

lowercase container element with children.

b374212

Uppercase container elements

c91aeda

Streamline automatic element calls

85ccab4

More older caml stuff

4e160f3

nojaf marked this pull request as ready for review March 31, 2025 15:12

nojaf requested a review from cristianoc April 1, 2025 07:40

cristianoc reviewed Apr 1, 2025

View reviewed changes

Remove unreached code in typecore

93eb5af

nojaf added 6 commits April 1, 2025 11:15

Remove leftover comments

6152b83

Preserve lines between jsx elements

2126fc9

Preserve newline between jsx children, streamline jsx fragment childr…

645b244

…en printing

Update analysis expected

a0c0d0d

Another location change in expected

f1da36f

One more update?

80c307a

nojaf added 2 commits April 1, 2025 20:41

Merge branch 'master' into jsx-ast

5e5dabd

Merge branch 'master' into jsx-ast

091f1a3

zth approved these changes Apr 2, 2025

View reviewed changes

nojaf added 2 commits April 2, 2025 10:30

Remove leftover comments

11ced99

Remove obsolete comment

0c825d5

shulhi reviewed Apr 3, 2025

View reviewed changes

cristianoc approved these changes Apr 3, 2025

View reviewed changes

Ensure fragment starts after arrow

8ff481d

nojaf added 3 commits April 3, 2025 19:11

Add changelog entry

8723426

Merge branch 'master' into jsx-ast

a1e4ca9

Add missing Pexp_await

e1ad251

nojaf merged commit 6bb7e0a into rescript-lang:master Apr 4, 2025
20 checks passed

Jsx ast #7286

Jsx ast #7286

Conversation

nojaf commented Feb 8, 2025

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

nojaf commented Feb 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Mar 3, 2025

cristianoc commented Mar 3, 2025

cristianoc commented Mar 3, 2025

nojaf commented Mar 3, 2025

cristianoc commented Mar 3, 2025

nojaf commented Mar 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Apr 1, 2025

nojaf commented Apr 1, 2025 • edited Loading

nojaf commented Apr 1, 2025

zth 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

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

cknitt commented Apr 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cknitt commented Apr 3, 2025

cristianoc left a comment

Choose a reason for hiding this comment

nojaf commented Apr 3, 2025

nojaf commented Apr 1, 2025 •

edited

Loading