Inconsistent replacement span inclusion between member completion types

[Andarist]

🔎 Search Terms

replacement span member completion optional insert mode replace suggest

🕗 Version & Regression Information

• This is the behavior in every version I tried

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.5.0-dev.20240301#code/C4TwDgpgBAysCGxoF4oG8BQUoQCYHMIAlCAMwGcAuKc4AJwEsA7fAbQF0BuLKJge1zEyVGvWZsuGAL7cMAYz5NaoxBGpxVUVJmz9BJCtQ4AaHkMNQT02QqXAVSAEzqESLeh57zIq9gBE3n5G7KYyGEA

💻 Code

type State = {
  edgeRefs: string[];
  nodeRefs: string[];
};

const state: State = {
  nodeRefs: [],
  e/*1*/Refs: [],
};

const state2: State = {
  nodeRefs: [],
  "e/*2*/Refs": [],
};

🙁 Actual behavior

Completion at 1 comes with an optional replacement range but no entry-level replacement range. Completion at 2 comes with both

🙂 Expected behavior

I see both of those to be the same type of completions - they both are meant to suggest members. For that reason, I'd expect both of those to have very similar payload

Additional information about the issue

I proposed to use optionalReplacementRange in absence of replacementRange here: microsoft/monaco-editor#4406

Currently, VS Code Insiders (ee69e2887fbe532588f74ae86560e7fdc1e59550) behaves the same way as TS playground with editor.suggest.insertMode: 'insert'. However, the same change (as the one that I proposed for Monaco) can't be introduced in VS Code. In fact, it briefly got introduced recently there in microsoft/vscode#200945 but later the behavior got changed/tweaked after bug reports came in. You can see the recent history of the changes here

I studied that recent history a little bit and it seems that replacementSpan and optionalReplacementSpan do not represent the same thing so such reuse is not always safe. I didn't have time to study in depth when they are different and why (beyond noticing - based on one of the reported issues - that some completions for imports have them different).

I believe that the current behavior in VS Code at the marker 2 is incorrect with editor.suggest.insertMode: 'insert'. I've struggled with where I should report this but I concluded that in great part the reason why this behaves differently on VS Code side of things is that TS includes this replacementSpan inconsistently at those 2 markers so I chose to report this here first.

cc @andrewbranch @mjbvz (you both were involved in the recent changes around this in VS Code repo)

[Andarist]

@RyanCavanaugh I have some context around this already so I'd gladly work on this - but I'm unsure what are the exact expected results here. Should we limit replacementSpan inclusion in TS for both of those to behave the same way?

[RyanCavanaugh]

@mjbvz ^?

[mjbvz]

This change on the VS Code side covers my understanding of the TS protocol: microsoft/vscode@428dd56

[Andarist]

So I was studying this very commit (among other ones) before opening this issue. I see what it's doing but I don't understand fully why the entry.replacementSpan is used for both inserting and replacing ranges while the optionalReplacementSpan is just used for replacing one. That said, this commit has been created in response to some issue so I assume it's just adjusting to some existing expectations and it's logic is kinda derived from how all of this works today.

I lack a precise definition of those 2 types of replacement spans. Like, why the entry.replacementRange "wins" over editor.suggest.insertMode: 'insert' but the same isn't true about the optionalReplacementRange? At the end of the day, I'm not sure if it matters all that much. Although a more precise definition for those would definitely be appreciated by future contributors.

It works how it works today. If you don't want to alter the protocol anyhow (by, I don't know, including some extra boolean prop to inform the desired behavior further) or to adjust how the protocol is interpreted on the VS Code side of things (both of those things are more than fine!) then I propose to remove replacementSpan from string-based member completions.

I feel like both of the presented examples should work in the same way with editor.suggest.insertMode: 'insert'. From the user's PoV, this is the same type of completion - it doesn't matter if the property name is quoted or not. Internally they are implemented using two different functions - so any difference between them might be more accidental than desired here. It very well may have been desired at some point but I really don't have any good way of tracking those past design decisions. I tried to look through the commit/PR history here but I didn't find any interesting information. From what I can tell, we also don't have any e2e test suite in this area that would inform us about how certain scenarios are meant to work.