Fix multiline snippets for completions without offsets

[andriigrynenko]

This fixes an issue reported by @leonqadirie on the main PR ... as well as some more subtle bugs.

1. The previous logic for completions without offset was trimming the prefix from the completion (it was also incorrectly using trim_start_matches instead of strip_prefix). This was messing up offsets for multi-line completions.
2. Fixing issue 1 exposed an issue with savepoint - it was not storing the original selection. This meant that reverting a replacement completion could result in cursor being misplaced (this would happen if placeholder selected was in the middle of a completion).
3. Fixing issue 2 made an existing bug more obvious - we were incorrectly computing the cursor from a range in generate_transaction_from_snippet. It was resulting in selection being shifted by 1 character if the range was facing backwards.

I think that fixes for issues 1 and 2 are reasonable.
I'm not sure about the fix for issue 3. This investigation also exposed the fact that generate_transaction_from_snippet currently depends on cursor ending up at the end of the completion after the completion transaction is applied (which seems like a reasonable expectation given that completion is triggered at a cursor position, but it also makes the code more fragile). If we don't want to have this dependency generate_transaction_from_snippet should be updated to not rely on selection.clone().map(transaction.changes()).

[Urgau]

This is definitely out of my expertise. cc @pascalkuthe

[pascalkuthe]

You have indentified the right bugs but there are open PRs to fix this on the main repo for the first two (maybe also for the third not sure right now). The fxes are more involved and we shouldn't bunch them in with the snippet PR.

[pascalkuthe]

helix-editor#6173
helix-editor#5728

[Urgau]

Given @pascalkuthe comments I'm going to close this PR in favor of the upstream fixes.

[andriigrynenko]

cc @pascalkuthe. This is the fix for issue 3.

[andriigrynenko]

@pascalkuthe I don't think we can just skip this. Both issue 1 and issue 3 are actually introduced by the snippet code.

I've seen issue 3 happen for Rust but it only shows up for the very first completion since after that the direction of the cursor is inversed and anchor matches the cursor position fixing the math.
Issue 1 happens because the snippet code enables snippets on the LS side, which results in different completions being emitted (including multi-line completions which are subject to this bug). It will be a regression introduced by the snippet code.

I'm happy to look into a different way to fix it that doesn't involve fixing issue 2. But I don't think we should be merging the snippet code without these fixes.

If you really want to minimize the amount of changes to the main PR at this point, we should at the very least include fix for 3, since it's only changing the newly added code and it just updates the math for selection computation.

[andriigrynenko]

Actually looking at helix-editor#5728 - moving away from replace to insert will introduce the same issue for multi-line snippets as issue 1 flagged here.

If we are going in that direction, an alternative fix for 1 that doesn't also depend on fixing issue 2, would be to pass the length of the prefix (in case the snippet was trimmed) to generate_transaction_from_snippet so that it can be accounted for when computing newline_with_offset.

[pascalkuthe]

No removing the prefix was always incorrect and not compliant with the lsp spec helix-editor#5728 contains multiple fixes, including completely removing any prefix stripping

[pascalkuthe]

The insert/replace config is comepltly orthogonal and only abkut which transaction is selected for servers that send multiple

[andriigrynenko]

The insert/replace config is comepltly orthogonal and only abkut which transaction is selected for servers that send multiple

The problem with insert is that we are currently assuming that every new line has to be offset by the same number of characters as the first position in the completion. Insert potentially hides what that first character was.

@pascalkuthe what's your opinion on fixing issue 3? It's just fixing a clear bug in the snippet logic. Would you prefer me to create a new PR just for that?

[pascalkuthe]

Yeah the third fix seems reasonable to fix inside the snippet PR. You can just post that as a suggestion on the main PR tough since that's just a couple of lines.

Not sure what you mean about insert/replace transactions. Both transactions are sent by the server, it's up to the server yo make sure they are valid

[andriigrynenko]

I tried applying just fix 3 and it's somehow dependent on fix for 2 :( So it's not safe to be merging fix 3 alone. I'll look into it more later today.

[andriigrynenko]

Not sure what you mean about insert/replace transactions. Both transactions are sent by the server, it's up to the server yo make sure they are valid

For multi-line completions the LS is currently not offsetting the completion and we are doing this in the snippet code. Let's assume I typed <space><space>fn and I receive a replace completion for fn - "fn $1($2) {\n $0\n}". I know that replacement happens at offset 2 and so the snippet rendering logic can insert extra two spaces at the beginning of every new line.
If the LS sent me an insert completion (probably something like " $1($2) {\n $0\n}" - I see two problems:

1. The insert would happen at offset 4, so I'm losing the information about the offset of the actual snippet
2. If the snippet had to be provided relative to that offset, it has to have some negative spaces to make it work - " $1($2) {\n$0<negative_space><negative_space>\n}"

[andriigrynenko]

Or is it a LS bug in the first place that mutli-line completions don't have the right offset?

[pascalkuthe]

I tried applying just fix 3 and it's somehow dependent on fix for 2 :( So it's not safe to be merging fix 3 alone. I'll look into it more later today.

I am not sure why that would be the case? It seems like the problem is just that the range is used the wrong way around. Fixing that shouldn't cause additional problems AFAIK.

That being said we might just merge helix-editor#6173 before the snippet PR and then you can rebase on that. There are a bunch more issues around savepoints that become a lot more apparent with snippets since it's not just a bit of text being lost. That PR is a bit more tricky to understand so it will take a bit more time to review tough.

[pascalkuthe]

Not sure what you mean about insert/replace transactions. Both transactions are sent by the server, it's up to the server yo make sure they are valid

For multi-line completions the LS is currently not offsetting the completion and we are doing this in the snippet code. Let's assume I typed <space><space>fn and I receive a replace completion for fn - "fn $1($2) {\n $0\n}". I know that replacement happens at offset 2 and so the snippet rendering logic can insert extra two spaces at the beginning of every new line. If the LS sent me an insert completion (probably something like " $1($2) {\n $0\n}" - I see two problems:

1. The insert would happen at offset 4, so I'm losing the information about the offset of the actual snippet

2. If the snippet had to be provided relative to that offset, it has to have some negative spaces to make it work - `" $1($2) {\n$0<negative_space><negative_space>\n}"`

You are missunderstanding what that PR does. The PR is a bit of a kitchen sink. Firstly it removes the prefix stripping and instead just always replaces all the way to the word boundary if the LSP sends no custom edits.

The option to insert instead of replace text is about something different. I explained it here: helix-editor#5728 (comment). It's not about the start of the replacement (that must always be sent). It's about whether we want to replace the rest of the current identifier after the cursor or keep it.

[andriigrynenko]

The option to insert instead of replace text is about something different. I explained it here: helix-editor#5728 (comment). It's not about the start of the replacement (that must always be sent). It's about whether we want to replace the rest of the current identifier after the cursor or keep it.

Thanks a lot for the detailed explanation it makes sense now! I should have spent more time reading through that PR before making conclusions based on naming :)

[andriigrynenko]

I am not sure why that would be the case? It seems like the problem is just that the range is used the wrong way around. Fixing that shouldn't cause additional problems AFAIK.

So I understand what's happening here better now. The current logic works correctly in 2 cases:
a. If the initial cursor was facing forwards and was of length 1. Then after applying the snippet transform, the updated range will have its anchor matching the end of inserted text.
b. If the text being replaced was selected and the selection was facing backwards. Then when applying the snippet transform, we ended up with the newly inserted text selected, with the selection being backwards and thus anchor is again at the end of the inserted text.
We get an off-by-one error in case:
c. If the initial cursor was facing backwards and was of length 1. Then after applying the snippet transform, the updated range will have its head matching the end of inserted text, and anchor will be 1 grapheme ahead.

(c) is easy to observe for the very first completion after switching from normal mode to insert mode. But after we jump to the second completion (or even just select the first completion) - the savepoint logic inverses the selection and we end hitting (b) so things seem to be working fine.

My fix to savepoint, prevented us from hitting (b) and so I was now seeing (c) all the time.

So my fix for issue 3 above only works when savepoint is fixed since it depends on selection being of length 1 after applying the transform.

[andriigrynenko]

@pascalkuthe, I think I actually managed to come up with a pretty elegant fix (at least that's what I'm telling to myself 🤡 before you reviewed it) that both keeps the code simple, but also handles all these cases nicely - helix-editor#6224