Partial acceptance of Inline suggestions skips command

[bmuskalla]

• VS Code Version: Version: 1.75.0-insider (Universal)
  Commit: 0a6c277
• OS Version: 12.6.1 (21G217)

Steps to Reproduce:

1. Implement a InlineCompletionItemProvider
2. Return InlineCompletionItems with a command to be called after the suggestion was accepted
3. Use partial acceptance to accept parts or all of the above completion

Few problems that arise from this:
a) even if you fully accept a suggestion, the provider will never know about this
b) providers don't know if a suggestion was partially or fully accepted (relevent for tracking quality metrics)
c) accepting a completion using partial acceptance leads to a very different lifecycle of shown/accepted events
Example:

• CompletionItem: foo.bar(xxx);
• Trigger completion
• Partially accept foo
• Partially accept bar(
• Partially accept xxx);

handleDidShowCompletionItem is called for each part that is accepted, while command is never executed (even after the last xxx); got accepted.

// cc @isidorn @hediet

[isidorn]

@bmuskalla thanks for opening this issue. What command do you actually run, what effect does it have on the user that the command is not run?

Some ideas to workaround the challenges:
a) The provider could look at the changes in the editor and compute if the suggestion was partially accepted or what exactly happened.
b) Enhance handleDidShowCompletionItem so that it contains the part that got accepted

Let me know what are your suggestions @bmuskalla @hediet

[bmuskalla]

What command do you actually run, what effect does it have on the user that the command is not run?

No user impact from the command (at least not this point) but for us to decide whether we cache things, keep track of good/bad suggestions, trigger async operations that influence the next completion.

While we could compute the changes between the old document and the new one to figure out what happend, it would not tell us if things got partially accepted or typed-as-suggested. Certainly sounds like a possibility to track this. I think I'd have hoped to have a similar callback for partial acceptance as we have for acceptance.
Not sure if enhancing handleDidShowCompletionItem would be the right approach (it's about shown) vs having an explicit callback for acceptance of completions.

[bmuskalla]

While we could compute the changes between the old document and the new one to figure out what happend, it would not tell us if things got partially accepted or typed-as-suggested

With the current events, I don't actually think this would be plausibel as the next shown events (after a partial acceptance) don't have enough information/we'd have to somewhere keep the state about lastShown==partially accepted. Especially in multi-line scenarios this is really tricky to capture.

It would be great to get your sentiment on a similar callback for partial acceptance as we have for acceptance. to further refine a design/proposal.

[hediet]

a similar callback for partial acceptance as we have for acceptance.

I think that is reasonable. What do you think @jrieken?

[jrieken]

Yeah, enriching and calling handleDidShowCompletionItem makes most sense. I really not try to reverse engineer partial acceptance based on change events because that's cumbersome and error-prone.

[jrieken]

@bmuskalla Is this something you urgently need? I am asking because we are wrapping up the January release and we will likely ship that without any improvement for you.

[bmuskalla]

This is indeed urgent for us as it prevents us from tracking which suggestions got accepted by the user; which is a core quality metric for Copilot. If we can agree on a design, I'm also happy to see it I can provide a PR for this

[jrieken]

How about an additional argument to the handleDidShowCompletionItem call that is the accepted chunk? Tho, unsure what to do about the command

[bmuskalla]

For me, the question is if handleDidShowCompletionItem is even the right API for this (as it's about showing a completion item, which might not be the case for the last part of a suggestion.
I wonder if something along those lines would be acceptable to be triggered for providers:

# vscode.proposed.inlineCompletionsAdditions.d.ts

export interface InlineCompletionItemProvider {
   # existing
   handleDidShowCompletionItem?(completionItem: InlineCompletionItem): void;

   # proposed
   export interface PartialInlineCompletionItem {
       backingItem: InlineCompletionItem;
       range: Range;
       insertText: string;
   }
   handleDidAcceptCompletionItem?(completionItem: InlineCompletionItem | PartialInlineCompletionItem)
   handleDidRejectCompletionItem?(completionItem: InlineCompletionItem | PartialInlineCompletionItem)
}

This could replace command in the long run with the advantage that it's tied to the completionItem instead of manually tracking the item through command arguments (but that's not required). One interesting case I have is that I want to know how much of a suggestion is accepted.
Taking the following example:

• suggestion: abc 123 foo
• User partially accepts abc
• User uses Tab to accept the rest

I think in this case when a user accepted a partial completion, it should be handled as partial completions until the suggestion is done/session is ended.

[hediet]

@bmuskalla It would be awesome if you could try this out once both PRs landed and we have a new build!
Alternatvely, you could also clone this branch and try it from Code OSS.

[bmuskalla]

Latest insider build just missed this by one commit :) I'll try to set this up locally. Can we expect a new insiders build every night? Or can this be triggered manually?

Also question regarding the API: Right now, you only pass the length of the back insertion back to the providers. What are the usecases for not passing along the range? I see you resolve the range against the document. For our purposes, we also want to track the offset at which the suggestion was inserted (and I should be able to compute that with proposal offset+sum(partial acceptance length) just wondering what edge cases you had in mind that I may need to account for.

[hediet]

I just thought the length would be more self-containing, than a range of the newly inserted text.
We can iterate on this, depending on your needs.

[isidorn]

Just to clarify - Insiders is realeased every day, but this week is special because we are preparing for a stable release - so Insiders is frozen. But I do expect us to have the insiders with this fix out soon.

The fix that @hediet linked will also be part of the stable release that is going out in 2 days.

@bmuskalla so the important thing is to figure out if the current solution is good enough for stable, and we are of course open to iterate on the solution so we fine-tune it in Insiders and for the next Stable release (start of March).

[rzhao271]

@bmuskalla as part of endgame (#174828), I'd like to confirm whether the issue has been resolved for you?

[bmuskalla]

@rzhao271 Thanks for checking, we're all good on this one.