Issue 19220 function parameter arity #24031

Kr4pper · 2018-05-10T17:57:43Z

Fixes #19220

Further changes could be made to rigorously check all available overload signatures in order to identify cases where emitting possible overloads as a range is applicable and thus respond with more accurate diagnostics.

msftclas · 2018-05-10T17:57:54Z

All CLA requirements met.

Kr4pper · 2018-05-10T19:27:22Z

I am currently working on further analyzing available signatures to convey more useful error information in complex cases.

Kr4pper · 2018-05-11T23:48:59Z

After further investigating all possible cases of function parameter arity mismatches I determined that adding them to this pull would require too many code changes. It would however be possible and relatively straightword to add those cases as more accurate diagnostics in the future.

Kr4pper · 2018-05-13T19:44:04Z

tests/cases/fourslash/signatureHelpOnOverloadsDifferentArity3.ts

@@ -20,7 +20,7 @@ verify.currentSignatureParameterCountIs(2);
 verify.currentParameterHelpArgumentNameIs("b");
 verify.currentParameterSpanIs("b: boolean");

-edit.insert("x, ");
+// edit.insert("x, "); // TODO: This block looks like a duplicate of the above code


Is this test case necessary?

mhegazy · 2018-05-25T22:59:40Z

@sandersn can you please review this change.

Kr4pper · 2018-07-06T09:05:05Z

@sandersn This PR is now up to date with master again.

sandersn · 2018-07-09T15:42:37Z

@rFlorian Just a brief note. I started looking at the PR.

However, I don't believe this is the right solution. I believe that it catches a case (discontinuous overload sets) where the target function is wrong, not the calling code, and so the error message should (1) point the user to the target function (2) be much easier to implement, since it's a rare case.

I don't have data to back me up yet, but I'm in the middle of making a version of the compiler that fails whenever it sees a discontinuous overload set, after which I'll run it on Definitely Typed to see whether real authors ever intentionally write them. I'll report back what I find.

sandersn · 2018-07-09T17:55:49Z

OK, I've got results from the experiment. Turns out I was wrong. The DOM, jquery and node all have discontinuous overload arities, as do many other projects, including Typescript itself. A few of these I looked at are just fussy, but do aid correctness. The others all follow a similar pattern: Sets of parameters get added in logical groups, such as x/y pairs. An example is (sourceX, sourceY); (sourceX, sourceY, targetX, targetY), where the second overload lets you specify both source and target locations.

So I do think this change is necessary. There is still a trade off between smart messages backed by very complex code versus a simple message backed by simple code. I'm not sure where we should end up on that; I'll take a closer look at the code to help me decide.

In case anybody wants to replicate the experiment, here is the code:

        function kaboom(signatures: ReadonlyArray<Signature>) {
            if (length(signatures) === 0) return;
            let spans = [];
            let min = Number.POSITIVE_INFINITY;
            let max = Number.NEGATIVE_INFINITY;
            for (const sig of signatures) {
                let x = { min: getMinArgumentCount(sig), max: getParameterCount(sig) }
                spans.push(x);
                min = Math.min(min, x.min);
                max = Math.max(max, x.max);
            }
            for (var i = min; i <= max; i++) {
                if (!find(spans, ({min,max}) => min <= i && i <= max)) {
                    console.log("Couldn't find " + i + " at " + getTextOfNode(signatures[0].declaration!));
                }
            }
        }

// ... early in checkFunctionOrMethodDeclaration ...
            const t = getTypeOfSymbol(getSymbolOfNode(node))
            const signatures = getSignaturesOfType(t, SignatureKind.Call);
            kaboom(signatures);

It's slow and doesn't handle rest parameters quite right but is good enough to find lots of examples.

sandersn

I'm pretty sure the implementation can be made more efficient (and somewhat simpler). Also I would like a slightly different wording of the error message.

sandersn · 2018-07-06T17:38:03Z

tests/baselines/reference/genericRestParameters1.errors.txt

@@ -26,7 +26,7 @@ tests/cases/conformance/types/rest/genericRestParameters1.ts(31,1): error TS2556
    f1(ns[0], ns[1], true);
    f1(...ns, true);  // Error, tuple spread only expanded when last
    ~~~~~~~~~~~~~~~
-!!! error TS2556: Expected 3 arguments, but got 1 or more.
+!!! error TS2557: Expected at least 0 arguments, but got 2 or more.


This PR needs to work with the new tuple-spreading code. Both the expected and actual argument count are now wrong.

sandersn · 2018-07-09T18:08:02Z

tests/baselines/reference/api/tsserverlibrary.d.ts

@@ -5344,6 +5344,7 @@ declare namespace ts {
        Rest_signatures_are_incompatible: DiagnosticMessage;
        Property_0_is_incompatible_with_rest_element_type: DiagnosticMessage;
        A_rest_element_type_must_be_an_array_type: DiagnosticMessage;
+        No_overload_expects_0_arguments_The_most_likely_overloads_that_match_expect_either_1_arguments_or_at_least_2_arguments: DiagnosticMessage;


I would reword this like so:

No overload expects 1 arguments, but overloads do exist that expect either 0 or at least 2 arguments.

sandersn · 2018-07-09T18:09:10Z

src/compiler/checker.ts

-                const hasRestParameter = some(signatures, hasEffectiveRestParameter);
+                const availableArgumentCounts: number[] = [];
+                forEach(signatures, (sig: Signature) => {
+                    const maxArgs = sig.parameters.length;


fixing the test failures might be as simple as calling the new helpers getMinArgumentCount and getParameterCount instead of directly accessing properties.

sandersn · 2018-07-09T18:15:13Z

src/compiler/checker.ts

+                                undefined;
+                    diagnostics.add(createDiagnosticForNode(node, <DiagnosticMessage>error, paramRange, argCount));
+                }
+                else {


collapse to else if

sandersn · 2018-07-09T18:23:33Z

src/compiler/checker.ts

+                    const error = hasRestParameter && hasSpreadArgument ? Diagnostics.Expected_at_least_0_arguments_but_got_1_or_more :
+                        hasRestParameter ? Diagnostics.Expected_at_least_0_arguments_but_got_1 :
+                            hasSpreadArgument ? Diagnostics.Expected_0_arguments_but_got_1_or_more :
+                                undefined;


skip the hasSpreadArgument check and just return Expected 0 arguments but got 1 or more in the else branch of hasRestParameter.

sandersn · 2018-07-09T20:07:45Z

src/compiler/checker.ts

@@ -19043,26 +19043,48 @@ namespace ts {
                diagnostics.add(getTypeArgumentArityError(node, signatures, typeArguments));
            }
            else if (args) {
-                let min = Number.POSITIVE_INFINITY;
-                let max = Number.NEGATIVE_INFINITY;


I think you should revert all the first section of this change and add two more variables. This worked for me in simple tests:

let belowArgCount = Number.NEGATIVE_INFINITY let aboveArgCount = Number.POSITIVE_INFINITY for (const sig of signatures) { minCount = getMinArgumentCount(sig) maxCount = getParameterCount(sig) if (minCount < argCount && argCount - minCount < argCount - belowArgCount) belowArgCount = minCount if (argCount < maxCount && maxCount - argCount < aboveArgCount - argCount) aboveArgCount = maxCount min = Math.min(min, minCount) max = Math.max(max, maxCount) }

(Note that you'll have to calculate argCount earlier.) Then you can use belowArgCount/aboveArgCount in the new error reporting branch you added in the second half of this change.

Kr4pper · 2018-07-09T23:54:20Z

tests/baselines/reference/genericRestParameters1.errors.txt

@@ -26,7 +26,7 @@ tests/cases/conformance/types/rest/genericRestParameters1.ts(31,1): error TS2556
    f1(ns[0], ns[1], true);
    f1(...ns, true);  // Error, tuple spread only expanded when last
    ~~~~~~~~~~~~~~~
-!!! error TS2556: Expected 3 arguments, but got 1 or more.
+!!! error TS2557: Expected at least 3 arguments, but got 1 or more.


These still emit a slightly changed error message. Is the changed behavior in this case wanted and/or acceptable?

Hmm. It's not desired, because f1 accepts exactly 3 arguments. There must be some way to notice that the rest param is a tuple type (with known length).

Kr4pper · 2018-07-09T23:58:10Z

src/compiler/diagnosticMessages.json

@@ -2056,6 +2056,10 @@
        "category": "Error",
        "code": 2574
    },
+    "No overload expects {0} arguments, but overloads do exist that expect either {1} or at least {2} arguments.": {


I added your simplified error message, however I feel like it might hide information from the user in cases where discontinuous overload sets with argument counts less than the current argument count exist.

So, for example, 4 arguments provided to an overload set that has arity [1,2,5,6,7] ? It will report "Overloads do exist that expect either 2 or at least 5 arguments"

Actually, after thinking a bit more, I'd rather go the other direction and say "Overloads do exist that expect either 2 or 5 arguments". It points out the nearest arities and is terse enough to avoid implying that it has enumerated all possible arities.

Kr4pper · 2018-07-10T00:18:08Z

@sandersn Thank you for the detailed analysis and hints on the PR. I incorporated your suggestions into the PR with slight modifications. I do not fully understand the requirements for diagnostics generation with regards to the new new tuple-spreading code so those might not work fully as intended.

sandersn

The test change in genericRestParameters1 shouldn't happen -- I think there is a check that now exists that you can use to find out whether the rest parameter is a tuple type. Maybe look at hasEffectiveRestParameter and its callers?

Plus a few small style changes.

sandersn · 2018-07-10T15:48:58Z

tests/baselines/reference/genericRestParameters1.errors.txt

@@ -26,7 +26,7 @@ tests/cases/conformance/types/rest/genericRestParameters1.ts(31,1): error TS2556
    f1(ns[0], ns[1], true);
    f1(...ns, true);  // Error, tuple spread only expanded when last
    ~~~~~~~~~~~~~~~
-!!! error TS2556: Expected 3 arguments, but got 1 or more.
+!!! error TS2557: Expected at least 3 arguments, but got 1 or more.


Hmm. It's not desired, because f1 accepts exactly 3 arguments. There must be some way to notice that the rest param is a tuple type (with known length).

sandersn · 2018-07-10T15:50:38Z

src/compiler/checker.ts

                const hasSpreadArgument = getSpreadArgumentIndex(args) > -1;
-                const paramCount = hasRestParameter ? min :


please revert lines 19062-19069 ( if (argCount <= ... min; )

sandersn · 2018-07-10T15:51:01Z

src/compiler/checker.ts

-                diagnostics.add(createDiagnosticForNode(node, error, paramCount, argCount));
+                if (argCount <= max && hasSpreadArgument) argCount--;
+
+                const hasRestParameter = some(signatures, (sig: Signature) => sig.hasRestParameter);


should still be some(signatures, hasEffectiveRestParameter)

sandersn · 2018-07-10T15:55:51Z

src/compiler/diagnosticMessages.json

@@ -2056,6 +2056,10 @@
        "category": "Error",
        "code": 2574
    },
+    "No overload expects {0} arguments, but overloads do exist that expect either {1} or at least {2} arguments.": {


So, for example, 4 arguments provided to an overload set that has arity [1,2,5,6,7] ? It will report "Overloads do exist that expect either 2 or at least 5 arguments"

Actually, after thinking a bit more, I'd rather go the other direction and say "Overloads do exist that expect either 2 or 5 arguments". It points out the nearest arities and is terse enough to avoid implying that it has enumerated all possible arities.

Kr4pper · 2018-07-11T23:06:26Z

When checking for rest parameters, hasEffectiveRestParameter is now properly utilized again. This fixed both cases where tuple unpacking parameters were erroneously assessed as rest parameters.

sandersn

Looks good. I'll merge after you revert a couple of small formatting changes.

sandersn · 2018-07-11T23:08:50Z

src/compiler/checker.ts

-                    Diagnostics.Expected_0_arguments_but_got_1;
-                diagnostics.add(createDiagnosticForNode(node, error, paramCount, argCount));
+                const hasSpreadArgument = getSpreadArgumentIndex(args) > -1;
+                if (argCount <= max && hasSpreadArgument) argCount--;


Please revert this change too. (I'm not sure whether the linter will let it through anyway.)

sandersn · 2018-07-11T23:12:08Z

src/compiler/checker.ts

+
+                if (hasRestParameter || hasSpreadArgument) {
+                    const error = hasRestParameter && hasSpreadArgument ? Diagnostics.Expected_at_least_0_arguments_but_got_1_or_more :
+                        hasRestParameter ? Diagnostics.Expected_at_least_0_arguments_but_got_1 : Diagnostics.Expected_0_arguments_but_got_1_or_more;


Please revert this line-join and put Diagnostics.Expected 0 arguments but got 1 or more back on its own line.

Kr4pper · 2018-07-11T23:23:35Z

Sounds good, I believe everything is now formatted as expected.

sandersn

Looks good. I'll merge after the CI finishes.

sandersn · 2018-07-12T00:20:35Z

Thanks for all your work on this one @rFlorian !

Kr4pper · 2018-07-12T08:31:56Z

Thanks for helping me with the PR @sandersn !

Kr4pper force-pushed the issue-19220-arity-error-msg branch from ace037c to 3998c54 Compare May 12, 2018 13:01

Kr4pper commented May 13, 2018

View reviewed changes

Kr4pper force-pushed the issue-19220-arity-error-msg branch from 7dd99e8 to 9cf84de Compare May 20, 2018 12:31

mhegazy requested a review from sandersn May 25, 2018 22:59

Kr4pper force-pushed the issue-19220-arity-error-msg branch from 9cf84de to 10e2dd3 Compare July 6, 2018 08:18

sandersn requested changes Jul 9, 2018

View reviewed changes

Kr4pper commented Jul 9, 2018

View reviewed changes

sandersn requested changes Jul 11, 2018

View reviewed changes

rFlorian added 13 commits July 11, 2018 20:06

Added reference test case and diagnostics message

4eb1b82

Adjusted arity checks to account for non-contiguous overloads

61e1c58

Code cleanup, baseline not yet commited

b335402

Accepted test baselines and minor implementation changes

26ee440

Cleaned up baseline tracking the now renamed arity check test

f056201

Add range response when range contains only 2 values

8048a7e

Added recent baseline

6864c04

Refined arity error messages when available overloads can be grouped

9dbf73a

Rolled back code formatting

339a6ad

WIP cleanup needed in a few edge cases

5f99dae

Finished adding new more descriptive error messages

32915dd

Code cleanup

98e2191

Added simplified version of bugfix for microsoft#19220

e93171e

rFlorian added 5 commits July 11, 2018 20:09

Rebased onto master

5b3d73d

Removed whitespace after type assertion

15116d4

Code review simplifications

848c61b

Use correct diagnostic name

922697f

Code review changes and simplification of diagnostic message

3f8a69d

Kr4pper force-pushed the issue-19220-arity-error-msg branch from e1ebdcf to 3f8a69d Compare July 11, 2018 22:27

sandersn requested changes Jul 11, 2018

View reviewed changes

Revert formatting changes

2c0e5d0

sandersn approved these changes Jul 11, 2018

View reviewed changes

sandersn merged commit 66e9aaa into microsoft:master Jul 12, 2018

sandersn mentioned this pull request Dec 10, 2018

Fix error message when type argument arity is wrong #28366

Merged

		const hasSpreadArgument = getSpreadArgumentIndex(args) > -1;
		const paramCount = hasRestParameter ? min :

Issue 19220 function parameter arity #24031

Issue 19220 function parameter arity #24031

Uh oh!

Conversation

Kr4pper commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kr4pper commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kr4pper commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented May 25, 2018

Uh oh!

Kr4pper commented Jul 6, 2018

Uh oh!

sandersn commented Jul 9, 2018

Uh oh!

sandersn commented Jul 9, 2018

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kr4pper Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kr4pper commented Jul 10, 2018

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kr4pper commented Jul 11, 2018

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kr4pper commented Jul 11, 2018

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jul 12, 2018

Uh oh!

Kr4pper commented May 10, 2018 •

edited

Loading

msftclas commented May 10, 2018 •

edited

Loading

Kr4pper commented May 10, 2018 •

edited

Loading

Kr4pper commented May 11, 2018 •

edited

Loading

Kr4pper Jul 9, 2018 •

edited

Loading