Skip to content

Heuristic for JSX empty prop expr completion #935

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 3 commits into from
Mar 1, 2024
Merged

Heuristic for JSX empty prop expr completion #935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a93ec9c
apply heuristic for allowing empty JSX prop expr completion
zth Feb 29, 2024
0354606
changelog
zth Feb 29, 2024
9649a82
add examples with punning
zth Mar 1, 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 @@ -15,6 +15,7 @@
#### :bug: Bug Fix

- Fix issue where completion inside of switch expression would not work in some cases. https://github.com/rescript-lang/rescript-vscode/pull/936
- Fix bug that made empty prop expressions in JSX not complete if in the middle of a JSX element. https://github.com/rescript-lang/rescript-vscode/pull/935

## 1.40.0

Expand Down
38 changes: 30 additions & 8 deletions analysis/src/CompletionBackEnd.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,7 +1053,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
~kind:(Completion.Value (Ctype.newty (Ttuple typeExrps)));
]
else []
| CJsxPropValue {pathToComponent; propName} -> (
| CJsxPropValue {pathToComponent; propName; emptyJsxPropNameHint} -> (
if Debug.verbose () then print_endline "[ctx_path]--> CJsxPropValue";
let findTypeOfValue path =
path
Expand All @@ -1067,7 +1067,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
| _ -> false
in
(* TODO(env-stuff) Does this need to potentially be instantiated with type args too? *)
let targetLabel =
let labels =
if lowercaseComponent then
let rec digToTypeForCompletion path =
match
Expand All @@ -1081,17 +1081,39 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
ReactDOM.domProps is an alias for JsxEvent.t. *)
let pathRev = p |> Utils.expandPath in
pathRev |> List.rev |> digToTypeForCompletion
| {kind = Type {kind = Record fields}} :: _ -> (
match fields |> List.find_opt (fun f -> f.fname.txt = propName) with
| None -> None
| Some f -> Some (f.fname.txt, f.typ, env))
| _ -> None
| {kind = Type {kind = Record fields}} :: _ ->
fields |> List.map (fun f -> (f.fname.txt, f.typ, env))
| _ -> []
in
TypeUtils.pathToElementProps package |> digToTypeForCompletion
else
CompletionJsx.getJsxLabels ~componentPath:pathToComponent
~findTypeOfValue ~package
|> List.find_opt (fun (label, _, _) -> label = propName)
in
(* We have a heuristic that kicks in when completing empty prop expressions in the middle of a JSX element,
like <SomeComp firstProp=test second=<com> third=123 />.
The parser turns that broken JSX into: <SomeComp firstProp=test second=<com>third />, 123.

So, we use a heuristic that covers this scenario by picking up on the cursor being between
the prop name and the prop expression, and the prop expression being an ident that's a
_valid prop name_ for that JSX element.

This works because the ident itself will always be the next prop name (since that's what the
parser eats). So, we do a simple lookup of that hint here if it exists, to make sure the hint
is indeed a valid label for this JSX element. *)
let emptyJsxPropNameHintIsCorrect =
match emptyJsxPropNameHint with
| Some identName when identName != propName ->
labels
|> List.find_opt (fun (f, _, _) -> f = identName)
|> Option.is_some
| Some _ -> false
| None -> true
in
let targetLabel =
if emptyJsxPropNameHintIsCorrect then
labels |> List.find_opt (fun (f, _, _) -> f = propName)
else None
in
match targetLabel with
| None -> []
Expand Down
1 change: 1 addition & 0 deletions analysis/src/CompletionFrontEnd.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,7 @@ let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor
pathToComponent =
Utils.flattenLongIdent ~jsx:true props.compName.txt;
propName = prop.name;
emptyJsxPropNameHint = None;
});
expr iterator prop.exp;
resetCurrentCtxPath previousCtxPath)
Expand Down
25 changes: 24 additions & 1 deletion analysis/src/CompletionJsx.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,27 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
print_endline
"[jsx_props_completable]--> Cursor between the prop name and expr \
assigned";
None)
match (firstCharBeforeCursorNoWhite, prop.exp) with
| Some '=', {pexp_desc = Pexp_ident {txt = Lident txt}} ->
if Debug.verbose () then
Printf.printf
"[jsx_props_completable]--> Heuristic for empty JSX prop expr \
completion.\n";
Some
(Cexpression
{
contextPath =
CJsxPropValue
{
pathToComponent =
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
propName = prop.name;
emptyJsxPropNameHint = Some txt;
};
nested = [];
prefix = "";
})
| _ -> None)
else if prop.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then (
if Debug.verbose () then
print_endline "[jsx_props_completable]--> Cursor on expr assigned";
Expand All @@ -851,6 +871,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
pathToComponent =
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
propName = prop.name;
emptyJsxPropNameHint = None;
};
nested = List.rev nested;
prefix;
Expand All @@ -871,6 +892,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
pathToComponent =
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
propName = prop.name;
emptyJsxPropNameHint = None;
};
prefix = "";
nested = [];
Expand All @@ -894,6 +916,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
pathToComponent =
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
propName = prop.name;
emptyJsxPropNameHint = None;
};
prefix = "";
nested = [];
Expand Down
7 changes: 6 additions & 1 deletion analysis/src/SharedTypes.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,12 @@ module Completable = struct
functionContextPath: contextPath;
argumentLabel: argumentLabel;
}
| CJsxPropValue of {pathToComponent: string list; propName: string}
| CJsxPropValue of {
pathToComponent: string list;
propName: string;
emptyJsxPropNameHint: string option;
(* This helps handle a special case in JSX prop completion. More info where this is used. *)
}
| CPatternPath of {rootCtxPath: contextPath; nested: nestedPath list}
| CTypeAtPos of Location.t
(** A position holding something that might have a *compiled* type. *)
Expand Down
18 changes: 18 additions & 0 deletions analysis/tests/src/CompletionJsx.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,21 @@ module IntrinsicElementLowercase = {

// <IntrinsicElementLowercase
// ^com

module MultiPropComp = {
type time = Now | Later
@react.component
let make = (~name, ~age, ~time: time) => {
ignore(time)
name ++ age
}
}

// <MultiPropComp name="Hello" time= age="35"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add examples with prop punning, and where there is more than one prop assignment already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, check the latest commit.

// ^com

// <MultiPropComp name="Hello" time= age
// ^com

// <MultiPropComp name time= age
// ^com
78 changes: 78 additions & 0 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 @@ -543,3 +543,81 @@ Path IntrinsicElementLowercase.make
"documentation": null
}]

Complete src/CompletionJsx.res 73:36
posCursor:[73:36] posNoWhite:[73:35] Found expr:[73:4->73:41]
JSX <MultiPropComp:[73:4->73:17] name[73:18->73:22]=...[73:23->73:30] time[73:31->73:35]=...[73:37->73:40]> _children:None
Completable: Cexpression CJsxPropValue [MultiPropComp] time
Package opens Pervasives.JsxModules.place holder
Resolved opens 1 pervasives
ContextPath CJsxPropValue [MultiPropComp] time
Path MultiPropComp.make
[{
"label": "Now",
"kind": 4,
"tags": [],
"detail": "Now\n\ntype time = Now | Later",
"documentation": null,
"insertText": "{Now}",
"insertTextFormat": 2
}, {
"label": "Later",
"kind": 4,
"tags": [],
"detail": "Later\n\ntype time = Now | Later",
"documentation": null,
"insertText": "{Later}",
"insertTextFormat": 2
}]

Complete src/CompletionJsx.res 76:36
posCursor:[76:36] posNoWhite:[76:35] Found expr:[76:4->76:40]
JSX <MultiPropComp:[76:4->76:17] name[76:18->76:22]=...[76:23->76:30] time[76:31->76:35]=...[76:37->76:40]> _children:None
Completable: Cexpression CJsxPropValue [MultiPropComp] time
Package opens Pervasives.JsxModules.place holder
Resolved opens 1 pervasives
ContextPath CJsxPropValue [MultiPropComp] time
Path MultiPropComp.make
[{
"label": "Now",
"kind": 4,
"tags": [],
"detail": "Now\n\ntype time = Now | Later",
"documentation": null,
"insertText": "{Now}",
"insertTextFormat": 2
}, {
"label": "Later",
"kind": 4,
"tags": [],
"detail": "Later\n\ntype time = Now | Later",
"documentation": null,
"insertText": "{Later}",
"insertTextFormat": 2
}]

Complete src/CompletionJsx.res 79:28
posCursor:[79:28] posNoWhite:[79:27] Found expr:[79:4->79:32]
JSX <MultiPropComp:[79:4->79:17] name[79:18->79:22]=...[79:18->79:22] time[79:23->79:27]=...[79:29->79:32]> _children:None
Completable: Cexpression CJsxPropValue [MultiPropComp] time
Package opens Pervasives.JsxModules.place holder
Resolved opens 1 pervasives
ContextPath CJsxPropValue [MultiPropComp] time
Path MultiPropComp.make
[{
"label": "Now",
"kind": 4,
"tags": [],
"detail": "Now\n\ntype time = Now | Later",
"documentation": null,
"insertText": "{Now}",
"insertTextFormat": 2
}, {
"label": "Later",
"kind": 4,
"tags": [],
"detail": "Later\n\ntype time = Now | Later",
"documentation": null,
"insertText": "{Later}",
"insertTextFormat": 2
}]

17 changes: 16 additions & 1 deletion analysis/tests/src/expected/Jsx2.res.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,22 @@ Path Outer.Inner.
Complete src/Jsx2.res 136:7
posCursor:[136:7] posNoWhite:[136:6] Found expr:[135:3->138:9]
JSX <div:[135:3->135:6] x[136:5->136:6]=...[138:4->138:8]> _children:None
[]
Completable: Cexpression CJsxPropValue [div] x
Package opens Pervasives.JsxModules.place holder
Resolved opens 1 pervasives
ContextPath CJsxPropValue [div] x
Path ReactDOM.domProps
Path PervasivesU.JsxDOM.domProps
[{
"label": "\"\"",
"kind": 12,
"tags": [],
"detail": "string",
"documentation": null,
"sortText": "A",
"insertText": "{\"$0\"}",
"insertTextFormat": 2
}]

Complete src/Jsx2.res 150:21
posCursor:[150:21] posNoWhite:[150:20] Found expr:[150:12->150:32]
Expand Down