Skip to content

Conversation

@rchl
Copy link
Member

@rchl rchl commented Sep 4, 2025

Resolve the completion before applying changes to the document. Before we have done it lazily, after applying initial changes to the document, which meant that the server might not resolve the item properly later (it would see the changed document state already).

The current implementation behaves differently depending on the type of completion that is being inserted. It considers those three cases:

  1. The completion doesn't need resolving (additionalTextEdits is already present or session doesn't support resolving) - we apply completion synchronously like before.
  2. The completion needs resolving and uses ST's bespoke logic for removing the prefix (KEEP_PREFIX flag not used) - we apply the completion synchronously but (if deemed so) also resolve completion after the fact and try to apply additionalTextEdits if added during resolving.
  3. The completion needs resolving and doesn't use ST's logic for prefix removal (KEEP_PREFIX flag used) - we resolve the completion first before applying it.

Case 2 is required due to limitations in ST API - we don't have a way to replicate ST's bespoke prefix removal so can't skip it. See relevant ST issue.

Fixes #2631

@rchl
Copy link
Member Author

rchl commented Sep 4, 2025

The test I can likely fix but there is a potential showstopper - there is noticable flicker due to applying edit after small delay...

EDIT: Actually I don't see the flicker in rust-analyzer. I see it in LSP code base where I run LSP-ruff, LSP-pyright and LSP-pylsp (for some reason). The reason for the flicker is the fact that ST removes the prefix when completion doesn't use the sublime.COMPLETION_FLAG_KEEP_PREFIX and that happens before lsp_select_completion command runs.

@rchl
Copy link
Member Author

rchl commented Sep 4, 2025

I'm thinking that if we can avoid using sublime.COMPLETION_FLAG_KEEP_PREFIX and are able to remove the prefix always then we should be able to avoid all the issues caused by async nature of this approach.

@rchl
Copy link
Member Author

rchl commented Sep 4, 2025

I've added code that manually removes the prefix instead of relying on sublime.COMPLETION_FLAG_KEEP_PREFIX for non-textedit completions. It seems to work but as I'm not sure what's the exact logic of native prefix removal, I'm not sure if my naive code handles all cases properly.

@rchl
Copy link
Member Author

rchl commented Sep 4, 2025

Well, that doesn't work because ST doesn't report correct prefix when completing $f to $foo. The $ is not part of the prefix and yet when sublime.COMPLETION_FLAG_KEEP_PREFIX is not specified then it's removed.

@rchl
Copy link
Member Author

rchl commented Sep 4, 2025

I have suspicion that ST's logic for inserting plain completions is just to replace the text to the left of the cursor as long as it matches against the completion text. If so then instead of relying on prefix, I could replicate that logic manually.

EDIT: No, that doesn't help when completing b to abc.

@rchl
Copy link
Member Author

rchl commented Sep 23, 2025

I'll switch this to draft since it's not perfect.

It is usable when relying on ST's prefix removal but that creates a visible flicker for completions that use it (non-textedit ones).

Not using ST's logic for prefix removal would be ideal but then we'd have to reverse-engineer how ST does it. It's probably not that hard but I'm afraid of potential edge cases. I've tried to ask in sublimehq/sublime_text#6766 but I suppose it might be considered a "trade secret" :)

@rchl rchl marked this pull request as draft September 23, 2025 11:42
@rchl
Copy link
Member Author

rchl commented Dec 6, 2025

I've updated the logic. The initial comment is updated to reflect the current logic.

I'm not sure we want this change because it will create slight delay in applying completions for case nr. 3. And the question is whether correctness is really worth the extra delay. It does fix the rust-analyzer issue though so correctness should in theory win.

@rchl rchl marked this pull request as ready for review December 6, 2025 17:21
@rchl
Copy link
Member Author

rchl commented Dec 8, 2025

Don't like how it works actually...

The delay on inserting completion for case nr. 3 is annoying and it also allows one to insert extra text before completion is applied. We could fix it by ignoring completion if document changed but that's not great either.

I suppose other editors get away with resolving completion first because they can start resolving when completion is highlighted in the popup. We'd probably need that in ST to make things usable. There is a corresponding ST issue for that.

@rchl rchl marked this pull request as draft December 8, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Importing types via auto-complete sometimes doesn't import the type

2 participants