Fix ambigious case with JSX brace wraps and record body (#894)

zth · web-flow · commit baf1d47c4226 · 2024-01-11T10:43:57.000+01:00
* fix ambigious case where JSX brace wraps in JSX props are interpreted as a record body, but we dont know whether it actually is a record body or just wraps for the prop value itself

* comment + changelog

* move comment

* update changelog desc
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,10 @@
 
 ## master
 
+#### :bug: Bug Fix
+
+- Fix issue with ambigious wraps in JSX prop values (`<SomeComp someProp={<com>}`) - need to figure out if we're completing for a record body or if `{}` are just wraps for the type of `someProp`. In the case of ambiguity, completions for both scenarios are provided. https://github.com/rescript-lang/rescript-vscode/pull/894
+
 #### :nail_care: Polish
 
 - More cases of not emitting `_` when completing in expressions. https://github.com/rescript-lang/rescript-vscode/pull/890
diff --git a/analysis/src/CompletionBackEnd.ml b/analysis/src/CompletionBackEnd.ml
@@ -697,41 +697,48 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
   let package = full.package in
   match contextPath with
   | CPString ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPString";
     [
       Completion.create "dummy" ~env
         ~kind:
           (Completion.Value
              (Ctype.newconstr (Path.Pident (Ident.create "string")) []));
     ]
   | CPBool ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPBool";
     [
       Completion.create "dummy" ~env
         ~kind:
           (Completion.Value
              (Ctype.newconstr (Path.Pident (Ident.create "bool")) []));
     ]
   | CPInt ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPInt";
     [
       Completion.create "dummy" ~env
         ~kind:
           (Completion.Value
              (Ctype.newconstr (Path.Pident (Ident.create "int")) []));
     ]
   | CPFloat ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPFloat";
     [
       Completion.create "dummy" ~env
         ~kind:
           (Completion.Value
              (Ctype.newconstr (Path.Pident (Ident.create "float")) []));
     ]
   | CPArray None ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPArray (no payload)";
     [
       Completion.create "array" ~env
         ~kind:
           (Completion.Value
              (Ctype.newconstr (Path.Pident (Ident.create "array")) []));
     ]
   | CPArray (Some cp) -> (
+    if Debug.verbose () then
+      print_endline "[ctx_path]--> CPArray (with payload)";
     match mode with
     | Regular -> (
       match
@@ -757,6 +764,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
                (Ctype.newconstr (Path.Pident (Ident.create "array")) []));
       ])
   | CPOption cp -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CPOption";
     match
       cp
       |> getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env
@@ -771,6 +779,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
             (Completion.ExtractedType (Toption (env, ExtractedType typ), `Type));
       ])
   | CPAwait cp -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CPAwait";
     match
       cp
       |> getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env
@@ -781,10 +790,12 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
       [Completion.create "dummy" ~env ~kind:(Completion.Value typ)]
     | _ -> [])
   | CPId (path, completionContext) ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPId";
     path
     |> getCompletionsForPath ~debug ~opens ~full ~pos ~exact ~completionContext
          ~env ~scope
   | CPApply (cp, labels) -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CPApply";
     match
       cp
       |> getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env
@@ -826,11 +837,13 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
       | _ -> [])
     | _ -> [])
   | CPField (CPId (path, Module), fieldName) ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CPField: M.field";
     (* M.field *)
     path @ [fieldName]
     |> getCompletionsForPath ~debug ~opens ~full ~pos ~exact
          ~completionContext:Field ~env ~scope
   | CPField (cp, fieldName) -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CPField";
     let completionsForCtxPath =
       cp
       |> getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env
@@ -869,6 +882,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
              else None))
   | CPObj (cp, label) -> (
     (* TODO: Also needs to support ExtractedType *)
+    if Debug.verbose () then print_endline "[ctx_path]--> CPObj";
     match
       cp
       |> getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env
@@ -896,6 +910,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
       | None -> [])
     | None -> [])
   | CPPipe {contextPath = cp; id = funNamePrefix; lhsLoc; inJsx} -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CPPipe";
     match
       cp
       |> getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env
@@ -1008,6 +1023,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
         | _ -> completions)
       | None -> []))
   | CTuple ctxPaths ->
+    if Debug.verbose () then print_endline "[ctx_path]--> CTuple";
     (* Turn a list of context paths into a list of type expressions. *)
     let typeExrps =
       ctxPaths
@@ -1027,6 +1043,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
       ]
     else []
   | CJsxPropValue {pathToComponent; propName} -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CJsxPropValue";
     let findTypeOfValue path =
       path
       |> getCompletionsForPath ~debug ~completionContext:Value ~exact:true
@@ -1038,6 +1055,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
       | [elName] when Char.lowercase_ascii elName.[0] = elName.[0] -> true
       | _ -> false
     in
+    (* TODO(env-stuff) Does this need to potentially be instantiated with type args too? *)
     let targetLabel =
       if lowercaseComponent then
         let rec digToTypeForCompletion path =
@@ -1072,6 +1090,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
           ~kind:(Completion.Value (Utils.unwrapIfOption typ));
       ])
   | CArgument {functionContextPath; argumentLabel} -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CArgument";
     if Debug.verbose () then
       Printf.printf "--> function argument: %s\n"
         (match argumentLabel with
@@ -1124,6 +1143,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
                (if expandOption then Utils.unwrapIfOption typ else typ));
       ])
   | CPatternPath {rootCtxPath; nested} -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CPatternPath";
     (* TODO(env-stuff) Get rid of innerType etc *)
     match
       rootCtxPath
@@ -1138,6 +1158,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact
       | None -> [])
     | None -> [])
   | CTypeAtPos loc -> (
+    if Debug.verbose () then print_endline "[ctx_path]--> CTypeAtPos";
     match
       References.getLocItem ~full ~pos:(Pos.ofLexing loc.loc_start) ~debug
     with
@@ -1872,6 +1893,22 @@ let rec processCompletable ~debug ~full ~scope ~env ~pos ~forHover completable =
         fallbackOrEmpty ~items ())
     | None -> fallbackOrEmpty ())
   | Cexpression {contextPath; prefix; nested} -> (
+    let isAmbigiousRecordBodyOrJsxWrap =
+      match (contextPath, nested) with
+      | CJsxPropValue _, [NRecordBody _] -> true
+      | _ -> false
+    in
+    if Debug.verbose () then
+      (* This happens in this scenario: `<SomeComponent someProp={<com>}`
+           Here, we don't know whether `{}` is just wraps for the type of
+           `someProp`, or if it's a record body where we want to complete
+            for the fields in the record. We need to look up what the type is
+           first before deciding what completions to show. So we do that here.*)
+      if isAmbigiousRecordBodyOrJsxWrap then
+        print_endline
+          "[process_completable]--> Cexpression special case: JSX prop value \
+           that might be record body or JSX wrap"
+      else print_endline "[process_completable]--> Cexpression";
     (* Completions for local things like variables in scope, modules in the
        project, etc. We only add completions when there's a prefix of some sort
        we can filter on, since we know we're in some sort of context, and
@@ -1890,17 +1927,32 @@ let rec processCompletable ~debug ~full ~scope ~env ~pos ~forHover completable =
     with
     | None ->
       if Debug.verbose () then
-        print_endline "--> could not get completions for context path";
+        print_endline
+          "[process_completable]--> could not get completions for context path";
       regularCompletions
     | Some (typ, env) -> (
       match typ |> TypeUtils.resolveNested ~env ~full ~nested with
       | None ->
         if Debug.verbose () then
-          print_endline "--> could not resolve nested expression path";
-        regularCompletions
+          print_endline
+            "[process_completable]--> could not resolve nested expression path";
+        if isAmbigiousRecordBodyOrJsxWrap then (
+          if Debug.verbose () then
+            print_endline
+              "[process_completable]--> case is ambigious Jsx prop vs record \
+               body case, complete also for the JSX prop value directly";
+          let itemsForRawJsxPropValue =
+            typ
+            |> completeTypedValue ~rawOpens ~mode:Expression ~full ~prefix
+                 ~completionContext:None
+          in
+          itemsForRawJsxPropValue @ regularCompletions)
+        else regularCompletions
       | Some (typ, _env, completionContext, typeArgContext) -> (
         if Debug.verbose () then
-          print_endline "--> found type in nested expression completion";
+          print_endline
+            "[process_completable]--> found type in nested expression \
+             completion";
         (* Wrap the insert text in braces when we're completing the root of a
            JSX prop value. *)
         let wrapInsertTextInBraces =
diff --git a/analysis/src/CompletionJsx.ml b/analysis/src/CompletionJsx.ml
@@ -816,20 +816,27 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
   let rec loop props =
     match props with
     | prop :: rest ->
-      if prop.posStart <= posBeforeCursor && posBeforeCursor < prop.posEnd then
-        (* Cursor on the prop name *)
+      if prop.posStart <= posBeforeCursor && posBeforeCursor < prop.posEnd then (
+        if Debug.verbose () then
+          print_endline "[jsx_props_completable]--> Cursor on the prop name";
+
         Some
           (Completable.Cjsx
              ( Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt,
                prop.name,
-               allLabels ))
+               allLabels )))
       else if
         prop.posEnd <= posBeforeCursor
         && posBeforeCursor < Loc.start prop.exp.pexp_loc
-      then (* Cursor between the prop name and expr assigned *)
-        None
-      else if prop.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then
-        (* Cursor on expr assigned *)
+      then (
+        if Debug.verbose () then
+          print_endline
+            "[jsx_props_completable]--> Cursor between the prop name and expr \
+             assigned";
+        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";
         match
           CompletionExpressions.traverseExpr prop.exp ~exprPath:[]
             ~pos:posBeforeCursor ~firstCharBeforeCursorNoWhite
@@ -848,9 +855,13 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
                  nested = List.rev nested;
                  prefix;
                })
-        | _ -> None
-      else if prop.exp.pexp_loc |> Loc.end_ = (Location.none |> Loc.end_) then
-        if CompletionExpressions.isExprHole prop.exp then
+        | _ -> None)
+      else if prop.exp.pexp_loc |> Loc.end_ = (Location.none |> Loc.end_) then (
+        if Debug.verbose () then
+          print_endline "[jsx_props_completable]--> Loc is broken";
+        if CompletionExpressions.isExprHole prop.exp then (
+          if Debug.verbose () then
+            print_endline "[jsx_props_completable]--> Expr was expr hole";
           Some
             (Cexpression
                {
@@ -863,13 +874,17 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
                      };
                  prefix = "";
                  nested = [];
-               })
-        else None
-      else if rest = [] && beforeChildrenStart && charAtCursor = '>' then
+               }))
+        else None)
+      else if rest = [] && beforeChildrenStart && charAtCursor = '>' then (
         (* This is a special case for: <SomeComponent someProp=> (completing directly after the '=').
            The completion comes at the end of the component, after the equals sign, but before any
            children starts, and '>' marks that it's at the end of the component JSX.
            This little heuristic makes sure we pick up this special case. *)
+        if Debug.verbose () then
+          print_endline
+            "[jsx_props_completable]--> Special case: last prop, '>' after \
+             cursor";
         Some
           (Cexpression
              {
@@ -882,16 +897,18 @@ let findJsxPropsCompletable ~jsxProps ~endPos ~posBeforeCursor
                    };
                prefix = "";
                nested = [];
-             })
+             }))
       else loop rest
     | [] ->
       let afterCompName = posBeforeCursor >= posAfterCompName in
-      if afterCompName && beforeChildrenStart then
+      if afterCompName && beforeChildrenStart then (
+        if Debug.verbose () then
+          print_endline "[jsx_props_completable]--> Complete for JSX prop name";
         Some
           (Cjsx
              ( Utils.flattenLongIdent ~jsx:true jsxProps.compName.txt,
                "",
-               allLabels ))
+               allLabels )))
       else None
   in
   loop jsxProps.props
diff --git a/analysis/tests/src/CompletionJsxProps.res b/analysis/tests/src/CompletionJsxProps.res
@@ -27,3 +27,10 @@
 // let _ = <CompletionSupport.TestComponent testArr={[]}
 //                                                    ^com
 
+let tsomeVar = #two
+
+// let _ = <CompletionSupport.TestComponent polyArg={}
+//                                                   ^com
+
+// let _ = <CompletionSupport.TestComponent on={t}
+//                                               ^com
diff --git a/analysis/tests/src/expected/CompletionJsxProps.res.txt b/analysis/tests/src/expected/CompletionJsxProps.res.txt
