Include replacementSpan in member completions

[Andarist]

fixes #57301

[Andarist]

I have confirmed manually that those were broken in the very same way as the referenced issue - the existing modifiers were not removed and since the insertText had them the new source text of the file was broken because they got duplicated

[Andarist]

The first commit is much smaller because it just includes converting the eraseRange to replacementSpan at one call site. However, I concluded that since we are in getEntryForMemberCompletion and completions are replacementSpan-oriented it should return replacementSpan and the other caller should become responsible for converting it back to a range (before passing it to tracker.deleteRange)

[iisaduan]

@typescript-bot pack this

[typescript-bot]

Heya @iisaduan, I've started to run the tarball bundle task on this PR at 7545dcd. You can monitor the build here.

[typescript-bot]

Hey @iisaduan, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160002/artifacts?artifactName=tgz&fileId=51A06F2D7BCC9E8EF272A344CC55DEE11BCC7888FE59FFE45FC86B0643DBF63202&fileName=/typescript-5.5.0-insiders.20240223.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

[iisaduan]

Thanks for finding the other cases that break in the same way!

However, I don't think this is the correct fix, as when I try to use this version in the editor, it completely removes the completions from (all) the marked locations.

I did by chance happen to be in an old version of TS where the first test case worked, so with some quick checking, the bug seems to be a regression bug that happened between 5.1.6 and 5.2.2.

[Andarist]

Interesting, I'll take another look at this soon.

[Andarist]

Thanks for testing this out in VS Code. I made the mistake of assuming that this would just work - the local tests here looked alright and this exact thing (replacementSpan inclusion) was mentioned as a fix for this by a VS Code team member (here)

However, it turns out... that it can't work today in this form. VS Code assumes certain things about the replace ranges. So while this technically could be a fine change in TS... it just doesn't work well when integrating with VS Code.

For starters, replace ranges can't span multiple lines (see the check here). The range computed by getPresentModifiers includes the leading trivia and that would usually contain the leading whitespace and the newline character after the previous member. That's an easy fix - we can just select a smaller range by using .getStart() over .po.

But that's still not good. VS Code also assumes that the replacement range contains part of the "word" that is meant to be replaced. Since the target word here is bar we can't get away with having the replace range to correspond to async b. In such a case, VS Code filters out our completion entry here

I could fight this by adjusting the label or filterText but none of that looks particularly good to me.

So my next bet was to use code actions to do this. I pushed out draft changes doing that (without adjusting the test cases, I hope that it's not the problem at this stage) so you could test it out. It's far from perfect right now. The eraseRange still corresponds to async b and then when the code action removes that range it removes those extra 2 characters (one whitespace and b) from the final result (and not just the old modifiers). This could be adjusted - no problem. But I still find the experience here to not be the best:

1. the code actions is a separate action from the original insertion - this sometimes is observable as it takes a moment to apply the follow up code action
2. undoing/redoing doesn't work like I'd expect it to. We end up with 2 items on the stack and it feels like we should only have 1

I either didn't find the correct combination of things to do this correctly today or there is some API gap here that would have to be filled. I somewhat doubt that VS Code would be able to change their assumptions about replacement ranges right now (no support for multiline and treating that whole range as part of the target word).

[Andarist]

I have implemented this using filterText and - from what I can tell - it works with VS Code and all. I feel like I'm abusing this API though and that it wasn't designed for this.

[iisaduan]

@typescript-bot pack this

[typescript-bot]

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
pack this	✅ Started

[typescript-bot]

Hey @iisaduan, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160188/artifacts?artifactName=tgz&fileId=C5A3A198C699BEEA8B8C32D8E6434C01D4D503490716A615313AE1AA5080437502&fileName=/typescript-5.5.0-insiders.20240304.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

[gabritto]

I have implemented this using filterText and - from what I can tell - it works with VS Code and all. I feel like I'm abusing this API though and that it wasn't designed for this.

From what I remember, if we include modifiers in the filterText, we'll run into the same problem that #52525 was addressing, in that now if you're typing that modifier, you'll get the method completion sorted before the keyword completion. I think the filterText is just the name of the method on purpose, to avoid the method completion from showing up as you type a modifier.

The other course of action I can think of to fix this is to omit existing modifiers from the completion, but now if the modifier order is wrong or you need to add a modifier before the existing one, you can't.

It really does look like what we need is to use a replacementSpan to replace the existing modifiers. Maybe I should check with vscode again if that's at all possible.

[Andarist]

It really does look like what we need is to use a replacementSpan to replace the existing modifiers. Maybe I should check with vscode again if that's at all possible.

That would be the most straightforward thing to do here (with the best UX and DX) - on this end at least :p So I very much agree that if that's the possibility then it would be great to explore it.

[iisaduan]

We discussed offline, and our current plan is to accept #57643 with the code action, leave filterText alone, and in the future, we may return to work on this with vscode to find a solution that doesn't require code actions/multiple user steps to undo.

Thank you for such a thorough investigation!