From 42ae38ddcc7c48867b7afee0c13032f8a243663d Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 11 Mar 2015 14:44:32 -0700 Subject: [PATCH 1/4] Add failing test. --- .../fourslash/signatureHelpWithInvalidArgumentList1.ts | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 tests/cases/fourslash/signatureHelpWithInvalidArgumentList1.ts diff --git a/tests/cases/fourslash/signatureHelpWithInvalidArgumentList1.ts b/tests/cases/fourslash/signatureHelpWithInvalidArgumentList1.ts new file mode 100644 index 00000000000..f280adb1d9e --- /dev/null +++ b/tests/cases/fourslash/signatureHelpWithInvalidArgumentList1.ts @@ -0,0 +1,9 @@ +/// + +////function foo(a) { } +////foo(hello my name /**/is + +goTo.marker(); +verify.signatureHelpPresent(); +verify.signatureHelpCountIs(1); + From d9d90b2c02e21087275d545b3b9e4dabc28164c0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 11 Mar 2015 15:05:31 -0700 Subject: [PATCH 2/4] Compute consistent argument indices and counts for signature help. --- src/services/services.ts | 4 +++ src/services/signatureHelp.ts | 61 +++++++++++++++++++++++------------ src/services/utilities.ts | 2 ++ 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/services/services.ts b/src/services/services.ts index 6ef044ad75a..502387c948f 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -207,6 +207,10 @@ module ts { if (pos < nodes.end) { this.addSyntheticNodes(list._children, pos, nodes.end); } + + if (nodes.hasTrailingComma) { + + } return list; } diff --git a/src/services/signatureHelp.ts b/src/services/signatureHelp.ts index 6f4f38aa736..80befc1bebd 100644 --- a/src/services/signatureHelp.ts +++ b/src/services/signatureHelp.ts @@ -238,7 +238,7 @@ module ts.SignatureHelp { invocation: callExpression, argumentsSpan: getApplicableSpanForArguments(list), argumentIndex: 0, - argumentCount: getCommaBasedArgCount(list) + argumentCount: getArgumentCount(list) }; } @@ -253,20 +253,11 @@ module ts.SignatureHelp { var list = listItemInfo.list; var isTypeArgList = callExpression.typeArguments && callExpression.typeArguments.pos === list.pos; - // 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 = (listItemInfo.listItemIndex + 1) >> 1; + var argumentIndex = getArgumentIndex(list, node); + var argumentCount = getArgumentCount(list); - var argumentCount = getCommaBasedArgCount(list); - - Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`); + Debug.assert(argumentIndex === 0 || argumentIndex < argumentCount, + `argumentCount < argumentIndex, ${argumentCount} < ${argumentIndex}`); return { kind: isTypeArgList ? ArgumentListKind.TypeArguments : ArgumentListKind.CallArguments, @@ -313,12 +304,42 @@ module ts.SignatureHelp { return undefined; } - function getCommaBasedArgCount(argumentsList: Node) { - // The number of arguments is the number of commas plus one, unless the list - // is completely empty, in which case there are 0 arguments. - return argumentsList.getChildCount() === 0 - ? 0 - : 1 + countWhere(argumentsList.getChildren(), arg => arg.kind === SyntaxKind.CommaToken); + function getArgumentIndex(argumentsList: Node, node: Node) { + // The list we got back can include commas. In the presence of errors it may + // also just have nodes without commas. For example "Foo(a b c)" will have 3 + // args without commas. We want to find what index we're at. So we count + // forward until we hit ourselves, only incrementing the index if it isn't a + // comma. + var argumentIndex = 0; + var listChildren = argumentsList.getChildren(); + for (var i = 0, n = listChildren.length; i < n; i++) { + var child = listChildren[i]; + if (child === node) { + break; + } + if (child.kind !== SyntaxKind.CommaToken) { + argumentIndex++; + } + } + + return argumentIndex; + } + + function getArgumentCount(argumentsList: Node) { + // The argument count for a list is normally the number of non-comma children it has. + // For example, if you have "Foo(a,b)" then there will be three children of the arg + // list 'a' '' 'b'. So, in this case the arg count will be 2. However, there + // is a small subtlety. If you have "Foo(a,)", then the child list will just have + // 'a' ''. So, in the case where the last child is a comma, we increase the + // arg count by one to compensate. + var listChildren = argumentsList.getChildren(); + + var argumentCount = countWhere(listChildren, arg => arg.kind !== SyntaxKind.CommaToken); + if (listChildren.length > 0 && lastOrUndefined(listChildren).kind === SyntaxKind.CommaToken) { + argumentCount++; + } + + return argumentCount; } // spanIndex is either the index for a given template span. diff --git a/src/services/utilities.ts b/src/services/utilities.ts index e0ff5293546..f2403217c24 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -95,6 +95,8 @@ module ts { } }); + // Either we didn't find an appropriate list, or the list must contain us. + Debug.assert(!syntaxList || contains(syntaxList.getChildren(), node)); return syntaxList; } From 63ba6457919c7dec29df5e727e7a99ad481123d7 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 11 Mar 2015 15:08:28 -0700 Subject: [PATCH 3/4] Remove unnecessary code. --- src/services/services.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/services/services.ts b/src/services/services.ts index 502387c948f..6ef044ad75a 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -207,10 +207,6 @@ module ts { if (pos < nodes.end) { this.addSyntheticNodes(list._children, pos, nodes.end); } - - if (nodes.hasTrailingComma) { - - } return list; } From 05c2a3ef892f096633448b01f5af106a9b8df045 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 11 Mar 2015 15:30:33 -0700 Subject: [PATCH 4/4] Add explanatory comments. --- src/services/signatureHelp.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/services/signatureHelp.ts b/src/services/signatureHelp.ts index 80befc1bebd..52a7b41d8a7 100644 --- a/src/services/signatureHelp.ts +++ b/src/services/signatureHelp.ts @@ -310,6 +310,12 @@ module ts.SignatureHelp { // args without commas. We want to find what index we're at. So we count // forward until we hit ourselves, only incrementing the index if it isn't a // comma. + // + // Note: the subtlety around trailing commas (in getArgumentCount) does not apply + // here. That's because we're only walking forward until we hit the node we're + // on. In that case, even if we're after the trailing comma, we'll still see + // that trailing comma in the list, and we'll have generated the appropriate + // arg index. var argumentIndex = 0; var listChildren = argumentsList.getChildren(); for (var i = 0, n = listChildren.length; i < n; i++) { @@ -332,6 +338,11 @@ module ts.SignatureHelp { // is a small subtlety. If you have "Foo(a,)", then the child list will just have // 'a' ''. So, in the case where the last child is a comma, we increase the // arg count by one to compensate. + // + // Note: this subtlety only applies to the last comma. If you had "Foo(a,," then + // we'll have: 'a' '' '' + // That will give us 2 non-commas. We then add one for the last comma, givin us an + // arg count of 3. var listChildren = argumentsList.getChildren(); var argumentCount = countWhere(listChildren, arg => arg.kind !== SyntaxKind.CommaToken);