Merge pull request #25422 from Microsoft/actualSignatureHelpTriggers

Actual signature help trigger filtering

src/harness/fourslash.ts

@@ -1515,7 +1515,9 @@ Actual: ${stringify(fullActual)}`);
                 "argumentCount",
             ];
             for (const key in options) {
-                ts.Debug.assert(ts.contains(allKeys, key));
+                if (!ts.contains(allKeys, key)) {
+                    ts.Debug.fail("Unexpected key " + key);
+                }
             }
         }
 

src/services/signatureHelp.ts

@@ -29,9 +29,12 @@ 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)) {
+        // Only need to be careful if the user typed a character and signature help wasn't showing.
+        const shouldCarefullyCheckContext = !!triggerReason && triggerReason.kind === "characterTyped";
+
+        // Bail out quickly in the middle of a string or comment, don't provide signature help unless the user explicitly requested it.
+        if (shouldCarefullyCheckContext) {
+            if (isInString(sourceFile, position, startingToken) || isInComment(sourceFile, position)) {
                 return undefined;
             }
         }
@@ -41,8 +44,8 @@ namespace ts.SignatureHelp {
 
         cancellationToken.throwIfCancellationRequested();
 
-        // Semantic filtering of signature help
-        const candidateInfo = getCandidateInfo(argumentInfo, typeChecker);
+        // Extra syntactic and semantic filtering of signature help
+        const candidateInfo = getCandidateInfo(argumentInfo, typeChecker, sourceFile, startingToken, shouldCarefullyCheckContext);
         cancellationToken.throwIfCancellationRequested();
 
         if (!candidateInfo) {
@@ -57,24 +60,57 @@ namespace ts.SignatureHelp {
         return typeChecker.runWithCancellationToken(cancellationToken, typeChecker => createSignatureHelpItems(candidateInfo.candidates, candidateInfo.resolvedSignature, argumentInfo, sourceFile, typeChecker));
     }
 
-    function shouldCarefullyCheckContext(reason: SignatureHelpTriggerReason | undefined) {
-        // Only need to be careful if the user typed a character and signature help wasn't showing.
-        return !!reason && reason.kind === "characterTyped";
-    }
+    function getCandidateInfo(
+        argumentInfo: ArgumentListInfo, checker: TypeChecker, sourceFile: SourceFile, startingToken: Node, onlyUseSyntacticOwners: boolean):
+            { readonly candidates: ReadonlyArray<Signature>, readonly resolvedSignature: Signature } | undefined {
 
-    function getCandidateInfo(argumentInfo: ArgumentListInfo, checker: TypeChecker): { readonly candidates: ReadonlyArray<Signature>, readonly resolvedSignature: Signature } | undefined {
         const { invocation } = argumentInfo;
         if (invocation.kind === InvocationKind.Call) {
+            if (onlyUseSyntacticOwners) {
+                if (isCallOrNewExpression(invocation.node)) {
+                    const invocationChildren = invocation.node.getChildren(sourceFile);
+                    switch (startingToken.kind) {
+                        case SyntaxKind.OpenParenToken:
+                            if (!contains(invocationChildren, startingToken)) {
+                                return undefined;
+                            }
+                            break;
+                        case SyntaxKind.CommaToken:
+                            const containingList = findContainingList(startingToken);
+                            if (!containingList || !contains(invocationChildren, findContainingList(startingToken))) {
+                                return undefined;
+                            }
+                            break;
+                        case SyntaxKind.LessThanToken:
+                            if (!lessThanFollowsCalledExpression(startingToken, sourceFile, invocation.node.expression)) {
+                                return undefined;
+                            }
+                            break;
+                        default:
+                            return undefined;
+                    }
+                }
+                else {
+                    return undefined;
+                }
+            }
+
             const candidates: Signature[] = [];
             const resolvedSignature = checker.getResolvedSignature(invocation.node, candidates, argumentInfo.argumentCount)!; // TODO: GH#18217
             return candidates.length === 0 ? undefined : { candidates, resolvedSignature };
         }
-        else {
+        else if (invocation.kind === InvocationKind.TypeArgs) {
+            if (onlyUseSyntacticOwners && !lessThanFollowsCalledExpression(startingToken, sourceFile, invocation.called)) {
+                return undefined;
+            }
             const type = checker.getTypeAtLocation(invocation.called)!; // TODO: GH#18217
             const signatures = isNewExpression(invocation.called.parent) ? type.getConstructSignatures() : type.getCallSignatures();
             const candidates = signatures.filter(candidate => !!candidate.typeParameters && candidate.typeParameters.length >= argumentInfo.argumentCount);
             return candidates.length === 0 ? undefined : { candidates, resolvedSignature: first(candidates) };
         }
+        else {
+            Debug.assertNever(invocation);
+        }
     }
 
     function createJavaScriptSignatureHelpItems(argumentInfo: ArgumentListInfo, program: Program, cancellationToken: CancellationToken): SignatureHelpItems | undefined {
@@ -107,6 +143,14 @@ namespace ts.SignatureHelp {
         }
     }
 
+    function lessThanFollowsCalledExpression(startingToken: Node, sourceFile: SourceFile, calledExpression: Expression) {
+        const precedingToken = Debug.assertDefined(
+            findPrecedingToken(startingToken.getFullStart(), sourceFile, startingToken.parent, /*excludeJsdoc*/ true)
+        );
+
+        return rangeContainsRange(calledExpression, precedingToken);
+    }
+
     export interface ArgumentInfoForCompletions {
         readonly invocation: CallLikeExpression;
         readonly argumentIndex: number;

tests/cases/fourslash/fourslash.ts

@@ -554,7 +554,6 @@ declare namespace FourSlashInterface {
         overloadsCount?: number;
         docComment?: string;
         text?: string;
-        name?: string;
         parameterName?: string;
         parameterSpan?: string;
         parameterDocComment?: string;

tests/cases/fourslash/signatureHelpFilteredTriggerCharacters01.ts

@@ -1,23 +0,0 @@
-/// <reference path="fourslash.ts" />
-
-////function foo<T>(x: T): T {
-////  throw null;
-////}
-////
-////foo("/**/")
-
-goTo.marker();
-for (const triggerCharacter of ["<", "(", ","]) {
-    edit.insert(triggerCharacter);
-    verify.noSignatureHelpForTriggerReason({
-        kind: "characterTyped",
-        triggerCharacter,
-    });
-    verify.signatureHelpPresentForTriggerReason({
-        kind: "retrigger",
-        triggerCharacter,
-    });
-    edit.backspace();
-}
-verify.signatureHelpPresentForTriggerReason(/*triggerReason*/ undefined);
-verify.signatureHelpPresentForTriggerReason({ kind: "invoked" });

tests/cases/fourslash/signatureHelpFilteredTriggers01.ts

@@ -0,0 +1,31 @@
+/// <reference path="fourslash.ts" />
+
+////function foo<T>(x: T): T {
+////  throw null;
+////}
+////
+////foo("/*1*/");
+////foo('/*2*/');
+////foo(` ${100}/*3*/`);
+////foo(/* /*4*/ */);
+////foo(
+////    ///*5*/
+////);
+
+for (const marker of test.markers()) {
+    goTo.marker(marker);
+    for (const triggerCharacter of ["<", "(", ","]) {
+        edit.insert(triggerCharacter);
+        verify.noSignatureHelpForTriggerReason({
+            kind: "characterTyped",
+            triggerCharacter,
+        });
+        verify.signatureHelpPresentForTriggerReason({
+            kind: "retrigger",
+            triggerCharacter,
+        });
+        edit.backspace();
+    }
+    verify.signatureHelpPresentForTriggerReason(/*triggerReason*/ undefined);
+    verify.signatureHelpPresentForTriggerReason({ kind: "invoked" });
+}

tests/cases/fourslash/signatureHelpFilteredTriggers02.ts

@@ -0,0 +1,36 @@
+/// <reference path="fourslash.ts" />
+
+////function foo<T>(x: T): T {
+////  throw null;
+////}
+////
+////foo(/*1*/"");
+////foo(` ${100/*2*/}`);
+////foo(/*3*/);
+////foo(100 /*4*/)
+////foo([/*5*/])
+////foo({ hello: "hello"/*6*/})
+
+const charMap = {
+    1: "(",
+    2: ",",
+    3: "(",
+    4: "<",
+    5: ",",
+    6: ",",
+}
+
+for (const markerName of Object.keys(charMap)) {
+    const triggerCharacter = charMap[markerName];
+    goTo.marker(markerName);
+    edit.insert(triggerCharacter);
+    verify.noSignatureHelpForTriggerReason({
+        kind: "characterTyped",
+        triggerCharacter,
+    });
+    verify.signatureHelpPresentForTriggerReason({
+        kind: "retrigger",
+        triggerCharacter,
+    });
+    edit.backspace(triggerCharacter.length);
+}

tests/cases/fourslash/signatureHelpFilteredTriggers03.ts

@@ -0,0 +1,23 @@
+/// <reference path="fourslash.ts" />
+
+////declare class ViewJayEss {
+////    constructor(obj: object);
+////}
+////new ViewJayEss({
+////    methods: {
+////        sayHello/**/
+////    }
+////});
+
+goTo.marker();
+edit.insert("(");
+verify.noSignatureHelpForTriggerReason({
+    kind: "characterTyped",
+    triggerCharacter: "(",
+});
+
+edit.insert(") {},");
+verify.noSignatureHelpForTriggerReason({
+    kind: "characterTyped",
+    triggerCharacter: ",",
+});

tests/cases/fourslash/signatureHelpWithTriggers01.ts

@@ -0,0 +1,31 @@
+/// <reference path="fourslash.ts" />
+
+////declare function foo<T>(x: T, y: T): T;
+////
+////foo/*1*//*2*/;
+////foo(/*3*/100/*4*/);
+////foo/*5*//*6*/();
+
+const charMap = {
+    1: "(",
+    2: "<",
+    3: ",",
+    4: ",",
+    5: "(",
+    6: "<",
+}
+
+for (const markerName of Object.keys(charMap)) {
+    const triggerCharacter = charMap[markerName];
+    goTo.marker(markerName);
+    edit.insert(triggerCharacter);
+    verify.signatureHelpPresentForTriggerReason({
+        kind: "characterTyped",
+        triggerCharacter,
+    });
+    verify.signatureHelpPresentForTriggerReason({
+        kind: "retrigger",
+        triggerCharacter,
+    });
+    edit.backspace(triggerCharacter.length);
+}

tests/cases/fourslash/signatureHelpWithTriggers02.ts

@@ -0,0 +1,38 @@
+/// <reference path="fourslash.ts" />
+
+////declare function foo<T>(x: T, y: T): T;
+////declare function bar<U>(x: U, y: U): U;
+////
+////foo(bar/*1*/)
+
+goTo.marker("1");
+
+edit.insert("(");
+verify.signatureHelp({
+    text: "bar<U>(x: U, y: U): U",
+    triggerReason: {
+        kind: "characterTyped",
+        triggerCharacter: "(",
+    }
+});
+edit.backspace();
+
+edit.insert("<");
+verify.signatureHelp({
+    text: "bar<U>(x: U, y: U): U",
+    triggerReason: {
+        kind: "characterTyped",
+        triggerCharacter: "(",
+    }
+});
+edit.backspace();
+
+edit.insert(",");
+verify.signatureHelp({
+    text: "foo(x: <U>(x: U, y: U) => U, y: <U>(x: U, y: U) => U): <U>(x: U, y: U) => U",
+    triggerReason: {
+        kind: "characterTyped",
+        triggerCharacter: "(",
+    }
+});
+edit.backspace();