Merge pull request #792 from Microsoft/sigHelp

Various small fixes related to signature help

Author: JsonFreeman

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 ((<NodeArray<Node>>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;
+            }
+
+            // 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;
             }
-            return isCorrect;
+
+            // 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.

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:
@@ -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;

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';
 
@@ -944,29 +951,16 @@ 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);
-
-            // 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];
         }
 

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 = <CallExpression>argumentList.parent;
+        var call = <CallExpression>argumentInfo.list.parent;
         var candidates = <Signature[]>[];
         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 = <CallExpression>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,35 @@ module ts.SignatureHelp {
             return undefined;
         }
 
-        function createSignatureHelpItems(candidates: Signature[], bestSignature: Signature, argumentListOrTypeArgumentList: Node): SignatureHelpItems {
+        /**
+         * 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 => {
                 var parameters = candidateSignature.parameters;
                 var parameterHelpItems: SignatureHelpParameter[] = parameters.length === 0 ? emptyArray : map(parameters, p => {
@@ -321,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,
             //
@@ -338,63 +365,38 @@ 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;
+
+            // 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,
                 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 = <CallExpression>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);

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<T>(x: T, y: T) {
     }
 
     f2(1, 2, );
            ~
-!!! error TS1009: Trailing comma not allowed.
+!!! error TS1009: Trailing comma not allowed.
+    ~~~~~~~~~~
+!!! error TS2346: Supplied parameters do not match any signature of call target.