Skip to content

Commit 54d9a4d

Browse files
zthzth
authored
Heuristic for JSX empty prop expr completion (#935)
* apply heuristic for allowing empty JSX prop expr completion * changelog * add examples with punning
1 parent d01a08c commit 54d9a4d

8 files changed

+174
-11
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#### :bug: Bug Fix
1616

1717
- Fix issue where completion inside of switch expression would not work in some cases. https://github.com/rescript-lang/rescript-vscode/pull/936
18+
- 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
1819

1920
## 1.40.0
2021

‎analysis/src/CompletionBackEnd.ml

+30-8
Original file line numberDiff line numberDiff line change
@@ -1053,7 +1053,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
10531053
~kind:(Completion.Value (Ctype.newty (Ttuple typeExrps)));
10541054
]
10551055
else []
1056-
| CJsxPropValue {pathToComponent; propName} -> (
1056+
| CJsxPropValue {pathToComponent; propName; emptyJsxPropNameHint} -> (
10571057
if Debug.verbose () then print_endline "[ctx_path]--> CJsxPropValue";
10581058
let findTypeOfValue path =
10591059
path
@@ -1067,7 +1067,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
10671067
| _ -> false
10681068
in
10691069
(* TODO(env-stuff) Does this need to potentially be instantiated with type args too? *)
1070-
let targetLabel =
1070+
let labels =
10711071
if lowercaseComponent then
10721072
let rec digToTypeForCompletion path =
10731073
match
@@ -1081,17 +1081,39 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
10811081
ReactDOM.domProps is an alias for JsxEvent.t. *)
10821082
let pathRev = p |> Utils.expandPath in
10831083
pathRev |> List.rev |> digToTypeForCompletion
1084-
| {kind = Type {kind = Record fields}} :: _ -> (
1085-
match fields |> List.find_opt (fun f -> f.fname.txt = propName) with
1086-
| None -> None
1087-
| Some f -> Some (f.fname.txt, f.typ, env))
1088-
| _ -> None
1084+
| {kind = Type {kind = Record fields}} :: _ ->
1085+
fields |> List.map (fun f -> (f.fname.txt, f.typ, env))
1086+
| _ -> []
10891087
in
10901088
TypeUtils.pathToElementProps package |> digToTypeForCompletion
10911089
else
10921090
CompletionJsx.getJsxLabels ~componentPath:pathToComponent
10931091
~findTypeOfValue ~package
1094-
|> List.find_opt (fun (label, _, _) -> label = propName)
1092+
in
1093+
(* We have a heuristic that kicks in when completing empty prop expressions in the middle of a JSX element,
1094+
like <SomeComp firstProp=test second=<com> third=123 />.
1095+
The parser turns that broken JSX into: <SomeComp firstProp=test second=<com>third />, 123.
1096+
1097+
So, we use a heuristic that covers this scenario by picking up on the cursor being between
1098+
the prop name and the prop expression, and the prop expression being an ident that's a
1099+
_valid prop name_ for that JSX element.
1100+
1101+
This works because the ident itself will always be the next prop name (since that's what the
1102+
parser eats). So, we do a simple lookup of that hint here if it exists, to make sure the hint
1103+
is indeed a valid label for this JSX element. *)
1104+
let emptyJsxPropNameHintIsCorrect =
1105+
match emptyJsxPropNameHint with
1106+
| Some identName when identName != propName ->
1107+
labels
1108+
|> List.find_opt (fun (f, _, _) -> f = identName)
1109+
|> Option.is_some
1110+
| Some _ -> false
1111+
| None -> true
1112+
in
1113+
let targetLabel =
1114+
if emptyJsxPropNameHintIsCorrect then
1115+
labels |> List.find_opt (fun (f, _, _) -> f = propName)
1116+
else None
10951117
in
10961118
match targetLabel with
10971119
| None -> []

‎analysis/src/CompletionFrontEnd.ml

+1
Original file line numberDiff line numberDiff line change
@@ -952,6 +952,7 @@ let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor
952952
pathToComponent =
953953
Utils.flattenLongIdent ~jsx:true props.compName.txt;
954954
propName = prop.name;
955+
emptyJsxPropNameHint = None;
955956
});
956957
expr iterator prop.exp;
957958
resetCurrentCtxPath previousCtxPath)

‎analysis/src/CompletionJsx.ml

+24-1
Original file line numberDiff line numberDiff line change
@@ -833,7 +833,27 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
833833
print_endline
834834
"[jsx_props_completable]--> Cursor between the prop name and expr \
835835
assigned";
836-
None)
836+
match (firstCharBeforeCursorNoWhite, prop.exp) with
837+
| Some '=', {pexp_desc = Pexp_ident {txt = Lident txt}} ->
838+
if Debug.verbose () then
839+
Printf.printf
840+
"[jsx_props_completable]--> Heuristic for empty JSX prop expr \
841+
completion.\n";
842+
Some
843+
(Cexpression
844+
{
845+
contextPath =
846+
CJsxPropValue
847+
{
848+
pathToComponent =
849+
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
850+
propName = prop.name;
851+
emptyJsxPropNameHint = Some txt;
852+
};
853+
nested = [];
854+
prefix = "";
855+
})
856+
| _ -> None)
837857
else if prop.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then (
838858
if Debug.verbose () then
839859
print_endline "[jsx_props_completable]--> Cursor on expr assigned";
@@ -851,6 +871,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
851871
pathToComponent =
852872
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
853873
propName = prop.name;
874+
emptyJsxPropNameHint = None;
854875
};
855876
nested = List.rev nested;
856877
prefix;
@@ -871,6 +892,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
871892
pathToComponent =
872893
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
873894
propName = prop.name;
895+
emptyJsxPropNameHint = None;
874896
};
875897
prefix = "";
876898
nested = [];
@@ -894,6 +916,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
894916
pathToComponent =
895917
Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt;
896918
propName = prop.name;
919+
emptyJsxPropNameHint = None;
897920
};
898921
prefix = "";
899922
nested = [];

‎analysis/src/SharedTypes.ml

+6-1
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,12 @@ module Completable = struct
622622
functionContextPath: contextPath;
623623
argumentLabel: argumentLabel;
624624
}
625-
| CJsxPropValue of {pathToComponent: string list; propName: string}
625+
| CJsxPropValue of {
626+
pathToComponent: string list;
627+
propName: string;
628+
emptyJsxPropNameHint: string option;
629+
(* This helps handle a special case in JSX prop completion. More info where this is used. *)
630+
}
626631
| CPatternPath of {rootCtxPath: contextPath; nested: nestedPath list}
627632
| CTypeAtPos of Location.t
628633
(** A position holding something that might have a *compiled* type. *)

‎analysis/tests/src/CompletionJsx.res

+18
Original file line numberDiff line numberDiff line change
@@ -61,3 +61,21 @@ module IntrinsicElementLowercase = {
6161

6262
// <IntrinsicElementLowercase
6363
// ^com
64+
65+
module MultiPropComp = {
66+
type time = Now | Later
67+
@react.component
68+
let make = (~name, ~age, ~time: time) => {
69+
ignore(time)
70+
name ++ age
71+
}
72+
}
73+
74+
// <MultiPropComp name="Hello" time= age="35"
75+
// ^com
76+
77+
// <MultiPropComp name="Hello" time= age
78+
// ^com
79+
80+
// <MultiPropComp name time= age
81+
// ^com

‎analysis/tests/src/expected/CompletionJsx.res.txt

+78
Original file line numberDiff line numberDiff line change
@@ -543,3 +543,81 @@ Path IntrinsicElementLowercase.make
543543
"documentation": null
544544
}]
545545

546+
Complete src/CompletionJsx.res 73:36
547+
posCursor:[73:36] posNoWhite:[73:35] Found expr:[73:4->73:41]
548+
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
549+
Completable: Cexpression CJsxPropValue [MultiPropComp] time
550+
Package opens Pervasives.JsxModules.place holder
551+
Resolved opens 1 pervasives
552+
ContextPath CJsxPropValue [MultiPropComp] time
553+
Path MultiPropComp.make
554+
[{
555+
"label": "Now",
556+
"kind": 4,
557+
"tags": [],
558+
"detail": "Now\n\ntype time = Now | Later",
559+
"documentation": null,
560+
"insertText": "{Now}",
561+
"insertTextFormat": 2
562+
}, {
563+
"label": "Later",
564+
"kind": 4,
565+
"tags": [],
566+
"detail": "Later\n\ntype time = Now | Later",
567+
"documentation": null,
568+
"insertText": "{Later}",
569+
"insertTextFormat": 2
570+
}]
571+
572+
Complete src/CompletionJsx.res 76:36
573+
posCursor:[76:36] posNoWhite:[76:35] Found expr:[76:4->76:40]
574+
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
575+
Completable: Cexpression CJsxPropValue [MultiPropComp] time
576+
Package opens Pervasives.JsxModules.place holder
577+
Resolved opens 1 pervasives
578+
ContextPath CJsxPropValue [MultiPropComp] time
579+
Path MultiPropComp.make
580+
[{
581+
"label": "Now",
582+
"kind": 4,
583+
"tags": [],
584+
"detail": "Now\n\ntype time = Now | Later",
585+
"documentation": null,
586+
"insertText": "{Now}",
587+
"insertTextFormat": 2
588+
}, {
589+
"label": "Later",
590+
"kind": 4,
591+
"tags": [],
592+
"detail": "Later\n\ntype time = Now | Later",
593+
"documentation": null,
594+
"insertText": "{Later}",
595+
"insertTextFormat": 2
596+
}]
597+
598+
Complete src/CompletionJsx.res 79:28
599+
posCursor:[79:28] posNoWhite:[79:27] Found expr:[79:4->79:32]
600+
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
601+
Completable: Cexpression CJsxPropValue [MultiPropComp] time
602+
Package opens Pervasives.JsxModules.place holder
603+
Resolved opens 1 pervasives
604+
ContextPath CJsxPropValue [MultiPropComp] time
605+
Path MultiPropComp.make
606+
[{
607+
"label": "Now",
608+
"kind": 4,
609+
"tags": [],
610+
"detail": "Now\n\ntype time = Now | Later",
611+
"documentation": null,
612+
"insertText": "{Now}",
613+
"insertTextFormat": 2
614+
}, {
615+
"label": "Later",
616+
"kind": 4,
617+
"tags": [],
618+
"detail": "Later\n\ntype time = Now | Later",
619+
"documentation": null,
620+
"insertText": "{Later}",
621+
"insertTextFormat": 2
622+
}]
623+

‎analysis/tests/src/expected/Jsx2.res.txt

+16-1
Original file line numberDiff line numberDiff line change
@@ -481,7 +481,22 @@ Path Outer.Inner.
481481
Complete src/Jsx2.res 136:7
482482
posCursor:[136:7] posNoWhite:[136:6] Found expr:[135:3->138:9]
483483
JSX <div:[135:3->135:6] x[136:5->136:6]=...[138:4->138:8]> _children:None
484-
[]
484+
Completable: Cexpression CJsxPropValue [div] x
485+
Package opens Pervasives.JsxModules.place holder
486+
Resolved opens 1 pervasives
487+
ContextPath CJsxPropValue [div] x
488+
Path ReactDOM.domProps
489+
Path PervasivesU.JsxDOM.domProps
490+
[{
491+
"label": "\"\"",
492+
"kind": 12,
493+
"tags": [],
494+
"detail": "string",
495+
"documentation": null,
496+
"sortText": "A",
497+
"insertText": "{\"$0\"}",
498+
"insertTextFormat": 2
499+
}]
485500

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

0 commit comments

Comments
 (0)