Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocomplete should prefer completions of a compatible type? #3265

Open
eseidel opened this issue Apr 8, 2021 · 17 comments
Open

Autocomplete should prefer completions of a compatible type? #3265

eseidel opened this issue Apr 8, 2021 · 17 comments
Labels
blocked on vs code / lsp / dap Requires a change in VS Code to progress in editor Relates to code editing or language features is enhancement
Milestone

Comments

@eseidel
Copy link

eseidel commented Apr 8, 2021

This is not a big deal, please feel free to close without comment. Just sharing micro-frictions as I encounter them writing Dart code.

Is your feature request related to a problem? Please describe.
The analyzer presumably knows the type of arguments as you type them.

However winnowing-search autocompletions don't seem to use that type information during selection?

Example:
Screen Shot 2021-04-08 at 8 25 16 AM

Describe the solution you'd like

I'm not sure how feasible doing so is, but it seems like an easy win at least in some cases (like enums).

e.g. in the above case, when I've typed "on" it should be completing first and foremost out of compatible types for the "foo:" argument, no?

Describe alternatives you've considered
My alternative is to type Foo. and then expect better auto-completion? Or just more characters to winnow the search further? Maybe tab could help me with the name of the Enum at least? Maybe this is really enum specific?

@eseidel
Copy link
Author

eseidel commented Apr 8, 2021

A (related) friction I have with the current Dart autocompletion (when writing pure-dart, command line) is that it keeps trying to autocompelte lots of dart:html code, which I've never used and never imported. You can see it in this example too!

@eseidel
Copy link
Author

eseidel commented Apr 8, 2021

Another example is bools. When I'm autocompleting a named parameter which is typed bool, why aren't true/false (or things which are also bool) the first thing. :)

@eseidel
Copy link
Author

eseidel commented Apr 8, 2021

Another example:
Screen Shot 2021-04-08 at 8 36 40 AM

@DanTup
Copy link
Member

DanTup commented Apr 8, 2021

In these screenshots the sorting is (frustratingly) controlled by VS Code and we have limited influence over it. The server-provided ranking is only used when there is no prefix (and the list has not been filtered client-side).

I previously raised an issue about this at microsoft/vscode#79516, but it was deemed to be by design. The reasoning is that the completion list is usually being filtered on the client (not going back to the server) so the ranking is not being recomputed frequently enough, and therefore VS Code does its own ranking based on the prefix you've typed (for example here, it's decided that min() perfectly matches what you've started typing so it's a good match). I can understand this, but it does lead to this frustrating situation.

https://github.com/microsoft/vscode/blob/cb1dce0ef30d48c4259eb7d0a923926b3e6e16af/src/vs/vscode.d.ts#L3855-L3863

		 * Note that `sortText` is only used for the initial ordering of completion
		 * items. When having a leading word (prefix) ordering is based on how
		 * well completion match that prefix and the initial ordering is only used
		 * when completions match equal. The prefix is defined by the
		 * [`range`](#CompletionItem.range)-property and can therefore be different
		 * for each completion.
		 */

In my opinion, it's not an ideal solution - specifically for this reason - VS Code lacks the context of things like types that might make other choices more likely, despite the perfectly matching prefix. Ranking is supposed to be our way of pushing the less relevant (yet valid) options down the list, but it doesn't really do enough here.

@bwilkerson FYI - I think you're aware of this and there aren't many options, but it comes up now and then and may be worth some more thought. Some random ideas I've not fully thought through:

  • Mess with the sortText/filterText fields to try and influence how VS Code filters/ranks these (it uses those fields rather than the displayed label if they're populated)
  • Have some relevance cut-off, like only showing the most relevant x items, or only items with relevance above a threshold (there's a risk of hiding things the user wants here)

@DanTup DanTup added blocked on vs code / lsp / dap Requires a change in VS Code to progress in editor Relates to code editing or language features labels Apr 8, 2021
@DanTup DanTup added this to the Backlog milestone Apr 8, 2021
@eseidel
Copy link
Author

eseidel commented Apr 8, 2021

Maybe there is a (separate bug to file?) about making dart:html autocompletions opt-in or that we somehow need to indicate in the pubspec.yaml that we want to use dart:html? Or maybe there is some larger plan to move dart:html into a package and that's the right solution. 🤷 I don't know how much non-Flutter, non-dart:html dart code there is written (maybe I'm a huge outlier here?). In at the above screenshots, 5/12 in the first screenshot are dart:html noise, and 8/12 in the second screenshot are dart:html noise. :)

@DanTup
Copy link
Member

DanTup commented Apr 8, 2021

Maybe there is a (separate bug to file?) about making dart:html autocompletions opt-in or that we somehow need to indicate in the pubspec.yaml that we want to use dart:html?

Yep - that specific one is being tracked by dart-lang/sdk#41641. I'm not sure what the solution will be - although I think you raise an interesting point about it working for non-Flutter code too. I've also accidentally imported dart:html quite a few times into some CLI projects (like static site generators which happen to reuse a lot of the class names that happen to be in dart:html). I'll add a note on that issue about this so it may be considered when coming up with a solution.

@bwilkerson
Copy link

Maybe there is a (separate bug to file?) about making dart:html autocompletions opt-in

This issue is definitely on our radar. It's definitely a pain point for users. My understanding, which is fairly limited, is that it is sometimes valid to use dart:html in a Flutter package, though it's rare. We either need to move dart:html into its own package, or we need to identify the conditions under which it should or should not be included. I'd prefer that it not require user action because too many users won't discover the way to disable it, but agree that a user setting would be better than nothing.

Some random ideas I've not fully thought through:

Mess with the sortText/filterText fields to try and influence how VS Code filters/ranks these (it uses those fields rather than the displayed label if they're populated)

If we can think of a sortText/filterText that we could return that would improve ranking without hurting other aspects of the UX, then we should definitely look into it.

Have some relevance cut-off, like only showing the most relevant x items, or only items with relevance above a threshold (there's a risk of hiding things the user wants here)

Correct. If a client doesn't ask the server after the initial query, then even if the user completely types the whole identifier it won't show up in the list, and that also hurts the UX.

@DanTup
Copy link
Member

DanTup commented Apr 8, 2021

If we can think of a sortText/filterText that we could return that would improve ranking without hurting other aspects of the UX, then we should definitely look into it.

I'll do some testing of some ideas I had (like adding whitespace or underscore prefixes or something) and see what impact they have.

Correct. If a client doesn't ask the server after the initial query, then even if the user completely types the whole identifier it won't show up in the list, and that also hurts the UX.

We can influence this and force the client to call back to us (by telling it the list is incomplete), but have avoided so far because it could cause a lot more traffic (the client likely does some debouncing, but it could still trigger multiple overlapping requests as the user types - it will send cancellations too, though the completion code is likely mostly synchronous so they likely wouldn't help cut any of the duplicated effort out besides the serialisation/transmission). Perhaps it's worth playing around with this and recording some logs to at least give us a better idea of how it might work.

@DanTup
Copy link
Member

DanTup commented Apr 15, 2021

I tried messing around with the filter/sort text:

return new CompletionList([
	c("min1", undefined, "zzz"),
	c("min2", "min", "zzz"),
	c("min3", "_min", "zzz"),
	c("min4", " min", "zzz"),
	c("min5", ".min", "zzz"),
	c("Foo.min1", undefined, "aaa"),
	c("Foo.min2", "Foo.min", "aaa"),
	c("Foo.min3", "min", "aaa"),
	c("Foo.min4", "min.Foo", "aaa"),
	c("Foo.min5", "min Foo", "aaa"),
	c("Foo.min6", "Foo min", "aaa"),
]);

When ranked by us (no prefix), it looked as you'd expect:

Screenshot 2021-04-15 at 15 46 40

When typing min, those that had min as (or at the start of) their filterText (the second argument) remained further up (which is good):

Screenshot 2021-04-15 at 15 47 27

However, the one that works best (has exactly min as its filterText) will now vanish if you type the enum name (Foo):

Screenshot 2021-04-15 at 15 48 40

I don't think there's going to be a good fix here (without VS Code being convinced to to change things). I did previously suggest allow multiple filterTexts (perhaps it could be named rankTexts to avoid confusing with filter) that would allow us to pass ['Foo.min', 'min'] for these values, and they would rank well for either. Although I think even that has issues - it only helps when the names match, it still doesn't allow any typing hints to be used.

I'm surprised I can't find many other complaints about this - more languages seem to support completing un-imported symbols, so it seems like others should also be hitting this.

I was going to try using isIncomplete=true to see if having VS Code come back to the server would help - however I now realise it won't help here. Unless we exclude min when you've actually typed min, then we'd still have the bad sorting here (and if we're going to exclude min when you've typed exactly min, then going back to the server doesn't help - we could just exclude it from the start).

I don't have any other ideas right now (beyond dropping things out of completion - but that would certainly lead to questions about "why isn't this function that I've typed exactly in the completion list?").

@bwilkerson
Copy link

Thanks for looking into this. It's too bad it doesn't help, but at least we know.

Unless we exclude min when you've actually typed min ...

Some editors (I haven't tested VS Code) choose the top-most completion when you type additional (non-identifier) characters, such as a period. Maybe it's only when the character is a trigger character. Anyway, that's one reason why some editors move exact matches to the top of the list, so that that behavior won't change what's been typed when what's typed is a valid choice.

@DanTup
Copy link
Member

DanTup commented Apr 20, 2021

Yeah, I guess that makes sense (VS Code does it for "commit characters" though we have them behind a flag because a lack of context makes them behave badly - for example when you type .. for cascade, it'll just insert the top item between the dots).

Although I used min as an example above, typing mi has the same effect - VS code puts it at the top because it prefers exact starting matches. I think this is a bit of a roadblock for improving this for now (not suggesting dart:html would help, but not really address the real request), though I'll leave it open because I think it's a real issue and a fair request to fix, even if LSP/VS Code don't give us the tools to do so currently.

@DanTup
Copy link
Member

DanTup commented Apr 20, 2021

I did some testing of the behaviour with isIncomplete=true (which tells the editor that the completion list we gave it is not complete, and therefore if the user continues to type it needs to call the server again). This actually worked better than I was expecting. I thought it might send repeated requests on each key press (cancelling the previous one) resulting in lots of traffic, but actually it just queues up a request and waits for the first one to complete - so although there may be multiple requests, they'll never overlap, and there will always be an additional request sent for the last key pressed by the user (it may just be delayed until the previous request is complete).

So although we can't affect the ranking, we could be more selective with what's included in the completion list (eg. dropping items that are less relevant until they better match the prefix the user has typed). For example, one interesting thing to try could be "only include the top 100 items sorted by relevance unless it's an exact match". I tried to test this out but found what I suspect is a VS Code bug where it doesn't resort the items unless it filters them which makes it hard to tell what's happening. I've raised microsoft/vscode#121743 and will do more testing once fixed or confirmed as by design (though I think that's unlikely).

@bwilkerson
Copy link

If we could predict the cutoff point at which it won't filter them, then that would give us a way to disable the sorting. :-) But I suspect that you're right, that that's unintentional.

@DanTup
Copy link
Member

DanTup commented Jul 19, 2021

I forgot to post back here after getting a response on the issue above. The behaviour above turned out to be by design, however it's not something we can take advantage of it because occurred only because VS Code computed exactly the same rank for those and uses a stable sort. In most cases it's quite unpredictable (because their ranking is quite finely-grained).

Related, I filed microsoft/vscode#127516 based on some discussions with Brian, although whether it will be implemented I couldn't say (going straight to the Backlog milestone is slightly more promising than the usual Backlog-Candidates milestone where it gets closed by a bot if it doesn't get enough 👍 's in so many days 🙃).

@DanTup
Copy link
Member

DanTup commented Dec 12, 2022

FYI, VS Code has closed the request for type-sensitive-completion noted above (microsoft/vscode#127516) as out-of-scope.

I did think of a non-standard way we could implement this, but it's a bit hacky and I'm not sure it's worth it:

  • register a new command: Dart: Trigger Type-Sensitive Completion with a keybinding
  • when the command is run, set a flag that this mode has been enabled and then trigger normal code completion
  • in VS Code extension LSP middleware, add the additional flag to the request to the server
  • clear the flag next time completion is invoked that isn't another call to this function (and is not a re-trigger caused by typing)

In theory (though I've not tested it), this would give the user two keybindings (their normal completion one, and this new one) and if they alternate between the two, they would get the full/a filtered list of completions.

If this is a particularly painful issue for users, it could be worth testing out (in a small hard-coded prototype without the real server changes, just filtering the list in middleware for some known sample)? 🤔

(another option ofc is to just do this filtering all the time when there is a type context, but also including non-type matches when their fuzzy match is above some threshold.. although I don't know how easily we could pick a reasonable threshold that everyone is happy with)

@eseidel
Copy link
Author

eseidel commented Dec 12, 2022

Honestly, copilot has probably solved this problem. I don't think I use built-in autocomplete nearly as much anymore (sometimes I'd prefer to turn it off so it stops offering me "LINK --" and dart:html suggestions).

Thank you very much for looking into this!

@DanTup
Copy link
Member

DanTup commented Dec 12, 2022

@eseidel I'm not too familiar with Copilot - do you mean you're just completing larger chunks of code and not using code completion at all? Or is it improving code completion in some way (my understanding is that it should have the same restrictions as our extension, so if it's doing something better, perhaps we can?).

(it looks like Copilot is also only free for a subset of users, so presumably there are users that would still like improvements in this area)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked on vs code / lsp / dap Requires a change in VS Code to progress in editor Relates to code editing or language features is enhancement
Projects
None yet
Development

No branches or pull requests

3 participants