From ab7f152a074bc2c485860ae088e4394090f77405 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 11 Jan 2024 10:22:02 +0100 Subject: [PATCH 1/4] 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 --- analysis/src/CompletionBackEnd.ml | 55 ++++++++++++++-- analysis/src/CompletionJsx.ml | 49 +++++++++----- analysis/tests/src/CompletionJsxProps.res | 7 ++ .../src/expected/CompletionJsxProps.res.txt | 64 +++++++++++++++++++ 4 files changed, 155 insertions(+), 20 deletions(-) diff --git a/analysis/src/CompletionBackEnd.ml b/analysis/src/CompletionBackEnd.ml index f77fa4195..c481fdfd4 100644 --- a/analysis/src/CompletionBackEnd.ml +++ b/analysis/src/CompletionBackEnd.ml @@ -697,6 +697,7 @@ 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: @@ -704,6 +705,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact (Ctype.newconstr (Path.Pident (Ident.create "string")) [])); ] | CPBool -> + if Debug.verbose () then print_endline "[ctx_path]--> CPBool"; [ Completion.create "dummy" ~env ~kind: @@ -711,6 +713,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact (Ctype.newconstr (Path.Pident (Ident.create "bool")) [])); ] | CPInt -> + if Debug.verbose () then print_endline "[ctx_path]--> CPInt"; [ Completion.create "dummy" ~env ~kind: @@ -718,6 +721,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact (Ctype.newconstr (Path.Pident (Ident.create "int")) [])); ] | CPFloat -> + if Debug.verbose () then print_endline "[ctx_path]--> CPFloat"; [ Completion.create "dummy" ~env ~kind: @@ -725,6 +729,7 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact (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: @@ -732,6 +737,8 @@ and getCompletionsForContextPath ~debug ~full ~opens ~rawOpens ~pos ~env ~exact (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,17 @@ 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 + 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 +1922,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 index 7feaa122f..e294bedba 100644 --- 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: (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 index 66f242636..4d053df6b 100644 --- a/analysis/tests/src/CompletionJsxProps.res +++ b/analysis/tests/src/CompletionJsxProps.res @@ -27,3 +27,10 @@ // let _ = 31:54] +JSX 31:43] polyArg[31:44->31:51]=...[31:52->31:54]> _children:None +Completable: Cexpression CJsxPropValue [CompletionSupport, TestComponent] polyArg->recordBody +Package opens Pervasives.JsxModules.place holder +Resolved opens 1 pervasives +ContextPath CJsxPropValue [CompletionSupport, TestComponent] polyArg +Path CompletionSupport.TestComponent.make +[{ + "label": "#one", + "kind": 4, + "tags": [], + "detail": "#one\n\n[#one | #three(int, bool) | #two | #two2]", + "documentation": null, + "insertText": "#one", + "insertTextFormat": 2 + }, { + "label": "#three(_, _)", + "kind": 4, + "tags": [], + "detail": "#three(int, bool)\n\n[#one | #three(int, bool) | #two | #two2]", + "documentation": null, + "insertText": "#three(${1:_}, ${2:_})", + "insertTextFormat": 2 + }, { + "label": "#two", + "kind": 4, + "tags": [], + "detail": "#two\n\n[#one | #three(int, bool) | #two | #two2]", + "documentation": null, + "insertText": "#two", + "insertTextFormat": 2 + }, { + "label": "#two2", + "kind": 4, + "tags": [], + "detail": "#two2\n\n[#one | #three(int, bool) | #two | #two2]", + "documentation": null, + "insertText": "#two2", + "insertTextFormat": 2 + }] + +Complete src/CompletionJsxProps.res 34:49 +posCursor:[34:49] posNoWhite:[34:48] Found expr:[34:12->34:50] +JSX 34:43] on[34:44->34:46]=...[34:48->34:49]> _children:None +Completable: Cexpression CJsxPropValue [CompletionSupport, TestComponent] on=t->recordBody +Package opens Pervasives.JsxModules.place holder +Resolved opens 1 pervasives +ContextPath CJsxPropValue [CompletionSupport, TestComponent] on +Path CompletionSupport.TestComponent.make +[{ + "label": "true", + "kind": 4, + "tags": [], + "detail": "bool", + "documentation": null + }, { + "label": "tsomeVar", + "kind": 12, + "tags": [], + "detail": "[> #two]", + "documentation": null + }] + From e07989dffa77c0098b1791970108d6708b45505c Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 11 Jan 2024 10:25:39 +0100 Subject: [PATCH 2/4] comment + changelog --- CHANGELOG.md | 4 ++++ analysis/src/CompletionBackEnd.ml | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f543ec136..4778cc239 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ ## master +#### :bug: Bug Fix + +- Fix issue with ambigious wraps in JSX prop values (`}`) - need to figure out if we're completing for a record body or if `{}` are just wraps for the type of `someProp`. 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 index c481fdfd4..b4648a039 100644 --- a/analysis/src/CompletionBackEnd.ml +++ b/analysis/src/CompletionBackEnd.ml @@ -1931,6 +1931,11 @@ let rec processCompletable ~debug ~full ~scope ~env ~pos ~forHover completable = if Debug.verbose () then print_endline "[process_completable]--> could not resolve nested expression path"; + (* This happens in this scenario: `}` + 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 ( if Debug.verbose () then print_endline From 65ae43826b8b6ba5cb966b509809fb857a08c477 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 11 Jan 2024 10:26:36 +0100 Subject: [PATCH 3/4] move comment --- analysis/src/CompletionBackEnd.ml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/analysis/src/CompletionBackEnd.ml b/analysis/src/CompletionBackEnd.ml index b4648a039..27b4ae002 100644 --- a/analysis/src/CompletionBackEnd.ml +++ b/analysis/src/CompletionBackEnd.ml @@ -1899,6 +1899,11 @@ let rec processCompletable ~debug ~full ~scope ~env ~pos ~forHover completable = | _ -> false in if Debug.verbose () then + (* This happens in this scenario: `}` + 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 \ @@ -1931,11 +1936,6 @@ let rec processCompletable ~debug ~full ~scope ~env ~pos ~forHover completable = if Debug.verbose () then print_endline "[process_completable]--> could not resolve nested expression path"; - (* This happens in this scenario: `}` - 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 ( if Debug.verbose () then print_endline From 3168de550b76ee29e246befc4a64c61c9e0638cf Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 11 Jan 2024 10:43:47 +0100 Subject: [PATCH 4/4] update changelog desc --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4778cc239..a2bb35524 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ #### :bug: Bug Fix -- Fix issue with ambigious wraps in JSX prop values (`}`) - need to figure out if we're completing for a record body or if `{}` are just wraps for the type of `someProp`. https://github.com/rescript-lang/rescript-vscode/pull/894 +- Fix issue with ambigious wraps in JSX prop values (`}`) - 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