New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context information to SignatureHelpProvider #54972

Closed
mjbvz opened this Issue Jul 24, 2018 · 8 comments

Comments

@mjbvz
Contributor

mjbvz commented Jul 24, 2018

Problem
Signature help providers sometimes need more information to determine which signature to return. Microsoft/TypeScript#1150 for example discusses how typing a comma inside of a string could mistakenly triggering TypeScript signature help. Using the current VS Code api, a signature help provider cannot easily distinguish the cases:

  • User just typed a comma in a string, triggering signature help.
  • Cursor is after a comma in a string and the user manually requested signature help.

Proposal
Add a context when requesting signature help. This context should supply at least the following:

  • The character that triggered signature help (optional)
  • Why signature help was triggered (manual, typing, retrigger, ...)

This proposal is based on what typescript has implemented which in turn is based on rosyln

retriggger presents one additional complication. This is needed for nested function calls and would be invoked when you type a closing brace with signature help already showing, such as:

console.log(1, Math.max(1, 2)|   )

when the user has just typed ). See Daniel's comment for more context: Microsoft/TypeScript#1150 (comment)

@Gama11

This comment has been minimized.

Contributor

Gama11 commented Jul 26, 2018

Same as #34737 / maybe one should be closed?

@mjbvz mjbvz modified the milestones: On Deck, September 2018 Sep 6, 2018

mjbvz added a commit that referenced this issue Sep 7, 2018

Initial work on signature help context
Fixes #54972

Adds `SignatureHelpContext`. This tells providers why signature help was requested

TODO:

- [ ] Better understand semantics of retrigger. Should `retrigger` be an flag instead of a `triggerReason`?
- [ ]  Fix skipped test
- [ ] Add more tests for trigger reasons / trigger characters

mjbvz added a commit that referenced this issue Sep 7, 2018

Initial work on signature help context
Fixes #54972

Adds `SignatureHelpContext`. This tells providers why signature help was requested

TODO:

- [ ] Better understand semantics of retrigger. Should `retrigger` be an flag instead of a `triggerReason`?
- [ ]  Fix skipped test
- [ ] Add more tests for trigger reasons / trigger characters

@mjbvz mjbvz referenced this issue Sep 7, 2018

Merged

Initial work on signature help context #58135

2 of 3 tasks complete
@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Sep 7, 2018

Prototype PR up at #58135

I'd like feedback on the proposed api from a few other language authors to make sure we design an api that will work well for a variety of languages. /cc @DonJayamanne @DustinCampbell @DanTup @ramya-rao-a @aeschli @fbricon @DanielRosenwasser

Some starting questions:

  • Does your extension / language server have a need for this extra context when computing signature help?
  • Are there other pieces of context beyond what is currently proposed that would be helpful?
  • Any concerns about the trigger reasons?

Also @dbaeumer heads-up for language server protocol

@DanTup

This comment has been minimized.

Contributor

DanTup commented Sep 7, 2018

Whoops - I hadn't noticed this was missing (and require it to implement automatic triggering in a way that doesn't annoy users).

The proposal looks good for me, though we'd planned on not requiring the specific character in the API to the server but rather just whether it was automatically triggered (and if not, only return data of the offset is in a position that is the start of an argument).

mjbvz added a commit that referenced this issue Sep 11, 2018

Initial work on signature help context
Fixes #54972

Adds `SignatureHelpContext`. This tells providers why signature help was requested

TODO:

- [ ] Better understand semantics of retrigger. Should `retrigger` be an flag instead of a `triggerReason`?
- [ ]  Fix skipped test
- [ ] Add more tests for trigger reasons / trigger characters

@mjbvz mjbvz closed this in #58135 Sep 11, 2018

@mjbvz mjbvz reopened this Sep 11, 2018

@mjbvz mjbvz closed this in d33b1c3 Sep 11, 2018

@mjbvz mjbvz reopened this Sep 11, 2018

@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Sep 11, 2018

Reopening :)

API is still in-design but I've merged in the current proposal so that we can get some real world testing of how the current design works for javascript and typescript. Please continue to share feedback here

@mjbvz mjbvz referenced this issue Sep 19, 2018

Closed

Adopt TypeScript 3.1 #58994

2 of 4 tasks complete
@KamasamaK

This comment has been minimized.

KamasamaK commented Sep 22, 2018

Are there other pieces of context beyond what is currently proposed that would be helpful?

Yes, the client value of activeSignature. See #33413

@mjbvz mjbvz modified the milestones: September 2018, October 2018 Sep 24, 2018

mjbvz added a commit that referenced this issue Oct 5, 2018

Add retrigger characters to SignatureHelpProvider
Introduces the concept of a re-trigger character to the signature help provider. This is a seperate set of characters that are registered with the provider. Typing a retrigger character fires a new signature help request if signature help is already showing.

#54972

mjbvz added a commit that referenced this issue Oct 25, 2018

Add `isRetrigger` field to signature help context
#54972

Instead of having a generic `Retrigger` reason, add a new field on the `SignatureHelpContext` that tracks if this was a retrigger or not. This allows retrigger for all the different trigger reasons, including invoke.

Replace the old `Retriggger` trigger reason with `ContentChange` which tracks cursor movement and text updates.
@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 29, 2018

#33413 will likely not be in first edition of this api but we've designed the api so that this data could be introduced easily in the future.

Current proposal is:

/**
 * How a [Signature provider](#SignatureHelpProvider) was triggered
 */
export enum SignatureHelpTriggerReason {
	/**
	 * Signature help was invoked manually by the user or by a command.
	 */
	Invoke = 1,

	/**
	 * Signature help was triggered by a trigger character.
	 */
	TriggerCharacter = 2,

	/**
	 * Signature help was triggered by the cursor moving or by the document content changing.
	 */
	ContentChange = 3,
}

/**
 * Contains additional information about the context in which a
 * [signature help provider](#SignatureHelpProvider.provideSignatureHelp) is triggered.
 */
export interface SignatureHelpContext {
	/**
	 * Action that caused signature help to be requested.
	 */
	readonly triggerReason: SignatureHelpTriggerReason;

	/**
	 * Character that caused signature help to be requested.
	 *
	 * This is `undefined` when signature help is not triggered by typing, such as when invoking signature help
	 * or when moving the cursor.
	 */
	readonly triggerCharacter?: string;

	/**
	 * Whether or not signature help was previously showing when triggered.
	 *
	 * Retriggers occur when the signature help is already active and can be caused by typing a trigger character
	 * or by a cursor move.
	 */
	readonly isRetrigger: boolean;
}

export interface SignatureHelpProvider {
	provideSignatureHelp(document: TextDocument, position: Position, token: CancellationToken, context: SignatureHelpContext): ProviderResult<SignatureHelp>;
}

export interface SignatureHelpProviderMetadata {
	readonly triggerCharacters: ReadonlyArray<string>;
	readonly retriggerCharacters: ReadonlyArray<string>;
}

namespace languages {
	export function registerSignatureHelpProvider(
		selector: DocumentSelector,
		provider: SignatureHelpProvider,
		metadata: SignatureHelpProviderMetadata
	): Disposable;
}

Will try finalizing next iteration. Possible changes at the moment:

  • Rename ContentChange or split it up to make it more clear what this tracks

  • Make isRetrigger only be true if the signature help widget was showing. Currently it is also true if the signature help will show at some point in the future. This is the root cause of #58492

    An alternative design would be to change isRetrigger to state or something similar that can be Inactive, Pending, or Showing.

@mjbvz mjbvz modified the milestones: October 2018, November 2018 Oct 29, 2018

@mjbvz mjbvz added api-finalization and removed api-proposal labels Oct 29, 2018

@jrieken jrieken removed their assignment Nov 12, 2018

@DanTup

This comment has been minimized.

Contributor

DanTup commented Nov 15, 2018

Is this being considered for LSP too? It's likely I'll get around to implementing it in an LSP server before the extension directly (since our existing server protocol doesn't support it, but for LSP I'm implementing it server-side).

@dbaeumer

This comment has been minimized.

Member

dbaeumer commented Nov 16, 2018

Yes, this will very likely make its way into LSP as well.

@mjbvz mjbvz closed this in 520106e Nov 30, 2018

@mjbvz mjbvz referenced this issue Dec 4, 2018

Closed

[Test] SignatureHelpContext #64278

2 of 2 tasks complete

@mjbvz mjbvz added the on-testplan label Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment