From ab3326f7b7b0631e2cb82f11d7b59035fc4d5d49 Mon Sep 17 00:00:00 2001 From: Jason Freeman Date: Fri, 26 Sep 2014 13:51:13 -0700 Subject: [PATCH 1/6] Fix arity checking for partial overload resolution --- src/compiler/checker.ts | 43 +++++++++++-------- ...trailingSeparatorInFunctionCall.errors.txt | 10 ++++- .../fourslash/signatureHelpOnOverloads.ts | 6 +-- .../signatureHelpOnOverloadsDifferentArity.ts | 20 +++++++++ 4 files changed, 56 insertions(+), 23 deletions(-) create mode 100644 tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 99b387dd18c..fa8b516924b 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -4331,25 +4331,32 @@ module ts { } function signatureHasCorrectArity(node: CallExpression, signature: Signature): boolean { - var args = node.arguments || emptyArray; - var isCorrect = args.length >= signature.minArgumentCount && - (signature.hasRestParameter || args.length <= signature.parameters.length) && - (!node.typeArguments || signature.typeParameters && node.typeArguments.length === signature.typeParameters.length); - - // For error recovery, since we have parsed OmittedExpressions for any extra commas - // in the argument list, if we see any OmittedExpressions, just return true. - // The reason this is ok is because omitted expressions here are syntactically - // illegal, and will cause a parse error. - // Note: It may be worth keeping the upper bound check on arity, but removing - // the lower bound check if there are omitted expressions. - if (!isCorrect) { - // Technically this type assertion is not safe because args could be initialized to emptyArray - // above. - if ((>args).hasTrailingComma || forEach(args, arg => arg.kind === SyntaxKind.OmittedExpression)) { - return true; - } + if (!node.arguments) { + // This only happens when we have something of the form: + // new C + // + return signature.minArgumentCount === 0; } - return isCorrect; + + // For IDE scenarios, since we may have an incomplete call, we make two modifications + // to arity checking. + // 1. A trailing comma is tantamount to adding another argument + // 2. If the call is incomplete (no closing paren) allow fewer arguments than expected + var args = node.arguments; + var numberOfArgs = args.hasTrailingComma ? args.length + 1 : args.length; + var hasTooManyArguments = !signature.hasRestParameter && numberOfArgs > signature.parameters.length; + var hasRightNumberOfTypeArguments = !node.typeArguments || + (signature.typeParameters && node.typeArguments.length === signature.typeParameters.length); + + if (hasTooManyArguments || !hasRightNumberOfTypeArguments) { + return false; + } + + // If we are missing the close paren, the call is incomplete, and we should skip + // the lower bound check. + var callIsIncomplete = args.end === node.end; + var hasEnoughArguments = numberOfArgs >= signature.minArgumentCount; + return callIsIncomplete || hasEnoughArguments; } // If type has a single call signature and no other members, return that signature. Otherwise, return undefined. diff --git a/tests/baselines/reference/trailingSeparatorInFunctionCall.errors.txt b/tests/baselines/reference/trailingSeparatorInFunctionCall.errors.txt index 3edeb834baa..986f2dedf6e 100644 --- a/tests/baselines/reference/trailingSeparatorInFunctionCall.errors.txt +++ b/tests/baselines/reference/trailingSeparatorInFunctionCall.errors.txt @@ -1,18 +1,24 @@ tests/cases/compiler/trailingSeparatorInFunctionCall.ts(4,7): error TS1009: Trailing comma not allowed. tests/cases/compiler/trailingSeparatorInFunctionCall.ts(9,8): error TS1009: Trailing comma not allowed. +tests/cases/compiler/trailingSeparatorInFunctionCall.ts(4,1): error TS2346: Supplied parameters do not match any signature of call target. +tests/cases/compiler/trailingSeparatorInFunctionCall.ts(9,1): error TS2346: Supplied parameters do not match any signature of call target. -==== tests/cases/compiler/trailingSeparatorInFunctionCall.ts (2 errors) ==== +==== tests/cases/compiler/trailingSeparatorInFunctionCall.ts (4 errors) ==== function f(x, y) { } f(1, 2, ); ~ !!! error TS1009: Trailing comma not allowed. + ~~~~~~~~~ +!!! error TS2346: Supplied parameters do not match any signature of call target. function f2(x: T, y: T) { } f2(1, 2, ); ~ -!!! error TS1009: Trailing comma not allowed. \ No newline at end of file +!!! error TS1009: Trailing comma not allowed. + ~~~~~~~~~~ +!!! error TS2346: Supplied parameters do not match any signature of call target. \ No newline at end of file diff --git a/tests/cases/fourslash/signatureHelpOnOverloads.ts b/tests/cases/fourslash/signatureHelpOnOverloads.ts index 83d7b75a4cf..9712c9dc301 100644 --- a/tests/cases/fourslash/signatureHelpOnOverloads.ts +++ b/tests/cases/fourslash/signatureHelpOnOverloads.ts @@ -13,6 +13,6 @@ verify.currentParameterSpanIs("x: string"); edit.insert("'',"); verify.signatureHelpCountIs(2); -// verify.currentSignatureHelpIs("fn(x: string, y: number): any"); -// verify.currentParameterHelpArgumentNameIs("y"); -// verify.currentParameterSpanIs("y: number"); +verify.currentSignatureHelpIs("fn(x: string, y: number): any"); +verify.currentParameterHelpArgumentNameIs("y"); +verify.currentParameterSpanIs("y: number"); diff --git a/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity.ts b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity.ts new file mode 100644 index 00000000000..fe18f655ed4 --- /dev/null +++ b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity.ts @@ -0,0 +1,20 @@ +/// + +////declare function f(s: string); +////declare function f(n: number); +////declare function f(s: string, b: boolean); +////declare function f(n: number, b: boolean); +//// +////f(1/**/ + +goTo.marker(); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(n: number): any"); +verify.currentParameterHelpArgumentNameIs("n"); +verify.currentParameterSpanIs("n: number"); + +edit.insert(", "); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(n: number, b: boolean): any"); +verify.currentParameterHelpArgumentNameIs("b"); +verify.currentParameterSpanIs("b: boolean"); \ No newline at end of file From a79a1d22489584f1175dd14ef3a019798e0d5df4 Mon Sep 17 00:00:00 2001 From: Jason Freeman Date: Tue, 30 Sep 2014 11:06:03 -0700 Subject: [PATCH 2/6] Record trailing comma even for incorrectly terminated lists --- src/compiler/parser.ts | 29 +++++++++++-------- ...signatureHelpOnOverloadsDifferentArity2.ts | 20 +++++++++++++ 2 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity2.ts diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index b06326f6925..f67873c2c40 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -1226,18 +1226,6 @@ module ts { error(Diagnostics._0_expected, ","); } else if (isListTerminator(kind)) { - // Check if the last token was a comma. - if (commaStart >= 0) { - if (!allowTrailingComma) { - if (file.syntacticErrors.length === errorCountBeforeParsingList) { - // Report a grammar error so we don't affect lookahead - grammarErrorAtPos(commaStart, scanner.getStartPos() - commaStart, Diagnostics.Trailing_comma_not_allowed); - } - } - // Always preserve a trailing comma by marking it on the NodeArray - result.hasTrailingComma = true; - } - break; } else { @@ -1248,6 +1236,23 @@ module ts { nextToken(); } } + + // Recording the trailing comma is deliberately done after the previous + // loop, and not just if we see a list terminator. This is because the list + // may have ended incorrectly, but it is still important to know if there + // was a trailing comma. + // Check if the last token was a comma. + if (commaStart >= 0) { + if (!allowTrailingComma) { + if (file.syntacticErrors.length === errorCountBeforeParsingList) { + // Report a grammar error so we don't affect lookahead + grammarErrorAtPos(commaStart, scanner.getStartPos() - commaStart, Diagnostics.Trailing_comma_not_allowed); + } + } + // Always preserve a trailing comma by marking it on the NodeArray + result.hasTrailingComma = true; + } + result.end = getNodeEnd(); parsingContext = saveParsingContext; return result; diff --git a/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity2.ts b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity2.ts new file mode 100644 index 00000000000..1f87f3b9a0c --- /dev/null +++ b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity2.ts @@ -0,0 +1,20 @@ +/// + +////declare function f(s: string); +////declare function f(n: number); +////declare function f(s: string, b: boolean); +////declare function f(n: number, b: boolean); +//// +////f(1/**/ var + +goTo.marker(); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(n: number): any"); +verify.currentParameterHelpArgumentNameIs("n"); +verify.currentParameterSpanIs("n: number"); + +edit.insert(", "); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(n: number, b: boolean): any"); +verify.currentParameterHelpArgumentNameIs("b"); +verify.currentParameterSpanIs("b: boolean"); \ No newline at end of file From 03821df591d75bbaf893cc2e509db783d8ad6ee4 Mon Sep 17 00:00:00 2001 From: Jason Freeman Date: Tue, 30 Sep 2014 13:46:07 -0700 Subject: [PATCH 3/6] Remove getCurrentArgumentState --- src/services/signatureHelp.ts | 94 ++++++++++++----------------------- 1 file changed, 32 insertions(+), 62 deletions(-) diff --git a/src/services/signatureHelp.ts b/src/services/signatureHelp.ts index 796472c030e..3af419b3f90 100644 --- a/src/services/signatureHelp.ts +++ b/src/services/signatureHelp.ts @@ -172,15 +172,15 @@ module ts.SignatureHelp { return undefined; } - var argumentList = getContainingArgumentList(startingToken); + var argumentInfo = getContainingArgumentInfo(startingToken); cancellationToken.throwIfCancellationRequested(); // Semantic filtering of signature help - if (!argumentList) { + if (!argumentInfo) { return undefined; } - var call = argumentList.parent; + var call = argumentInfo.list.parent; var candidates = []; var resolvedSignature = typeInfoResolver.getResolvedSignature(call, candidates); cancellationToken.throwIfCancellationRequested(); @@ -189,13 +189,13 @@ module ts.SignatureHelp { return undefined; } - return createSignatureHelpItems(candidates, resolvedSignature, argumentList); + return createSignatureHelpItems(candidates, resolvedSignature, argumentInfo); /** * If node is an argument, returns its index in the argument list. * If not, returns -1. */ - function getImmediatelyContainingArgumentList(node: Node): Node { + function getImmediatelyContainingArgumentInfo(node: Node): ListItemInfo { if (node.parent.kind !== SyntaxKind.CallExpression && node.parent.kind !== SyntaxKind.NewExpression) { return undefined; } @@ -216,10 +216,14 @@ module ts.SignatureHelp { var parent = node.parent; // Find out if 'node' is an argument, a type argument, or neither if (node.kind === SyntaxKind.LessThanToken || node.kind === SyntaxKind.OpenParenToken) { - // Find the list that starts right *after* the < or ( token + // Find the list that starts right *after* the < or ( token. + // If the user has just opened a list, consider this item 0. var list = getChildListThatStartsWithOpenerToken(parent, node, sourceFile); Debug.assert(list); - return list; + return { + list: list, + listItemIndex: 0 + }; } if (node.kind === SyntaxKind.GreaterThanToken @@ -228,18 +232,18 @@ module ts.SignatureHelp { return undefined; } - return findContainingList(node); + return findListItemInfo(node); } - function getContainingArgumentList(node: Node): Node { + function getContainingArgumentInfo(node: Node): ListItemInfo { for (var n = node; n.kind !== SyntaxKind.SourceFile; n = n.parent) { if (n.kind === SyntaxKind.FunctionBlock) { return undefined; } - var argumentList = getImmediatelyContainingArgumentList(n); - if (argumentList) { - return argumentList; + var argumentInfo = getImmediatelyContainingArgumentInfo(n); + if (argumentInfo) { + return argumentInfo; } @@ -248,7 +252,8 @@ module ts.SignatureHelp { return undefined; } - function createSignatureHelpItems(candidates: Signature[], bestSignature: Signature, argumentListOrTypeArgumentList: Node): SignatureHelpItems { + function createSignatureHelpItems(candidates: Signature[], bestSignature: Signature, argumentInfoOrTypeArgumentInfo: ListItemInfo): SignatureHelpItems { + var argumentListOrTypeArgumentList = argumentInfoOrTypeArgumentInfo.list; var items: SignatureHelpItem[] = map(candidates, candidateSignature => { var parameters = candidateSignature.parameters; var parameterHelpItems: SignatureHelpParameter[] = parameters.length === 0 ? emptyArray : map(parameters, p => { @@ -338,63 +343,28 @@ module ts.SignatureHelp { var applicableSpanEnd = skipTrivia(sourceFile.text, argumentListOrTypeArgumentList.end, /*stopAfterLineBreak*/ false); var applicableSpan = new TypeScript.TextSpan(applicableSpanStart, applicableSpanEnd - applicableSpanStart); - var state = getSignatureHelpCurrentArgumentState(sourceFile, position, applicableSpanStart); + // The listItemIndex we got back includes commas. Our goal is to return the index of the proper + // item (not including commas). Here are some examples: + // 1. foo(a, b, c $) -> the listItemIndex is 4, we want to return 2 + // 2. foo(a, b, $ c) -> listItemIndex is 3, we want to return 2 + // 3. foo($a) -> listItemIndex is 0, we want to return 0 + // + // In general, we want to subtract the number of commas before the current index. + // But if we are on a comma, we also want to pretend we are on the argument *following* + // the comma. That amounts to taking the ceiling of half the index. + var argumentIndex = (argumentInfoOrTypeArgumentInfo.listItemIndex + 1) >> 1; + var numberOfCommas = countWhere(argumentListOrTypeArgumentList.getChildren(), arg => arg.kind === SyntaxKind.CommaToken); + var argumentCount = numberOfCommas + 1; return { items: items, applicableSpan: applicableSpan, selectedItemIndex: selectedItemIndex, - argumentIndex: state.argumentIndex, - argumentCount: state.argumentCount + argumentIndex: argumentIndex, + argumentCount: argumentCount }; } } - function getSignatureHelpCurrentArgumentState(sourceFile: SourceFile, position: number, applicableSpanStart: number): { argumentIndex: number; argumentCount: number } { - var tokenPrecedingSpanStart = findPrecedingToken(applicableSpanStart, sourceFile); - if (!tokenPrecedingSpanStart) { - return undefined; - } - - if (tokenPrecedingSpanStart.kind !== SyntaxKind.OpenParenToken && tokenPrecedingSpanStart.kind !== SyntaxKind.LessThanToken) { - // The span start must have moved backward in the file (for example if the open paren was backspaced) - return undefined; - } - - var tokenPrecedingCurrentPosition = findPrecedingToken(position, sourceFile); - var call = tokenPrecedingSpanStart.parent; - Debug.assert(call.kind === SyntaxKind.CallExpression || call.kind === SyntaxKind.NewExpression, "wrong call kind " + SyntaxKind[call.kind]); - if (tokenPrecedingCurrentPosition.kind === SyntaxKind.CloseParenToken || tokenPrecedingCurrentPosition.kind === SyntaxKind.GreaterThanToken) { - if (tokenPrecedingCurrentPosition.parent === call) { - // This call expression is complete. Stop signature help. - return undefined; - } - } - - var argumentListOrTypeArgumentList = getChildListThatStartsWithOpenerToken(call, tokenPrecedingSpanStart, sourceFile); - // Debug.assert(argumentListOrTypeArgumentList.getChildCount() === 0 || argumentListOrTypeArgumentList.getChildCount() % 2 === 1, "Even number of children"); - - // The call might be finished, but incorrectly. Check if we are still within the bounds of the call - if (position > skipTrivia(sourceFile.text, argumentListOrTypeArgumentList.end, /*stopAfterLineBreak*/ false)) { - return undefined; - } - - var numberOfCommas = countWhere(argumentListOrTypeArgumentList.getChildren(), arg => arg.kind === SyntaxKind.CommaToken); - var argumentCount = numberOfCommas + 1; - if (argumentCount <= 1) { - return { argumentIndex: 0, argumentCount: argumentCount }; - } - - var indexOfNodeContainingPosition = findListItemIndexContainingPosition(argumentListOrTypeArgumentList, position); - - // indexOfNodeContainingPosition checks that position is between pos and end of each child, so it is - // possible that we are to the right of all children. Assume that we are still within - // the applicable span and that we are typing the last argument - // Alternatively, we could be in range of one of the arguments, in which case we need to divide - // by 2 to exclude commas. Use bit shifting in order to take the floor of the division. - var argumentIndex = indexOfNodeContainingPosition < 0 ? argumentCount - 1 : indexOfNodeContainingPosition >> 1; - return { argumentIndex: argumentIndex, argumentCount: argumentCount }; - } - function getChildListThatStartsWithOpenerToken(parent: Node, openerToken: Node, sourceFile: SourceFile): Node { var children = parent.getChildren(sourceFile); var indexOfOpenerToken = children.indexOf(openerToken); From a89710471d9b55dd4abc6cb8e5a40101e6b065b9 Mon Sep 17 00:00:00 2001 From: Jason Freeman Date: Tue, 30 Sep 2014 15:26:39 -0700 Subject: [PATCH 4/6] Add test for empty arguments and arity filtering --- src/harness/fourslash.ts | 5 ---- ...signatureHelpOnOverloadsDifferentArity3.ts | 26 +++++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index c264c59348f..64d87f45d1b 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -944,11 +944,6 @@ module FourSlash { this.validate("error", message, renameInfo.localizedErrorMessage); } - //private getFormalParameter() { - // var help = this.languageService.getSignatureHelpItems(this.activeFile.fileName, this.currentCaretPosition); - // return help.formal; - //} - private getActiveSignatureHelpItem() { var help = this.languageService.getSignatureHelpItems(this.activeFile.fileName, this.currentCaretPosition); diff --git a/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts new file mode 100644 index 00000000000..4d2db0922ee --- /dev/null +++ b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts @@ -0,0 +1,26 @@ +/// + +////declare function f(); +////declare function f(s: string); +////declare function f(s: string, b: boolean); +////declare function f(n: number, b: boolean); +//// +////f(/**/ + +goTo.marker(); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(): any"); +verify.currentSignatureParamterCountIs(0); + +edit.insert(", "); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(s: string, b: boolean): any"); +verify.currentSignatureParamterCountIs(2); +verify.currentParameterHelpArgumentNameIs("b"); +verify.currentParameterSpanIs("b: boolean"); + +// What should the intended behavior be if there are too many arguments? +edit.insert(", "); +verify.signatureHelpCountIs(4); +verify.currentSignatureHelpIs("f(): any"); +verify.currentSignatureParamterCountIs(0); \ No newline at end of file From 23843ffa92519a970c1ef460035e07a9ba8177ab Mon Sep 17 00:00:00 2001 From: Jason Freeman Date: Tue, 30 Sep 2014 16:36:20 -0700 Subject: [PATCH 5/6] Parse OmittedExpression for missing arguments followed by commas --- src/compiler/parser.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index f67873c2c40..fcc8c62ba16 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -1073,7 +1073,7 @@ module ts { case ParsingContext.TypeParameters: return isIdentifier(); case ParsingContext.ArgumentExpressions: - return isExpression(); + return token === SyntaxKind.CommaToken || isExpression(); case ParsingContext.ArrayLiteralMembers: return token === SyntaxKind.CommaToken || isExpression(); case ParsingContext.Parameters: From b2bca72bb65cbfcc360020753875c1b8a34c9ae4 Mon Sep 17 00:00:00 2001 From: Jason Freeman Date: Tue, 30 Sep 2014 17:13:27 -0700 Subject: [PATCH 6/6] Fix argumentCount and selectedItemIndex --- src/harness/fourslash.ts | 19 ++++---- src/services/signatureHelp.ts | 46 ++++++++++++++++--- tests/cases/fourslash/fourslash.ts | 4 ++ ...signatureHelpOnOverloadsDifferentArity3.ts | 6 +-- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 64d87f45d1b..d33b5c0715d 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -891,6 +891,13 @@ module FourSlash { assert.equal(actual, expected); } + public verifySignatureHelpArgumentCount(expected: number) { + this.taoInvalidReason = 'verifySignatureHelpArgumentCount NYI'; + var signatureHelpItems = this.languageService.getSignatureHelpItems(this.activeFile.fileName, this.currentCaretPosition); + var actual = signatureHelpItems.argumentCount; + assert.equal(actual, expected); + } + public verifySignatureHelpPresent(shouldBePresent = true) { this.taoInvalidReason = 'verifySignatureHelpPresent NYI'; @@ -946,22 +953,14 @@ module FourSlash { private getActiveSignatureHelpItem() { var help = this.languageService.getSignatureHelpItems(this.activeFile.fileName, this.currentCaretPosition); - - // If the signature hasn't been narrowed down yet (e.g. no parameters have yet been entered), - // 'activeFormal' will be -1 (even if there is only 1 signature). Signature help will show the - // first signature in the signature group, so go with that - var index = help.selectedItemIndex < 0 ? 0 : help.selectedItemIndex; - + var index = help.selectedItemIndex; return help.items[index]; } private getActiveParameter(): ts.SignatureHelpParameter { var help = this.languageService.getSignatureHelpItems(this.activeFile.fileName, this.currentCaretPosition); - var item = help.items[help.selectedItemIndex]; - - // Same logic as in getActiveSignatureHelp - this value might be -1 until a parameter value actually gets typed - var currentParam = help.argumentIndex < 0 ? 0 : help.argumentIndex; + var currentParam = help.argumentIndex; return item.parameters[currentParam]; } diff --git a/src/services/signatureHelp.ts b/src/services/signatureHelp.ts index 3af419b3f90..080e4704b83 100644 --- a/src/services/signatureHelp.ts +++ b/src/services/signatureHelp.ts @@ -252,6 +252,33 @@ module ts.SignatureHelp { return undefined; } + /** + * The selectedItemIndex could be negative for several reasons. + * 1. There are too many arguments for all of the overloads + * 2. None of the overloads were type compatible + * The solution here is to try to pick the best overload by picking + * either the first one that has an appropriate number of parameters, + * or the one with the most parameters. + */ + function selectBestInvalidOverloadIndex(candidates: Signature[], argumentCount: number): number { + var maxParamsSignatureIndex = -1; + var maxParams = -1; + for (var i = 0; i < candidates.length; i++) { + var candidate = candidates[i]; + + if (candidate.hasRestParameter || candidate.parameters.length >= argumentCount) { + return i; + } + + if (candidate.parameters.length > maxParams) { + maxParams = candidate.parameters.length; + maxParamsSignatureIndex = i; + } + } + + return maxParamsSignatureIndex; + } + function createSignatureHelpItems(candidates: Signature[], bestSignature: Signature, argumentInfoOrTypeArgumentInfo: ListItemInfo): SignatureHelpItems { var argumentListOrTypeArgumentList = argumentInfoOrTypeArgumentInfo.list; var items: SignatureHelpItem[] = map(candidates, candidateSignature => { @@ -326,11 +353,6 @@ module ts.SignatureHelp { }; }); - var selectedItemIndex = candidates.indexOf(bestSignature); - if (selectedItemIndex < 0) { - selectedItemIndex = 0; - } - // We use full start and skip trivia on the end because we want to include trivia on // both sides. For example, // @@ -353,8 +375,18 @@ module ts.SignatureHelp { // But if we are on a comma, we also want to pretend we are on the argument *following* // the comma. That amounts to taking the ceiling of half the index. var argumentIndex = (argumentInfoOrTypeArgumentInfo.listItemIndex + 1) >> 1; - var numberOfCommas = countWhere(argumentListOrTypeArgumentList.getChildren(), arg => arg.kind === SyntaxKind.CommaToken); - var argumentCount = numberOfCommas + 1; + + // argumentCount is the number of commas plus one, unless the list is completely empty, + // in which case there are 0. + var argumentCount = argumentListOrTypeArgumentList.getChildCount() === 0 + ? 0 + : 1 + countWhere(argumentListOrTypeArgumentList.getChildren(), arg => arg.kind === SyntaxKind.CommaToken); + + var selectedItemIndex = candidates.indexOf(bestSignature); + if (selectedItemIndex < 0) { + selectedItemIndex = selectBestInvalidOverloadIndex(candidates, argumentCount); + } + return { items: items, applicableSpan: applicableSpan, diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 35b6894b1ce..c204a98c4a4 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -294,6 +294,10 @@ module FourSlashInterface { FourSlash.currentTestState.verifySignatureHelpCount(expected); } + public signatureHelpArgumentCountIs(expected: number) { + FourSlash.currentTestState.verifySignatureHelpArgumentCount(expected); + } + public currentSignatureParamterCountIs(expected: number) { FourSlash.currentTestState.verifyCurrentSignatureHelpParameterCount(expected); } diff --git a/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts index 4d2db0922ee..c217a12ef58 100644 --- a/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts +++ b/tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts @@ -11,6 +11,7 @@ goTo.marker(); verify.signatureHelpCountIs(4); verify.currentSignatureHelpIs("f(): any"); verify.currentSignatureParamterCountIs(0); +verify.signatureHelpArgumentCountIs(0); edit.insert(", "); verify.signatureHelpCountIs(4); @@ -19,8 +20,7 @@ verify.currentSignatureParamterCountIs(2); verify.currentParameterHelpArgumentNameIs("b"); verify.currentParameterSpanIs("b: boolean"); -// What should the intended behavior be if there are too many arguments? edit.insert(", "); verify.signatureHelpCountIs(4); -verify.currentSignatureHelpIs("f(): any"); -verify.currentSignatureParamterCountIs(0); \ No newline at end of file +verify.currentSignatureHelpIs("f(s: string, b: boolean): any"); +verify.currentSignatureParamterCountIs(2); \ No newline at end of file