correctly handle completion rerequest #6594
Conversation
pascalkuthe
added
A-language-server
pascalkuthe
force-pushed
the
robust_completions
branch
3 times, most recently
from
b413364 to
a80c4ff
Compare
When re requesting a completion that already has a selected item we reuse that selections savepoint. However, the selection has likely changed since that savepoint which requires us to use the selection from that savepoint
Co-authored-by: Michael Davis <[email protected]>
pascalkuthe
force-pushed
the
robust_completions
branch
from
a80c4ff to
e7ac9ec
Compare
There was a problem hiding this comment.
... esc now reset the document to the savepoint created before previewing a transaction
Doesn't do that for me, esc applies the current state (selected completion) and exits to normal mode for me, but I think this behavior is fine IMHO, C-c works as described though (which IMHO also makes sense). The PR description might not be up to date after having a second look, as this seems to be handled in the comments.
Took some time to wrap my head around this change.
I still think that showing the completion in the document should probably not be an actual edit of the document until the completion item is really applied in a logic sense at least, but this is nitpicking, and doesn't really have anything to do with this PR (as it's existing behavior)...
For now I think this is a good approach to the issue(s) at hand, though it would be nice at some point in the future to untangle complexity a little bit (editor <-> completion menu <-> current doc state)
This would definitely be simpler but I think this preview behavior is quite useful and many people prefer it to the VSCode/Intelij approach. I don't think the virtual text is really a good fit here (it would need to be concealed which is not implemented yet and conceal hides the cursor) and would be very complex to implement.
I am not sure that's really possible. I can't think of a way at least. And if we ever have any other operation that "previews" changes like this then this general system is needed so these are really three separate systems: Create document snapshots you can cheaply rollback to (already exists, arbitrarily many possible), create a snapshot after which edits are not sent to the LS (must be stored on the editor since there can be only one of those) and the completion menu then just strings these general concepts together. If you change the document using potentially asynchronously computed edits that are purposefully unsynchronized that's fundamentally just very complex to handle. |
Yeah we should definitely keep that behavior, thanks for the explanation regarding complexity with a potential virtual text implementation, with snippets and jumping of the cursor etc. this would certainly make things more difficult, I can imagine.
Yeah it's definitely not trivial and may require a bigger refactor. I was thinking a little about having some kind of branching and merging behavior for all kinds of edits and likely the whole history(-tree) (maybe by using CRDTs, for generally easier async handling of edits/transactions). So it would be something like this:
It's already kind of going this way but I think it may be easier to comprehend, when this logic is abstracted away at a lower level (I may be wrong though). I think this behavior could be beneficial for different kinds of applications as well (e.g. previewing code completions before applying them). |
|
I have been testing this branch for a few weeks now and have encountered the following problem: When editing Rust code, at some point there is an edit in Helix that is not synchronized with Rust-Analyzer, which is then crashes in two different ways (I haven't taken the time to reliably reproduce them yet): Way 1,
|
We actually had multiple bug reports of the exact same bug report from people on matrix that were just using the latest helix release so I don't think that bug is actually related to this PR at all |
Creating a lower abstraction that we currently have is very difficult. Helix is fundamentally build around a CRDT like transactions. Every change to a document must be a transaction. This even includes any temporary changes. The entire editor is fundamentally built around that model. For example when a transaction is applied to a buffer:
This "transactional" view of the world is very core so any snapshots we may have must also be updated using this transactional model. The savepoint system is exactly this: A snapshot of a document that is internally represented as an automatically updated CRDT transaction that can be used to revert the buffer to a previous snapshot. I guess what you are getting at is that there should be somekind of "LSP snapshot" which is automatically used. However, this doesn't really work. Basically all LSP actions require interactiing with the buffer. This can only work if the server and client buffer matches. Completions are very much special here because these "LSP snapshots" are created by the completion menu so for completion rerequest its ok to revert this LSP snapshot. Everything else should just fail/revert the completion snapshot. Or in other words: the low level snapshot system already exists: It's the savepoint system. The only difference is that completions are very special and require special interaction with its own savepoints. |
|
@poliorcetics that crash is an existing issue see #6921 that is unrelated to this pr |
|
Thanks for the explanation, though I'm aware of most of that. What I meant with "CRDTs may be helpful" is the commutative property of transactions (which is pretty much all what CRDTs are about), so that no matter when the transaction occurred it all leads to the same (expected) result. This is not (yet) implemented (and not a trivial task, especially with multi line/file transactions), but would be quite helpful for various different domains (including this). Anyway all of this is orthogonal to this PR and would require quite a few changes in the core. Is there anything blocking this PR? AFAIK it's the only missing thing for the (bugfix-)release isn't it? |
Transaction::map is not yet implemented, it seems attractive and I thaught about implementing it before but its not actually a good fit for completions. The important realization is that the text applied by the user must always be removed before a completion is applied even with a CRDT. The text by the user is just used to filter the completion but its not part of the transactions send by the server. So for example if you complete at I think this PR is ready to merge just waiting until @archseer has time to review it. |
Alternative to #6511
Closes #6259
Closes #6261
Fixes a similar bug with the typescript LSP reported on matrix where imports would be duplicated.
This PR resolved the same issue as #6511 but keeps the current behavior of previewing the current completion. This is achieved by generating "ghost" transactions. Ghost transactions are normal transactions that are never sent to the LS.
Maintaining such transactions is tricky/dangerous because we always need to undo them (with the undo transaction also not sent to the LS) before we apply any other transaction to the document. Otherwise, we would send invalid changes to LS with incremental sync and therefore break the server (until
:lsp-restart).To that end I used the more generic savepoints introduced in #6173. The completion menu now saves a second savepoint before a completion option is first selected with tab/s-tab. The completion preview (so what you see when hitting tab but before pressing
<ret>) is now such a ghost transaction. To ensure these transactions are always removed there have been some behavioral changes:This behavior matches Kakoune (which also automatically applies the transaction when hitting enter as discovered by @the-mikedavis).
This approach is safe regarding the hazards mentioned above because as soon as a ghost transaction is applied it will always be realized when pressing any key (that isn't consumed by the completion menu). The only remaining hazard are "async" edits (or savepoints like completions request) that are created without an explicit key press. These might pickup the ghost document state. I think there aren't really any of those (besides completions themselves where I habe taken great care to cover all edge-cases) because any async transaction would require a savepoint (otherwise the transaction would refer to a potentially outdated document state) and the only use of savepoint is completions/completion replay (which always require a keypress). If we ever want to add other async transaction we might think about generalizing the system a bit but that will require some more complex design. For now I simply expanded the existing
Editor::last_completefield to cover the usecase here.The type script import problem was actually a special case of the more general problem fixed by this PR. It was caused by resolving completions after they were already applied. The general problem was the same (we send didChange and then rerequested a completion) but fixing it furthermore required moving the request in a3a4645. It seems other clients are running into this issue too (for example nvim) so getting this right is difficult but vscode seems to handle this correctly (also using some ghost text it seems). See typescript-language-server/typescript-language-server#699 (the examples there also cause bugs on master but work with this PR or #6511).
#6261 was caused by us resolving a completion multiple times. The LSP standard doesn't explicitly state it but the intention seems to be that completion items are only resolved once (since resolving is intended to delay expensive calculations). Helix now separately tracks whether a completion item has been resolved and therefore mirrors vscode in that regard. I think that this was really always the intention of the helix code (continuously querying the server is quite inefficient) and the check we did just wasn't quite correct