Closed jrieken closed 4 years ago
The first version for this is now merged to master, it's a pragmatic approach that's better then not resolving but worst than (costly) upfront resolving. Items to followup on
/cc @jdneo - My understanding is that Java resolves all items upfront, e.g compute additional text edits during "provide", and that it is quite expensive and therefore Java only returns a small amount of items. Is that correct? Could Java return results faster when additional text edits are computed during resolve (and therefore only for selected items)?
My understanding is that Java resolves all items upfront, e.g compute additional text edits during "provide", and that it is quite expensive and therefore Java only returns a small amount of items. Is that correct?
Yes.
Could Java return results faster when additional text edits are computed during resolve (and therefore only for selected items)?
I think so!
@Eskibear is the dev who was working on Java completion. I believe he can provide more insights toward this topic.
// cc @akaroml @testforstephen @fbricon
@jrieken this is exactly the behavior that jdt.ls used to have, ie compute all the completion items first (fast), then the text edits were computed during resolveCompletion. We were asked to not do that as it was not spec'ed that way and changed to return a small subset of completion items (with incomplete=true), with full text edits computed upfront. The user experience is definitely not as good as with our initial implementation.
To be clear, computing any Java textedit upfront is costly when returning 100+ items (and we can return several thousands items). We'd prefer if the async insertion of textedits covered all textedits, not just the additional ones.
The requirements discussed in https://github.com/eclipse/eclipse.jdt.ls/issues/465 are still true and there are no plans to change that.
There is insertText
(which might be derived from textEdit
) and additionalTextEdits
. The former (insertText
) is inserted at the cursor and must be inserted synchronous to accepting a completion (we cannot block typing while waiting for that). The latter (additionalTextEdits
) is meant for things like import-statements and we now want to relax that, e.g it can come late and we try to apply those edits despite the user having typed more.
When exactly does resolveCompletionItem
run, is it only when an item gets highlighted in the menu? Can't tell from the docs.
I only need to generate additionalTextEdits
for the completion item the user ends up applying -- doing it for other items is unnecessary work that's bogging down the extension host, and I will have to implement yielding.
I assume no one would need to generate additionalTextEdits
before a completion item is applied?
Is there any kind of completion applied event we can hook into to work around this for the time being?
When exactly does resolveCompletionItem run, is it only when an item gets highlighted in the menu?
That's exactly what this issue is about 🔝. Today, there is no guarantee that resolveCompletionItem
is ever called because its original design is to provide data for the suggest details widget. However, we see that additionalTextEdits
(amongst other things) are computed during resolve and leads to subtle bugs (because don't always call resolve or always await it)
I assume no one would need to generate additionalTextEdits before a completion item is applied?
Well, this is the problem if you do not generate them upfront.
insertText
stringresolveCompletionItem
for additional text editsresolveCompletionItem
-call eventually returns but it is unclear if the edits are still valid because the state changed many types in-between Solutions:
(*) "if possible" is currently defined as "no edit has occurred before the first additional edit". That works well if you use additional edits for things like imports or prefixes (which is the indented use-case) but won't work if additional text edits change the main edit, like appending a call-operators with arguments etc
I mean JS can't block but ignoring typing while awaiting is possible -- I was imagining that VSCode would keep the completions menu open and ignore any typing until resolveCompletionItem
is resolved and all text edits applied for the accepted item.
Obviously this would be bad if resolving takes a long time, but the API has cancelation tokens already, so the UI would just need to show a spinner/cancel icon, and also cancel if the user presses ESC or navigates to a different editor, for a decent UX.
I think this would be less confusing for users too; it's the only way it would be clear when VSCode is completely done applying the completion. I think many users would be confused if imports usually get added but not when they resume typing too soon.
FWIW I can syncronously generate the additional text edits I need, so I'd be happy if there were an API to synchronously generate them when the completion item is accepted. I know not everyone would be able to generate them sync though.
Also I know my questions may have seemed redundant, but I hadn't seen anyone on this thread point out that it would be ideal to avoid ever computing additional text edits (or even the primary insert text) for unselected completions.
Since resolveCompletionItem
can get called before any item is accepted (I assume, since it seems to be for showing additional details in the menu), I'd think any solution via resolveCompletionItem
could still result in unnecessarily generating additional edits that are ultimately discarded.
I was imagining that VSCode would keep the completions menu open and ignore any typing until resolveCompletionItem is resolved and all text edits applied for the accepted item.
The golden rule is that "you can always type inside the editor". Ignoring typing is not a good solution and will bug users. Just think of quick suggest and auto accept characters - you don't want the editor to stop typing just because an extension is taking a little longer (+50ms).
I think many users would be confused if imports usually get added but not when they resume typing too soon.
To be precise you should have written "typing at or before the offset at which additional edits are going to happen"
so I'd be happy if there were an API to synchronously generate them when the completion item is accepted
I honestly think the majority can generate them synchronously but this happens in the extension host process or the language server process, and all of that might happen on a different machine and there is no way to synchronously transport that into the UI.
@fbricon re https://github.com/microsoft/vscode/issues/96779#issuecomment-627954429 with the current changes you should be able to have a "simple insert text", say List
, and compute an additional text edit, like java.foo.
, that goes in front of it so that you eventually have java.foo.List
. Right? Or am I missing something?
@jrieken that might actually be a good idea ;-) In practice though, we might need to revisit the way the principal TextEdit is computed (i.e rewrite a bunch of stuff from upstream JDT)
I get the desire to be able to always be able to type inside the editor and agree that's an important principle. The only reason I think this case is an exception is that:
On the other hand, I guess it's unlikely that the additional text edits would overlap whatever the user continues typing...
I hadn't thought about auto accept characters though, that's a good point. That's something I don't have experience with.
Also considering that VSCode could pop up a warning if it has to bail on inserting additional edits, it's not a big deal if it ever happens
@eskibear I remember that we have some new data points regarding the time consumption in different stages of code completion in jdt.ls. Can you share that data and see how we can tune it with this change?
For Java language server, below is a sample of call tree when triggering completion. (time value is larger than expected because of extra overhead from JProfiler)
Total time of completion stage mainly consists of three parts:
I did a simple experiment on java language server, with vscode-insider built on 2020-5-15. I was using spring-petclinic, which is not a big one, but with some dependencies. Type a initial letter, e.g. "O", and observe perf of completion and resolve. Do multiple times for statistics. Calculating additionalTextEdits async in resolve stage does decrease the time cost a lot.
textEdits and additionalTextEdits are calculated during completion stage.
completion: ~150 ms resolve(avg of top 10 items): ~10 ms
Now I simply move a bunch of textedit calculation from "completion" stage into "resolve" stage.
completion: ~100 ms resolve(avg of top 10 items): ~17 ms
I just realized that provideCompletionItems
can return a thenable, but I assume that would delay the appearance of the entire menu?
Closing as we haven't seen issues with the new pragmatic approach and the plan is to stick with it.
@jrieken so how do servers know that clients support lazy-loading of additional textedits? Surely this should be a client capability advertised by vscode (that needs to be specced in the LSP)
@dbaeumer is the right person for that ☝️ and you should probably create a follow up issue in the LSP land
corresponding LSP feature request created here https://github.com/microsoft/language-server-protocol/issues/1003
please add more details if I miss anything.
Thanks @Eskibear
To verify:
This is somewhat the opposite of https://github.com/microsoft/vscode/issues/87187 and the acknowledgement that computing additional text edits for all items upfront is too demanding. This issue is only about
additionalTextEdits
and the goal is a pragmatic solution, similar to the one that we have for format on type. Idea