Explore type based autocomplete #493

zth · 2022-07-14T19:57:11Z

Work in progress. Exploring type based autocomplete.

cristianoc

First, let's walk through what's happening with this example.

cristianoc · 2022-07-15T01:46:51Z

analysis/tests/src/expected/Dce.res.txt

@@ -1,3 +1,3 @@
 DCE src/Dce.res
-issues:235
+issues:241


I should get rid of these differences. This is testing several things at once and it's not necessary.

cristianoc · 2022-07-15T01:49:27Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

@@ -0,0 +1,12 @@
+Complete src/TypeContextCompletion.res 10:44


Let's see what's going on here first.
This is the position of the cursor: 10:44 unfortunately expressed in 0-based lsp format so you need to do some +1 gymnastics when looking in the actual editor

But if you look at 11:45 in the opened file in the editor that's where ^ is pointing.

cristianoc · 2022-07-15T01:51:09Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

@@ -0,0 +1,12 @@
+Complete src/TypeContextCompletion.res 10:44
+posCursor:[10:44] posNoWhite:[10:43] Found expr:[10:11->18:1]


Now, since the cursor in autocomplete is always after what you want to complete, we need to go back to find anything interesting. And eat up any white space. In this case, just one character back to 10:43.

We have hit an AST expression which goes from line 10 to 18. Next we'll dig deeper into that expression.

Why so many lines? That's parser error recovery, which apparently is taking in also the module definition.
Let's see if this gets in the way. If so, we'll remove the second part of the example and start again. As dealing with parser recovery is orthogonal to this.

cristianoc · 2022-07-15T01:57:50Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

@@ -0,0 +1,12 @@
+Complete src/TypeContextCompletion.res 10:44
+posCursor:[10:44] posNoWhite:[10:43] Found expr:[10:11->18:1]
+Pexp_apply ...[10:11->10:30] (~someVariant10:32->10:43=...[13:0->17:3])


Digging inside, we found a sub-expression that is an application.
This is looking interesting. The cursor points right after the label someVariant.

cristianoc · 2022-07-15T02:01:06Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

+Complete src/TypeContextCompletion.res 10:44
+posCursor:[10:44] posNoWhite:[10:43] Found expr:[10:11->18:1]
+Pexp_apply ...[10:11->10:30] (~someVariant10:32->10:43=...[13:0->17:3])
+Completable: CnamedArg(Value[someVariantToString], "", [someVariant])


So this is where it gets a bit hairy. Not sure why it decides it's a named arg completion given that it's in the expression assigned to the label. It could be because that expression is exactly where parser recovery happened and some heuristic is needed because of the uncertainty that parser recovery brings.
Something orthogonal, but need to figure it out in order to understand this example.

Anyhow the CnamedArg completion finds the function name, and the labels already visited: [someVariant] so they are not repeated. This is not going to find anything as it's the only label.

OK looking at the code, I think we're hitting this case:

| [] -> if posAfterFunExpr <= posBeforeCursor && posBeforeCursor < endPos then Some (CnamedArg (contextPath, "", allNames))

So we went through the entire application and did not find anything interesting. So we'll just default to named arg completion.
This can anyway be overridden by visiting sub-expressions later. The innermost AST expression where something interesting is found wins. Because the innermost is the most precise hit of the cursor.

To see why this case is needed one can just turn it off and see what tests fail.
Looks like this is one such example: let x = Lib.foo(~

cristianoc · 2022-07-15T02:01:40Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

+posCursor:[10:44] posNoWhite:[10:43] Found expr:[10:11->18:1]
+Pexp_apply ...[10:11->10:30] (~someVariant10:32->10:43=...[13:0->17:3])
+Completable: CnamedArg(Value[someVariantToString], "", [someVariant])
+Found type for function (~someVariant: someVariant) => string


Need to check where this is coming from: presumably named arg completion finding the type of the function.

cristianoc · 2022-07-15T02:01:53Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

+Pexp_apply ...[10:11->10:30] (~someVariant10:32->10:43=...[13:0->17:3])
+Completable: CnamedArg(Value[someVariantToString], "", [someVariant])
+Found type for function (~someVariant: someVariant) => string
+[]


As predicted, no result.

cristianoc · 2022-07-15T02:22:53Z

So let's avoid getting carried away by parser recovery which needs to be taken into account but is orthogonal.

What's going on is we do find a label, a function, and an expression being assigned to the label which hits the cursor. Only there's no code to mark that event.
What's needed is code to recognise that event, and emit a new completion primitive for expressions in a type context. E.g. CtypedContext.

I think the next step is to

Recognise when it is the case that we hit an expression assigned to a label and the label does not hit the cursor but the expression does.
When that happens, print "found typed context" in debug mode. That print should be visible in the test output.
As a refinement, also print the name of the function and the name of the label.

Back to you @zth

cristianoc · 2022-07-15T02:24:54Z

A print just like others in the file. E.g. here is another example of print from the file if debug then Printf.printf "Attribute id:%s:%s label:%s\n" id.txt (Loc.toString id.loc) label;

zth · 2022-07-15T19:25:27Z

@cristianoc I think I got it: 5ed8bb4

I added a regular labelled argument completion in TypedContextCompletion.res to help distinguish between the various types.

Awaiting the next steps. Meanwhile, I'll continue exploring where this takes me.

zth · 2022-07-15T20:42:07Z

Continued my exploration here: #497. Separate PR to make things a bit easier to review.

cristianoc · 2022-07-16T00:03:23Z

analysis/src/CompletionFrontEnd.ml

-      else if exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then None
+      else if
+        exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor
+        || charBeforeCursor = Some '='


Let's go slowly and stick to the script.
This is what you do when there is no other choice.
I would like to see how far we can go before trying to do this.

Also, we don't know if it's the character before the cursor.

cristianoc · 2022-07-16T00:05:38Z

I'd like to go slowly and make a tiny step at a time.

Let's see how far we can push it before checking for =.
It will mean trying several examples.
And if it does not work for all of them, something will be learned from that.

zth · 2022-07-16T19:11:00Z

@cristianoc I've removed the '=' special treatment, and added a few more examples. Looks like a typed context is picked up for everything but an empty assignment, which is likely due to parser recovery omitting the assignment if it's empty.

Having type based completion on an empty assignment is critical I'd say. What do you think is the next step? And are there more examples you'd like to see added?

cristianoc · 2022-07-16T19:15:53Z

Need to dig into the example that does not work. With the usual checking in and commenting on the debug output.

…belled argument

zth · 2022-07-16T19:36:54Z

@cristianoc Ok, think I made some more progress. I stole a trick from the JSX prop completion that helps me identify the empty assignment.

This is not in the commit, but I also verified that it's indeed a %rescript.exprhole that's being inserted in the expression by the parser error recovery. So the parser does recover the error properly and nicely, I was wrong.

cristianoc · 2022-07-17T01:08:23Z

analysis/src/SharedTypes.ml

@@ -433,6 +433,7 @@ module Completable = struct
    | Cpath of contextPath


Would you start adding comments to Cpath and contextPath?
There's the question of whether there should be an entirely different case of type t, or whether typed context info should be integrated into the existing ones.

Would you mind helping me out with adding comments to those that exist already? I have a hard time parsing what they are/mean.

Cpath is for complex expressions

CPString is for string constants, used in pipe expressions 34 -> ...
CPArray is for arrays used in pipe expressions [] -> ....
CPApply of contextPath * Asttypes.arg_label list is application with a list of labels

CPId of string list * completionContext is for a path A. or A.B.C. and the rhs is the rest to be completed

CPField of contextPath * string is the .z of record access expr.z
CPObj of contextPath * string is the ["hello"] of object access expr["hello"]
CPPipe of contextPath * string is expr->foo where foo is the function whose first argument is expr.

cristianoc · 2022-07-17T01:12:28Z

This is great progress!
I think we can move on to the next phase and produce the desired result for all the existing examples. Without worrying too much about how things are represented or code duplication for now. (We'll do that after tackling at least one more kind of type context).

zth · 2022-07-17T06:09:00Z

This is great progress! I think we can move on to the next phase and produce the desired result for all the existing examples. Without worrying too much about how things are represented or code duplication for now. (We'll do that after tackling at least one more kind of type context).

Awesome! So, essentially, the next step is to produce the list of constructors for completion, disregarding exactly how, what code is shared, how it's represented exactly etc. Did I get that right?

cristianoc · 2022-07-17T06:48:13Z

Right

zth · 2022-07-17T10:26:26Z

analysis/src/SharedTypes.ml

+  type typedContext = NamedArg of string
+
+  let typedContextToString typedContext =
+    match typedContext with
+    | NamedArg argName -> "NamedArg(" ^ argName ^ ")"


This was a very quick solution just to complete the actual items. I understand this is probably not the right way to do it.

I think this is just debug printing. Unless the comment refers to the type definition, which is perfectly fine for now.

Yes, it was referring to the type def.

zth · 2022-07-17T10:27:26Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

+[{
+    "label": "One",
+    "kind": 4,
+    "tags": [],
+    "detail": "One\n\n",
+    "documentation": null
+  }, {
+    "label": "Two",
+    "kind": 4,
+    "tags": [],
+    "detail": "Two\n\n",
+    "documentation": null
+  }, {
+    "label": "Three",
+    "kind": 4,
+    "tags": [],
+    "detail": "Three\n\n",
+    "documentation": null
+  }, {
+    "label": "Four",
+    "kind": 4,
+    "tags": [],
+    "detail": "Four\n\n",
+    "documentation": null
+  }, {
+    "label": "Five(_)",
+    "kind": 4,
+    "tags": [],
+    "detail": "Five(int)\n\n",
+    "documentation": null
+  }, {
+    "label": "Six(_)",
+    "kind": 4,
+    "tags": [],
+    "detail": "Six(option<string>)\n\n",
+    "documentation": null
+  }]


Barring details like the payloads, filtering according to what the user has already typed, whether we should use snippets etc, this at least produces the output we're after - all of the variant constructors.

Filtering is the next step. Nothing else need to be done.
Snippets etc we don't have to worry about. Perhaps later.

zth · 2022-07-17T10:28:25Z

@cristianoc very crude version implemented that produces the correct output. I've tried to familiarize myself with the code a bit. Feel like I'm slowly getting there.

cristianoc

Nice!
Very good progress.

cristianoc · 2022-07-17T12:40:07Z

analysis/src/CompletionBackEnd.ml

@@ -992,6 +992,18 @@ let rec extractRecordType ~env ~package (t : Types.type_expr) =
    | _ -> None)
  | _ -> None

+let rec extractVariantType ~env ~package (t : Types.type_expr) =


There's probably something like that already. Or maybe not.
Anyhow no need to worry about this for now.

I refactored this into a general one for extracting a type, so the code is slightly more ready for completing more things than just variants.

cristianoc · 2022-07-17T12:40:53Z

analysis/src/CompletionBackEnd.ml

+          |> completionsGetTypeEnv
+        with
+        | Some (typ, _env) ->
+          let rec getLabels ~env (t : Types.type_expr) =


This must exist already. Doesn't matter for now.

Yeah, it exists in another branch. I think it can be done in a better way, so can refactor that later.

cristianoc · 2022-07-17T12:42:38Z

analysis/src/SharedTypes.ml

+  type typedContext = NamedArg of string
+
+  let typedContextToString typedContext =
+    match typedContext with
+    | NamedArg argName -> "NamedArg(" ^ argName ^ ")"


I think this is just debug printing. Unless the comment refers to the type definition, which is perfectly fine for now.

cristianoc · 2022-07-17T12:44:15Z

analysis/src/SharedTypes.ml

@@ -425,6 +425,12 @@ module Completable = struct
    | CPObj of contextPath * string
    | CPPipe of contextPath * string

+  type typedContext = NamedArg of string


You can add a comment: the argument name of a function type.

I made it a record instead and added comments.

cristianoc · 2022-07-17T12:46:22Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

+[{
+    "label": "One",
+    "kind": 4,
+    "tags": [],
+    "detail": "One\n\n",
+    "documentation": null
+  }, {
+    "label": "Two",
+    "kind": 4,
+    "tags": [],
+    "detail": "Two\n\n",
+    "documentation": null
+  }, {
+    "label": "Three",
+    "kind": 4,
+    "tags": [],
+    "detail": "Three\n\n",
+    "documentation": null
+  }, {
+    "label": "Four",
+    "kind": 4,
+    "tags": [],
+    "detail": "Four\n\n",
+    "documentation": null
+  }, {
+    "label": "Five(_)",
+    "kind": 4,
+    "tags": [],
+    "detail": "Five(int)\n\n",
+    "documentation": null
+  }, {
+    "label": "Six(_)",
+    "kind": 4,
+    "tags": [],
+    "detail": "Six(option<string>)\n\n",
+    "documentation": null
+  }]


Filtering is the next step. Nothing else need to be done.
Snippets etc we don't have to worry about. Perhaps later.

cristianoc · 2022-07-17T12:47:49Z

analysis/tests/src/expected/TypeContextCompletion.res.txt

+    "label": "Five(_)",
+    "kind": 4,
+    "tags": [],
+    "detail": "Five(int)\n\n",


There's the question of what one would like to show here.
O I get it. Now I understand what "snippets" means. They're the templates for a variant with payloads?
If so then sure why not tackle them next.

Yeah, essentially we could return this: Five($1, $2) and the editor would put the cursor on $1 first, and then a tab would put the cursor on $2. This can be quite convenient when autocompleting things like this. For example, later when we do sub expression type aware completion (that's not a proper sentence...), the experience could be quite nice. Something like:

Complete someFn(~somethingOptional=, gives Some($1)

Select that completion, trigger autocomplete again. Get a list of all variant constructors if it's a variant, or all identifiers in scope matching the inner type of somethingOptional, etc

We do need to support both with and without snippets, because I'm unsure if all editors support them. I'll have a look at that later.

Don't know if "sub expression type aware completion" is a proper sentence but is sure sounds cool.
What kind of format does vocode support for these kinds of templates? Does it expect things with dollar signs.
It would be great if there were a way to convey the type too, but don't know what format is possible and one would not like super-long names. But if the names were hoverable, then that would be pretty sweet.

Not sure if you want to go down the rabbit hole now, or move on with other aspects and come back to this later.

We can come back later, there's other things snippet syntax would be nice to leverage for too. But yeah, too much of a side track now.

cristianoc · 2022-07-17T13:22:38Z

analysis/src/SharedTypes.ml

+  let typedContextToString typedContext =
+    match typedContext with
+    | NamedArg argName -> "NamedArg(" ^ argName ^ ")"
+
  type t =
    | Cdecorator of string  (** e.g. @module *)
    | CnamedArg of contextPath * string * string list


~prefix with [this, that] seen.
So it would complete a label whose name starts with prefix and excludes labels this and that which have been used already and should not be suggested.

I've done that now, but I haven't done anything about this/that seen, because I don't think it makes sense in the context of completing a labelled argument? There's nothing else to have been seen already, right?

…r labelled argument completion

…it to

cristianoc · 2022-08-19T05:18:04Z

analysis/tests/src/expected/BrokenParserCases.res.txt

@@ -0,0 +1,62 @@
+Dump AST src/BrokenParserCases.res 1:22
+
+Source:


If the source is repeated, how about also indicating what line number this is?

Yes, for sure! I need to re-do this part though, it's very hacky and breaks for a lot of cases. I did however find it quite convenient to be able to just scan the resulting txt and have everything I need to think about the AST there.

cristianoc · 2022-08-19T05:18:51Z

analysis/tests/src/expected/BrokenParserCases.res.txt

+// let _ = ({someProp: , otherProp: 123}, 123)
+//                    ^ast
+
+<*>Pstr_value(


What does <*> indicate? Seems redundant information-wise, but I guess it follows some existing convention of something.

No existing convention, just something I made up. Here's a legend:

<*> means it has the cursor

<x> means it has an empty loc

It's redundant at the top level Pstr_value etc because the value itself would never get printed unless the cursor was there. But, I was confused several times myself not seeing <*>, so I added it there too. Imo having it there too makes it easier to scan for "anchor points" of the cursor, like "there's no cursor (but a broken loc) inside of the structure item, but the structure item itself has the cursor, so we can use that to figure out the context".

Got it. How about the ^ symbol instead of * so it's consistent with other tests.

Also question on terminology: is this a hit test? So everything containing the cursor has it, not a single node?

Sure, that sounds reasonable.

If I understand what you mean then yes, it's a hit test in the sense that the structure item printed needs to have it, but nothing else inside of the structure item technically needs to have the cursor for it to hit.

https://en.wikipedia.org/wiki/Hit-testing

Ahhh yes. Now I understand. The answer is yes, it's a hit test, and everything containing the cursor has it, not just a single node.

…expression strictly does not compile (but the compiler understands what type it will be eventually

zth · 2023-01-24T09:17:12Z

This work is superseeded by a bunch of other PRs. Closing.

zth added 3 commits July 14, 2022 21:56

add base test case for completing jsx prop and labelled argument

7db9012

commit the rest of the updated test result files

4362bdf

fix accidentally broken completion example

87508b3

cristianoc reviewed Jul 15, 2022

View reviewed changes

identify typed context for labelled argument assignments

5ed8bb4

zth mentioned this pull request Jul 15, 2022

basic completion for variant constructors #497

Closed

cristianoc reviewed Jul 16, 2022

View reviewed changes

remove special handling of equality char

d121e73

use trick from JSX prop completion to identify empty assignment of la…

57e7cce

…belled argument

cristianoc reviewed Jul 17, 2022

View reviewed changes

crude implementation of completing variant constructors

b125207

zth commented Jul 17, 2022

View reviewed changes

cristianoc reviewed Jul 17, 2022

View reviewed changes

zth added 3 commits July 17, 2022 20:37

rename and specialize test file so that it will only contain tests fo…

431ab48

…r labelled argument completion

generalize completion so it theoretically can handle more than variants

bdd2e11

add assigned identifier to test file

bea78fe

zth added 17 commits August 13, 2022 11:46

more work, plugging holes etc

1679769

complete patterns in arguments of fns passed to named arguments

f262a50

rename

28dddca

add debug tool and start setting up spec for typed context completion

8a98793

start moving to rewrite of finding pattern completable

5647a1e

hook up completion backend with new pattern completion stuff

2c1b721

complete clean up of the old way of doing pattern completion

5d69adb

simplify

f061119

complete empty tuples

82796f9

introduce snippets

6a0a884

properly apply command

f98db04

implement support for tuple context paths in completion

5558995

dump pexp_field

5320d9e

add typed expression completion type

36832cc

some cleanup and other fixes

eb3eb80

track seen record fields when constructing a record

859c3fd

add a few test fixtures where the parser doesnt do quite what I need …

e67f63c

…it to

cristianoc reviewed Aug 19, 2022

View reviewed changes

zth added 7 commits August 20, 2022 09:27

another broken case

64f4f77

support completing expressions when assigning variables, even if the …

40e3ec1

…expression strictly does not compile (but the compiler understands what type it will be eventually

pull put var from constraint pattern for now

e330b68

more cases

fc48ca8

make sure the newest env is always propagated

cd2161a

fix issue where Some variants wouldnt be handled properly

0522d01

complete inline records

16a6f0f

zth mentioned this pull request Dec 25, 2022

Complete function argument values #665

Merged

zth closed this Jan 24, 2023

zth deleted the type-based-autocomplete branch December 25, 2023 21:12

		@@ -0,0 +1,12 @@
		Complete src/TypeContextCompletion.res 10:44

		@@ -0,0 +1,12 @@
		Complete src/TypeContextCompletion.res 10:44
		posCursor:[10:44] posNoWhite:[10:43] Found expr:[10:11->18:1]

		@@ -433,6 +433,7 @@ module Completable = struct
		\| Cpath of contextPath

		@@ -0,0 +1,62 @@
		Dump AST src/BrokenParserCases.res 1:22

		Source:

Explore type based autocomplete #493

Explore type based autocomplete #493

Conversation

zth commented Jul 14, 2022

cristianoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Jul 15, 2022

cristianoc commented Jul 15, 2022 • edited Loading

zth commented Jul 15, 2022 • edited Loading

zth commented Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

cristianoc commented Jul 16, 2022

zth commented Jul 16, 2022

cristianoc commented Jul 16, 2022

zth commented Jul 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Jul 17, 2022

zth commented Jul 17, 2022

cristianoc commented Jul 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zth commented Jul 17, 2022

cristianoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc Jul 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zth commented Jan 24, 2023

cristianoc commented Jul 15, 2022 •

edited

Loading

zth commented Jul 15, 2022 •

edited

Loading

zth commented Jul 15, 2022 •

edited

Loading

cristianoc Jul 17, 2022 •

edited

Loading