TriggerKind::Auto violates lsp spec

[nkitsaini]

For autocomplete triggers due to actions like Change mode to Insert, Idle Timeout etc, we send wrong CompletionTriggerKind to language servers. And following that CompletionContext is also invalid.

According to spec, these should be the values of CompletionContext:

// 24x7 trigger, Manual Trigger (Ctrl-X) etc.
{triggerKind: 1} // Invoked

// Trigger due to trigger char
{triggerKind: 2, triggerCharacter: ">"} //  TriggerCharacter

// Trigger due to previously incomplete completion list
{triggerKind: 3}

Violation by helix

For Auto Triggers (like after typing non-trigger characters) we send triggerKind: 2 instead of triggerKind: 1. Due to this we also end up sending triggerKind: 2 without a triggerCharacter.

Sidenote: We don't seem to use triggerKind: 3. Ideally this should be used for incomplete completions but I don't understand this part quiet well so not sure if this a violation or not.

Asciinema

• https://asciinema.org/a/naZB2MlsH6MrERB4e5cEIsD99
  • First, I try to use current implementation of helix with svelteserver, resulting in no completion. And logs showing that we send invalid CompletionContext = {triggerKind: 2}
  • Then I try to use a modified implementation by sending {triggerKind: 1}, which results in completion list by svelteserver.
    • Note that { is not a trigger character of svelteserver, the autocomplete was triggered after I typed 2 characters.
  • Please ignore the copilot completions in the recording.

Helix code responsible for handling this

helix/helix-term/src/handlers/completion.rs

Lines 207 to 228 in 78c3419

	let context = if trigger.kind == TriggerKind::Manual {
	lsp::CompletionContext {
	trigger_kind: lsp::CompletionTriggerKind::INVOKED,
	trigger_character: None,
	}
	} else {
	let trigger_char =
	ls.capabilities()
	.completion_provider
	.as_ref()
	.and_then(|provider| {
	provider
	.trigger_characters
	.as_deref()?
	.iter()
	.find(|&trigger| trigger_text.ends_with(trigger))
	});
	lsp::CompletionContext {
	trigger_kind: lsp::CompletionTriggerKind::TRIGGER_CHARACTER,
	trigger_character: trigger_char.cloned(),
	}
	};

[the-mikedavis]

Incomplete completions are not currently implemented but I believe @pascalkuthe was interested in looking at adding that since it's widely used.

[the-mikedavis]

From the spec:

/**
 * Contains additional information about the context in which a completion
 * request is triggered.
 */
export interface CompletionContext {
	/**
	 * How the completion was triggered.
	 */
	triggerKind: CompletionTriggerKind;

	/**
	 * The trigger character (a single character) that has trigger code
	 * complete. Is undefined if
	 * `triggerKind !== CompletionTriggerKind.TriggerCharacter`
	 */
	triggerCharacter?: string;
}

This doesn't explicitly imply that triggerCharacter must be included if triggerKind == CompletionTriggerKind.TriggerCharacter, just that triggerCharacter must not be included for the other trigger kinds.

The definition of CompletionTriggerKind.TriggerCharacter does support your interpretation though IMO:

/**
 * How a completion was triggered
 */
export namespace CompletionTriggerKind {
	/**
	 * Completion was triggered by typing an identifier (24x7 code
	 * complete), manual invocation (e.g Ctrl+Space) or via API.
	 */
	export const Invoked: 1 = 1;

	/**
	 * Completion was triggered by a trigger character specified by
	 * the `triggerCharacters` properties of the
	 * `CompletionRegistrationOptions`.
	 */
	export const TriggerCharacter: 2 = 2;

	/**
	 * Completion was re-triggered as the current completion list is incomplete.
	 */
	export const TriggerForIncompleteCompletions: 3 = 3;
}
export type CompletionTriggerKind = 1 | 2 | 3;

Using CompletionTriggerKind.Invoked for TriggerKind::Auto seems like an ok change to me.

[nkitsaini]

doesn't explicitly imply that triggerCharacter must be included

Yes, it was an interpolation.

Using CompletionTriggerKind.Invoked for TriggerKind::Auto seems like an ok change to me.

Cool, raised a PR at #9660

[pascalkuthe]

I actually am not sure what the spec meant. The current behavior was intentional. We should check what vscode does and mirror that behavior. I was under the impression it behaves the same way helix does