Skip to content

Latest version of parser #917

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 2 commits into from
Feb 11, 2024
Merged

Latest version of parser #917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa91f47
latest version of parser
zth Feb 7, 2024
043dd72
changelog
zth Feb 7, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
- Completion for import attributes in `@module`. https://github.com/rescript-lang/rescript-vscode/pull/913
- Relax filter for what local files that come up in from and regular string completion in `@module`. https://github.com/rescript-lang/rescript-vscode/pull/918
- Make from completion trigger for expr hole so we get a nice experience when completing {from: <com>} in `@module`. https://github.com/rescript-lang/rescript-vscode/pull/918
- Latest parser for newest syntax features. https://github.com/rescript-lang/rescript-vscode/pull/917

## 1.38.0

Expand Down
22 changes: 11 additions & 11 deletions analysis/tests/src/expected/CompletionJsx.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,17 +429,17 @@ posCursor:[30:12] posNoWhite:[30:11] Found expr:[11:4->32:10]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[12:4->32:10]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[15:5->32:10]
JSX <div:[15:5->15:8] > _children:15:8
posCursor:[30:12] posNoWhite:[30:11] Found expr:[15:8->32:4]
Pexp_construct :::[16:7->32:4] [16:7->32:4]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[16:7->32:4]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[17:7->32:4]
Pexp_construct :::[17:7->32:4] [17:7->32:4]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[17:7->32:4]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->32:4]
Pexp_construct :::[30:10->32:4] [30:10->32:4]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->32:4]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->30:12]
JSX <di:[30:10->30:12] > _children:None
posCursor:[30:12] posNoWhite:[30:11] Found expr:[15:8->33:2]
Copy link
Collaborator

Choose a reason for hiding this comment

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

which location is correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure actually. Think it's a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Depends on whether they're wrong or fixed now.

Copy link
Collaborator

@cristianoc cristianoc Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure why the output is different, but it's this test:

    <div>
      {React.string(someProp)}
      <div> {React.null} </div>
      // {someString->st}
      //                ^com
      // {"Some string"->st}
      //                   ^com
      // {"Some string"->Js.String2.trim->st}
      //                                    ^com
      // {someInt->}
      //           ^com
      // {12->}
      //      ^com
      // {someArr->a}
      //            ^com
      // <di
      //    ^com
    </div>

I think when completing di, it finds the following /div and now considers that "div" as a prop of di, while before it did not.

The one thing that comes to mind is that the treatment of nested jsx could have changed with the jsx mode pops etc in this:
rescript-lang/rescript#6609

@tsnobip any thoughts?
Not sure there's anything to do, just double checking if anything comes to mind. Otherwise we can just go ahead.

Copy link
Member

Choose a reason for hiding this comment

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

it is totally possible that my PR accidentally created some regressions, do you have an example in mind of syntax that should not be recognized but is now? I could add tests. I'm not really happy about working with the syntax parser right now, especially around mode, I think it can introduce bugs that are hard to spot.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we should likely add tests around this. I'm not sure what's the best way to do that.
Basically, before, jsx mode tended to "leak" outside of jsx, which created unwanted behavior like accepting hyphens in identifiers outside of jsx. I tightened the jsx mode to avoid that, but I might have restricted it too much. I might have popped jsx mode one time too many.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be that the difference here is because jsx mode is now correctly restored while before it was not.
In which case that's great.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tested this for a bit and I can't find any obvious issues with it. Not tests fail either from what I see. I'd like to merge this, because 2 other PRs depend on this, but I'd also not want to drop the ball here on if this change actually is problematic. How can we investigate it further? Add tests in the syntax for nested JSX mode?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be merged.
One can add a couple of tests of nested jsx separately in the compiler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tsnobip would you be up for adding tests for various forms of nested JSX in the compiler repo?

Pexp_construct :::[16:7->33:2] [16:7->33:2]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[16:7->33:2]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[17:7->33:2]
Pexp_construct :::[17:7->33:2] [17:7->33:2]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[17:7->33:2]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->33:2]
Pexp_construct :::[30:10->33:2] [30:10->33:2]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->33:2]
posCursor:[30:12] posNoWhite:[30:11] Found expr:[30:10->32:10]
JSX <di:[30:10->30:12] div[32:6->32:9]=...[32:6->32:9]> _children:32:9
Completable: ChtmlElement <di
Package opens Pervasives.JsxModules.place holder
Resolved opens 1 pervasives
Expand Down
22 changes: 15 additions & 7 deletions analysis/vendor/res_syntax/jsx_ppx.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,23 @@ let getString ~key fields = fields |> getJsxConfigByKey ~key ~type_:String

let updateConfig config payload =
let fields = getPayloadFields payload in
(match getInt ~key:"version" fields with
| None -> ()
| Some i -> config.Jsx_common.version <- i);
(match getString ~key:"module_" fields with
let moduleRaw = getString ~key:"module_" fields in
let isGeneric =
match moduleRaw |> Option.map (fun m -> String.lowercase_ascii m) with
| Some "react" | None -> false
| Some _ -> true
in
(match (isGeneric, getInt ~key:"version" fields) with
| true, _ -> config.Jsx_common.version <- 4
| false, Some i -> config.Jsx_common.version <- i
| _ -> ());
(match moduleRaw with
| None -> ()
| Some s -> config.module_ <- s);
match getString ~key:"mode" fields with
| None -> ()
| Some s -> config.mode <- s
match (isGeneric, getString ~key:"mode" fields) with
| true, _ -> config.mode <- "automatic"
| false, Some s -> config.mode <- s
| _ -> ()

let isJsxConfigAttr ((loc, _) : attribute) = loc.txt = "jsxConfig"

Expand Down
67 changes: 18 additions & 49 deletions analysis/vendor/res_syntax/jsx_v4.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ open Asttypes
open Parsetree
open Longident

let moduleAccessName config = String.capitalize_ascii config.Jsx_common.module_
let moduleAccessName config value =
String.capitalize_ascii config.Jsx_common.module_ ^ "." ^ value
|> Longident.parse

let nolabel = Nolabel

Expand Down Expand Up @@ -384,10 +386,7 @@ let transformUppercaseCall3 ~config modulePath mapper jsxExprLoc callExprLoc
( labelled "children",
Exp.apply
(Exp.ident
{
txt = Ldot (Lident (moduleAccessName config), "array");
loc = Location.none;
})
{txt = moduleAccessName config "array"; loc = Location.none})
[(Nolabel, expression)] );
]
| _ ->
Expand Down Expand Up @@ -431,31 +430,17 @@ let transformUppercaseCall3 ~config modulePath mapper jsxExprLoc callExprLoc
match (!childrenArg, keyProp) with
| None, key :: _ ->
( Exp.ident
{
loc = Location.none;
txt = Ldot (Lident (moduleAccessName config), "jsxKeyed");
},
{loc = Location.none; txt = moduleAccessName config "jsxKeyed"},
[key; (nolabel, unitExpr ~loc:Location.none)] )
| None, [] ->
( Exp.ident
{
loc = Location.none;
txt = Ldot (Lident (moduleAccessName config), "jsx");
},
( Exp.ident {loc = Location.none; txt = moduleAccessName config "jsx"},
[] )
| Some _, key :: _ ->
( Exp.ident
{
loc = Location.none;
txt = Ldot (Lident (moduleAccessName config), "jsxsKeyed");
},
{loc = Location.none; txt = moduleAccessName config "jsxsKeyed"},
[key; (nolabel, unitExpr ~loc:Location.none)] )
| Some _, [] ->
( Exp.ident
{
loc = Location.none;
txt = Ldot (Lident (moduleAccessName config), "jsxs");
},
( Exp.ident {loc = Location.none; txt = moduleAccessName config "jsxs"},
[] )
in
Exp.apply ~loc:jsxExprLoc ~attrs jsxExpr
Expand Down Expand Up @@ -500,9 +485,9 @@ let transformLowercaseCall3 ~config mapper jsxExprLoc callExprLoc attrs
(* the new jsx transform *)
| "automatic" ->
let elementBinding =
match moduleAccessName config with
| "React" -> Lident "ReactDOM"
| generic -> Ldot (Lident generic, "DOM")
match config.module_ |> String.lowercase_ascii with
| "react" -> Lident "ReactDOM"
| _generic -> moduleAccessName config "Elements"
in

let children, nonChildrenProps =
Expand Down Expand Up @@ -539,10 +524,7 @@ let transformLowercaseCall3 ~config mapper jsxExprLoc callExprLoc attrs
( labelled "children",
Exp.apply
(Exp.ident
{
txt = Ldot (Lident (moduleAccessName config), "array");
loc = Location.none;
})
{txt = moduleAccessName config "array"; loc = Location.none})
[(Nolabel, expression)] );
]
in
Expand Down Expand Up @@ -1203,10 +1185,7 @@ let transformStructureItem ~config item =
(* can't be an arrow because it will defensively uncurry *)
let newExternalType =
Ptyp_constr
( {
loc = pstr_loc;
txt = Ldot (Lident (moduleAccessName config), "componentLike");
},
( {loc = pstr_loc; txt = moduleAccessName config "componentLike"},
[retPropsType; innerType] )
in
let newStructure =
Expand Down Expand Up @@ -1321,10 +1300,7 @@ let transformSignatureItem ~config item =
(* can't be an arrow because it will defensively uncurry *)
let newExternalType =
Ptyp_constr
( {
loc = psig_loc;
txt = Ldot (Lident (moduleAccessName config), "componentLike");
},
( {loc = psig_loc; txt = moduleAccessName config "componentLike"},
[retPropsType; innerType] )
in
let newStructure =
Expand Down Expand Up @@ -1419,8 +1395,7 @@ let expr ~config mapper expression =
let fragment =
match config.mode with
| "automatic" ->
Exp.ident ~loc
{loc; txt = Ldot (Lident (moduleAccessName config), "jsxFragment")}
Exp.ident ~loc {loc; txt = moduleAccessName config "jsxFragment"}
| "classic" | _ ->
Exp.ident ~loc {loc; txt = Ldot (Lident "React", "fragment")}
in
Expand All @@ -1431,10 +1406,7 @@ let expr ~config mapper expression =
let applyJsxArray expr =
Exp.apply
(Exp.ident
{
txt = Ldot (Lident (moduleAccessName config), "array");
loc = Location.none;
})
{txt = moduleAccessName config "array"; loc = Location.none})
[(Nolabel, expr)]
in
let countOfChildren = function
Expand Down Expand Up @@ -1472,11 +1444,8 @@ let expr ~config mapper expression =
(match config.mode with
| "automatic" ->
if countOfChildren childrenExpr > 1 then
Exp.ident ~loc
{loc; txt = Ldot (Lident (moduleAccessName config), "jsxs")}
else
Exp.ident ~loc
{loc; txt = Ldot (Lident (moduleAccessName config), "jsx")}
Exp.ident ~loc {loc; txt = moduleAccessName config "jsxs"}
else Exp.ident ~loc {loc; txt = moduleAccessName config "jsx"}
| "classic" | _ ->
if countOfChildren childrenExpr > 1 then
Exp.ident ~loc
Expand Down
Loading