Skip to content

Refactor jsx mode in parser #7751

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

Merged
merged 16 commits into from
Aug 6, 2025
Merged

Refactor jsx mode in parser #7751

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2699aa7
Add token dump printer
nojaf Jul 30, 2025
b60bcae
Remove ForwardSlash token
nojaf Aug 2, 2025
0e8b542
Document RESCRIPT_BSC_EXE for local usage
nojaf Aug 2, 2025
7d6a563
Process hypens in parse_module_long_ident for jsx
nojaf Aug 2, 2025
b0f4806
Update test snapshot
nojaf Aug 2, 2025
50d46ef
Add how to view tokens.
nojaf Aug 2, 2025
849d19f
fmt
nojaf Aug 2, 2025
28cfceb
Clean up
nojaf Aug 2, 2025
bd46a3e
Correct fragment range
nojaf Aug 2, 2025
8bb4e41
Use legacy clean for analysis projects
nojaf Aug 2, 2025
9d8641e
Update analysis snapshot
nojaf Aug 2, 2025
69054c5
Copilot review suggestion
nojaf Aug 5, 2025
452301a
Return original error directly
nojaf Aug 5, 2025
5d05a08
Add trailing hypen error
nojaf Aug 6, 2025
7665a05
Mutate token on the call side.
nojaf Aug 6, 2025
0ea79fd
Add changelog entry
nojaf Aug 6, 2025
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
22 changes: 22 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,24 @@ make lib # Build compiler and standard library
./cli/bsc.js myTestFile.res
```

To view the tokens of a file run:

```sh
dune exec res_parser -- -print tokens myTestFile.res
```

To view the untyped tree of the file run:

```sh
./cli/bsc.js -dparsetree myTestFile.res
```

or

```sh
dune exec res_parser -- -print ast -recover myTestFile.res
```

To view the typed tree of the file run:

```sh
Expand All @@ -148,6 +160,16 @@ npm install
npm link rescript
```

#### Use Local BSC with Existing ReScript Installation

Alternatively, you can set the `RESCRIPT_BSC_EXE` environment variable to point to your locally compiled `bsc.exe`.

```sh
RESCRIPT_BSC_EXE=your-rescript-repo/packages/@rescript/darwin-arm64/bin/bsc.exe npx rescript
```

This will test the local compiler while still using the build system from the installed Node module.

### Running Automatic Tests

We provide different test suites for different levels of the compiler and build system infrastructure. Always make sure to locally build your compiler before running any tests.
Expand Down
14 changes: 10 additions & 4 deletions compiler/syntax/cli/res_cli.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,8 @@ end = struct
("-recover", Arg.Unit (fun () -> recover := true), "Emit partial ast");
( "-print",
Arg.String (fun txt -> print := txt),
"Print either binary, ml, ast, sexp, comments or res. Default: res" );
"Print either binary, ml, ast, sexp, comments, tokens or res. Default: \
res" );
( "-width",
Arg.Int (fun w -> width := w),
"Specify the line length for the printer (formatter)" );
Expand Down Expand Up @@ -239,11 +240,12 @@ module CliArgProcessor = struct
| "ast" -> Res_ast_debugger.print_engine
| "sexp" -> Res_ast_debugger.sexp_print_engine
| "comments" -> Res_ast_debugger.comments_print_engine
| "tokens" -> Res_token_debugger.token_print_engine
| "res" -> Res_driver.print_engine
| target ->
print_endline
("-print needs to be either binary, ml, ast, sexp, comments or res. \
You provided " ^ target);
("-print needs to be either binary, ml, ast, sexp, comments, tokens \
or res. You provided " ^ target);
exit 1
in

Expand All @@ -256,7 +258,11 @@ module CliArgProcessor = struct
let (Parser backend) = parsing_engine in
(* This is the whole purpose of the Color module above *)
Color.setup None;
if process_interface then

(* Special case for tokens - bypass parsing entirely *)
if target = "tokens" then
print_engine.print_implementation ~width ~filename ~comments:[] []
else if process_interface then
let parse_result = backend.parse_interface ~for_printer ~filename in
if parse_result.invalid then (
backend.string_of_diagnostics ~source:parse_result.source
Expand Down
172 changes: 59 additions & 113 deletions compiler/syntax/src/res_core.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,10 +715,42 @@ let parse_module_long_ident_tail ~lowercase p start_pos ident =
in
loop p ident

(* jsx allows for `-` token in the name, we need to combine some tokens into a single ident *)
let parse_jsx_ident (p : Parser.t) : unit =
(* check if the next tokens are minus and ident, if so, add them to the buffer *)
let rec visit buffer =
match p.Parser.token with
| Minus -> (
Parser.next p;
match p.Parser.token with
| Lident txt | Uident txt ->
Buffer.add_char buffer '-';
Buffer.add_string buffer txt;
if Scanner.peekMinus p.scanner then visit buffer else buffer
| _ -> buffer)
| _ -> buffer
in
match p.Parser.token with
| Lident txt when Scanner.peekMinus p.scanner ->
let buffer = Buffer.create (String.length txt) in
Buffer.add_string buffer txt;
Parser.next p;
let name = visit buffer |> Buffer.contents in
let token = Token.Lident name in
p.token <- token
| Uident txt when Scanner.peekMinus p.scanner ->
let buffer = Buffer.create (String.length txt) in
Buffer.add_string buffer txt;
Parser.next p;
let name = visit buffer |> Buffer.contents in
let token = Token.Uident name in
p.token <- token
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

[nitpick] Direct mutation of parser state (p.token <-) breaks encapsulation and makes the code harder to reason about. Consider using a proper parser method or returning the modified token instead of mutating parser state directly.

Suggested change
p.token <- token
| Uident txt when Scanner.peekMinus p.scanner ->
let buffer = Buffer.create (String.length txt) in
Buffer.add_string buffer txt;
Parser.next p;
let name = visit buffer |> Buffer.contents in
let token = Token.Uident name in
p.token <- token
set_token p token
| Uident txt when Scanner.peekMinus p.scanner ->
let buffer = Buffer.create (String.length txt) in
Buffer.add_string buffer txt;
Parser.next p;
let name = visit buffer |> Buffer.contents in
let token = Token.Uident name in
set_token p token

Copilot uses AI. Check for mistakes.

Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

[nitpick] Direct mutation of parser state (p.token <-) breaks encapsulation and makes the code harder to reason about. Consider using a proper parser method or returning the modified token instead of mutating parser state directly.

Suggested change
p.token <- token
| Uident txt when Scanner.peekMinus p.scanner ->
let buffer = Buffer.create (String.length txt) in
Buffer.add_string buffer txt;
Parser.next p;
let name = visit buffer |> Buffer.contents in
let token = Token.Uident name in
p.token <- token
set_token p token
| Uident txt when Scanner.peekMinus p.scanner ->
let buffer = Buffer.create (String.length txt) in
Buffer.add_string buffer txt;
Parser.next p;
let name = visit buffer |> Buffer.contents in
let token = Token.Uident name in
set_token p token

Copilot uses AI. Check for mistakes.

| _ -> ()

(* Parses module identifiers:
Foo
Foo.Bar *)
let parse_module_long_ident ~lowercase p =
let parse_module_long_ident ~lowercase ?(is_jsx_name : bool = false) p =
(* Parser.leaveBreadcrumb p Reporting.ModuleLongIdent; *)
let start_pos = p.Parser.start_pos in
let module_ident =
Expand All @@ -735,6 +767,7 @@ let parse_module_long_ident ~lowercase p =
match p.Parser.token with
| Dot ->
Parser.next p;
if is_jsx_name then parse_jsx_ident p;
parse_module_long_ident_tail ~lowercase p start_pos lident
| _ -> Location.mkloc lident (mk_loc start_pos end_pos))
| t ->
Expand All @@ -751,7 +784,8 @@ let verify_jsx_opening_closing_name p
| Lident lident ->
Parser.next p;
Longident.Lident lident
| Uident _ -> (parse_module_long_ident ~lowercase:true p).txt
| Uident _ ->
(parse_module_long_ident ~lowercase:true ~is_jsx_name:true p).txt
| _ -> Longident.Lident ""
in
let opening = name_longident.txt in
Expand Down Expand Up @@ -2540,6 +2574,8 @@ and parse_let_bindings ~attrs ~start_pos p =
(rec_flag, loop p [first])

and parse_jsx_name p : Longident.t Location.loc =
(* jsx allows for `-` token in the name, we need to combine some tokens *)
parse_jsx_ident p;
match p.Parser.token with
| Lident ident ->
let ident_start = p.start_pos in
Expand All @@ -2548,7 +2584,9 @@ and parse_jsx_name p : Longident.t Location.loc =
let loc = mk_loc ident_start ident_end in
Location.mkloc (Longident.Lident ident) loc
| Uident _ ->
let longident = parse_module_long_ident ~lowercase:true p in
let longident =
parse_module_long_ident ~lowercase:true ~is_jsx_name:true p
in
longident
| _ ->
let msg =
Expand All @@ -2566,7 +2604,6 @@ and parse_jsx_opening_or_self_closing_element (* start of the opening < *)
| Forwardslash ->
(* <foo a=b /> *)
Parser.next p;
Scanner.pop_mode p.scanner Jsx;
let jsx_end_pos = p.end_pos in
Parser.expect GreaterThan p;
let loc = mk_loc start_pos jsx_end_pos in
Expand All @@ -2578,24 +2615,25 @@ and parse_jsx_opening_or_self_closing_element (* start of the opening < *)
let children = parse_jsx_children p in
let closing_tag_start =
match p.token with
| LessThanSlash ->
| LessThan when Scanner.peekSlash p.scanner ->
let pos = p.start_pos in
(* Move to slash *)
Parser.next p;
Some pos
| LessThan ->
let pos = p.start_pos in
(* Move to ident *)
Parser.next p;
Parser.expect Forwardslash p;
Some pos
| token when Grammar.is_structure_item_start token -> None
| _ ->
Parser.expect LessThanSlash p;
Parser.expect LessThan p;
Parser.expect Forwardslash p;
None
in
(* Again, the ident in the closing tag can have a minus.
We combine these tokens into a single ident *)
parse_jsx_ident p;
match p.Parser.token with
| (Lident _ | Uident _) when verify_jsx_opening_closing_name p name ->
let end_tag_name = {name with loc = mk_loc p.start_pos p.end_pos} in
Scanner.pop_mode p.scanner Jsx;
let closing_tag_end = p.start_pos in
Parser.expect GreaterThan p;
let loc = mk_loc start_pos p.prev_end_pos in
Expand All @@ -2612,7 +2650,6 @@ and parse_jsx_opening_or_self_closing_element (* start of the opening < *)
Ast_helper.Exp.jsx_container_element ~loc name jsx_props opening_tag_end
children closing_tag
| token ->
Scanner.pop_mode p.scanner Jsx;
let () =
if Grammar.is_structure_item_start token then
let closing = "</" ^ string_of_longident name ^ ">" in
Expand All @@ -2632,91 +2669,11 @@ and parse_jsx_opening_or_self_closing_element (* start of the opening < *)
~loc:(mk_loc start_pos p.prev_end_pos)
name jsx_props opening_tag_end children None)
| token ->
Scanner.pop_mode p.scanner Jsx;
Parser.err p (Diagnostics.unexpected token p.breadcrumbs);
Ast_helper.Exp.jsx_unary_element
~loc:(mk_loc start_pos p.prev_end_pos)
name jsx_props

(* and parse_jsx_opening_or_self_closing_element_old ~start_pos p =
let jsx_start_pos = p.Parser.start_pos in
let name = parse_jsx_name p in
let jsx_props = parse_jsx_props p in
let children =
match p.Parser.token with
| Forwardslash ->
(* <foo a=b /> *)
let children_start_pos = p.Parser.start_pos in
Parser.next p;
let children_end_pos = p.Parser.start_pos in
Scanner.pop_mode p.scanner Jsx;
Parser.expect GreaterThan p;
let loc = mk_loc children_start_pos children_end_pos in
Ast_helper.Exp.make_list_expression loc [] None (* no children *)
| GreaterThan -> (
(* <foo a=b> bar </foo> *)
let children_start_pos = p.Parser.start_pos in
Parser.next p;
let spread, children = parse_jsx_children p in
let children_end_pos = p.Parser.start_pos in
let () =
match p.token with
| LessThanSlash -> Parser.next p
| LessThan ->
Parser.next p;
Parser.expect Forwardslash p
| token when Grammar.is_structure_item_start token -> ()
| _ -> Parser.expect LessThanSlash p
in
match p.Parser.token with
| (Lident _ | Uident _) when verify_jsx_opening_closing_name p name -> (
Scanner.pop_mode p.scanner Jsx;
Parser.expect GreaterThan p;
let loc = mk_loc children_start_pos children_end_pos in
match (spread, children) with
| true, child :: _ -> child
| _ -> Ast_helper.Exp.make_list_expression loc children None)
| token -> (
Scanner.pop_mode p.scanner Jsx;
let () =
if Grammar.is_structure_item_start token then
let closing = "</" ^ string_of_pexp_ident name ^ ">" in
let msg = Diagnostics.message ("Missing " ^ closing) in
Parser.err ~start_pos ~end_pos:p.prev_end_pos p msg
else
let opening = "</" ^ string_of_pexp_ident name ^ ">" in
let msg =
"Closing jsx name should be the same as the opening name. Did \
you mean " ^ opening ^ " ?"
in
Parser.err ~start_pos ~end_pos:p.prev_end_pos p
(Diagnostics.message msg);
Parser.expect GreaterThan p
in
let loc = mk_loc children_start_pos children_end_pos in
match (spread, children) with
| true, child :: _ -> child
| _ -> Ast_helper.Exp.make_list_expression loc children None))
| token ->
Scanner.pop_mode p.scanner Jsx;
Parser.err p (Diagnostics.unexpected token p.breadcrumbs);
Ast_helper.Exp.make_list_expression Location.none [] None
in
let jsx_end_pos = p.prev_end_pos in
let loc = mk_loc jsx_start_pos jsx_end_pos in
Ast_helper.Exp.apply ~loc name
(List.concat
[
jsx_props;
[
(Asttypes.Labelled {txt = "children"; loc = Location.none}, children);
( Asttypes.Nolabel,
Ast_helper.Exp.construct
(Location.mknoloc (Longident.Lident "()"))
None );
];
]) *)

(*
* jsx ::=
* | <> jsx-children </>
Expand All @@ -2726,7 +2683,6 @@ and parse_jsx_opening_or_self_closing_element (* start of the opening < *)
* jsx-children ::= primary-expr* * => 0 or more
*)
and parse_jsx p =
Scanner.set_jsx_mode p.Parser.scanner;
Parser.leave_breadcrumb p Grammar.Jsx;
let start_pos = p.Parser.start_pos in
Parser.expect LessThan p;
Expand Down Expand Up @@ -2754,9 +2710,8 @@ and parse_jsx_fragment start_pos p =
Parser.expect GreaterThan p;
let children = parse_jsx_children p in
let children_end_pos = p.Parser.start_pos in
if p.token = LessThan then p.token <- Scanner.reconsider_less_than p.scanner;
Parser.expect LessThanSlash p;
Scanner.pop_mode p.scanner Jsx;
Parser.expect LessThan p;
Parser.expect Forwardslash p;
let end_pos = p.Parser.end_pos in
Parser.expect GreaterThan p;
(* location is from starting < till closing > *)
Expand All @@ -2773,6 +2728,8 @@ and parse_jsx_fragment start_pos p =
* | {...jsx_expr}
*)
and parse_jsx_prop p : Parsetree.jsx_prop option =
(* prop can have `-`, we need to combine some tokens into a single ident *)
parse_jsx_ident p;
match p.Parser.token with
| Question | Lident _ -> (
let optional = Parser.optional p Question in
Expand All @@ -2785,26 +2742,22 @@ and parse_jsx_prop p : Parsetree.jsx_prop option =
Parser.next p;
(* no punning *)
let optional = Parser.optional p Question in
Scanner.pop_mode p.scanner Jsx;
let attr_expr = parse_primary_expr ~operand:(parse_atomic_expr p) p in
Some (Parsetree.JSXPropValue ({txt = name; loc}, optional, attr_expr))
| _ -> Some (Parsetree.JSXPropPunning (false, {txt = name; loc})))
(* {...props} *)
| Lbrace -> (
Scanner.pop_mode p.scanner Jsx;
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 is rather confusing when you are in a nested jsx scenario:

<div>
  <p>
    {foo}
  </p>
</div>

Popping Jsx from p requires the pop of div also to happen to get out of Jsx mode.

let spread_start = p.Parser.start_pos in
Parser.next p;
match p.Parser.token with
| DotDotDot -> (
Scanner.pop_mode p.scanner Jsx;
Parser.next p;
let attr_expr = parse_primary_expr ~operand:(parse_expr p) p in
match p.Parser.token with
| Rbrace ->
let spread_end = p.Parser.end_pos in
let loc = mk_loc spread_start spread_end in
Parser.next p;
Scanner.set_jsx_mode p.scanner;
Some (Parsetree.JSXPropSpreading (loc, attr_expr))
(* Some (label, attr_expr) *)
| _ -> None)
Expand All @@ -2815,26 +2768,20 @@ and parse_jsx_props p : Parsetree.jsx_prop list =
parse_region ~grammar:Grammar.JsxAttribute ~f:parse_jsx_prop p

and parse_jsx_children p : Parsetree.jsx_children =
Scanner.pop_mode p.scanner Jsx;
let rec loop p children =
match p.Parser.token with
| Token.Eof | LessThanSlash -> children
| Token.Eof -> children
| LessThan when Scanner.peekSlash p.scanner -> children
| LessThan ->
(* Imagine: <div> <Navbar /> <
* is `<` the start of a jsx-child? <div …
* or is it the start of a closing tag? </div>
* reconsiderLessThan peeks at the next token and
* determines the correct token to disambiguate *)
let token = Scanner.reconsider_less_than p.scanner in
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 is what bother me a bit, there is LessThanSlash above, yet we still need to do the reconsider_less_than call.

if token = LessThan then
let child =
parse_primary_expr ~operand:(parse_atomic_expr p) ~no_call:true p
in
loop p (child :: children)
else
(* LessThanSlash *)
let () = p.token <- token in
children
let child =
parse_primary_expr ~operand:(parse_atomic_expr p) ~no_call:true p
in
loop p (child :: children)
| token when Grammar.is_jsx_child_start token ->
let child =
parse_primary_expr ~operand:(parse_atomic_expr p) ~no_call:true p
Expand All @@ -2854,7 +2801,6 @@ and parse_jsx_children p : Parsetree.jsx_children =
let children = List.rev (loop p []) in
Parsetree.JSXChildrenItems children
in
Scanner.set_jsx_mode p.scanner;
children

and parse_braced_or_record_expr p =
Expand Down
Loading
Loading