Completion textedits cursor #60499
Completion textedits cursor #60499
Conversation
Great catch @dibarbet! From Razor's perspective it's mostly a self-induced problem that hopefully we can move away from. The self induced issue:
Some "easier" fixes that come to mind (in no particular order):
Lots of options above and lots of pros/cons with each. Hopefully it helps! |
|
A couple of thoughts before I sleep and forget them
Is the cost due to having to map a lot of edits for a lot of completion items, or is simply mapping one too slow?
I might go this way. It isn't perfect though - resolving later means that we cannot inform the client of what range to filter on (it uses the text edit range to perform the matching) so you'd just get whatever the client thinks is the appropriate range. I can try and see if that works but in these cases where text edit matters I have a feeling it might not (todo on me to check). I was thinking about just providing the main text edit upfront then resolving the additional text edits on resolve only which would resolve the filtering issue. However if the resolve gets cancelled for whatever reason (I think this is more likely in vscode) then the commit would do weird things. For example in the cast completion example above the main text edit is just replacing
This is also possible but I think less good - because now the client may be using some completely random range to match against (iirc Fred hit this in vscode with async completion).
Hmmm - this would work for some cases (cast completion) but not others (await completion). And theres nothing restricting them on the roslyn side.
Unfortunately would wreck cast completion :(
Yes pls :) |
Map a lot of edits for a lot of completion items (FWIW, having the additional text edits per-item also bloats the completion payload).
This might be a bug in the VS LSP platform code. I believe VSCode will always "commit" an item whether or not resolve has finished BUT it will always wait for resolve to finish to get 'additionalTextEdits' so it can asynchronously apply them
Great point! Ya this would break filtering for sure |
Yep, unfortunately there is a bug in the LSP client platform - https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1513155 Essentially if I 'resolve' the item by waiting for documentation to appear, everything works as expected. However, if I commit before resolve has happened we never get called. I think I have to switch to providing both the textEdit and additionalTextEdits on resolve only. This isn't ideal for filtering as we'll be using the client inferred range, but better than the alternatives. |
| // Use CompletionChange.TextChanges so that we can get minimal edits around the cursor for better filtering. | ||
| // For the rest of the edits that are not around the cursor, classify them as additional edits. | ||
| var mainEdit = completionChange.TextChanges.Single(change => change.Span.IntersectsWith(listSpan)); |
There was a problem hiding this comment.
An important thing to note for this edit is that it must be replacing a single line (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItem):
/**
* An edit which is applied to a document when selecting this completion.
* When an edit is provided the value of `insertText` is ignored.
*
* *Note:* The range of the edit must be a single line range and it must
* contain the position at which completion has been requested.
If this is not a single line (and trust me, it's currently not guaranteed to be), you'll need to break that change up.
This also creates complications with filtering, as there is no separate range for what LSP clients should filter on. I've forwarded an email discussing the recent problem we fixed with async completion, where the prefix of the line was getting included in the change, but this is a general problem that will need to be addressed. The range of the textEdit is used as the range for filtering as well. I would bet VS works differently here, but in general the LSP spec will cause many completions to be immediately filtered out. I would recommend looking through the omnisharp-roslyn code here, where I've tried to document examples of what will happen for various scenarios.
There was a problem hiding this comment.
If this is not a single line (and trust me, it's currently not guaranteed to be), you'll need to break that change up.
Good callout - currently VS doesn't care at all if the text edit spans multiple lines 😆. And at least for override completion it doesn't impact filtering since VS has the behavior where we only resolve on commit for complex textedits. So the VS client infers the range and seems to be filtering ok for the most part, though I'll need to verify some of the scenarios you linked. I'm hoping that they are covered by IsComplexTextEdit.
I definitely do see the benefit of an explicit filter range though - instead of the client inferring the range for complex edits, we can just tell them what the filter range is without providing a text edit.
dibarbet
force-pushed
the
completion_textedits_cursor
branch
from
20d29fd
to
7e2d6ee
Compare
dibarbet
force-pushed
the
completion_textedits_cursor
branch
from
7e2d6ee
to
86bd721
Compare
| return new LSP.VSInternalCompletionList | ||
| { | ||
| Items = Array.Empty<LSP.CompletionItem>(), | ||
| SuggestionMode = list.SuggestionModeItem != null, |
There was a problem hiding this comment.
Interesting an empty list with suggestion mode on is a thing?
| defaultSpan = completionChange.TextChange.Span; | ||
| defaultRange = ProtocolConversions.TextSpanToRange(defaultSpan.Value, documentText); | ||
| } | ||
| // We use the first item in the completion list as our comparison point for span |
There was a problem hiding this comment.
Lol the hack, is this ever not true?
| // on resolve only as the client may not call us. Instead we provide nothing and force the client to resolve us. | ||
| // This is not ideal for filtering as we have to rely on the client inferred range, but is currently the best we can do. | ||
| // | ||
| // We also cannot provide the additional edits and the main text edit upfront as Razor currently cannot performantly |
There was a problem hiding this comment.
To be honest, I'm not sure we'd want to anyways. Additional Text Edits for every item is expensive serialization wise 😄
| } | ||
| } | ||
|
|
||
| static async Task AddTextEditsAsync( |
There was a problem hiding this comment.
Given this doesn't always add a text edit and occasionally depends on Resolve to do the needful I wonder if there's a better name we could use to represent that
| // | ||
| // We also cannot provide the additional edits and the main text edit upfront as Razor currently cannot performantly | ||
| // map the additional text edits for all the completion items. | ||
| // Tracking issue: https://github.com/dotnet/razor-tooling/issues/6242 |
There was a problem hiding this comment.
This razor-tooling issue is closed now 😁
Followup to #60466
Only 81243d8 needs to be reviewed.
TODO
where a completion is offered to cast a to a byte and turn it into
((byte)a)2. Razor currently Debug asserts in
FormattingContentValidationPasswhen we give itInsertTextFormat.PlainTextedits from the resolve handler in await completion, e.g. dotnet/razor#6127In that scenario we split up the edit into an additional edit to change the method to
asyncand then another one to just insertawait, so no snippet formatting is needed.cc @NTaylorMullen @davidwengier in case they have easy fixes, otherwise I will investigate tomorrow :)