Trigger characters in signature help #24915
Conversation
|
TSServer Api looks good. Thank you for looking into this! We'll need to do a little work on the vs code side (microsoft/vscode#51731) to make sure we have this info but I don't expect that to be difficult |
src/services/signatureHelp.ts
Outdated
| @@ -29,6 +29,11 @@ namespace ts.SignatureHelp { | |||
| return undefined; | |||
| } | |||
|
|
|||
| // In the middle of a string, don't provide signature help unless the user explicitly requested it. | |||
| if (triggerCharacter !== undefined && isInString(sourceFile, position, startingToken)) { | |||
There was a problem hiding this comment.
what about , in an object literal? or < in jsxElement?
There was a problem hiding this comment.
consider extracting this to isValidTriggerCharacter function and handling each accordingly.
There was a problem hiding this comment.
I actually don't think you need to look at the triggerCharacter - you just need to use the startingToken and check whether you were given a trigger in the first place. That's why in #1150 we had a more abstract idea of a trigger reason rather than the character itself
There was a problem hiding this comment.
But I think I'd rather handle those cases in another PR.
There was a problem hiding this comment.
not sure i follow.. why would not it be sufficient, and why anohter PR?
|
It looks like it would be very easy for VS to consume this change. However, as Daniel and/or Mohamed pointed out, returning undefined while sighelp is already displayed will (apparently) dismiss it. Edit: I've created a branch on VSTS so you can play with the new functionality. |
| @@ -4467,7 +4467,6 @@ declare namespace ts { | |||
| useCaseSensitiveFileNames?(): boolean; | |||
| fileExists?(path: string): boolean; | |||
| readFile?(path: string): string | undefined; | |||
| getSourceFiles?(): ReadonlyArray<SourceFile>; | |||
There was a problem hiding this comment.
It looks like there are a few breaking changes in this file.
| verify.noSignatureHelpForTriggerCharacter(triggerCharacter); | ||
| edit.backspace(); | ||
| } | ||
| verify.signatureHelp({ triggerCharacter: undefined }); |
There was a problem hiding this comment.
Signature help will fail if nothing is returned, but I'll make a more explicit version.
|
🔔 |
| @@ -50,6 +57,11 @@ namespace ts.SignatureHelp { | |||
| return typeChecker.runWithCancellationToken(cancellationToken, typeChecker => createSignatureHelpItems(candidateInfo.candidates, candidateInfo.resolvedSignature, argumentInfo, sourceFile, typeChecker)); | |||
| } | |||
|
|
|||
| function shouldCarefullyCheckContext(reason: SignatureHelpTriggerReason | undefined) { | |||
| @@ -29,6 +29,13 @@ namespace ts.SignatureHelp { | |||
| return undefined; | |||
| } | |||
|
|
|||
| if (shouldCarefullyCheckContext(triggerReason)) { | |||
| // In the middle of a string, don't provide signature help unless the user explicitly requested it. | |||
| if (isInString(sourceFile, position, startingToken)) { | |||
There was a problem hiding this comment.
we still need to do some additional checks here.
There was a problem hiding this comment.
e.g.:
(: in a parenthesized expression
,: not in an object literal, array literal, or comma expression
default: instring | incomment | injsxtext,
ghost
mentioned this pull request
This change implements trigger character handling for signature help. When an editor provides us with a trigger character, the language service is now able to use that as a signal to distinguish whether a user is typing in some unrelated construct (e.g. commas in an array) and whether the user explicitly requested signature help (e.g. typing in a comma as part of an argument list).
Right now the implementation only handles filtering completion characters inside of a string literal; we can expand to other syntactic constructs later on.
Fixes #1150, one of our oldest-standing language service bugs.
@amcasey and @mjbvz, we'll need support from the VS and VS Code side.