From dc6653eb74f40ea1098b25ba628d1a918ecdff83 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Tue, 3 Jul 2018 12:19:39 -0700 Subject: [PATCH 1/6] Renamed test. --- ...dTriggerCharacters01.ts => signatureHelpFilteredTriggers01.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/cases/fourslash/{signatureHelpFilteredTriggerCharacters01.ts => signatureHelpFilteredTriggers01.ts} (100%) diff --git a/tests/cases/fourslash/signatureHelpFilteredTriggerCharacters01.ts b/tests/cases/fourslash/signatureHelpFilteredTriggers01.ts similarity index 100% rename from tests/cases/fourslash/signatureHelpFilteredTriggerCharacters01.ts rename to tests/cases/fourslash/signatureHelpFilteredTriggers01.ts From 3e38693425dd4fd732ceb04051b346deea4fad17 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Tue, 3 Jul 2018 13:01:55 -0700 Subject: [PATCH 2/6] Amend test for comments, other string types. --- .../signatureHelpFilteredTriggers01.ts | 38 +++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/tests/cases/fourslash/signatureHelpFilteredTriggers01.ts b/tests/cases/fourslash/signatureHelpFilteredTriggers01.ts index ed97ef020d07e..ebf5ba10f2d58 100644 --- a/tests/cases/fourslash/signatureHelpFilteredTriggers01.ts +++ b/tests/cases/fourslash/signatureHelpFilteredTriggers01.ts @@ -4,20 +4,28 @@ //// throw null; ////} //// -////foo("/**/") +////foo("/*1*/"); +////foo('/*2*/'); +////foo(` ${100}/*3*/`); +////foo(/* /*4*/ */); +////foo( +//// ///*5*/ +////); -goTo.marker(); -for (const triggerCharacter of ["<", "(", ","]) { - edit.insert(triggerCharacter); - verify.noSignatureHelpForTriggerReason({ - kind: "characterTyped", - triggerCharacter, - }); - verify.signatureHelpPresentForTriggerReason({ - kind: "retrigger", - triggerCharacter, - }); - edit.backspace(); +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" }); } -verify.signatureHelpPresentForTriggerReason(/*triggerReason*/ undefined); -verify.signatureHelpPresentForTriggerReason({ kind: "invoked" }); \ No newline at end of file From 0fd587a3c32c793ac79beced19d70c8eed487c7c Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Tue, 3 Jul 2018 13:02:14 -0700 Subject: [PATCH 3/6] Account for comments. --- src/services/signatureHelp.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/signatureHelp.ts b/src/services/signatureHelp.ts index 2884f7a7a0ccc..a923959258303 100644 --- a/src/services/signatureHelp.ts +++ b/src/services/signatureHelp.ts @@ -31,7 +31,7 @@ namespace ts.SignatureHelp { 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)) { + if (isInString(sourceFile, position, startingToken) || isInComment(sourceFile, position)) { return undefined; } } From 53a0f2374aa5e270e360d041674ab613d09f5912 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Tue, 3 Jul 2018 17:00:34 -0700 Subject: [PATCH 4/6] Remove 'name' property which was invalid. --- src/harness/fourslash.ts | 4 +++- tests/cases/fourslash/fourslash.ts | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 859eff93575b6..9463e5c8f69c9 100644 --- a/src/harness/fourslash.ts +++ b/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); + } } } diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 2c62f5161d619..6e810c597772d 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -553,7 +553,6 @@ declare namespace FourSlashInterface { overloadsCount?: number; docComment?: string; text?: string; - name?: string; parameterName?: string; parameterSpan?: string; parameterDocComment?: string; From a3b22374375a122ad8ff075fd9cace73e3568ab9 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Tue, 3 Jul 2018 17:01:19 -0700 Subject: [PATCH 5/6] Added tests for syntactic context. --- .../signatureHelpFilteredTriggers02.ts | 36 ++++++++++++++++++ .../signatureHelpFilteredTriggers03.ts | 23 +++++++++++ .../fourslash/signatureHelpWithTriggers01.ts | 31 +++++++++++++++ .../fourslash/signatureHelpWithTriggers02.ts | 38 +++++++++++++++++++ 4 files changed, 128 insertions(+) create mode 100644 tests/cases/fourslash/signatureHelpFilteredTriggers02.ts create mode 100644 tests/cases/fourslash/signatureHelpFilteredTriggers03.ts create mode 100644 tests/cases/fourslash/signatureHelpWithTriggers01.ts create mode 100644 tests/cases/fourslash/signatureHelpWithTriggers02.ts diff --git a/tests/cases/fourslash/signatureHelpFilteredTriggers02.ts b/tests/cases/fourslash/signatureHelpFilteredTriggers02.ts new file mode 100644 index 0000000000000..e193f919e05b5 --- /dev/null +++ b/tests/cases/fourslash/signatureHelpFilteredTriggers02.ts @@ -0,0 +1,36 @@ +/// + +////function foo(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); +} diff --git a/tests/cases/fourslash/signatureHelpFilteredTriggers03.ts b/tests/cases/fourslash/signatureHelpFilteredTriggers03.ts new file mode 100644 index 0000000000000..7b17082361075 --- /dev/null +++ b/tests/cases/fourslash/signatureHelpFilteredTriggers03.ts @@ -0,0 +1,23 @@ +/// + +////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: ",", +}); diff --git a/tests/cases/fourslash/signatureHelpWithTriggers01.ts b/tests/cases/fourslash/signatureHelpWithTriggers01.ts new file mode 100644 index 0000000000000..ae73b5fe00041 --- /dev/null +++ b/tests/cases/fourslash/signatureHelpWithTriggers01.ts @@ -0,0 +1,31 @@ +/// + +////declare function foo(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); +} diff --git a/tests/cases/fourslash/signatureHelpWithTriggers02.ts b/tests/cases/fourslash/signatureHelpWithTriggers02.ts new file mode 100644 index 0000000000000..cb73237f8fd2d --- /dev/null +++ b/tests/cases/fourslash/signatureHelpWithTriggers02.ts @@ -0,0 +1,38 @@ +/// + +////declare function foo(x: T, y: T): T; +////declare function bar(x: U, y: U): U; +//// +////foo(bar/*1*/) + +goTo.marker("1"); + +edit.insert("("); +verify.signatureHelp({ + text: "bar(x: U, y: U): U", + triggerReason: { + kind: "characterTyped", + triggerCharacter: "(", + } +}); +edit.backspace(); + +edit.insert("<"); +verify.signatureHelp({ + text: "bar(x: U, y: U): U", + triggerReason: { + kind: "characterTyped", + triggerCharacter: "(", + } +}); +edit.backspace(); + +edit.insert(","); +verify.signatureHelp({ + text: "foo(x: (x: U, y: U) => U, y: (x: U, y: U) => U): (x: U, y: U) => U", + triggerReason: { + kind: "characterTyped", + triggerCharacter: "(", + } +}); +edit.backspace(); \ No newline at end of file From 9481faab98bae79b897b9f130ae0bf508e280e26 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Tue, 3 Jul 2018 17:04:03 -0700 Subject: [PATCH 6/6] Only provide signature help contextually on a character trigger. --- src/services/signatureHelp.ts | 64 +++++++++++++++++++++++++++++------ 1 file changed, 54 insertions(+), 10 deletions(-) diff --git a/src/services/signatureHelp.ts b/src/services/signatureHelp.ts index a923959258303..491c905785e68 100644 --- a/src/services/signatureHelp.ts +++ b/src/services/signatureHelp.ts @@ -29,8 +29,11 @@ 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. + // 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, readonly resolvedSignature: Signature } | undefined { - function getCandidateInfo(argumentInfo: ArgumentListInfo, checker: TypeChecker): { readonly candidates: ReadonlyArray, 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;