From 6551a1e55e14f064ef5bff66dc7ba692d7b553c0 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Fri, 1 Mar 2024 08:30:21 +0100 Subject: [PATCH] Heuristic for JSX empty prop expr completion (#935) * apply heuristic for allowing empty JSX prop expr completion * changelog * add examples with punning --- CHANGELOG.md | 1 + analysis/src/CompletionBackEnd.ml | 38 +++++++-- analysis/src/CompletionFrontEnd.ml | 1 + analysis/src/CompletionJsx.ml | 25 +++++- analysis/src/SharedTypes.ml | 7 +- analysis/tests/src/CompletionJsx.res | 18 +++++ .../tests/src/expected/CompletionJsx.res.txt | 78 +++++++++++++++++++ analysis/tests/src/expected/Jsx2.res.txt | 17 +++- 8 files changed, 174 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4533fff9..4fa7d2073 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/analysis/src/CompletionBackEnd.ml b/analysis/src/CompletionBackEnd.ml index 891b41d69..1b79c1434 100644 --- a/analysis/src/CompletionBackEnd.ml +++ b/analysis/src/CompletionBackEnd.ml @@ -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 @@ -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 @@ -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 third=123 />. + The parser turns that broken JSX into: 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 -> [] diff --git a/analysis/src/CompletionFrontEnd.ml b/analysis/src/CompletionFrontEnd.ml index 30709170a..07cec8cae 100644 --- a/analysis/src/CompletionFrontEnd.ml +++ b/analysis/src/CompletionFrontEnd.ml @@ -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) diff --git a/analysis/src/CompletionJsx.ml b/analysis/src/CompletionJsx.ml index e294bedba..b8a279a2e 100644 --- a/analysis/src/CompletionJsx.ml +++ b/analysis/src/CompletionJsx.ml @@ -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"; @@ -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; @@ -871,6 +892,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor pathToComponent = Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt; propName = prop.name; + emptyJsxPropNameHint = None; }; prefix = ""; nested = []; @@ -894,6 +916,7 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor pathToComponent = Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt; propName = prop.name; + emptyJsxPropNameHint = None; }; prefix = ""; nested = []; diff --git a/analysis/src/SharedTypes.ml b/analysis/src/SharedTypes.ml index 50ae41f8f..9b569015c 100644 --- a/analysis/src/SharedTypes.ml +++ b/analysis/src/SharedTypes.ml @@ -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. *) diff --git a/analysis/tests/src/CompletionJsx.res b/analysis/tests/src/CompletionJsx.res index 0dfa5db8d..540dfe9b9 100644 --- a/analysis/tests/src/CompletionJsx.res +++ b/analysis/tests/src/CompletionJsx.res @@ -61,3 +61,21 @@ module IntrinsicElementLowercase = { // { + ignore(time) + name ++ age + } +} + +// 73:41] +JSX 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 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 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 + }] + diff --git a/analysis/tests/src/expected/Jsx2.res.txt b/analysis/tests/src/expected/Jsx2.res.txt index b6f0c10e3..ed35db333 100644 --- a/analysis/tests/src/expected/Jsx2.res.txt +++ b/analysis/tests/src/expected/Jsx2.res.txt @@ -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 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]