Enhance variant constructor payload completion (#946)

* enhance variant constructor payload completion

* changelog

* fix

* fix

* fix tests

.gitignore

@@ -9,6 +9,9 @@ analysis/tests/.bsb.lock
 analysis/tests-generic-jsx-transform/lib
 analysis/tests-generic-jsx-transform/.bsb.lock
 
+analysis/tests-incremental-typechecking/lib
+analysis/tests-incremental-typechecking/.bsb.lock
+
 tools/node_modules
 tools/lib
 tools/**/*.res.js

CHANGELOG.md

@@ -16,6 +16,10 @@
 
 - Extend signature help to work on constructor payloads as well. Can be turned off if wanted through settings. https://github.com/rescript-lang/rescript-vscode/pull/947
 
+#### :nail_care: Polish
+
+- Enhance variant constructor payload completion. https://github.com/rescript-lang/rescript-vscode/pull/946
+
 ## 1.48.0
 
 #### :bug: Bug Fix

analysis/Makefile

@@ -6,17 +6,21 @@ build-tests:
 build-tests-generic-jsx-transform:
 	make -C tests-generic-jsx-transform build
 
+build-tests-incremental-typechecking:
+	make -C tests-incremental-typechecking build
+
 build-reanalyze:
 	make -C reanalyze build
 
-build: build-reanalyze build-tests build-tests-generic-jsx-transform
+build: build-reanalyze build-tests build-tests-generic-jsx-transform build-tests-incremental-typechecking
 
 dce: build-analysis-binary
 	opam exec reanalyze.exe -- -dce-cmt _build -suppress vendor
 
 test-analysis-binary:
 	make -C tests test
 	make -C tests-generic-jsx-transform test
+	make -C tests-incremental-typechecking test
 
 test-reanalyze:
 	make -C reanalyze test
@@ -25,6 +29,8 @@ test: test-analysis-binary test-reanalyze
 
 clean:
 	make -C tests clean
+	make -C tests-generic-jsx-transform clean
+	make -C tests-incremental-typechecking clean
 	make -C reanalyze clean
 
 .PHONY: build-reanalyze build-tests dce clean test

analysis/src/CompletionExpressions.ml

@@ -255,3 +255,37 @@ let prettyPrintFnTemplateArgName ?currentIndex ~env ~full
         (someName |> Utils.lowercaseFirstChar) ^ suffix
       | _ -> defaultVarName)
     | _ -> defaultVarName)
+
+let completeConstructorPayload ~posBeforeCursor ~firstCharBeforeCursorNoWhite
+    (constructorLid : Longident.t Location.loc) expr =
+  match
+    traverseExpr expr ~exprPath:[] ~pos:posBeforeCursor
+      ~firstCharBeforeCursorNoWhite
+  with
+  | None -> None
+  | Some (prefix, nested) ->
+    (* The nested path must start with the constructor name found, plus
+       the target argument number for the constructor. We translate to
+       that here, because we need to account for multi arg constructors
+       being represented as tuples. *)
+    let nested =
+      match List.rev nested with
+      | Completable.NTupleItem {itemNum} :: rest ->
+        [
+          Completable.NVariantPayload
+            {constructorName = Longident.last constructorLid.txt; itemNum};
+        ]
+        @ rest
+      | nested ->
+        [
+          Completable.NVariantPayload
+            {constructorName = Longident.last constructorLid.txt; itemNum = 0};
+        ]
+        @ nested
+    in
+    let variantCtxPath =
+      Completable.CTypeAtPos
+        {constructorLid.loc with loc_start = constructorLid.loc.loc_end}
+    in
+    Some
+      (Completable.Cexpression {contextPath = variantCtxPath; prefix; nested})

analysis/src/CompletionFrontEnd.ml

@@ -1058,7 +1058,9 @@ let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor
                         && expr |> Res_parsetree_viewer.isBracedExpr
                       then ValueOrField
                       else Value )))
-        | Pexp_construct (lid, eOpt) ->
+        | Pexp_construct ({txt = Lident ("::" | "()")}, _) ->
+          (* Ignore list expressions, used in JSX, unit, and more *) ()
+        | Pexp_construct (lid, eOpt) -> (
           let lidPath = flattenLidCheckDot lid in
           if debug && lid.txt <> Lident "Function$" then
             Printf.printf "Pexp_construct %s:%s %s\n"
@@ -1071,6 +1073,19 @@ let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor
             eOpt = None && (not lid.loc.loc_ghost)
             && lid.loc |> Loc.hasPos ~pos:posBeforeCursor
           then setResult (Cpath (CPId (lidPath, Value)))
+          else
+            match eOpt with
+            | Some e when locHasCursor e.pexp_loc -> (
+              match
+                CompletionExpressions.completeConstructorPayload
+                  ~posBeforeCursor ~firstCharBeforeCursorNoWhite lid e
+              with
+              | Some result ->
+                (* Check if anything else more important completes before setting this completion. *)
+                Ast_iterator.default_iterator.expr iterator e;
+                setResult result
+              | None -> ())
+            | _ -> ())
         | Pexp_field (e, fieldName) -> (
           if debug then
             Printf.printf "Pexp_field %s %s:%s\n" (Loc.toString e.pexp_loc)

analysis/tests-generic-jsx-transform/src/GenericJsxCompletion.res

@@ -1,25 +1,25 @@
-// <div 
+// <div
 //      ^com
 
-// <div testing={} 
+// <div testing={}
 //               ^com
 
 module SomeComponent = {
   @jsx.component
   let make = (~someProp) => {
-	let someString = ""
+    let someString = ""
     let someInt = 12
     let someArr = [GenericJsx.null]
     ignore(someInt)
     ignore(someArr)
     // someString->st
     //               ^com
-	open GenericJsx
+    open GenericJsx
     <div>
-      {GenericJsx.string(someProp)}
+      {GenericJsx.string(someProp ++ someString)}
       <div> {GenericJsx.null} </div>
       // {someString->st}
       //                ^com
     </div>
   }
-}
+}

analysis/tests-generic-jsx-transform/src/expected/GenericJsxCompletion.res.txt

@@ -50,7 +50,7 @@ Path GenericJsx.Elements.props
 Complete src/GenericJsxCompletion.res 14:21
 posCursor:[14:21] posNoWhite:[14:20] Found expr:[8:13->23:3]
 posCursor:[14:21] posNoWhite:[14:20] Found expr:[8:14->23:3]
-posCursor:[14:21] posNoWhite:[14:20] Found expr:[9:1->22:10]
+posCursor:[14:21] posNoWhite:[14:20] Found expr:[9:4->22:10]
 posCursor:[14:21] posNoWhite:[14:20] Found expr:[10:4->22:10]
 posCursor:[14:21] posNoWhite:[14:20] Found expr:[11:4->22:10]
 posCursor:[14:21] posNoWhite:[14:20] Found expr:[12:4->22:10]
@@ -90,22 +90,19 @@ Path Js.String2.st
 Complete src/GenericJsxCompletion.res 20:24
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[8:13->23:3]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[8:14->23:3]
-posCursor:[20:24] posNoWhite:[20:23] Found expr:[9:1->22:10]
+posCursor:[20:24] posNoWhite:[20:23] Found expr:[9:4->22:10]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[10:4->22:10]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[11:4->22:10]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[12:4->22:10]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[13:4->22:10]
-posCursor:[20:24] posNoWhite:[20:23] Found expr:[16:1->22:10]
+posCursor:[20:24] posNoWhite:[20:23] Found expr:[16:4->22:10]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[17:5->22:10]
 JSX <div:[17:5->17:8] > _children:17:8
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[17:8->22:4]
-Pexp_construct :::[18:7->22:4] [18:7->22:4]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[18:7->22:4]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[19:7->22:4]
-Pexp_construct :::[19:7->22:4] [19:7->22:4]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[19:7->22:4]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[20:10->22:4]
-Pexp_construct :::[20:10->22:4] [20:10->22:4]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[20:10->22:4]
 posCursor:[20:24] posNoWhite:[20:23] Found expr:[20:10->20:24]
 Completable: Cpath Value[someString]->st <<jsx>>

analysis/tests-incremental-typechecking/Makefile

@@ -0,0 +1,17 @@
+SHELL = /bin/bash
+
+node_modules/.bin/rescript:
+	npm install
+
+build: node_modules/.bin/rescript
+	node_modules/.bin/rescript > /dev/null || true
+
+test: build
+	./test.sh
+
+clean:
+	rm -r node_modules lib
+
+.DEFAULT_GOAL := test
+
+.PHONY: clean test

analysis/tests-incremental-typechecking/package.json

@@ -0,0 +1,10 @@
+{
+  "scripts": {
+    "build": "rescript",
+    "clean": "rescript clean -with-deps"
+  },
+  "private": true,
+  "dependencies": {
+    "rescript": "11.1.0-rc.2"
+  }
+}

analysis/tests-incremental-typechecking/rescript.json

@@ -0,0 +1,11 @@
+{
+  "name": "test-generic-jsx-transform",
+  "sources": [
+    {
+      "dir": "src",
+      "subdirs": true
+    }
+  ],
+  "bsc-flags": ["-w -33-44-8"],
+  "jsx": { "module": "GenericJsx" }
+}

analysis/tests-incremental-typechecking/src/ConstructorCompletion__Json.res

@@ -0,0 +1,2 @@
+let x = Js.Json.Array()
+//                    ^com

analysis/tests-incremental-typechecking/src/ConstructorCompletion__Own.res

@@ -0,0 +1,6 @@
+module WithVariant = {
+  type t = One({miss: bool}) | Two(bool)
+}
+
+let x = WithVariant.One()
+//                      ^com

analysis/tests-incremental-typechecking/src/expected/ConstructorCompletion__Json.res.txt

@@ -0,0 +1,20 @@
+Complete src/ConstructorCompletion__Json.res 0:22
+posCursor:[0:22] posNoWhite:[0:21] Found expr:[0:8->0:23]
+Pexp_construct Js
+Json
+Array:[0:8->0:21] [0:21->0:23]
+Completable: Cexpression CTypeAtPos()->variantPayload::Array($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CTypeAtPos()
+[{
+    "label": "[]",
+    "kind": 12,
+    "tags": [],
+    "detail": "t",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "[$0]",
+    "insertTextFormat": 2
+  }]
+

analysis/tests-incremental-typechecking/src/expected/ConstructorCompletion__Own.res.txt

@@ -0,0 +1,19 @@
+Complete src/ConstructorCompletion__Own.res 4:24
+posCursor:[4:24] posNoWhite:[4:23] Found expr:[4:8->4:25]
+Pexp_construct WithVariant
+One:[4:8->4:23] [4:23->4:25]
+Completable: Cexpression CTypeAtPos()->variantPayload::One($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CTypeAtPos()
+[{
+    "label": "{}",
+    "kind": 4,
+    "tags": [],
+    "detail": "Inline record",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "{$0}",
+    "insertTextFormat": 2
+  }]
+

analysis/tests-incremental-typechecking/test.sh

@@ -0,0 +1,21 @@
+for file in src/*.res; do
+  output="$(dirname $file)/expected/$(basename $file).txt"
+  ../../rescript-editor-analysis.exe test $file &> $output
+  # CI. We use LF, and the CI OCaml fork prints CRLF. Convert.
+  if [ "$RUNNER_OS" == "Windows" ]; then
+    perl -pi -e 's/\r\n/\n/g' -- $output
+  fi
+done
+
+warningYellow='\033[0;33m'
+successGreen='\033[0;32m'
+reset='\033[0m'
+
+diff=$(git ls-files --modified src/expected)
+if [[ $diff = "" ]]; then
+  printf "${successGreen}✅ No unstaged tests difference.${reset}\n"
+else
+  printf "${warningYellow}⚠️ There are unstaged differences in tests/! Did you break a test?\n${diff}\n${reset}"
+  git --no-pager diff src/expected
+  exit 1
+fi