Skip overloads with too short function parameters #11905

sandersn · 2016-10-27T21:48:09Z

If the parameter of an overload is a function and the argument is also a
function, skip the overload if the parameter has fewer arguments than
the argument does. That overload cannot possibly apply, and should not
participate in, for example, contextual typing.

Example:

interface I {
  (a: number): void;
  (b: string, c): void;
}
declare function f(i: I): void;
f((x, y) => {});

This code now skips the first overload instead of considering it.

This was a longstanding bug but was only uncovered now that more function expressions are context sensitive.

@mhegazy @RyanCavanaugh do you mind taking a look? I need to skip checkExpressionWithContextualType so I think the code has to be here, but it seems like it is operating at the wrong level of abstraction overall.

If the parameter of an overload is a function and the argument is also a function, skip the overload if the parameter has fewer arguments than the argument does. That overload cannot possibly apply, and should not participate in, for example, contextual typing. Example: ```ts interface I { (a: number): void; (b: string, c): void; } declare function f(i: I): void; f((x, y) => {}); ``` This code now skips the first overload instead of considering. This was a longstanding bug but was only uncovered now that more functions expressions are context sensitive.

1. Update changed baseline. 2. Add a new test with baseline.

RyanCavanaugh · 2016-10-27T22:04:21Z

src/compiler/checker.ts

@@ -11890,6 +11890,11 @@ namespace ts {
                    // If the effective argument type is 'undefined', there is no synthetic type
                    // for the argument. In that case, we should check the argument.
                    if (argType === undefined) {
+                        // If the parameter and argument are both functions and the parameter has fewer arguments than the argument,
+                        // then this signature is not applicable. Exit early.


"Exit early to avoid fixing incorrect contextual types to the function expression parameters"

RyanCavanaugh · 2016-10-27T22:05:01Z

src/compiler/checker.ts

@@ -11904,6 +11909,21 @@ namespace ts {
            return true;
        }

+        function isAritySmaller(sourceType: Type, target: Expression) {


Explicit return false at the bottom here

RyanCavanaugh · 2016-10-27T22:06:33Z

src/compiler/checker.ts

+                let targetParameterCount = 0;
+                for (; targetParameterCount < target.parameters.length; targetParameterCount++) {
+                    const param = target.parameters[targetParameterCount];
+                    if (param.initializer || param.questionToken || param.dotDotDotToken || isJSDocOptionalParameter(param)) {


To be consistent, we should actually remove these checks. Optional parameters are considered to be present for the purposes of callback type checking.

This skips some additional overloads. It is not bad but it is worse than what we do today. Here's the test failure:

function foo6<T>(cb: { (x: T): string; (x: T, y?: T): string }) { return cb; } var r11 = foo6(<T>(x: T, y?: T) => ''); // any => string (+1 overload)

This gives r11: any instead of r11: { (x: T): string; (x: T, y?: T): string }

I made the change; it's easy enough to revert if we decide against it.

ahejlsberg · 2016-10-28T00:26:08Z

src/compiler/checker.ts

+            if (isFunctionExpressionOrArrowFunction(target) && isFunctionType(sourceType)) {
+                const sourceSignatures = getSignaturesOfType(sourceType, SignatureKind.Call);
+                const sourceLengths = sourceSignatures.map(sig => !sig.hasRestParameter ? sig.parameters.length : Number.MAX_VALUE);
+                return forEach(sourceLengths, len => len < target.parameters.length);


A target with more parameters than the source might still be applicable if some of the target's parameters are optional. This doesn't appear to factor that in.

The previous commit took optional parameters into account, but @RyanCavanaugh pointed out that optional parameters are considered to be present for checking function parameters. The change is in a single commit so it's easy to revert if you think that optionality should be taken into account, though.

ahejlsberg · 2016-10-28T00:48:04Z

I think a better approach would be to simply disqualify contextual signatures that have fewer parameters than the number of non-optional parameters in the function expression or arrow function. In other words, never contextually type from a signature that couldn't validly call the function. We already disqualify anything but a single non-generic signature, this could just be an additional check. I think it would solve the issue in #11875, but it would be a more local change with no effect on the overload resolution rules.

sandersn · 2016-10-28T01:00:45Z

Unfortunately, that results in any for both argument types below when the first overload is being explored. That's because checkApplicableSignature calls callExpressionWithContextualType on (req, res) => {}. Even if we now reject the contextual type from handler1, callExpression still ends up fixing the parameter types of (req, res) => {}. It would just fix them to (any, any) => void.

var use: Overload;
use((req, res) => {});

interface Overload {
    (handler1: (req1: string) => void): void;
    (handler2: (req2: number, res2: number) => void): void;
}

ahejlsberg · 2016-10-28T04:39:26Z

Unfortunately, that results in any for both argument types below when the first overload is being explored.

Yes, but that example didn't work before the change either, and (req: any, res: any) actually seems preferable to the (req: string, res: any) that we would have produced earlier. In fact, it would make the example compile, but wouldn't give you the more specific parameter types.

It just never makes sense to contextually type part of a function expression's parameter list from a signature that couldn't actually call that function expression. Also, it is less of a breaking change to go from some specific type to any than the other way around.

The core issue here is that once we contextually type a parameter list, the assigned types are permanent. Changing that is much more involved and out of scope for this fix. Until then, I think what I'm proposing is the best we can do.

sandersn · 2016-10-28T05:53:12Z

OK, I moved the check to getContextualSignature.

ahejlsberg · 2016-10-28T14:38:37Z

src/compiler/checker.ts

+            // If the contextual signature has fewer parameters than the function expression, do not use it
+            if (isFunctionExpressionOrArrowFunction(node) && isAritySmaller(type, node)) {
+                return undefined;
+            }


This should be the last check you do. That way you only have to deal with a single signature as everything else will have been disqualified.

If it goes after the !Union check, then it only applies to unions of signatures. This is as late as it can go and still apply to non-union signatures.

I moved the check inside getNonGenericSignature so that it gets called in both the union and non-union paths. Let me know if that looks good to you.

ahejlsberg · 2016-10-28T14:40:41Z

src/compiler/checker.ts

@@ -10377,6 +10381,26 @@ namespace ts {
            return result;
        }

+        function isAritySmaller(sourceType: Type, target: FunctionExpression | ArrowFunction) {


Change this to operate on a single source signature and call it as the last check.

Done. The call is inside getNonGenericSignature now.

sandersn added 2 commits October 27, 2016 14:41

Test skip overloads w/too-short function params

cff586d

1. Update changed baseline. 2. Add a new test with baseline.

msftclas added the cla-not-required label Oct 27, 2016

RyanCavanaugh reviewed Oct 27, 2016

View reviewed changes

sandersn added 2 commits October 27, 2016 15:38

Minor style improvements

5a336fe

Ignore optionality when skipping overloads

3e71d50

ahejlsberg reviewed Oct 28, 2016

View reviewed changes

Do not use contextual signatures with too few parameters

278d0a4

ahejlsberg requested changes Oct 28, 2016

View reviewed changes

isAritySmaller runs later: getNonGenericSignature

1796826

ahejlsberg approved these changes Oct 28, 2016

View reviewed changes

sandersn merged commit fdcc7cc into master Oct 28, 2016

sandersn deleted the skip-overloads-with-too-short-function-parameters branch October 28, 2016 16:59

ghost mentioned this pull request Oct 29, 2016

Fix failing tests DefinitelyTyped/DefinitelyTyped#12344

Merged

HerringtonDarkholme mentioned this pull request Oct 31, 2016

Implicit any issue with overloaded methods #11936

Closed

mhegazy mentioned this pull request Dec 8, 2016

Regression in type inference? #12733

Closed

mhegazy mentioned this pull request Aug 18, 2017

Extra arguments with defaults get implicit any even with noImplicitAny #17904

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip overloads with too short function parameters #11905

Skip overloads with too short function parameters #11905

sandersn commented Oct 27, 2016

RyanCavanaugh Oct 27, 2016

sandersn Oct 27, 2016

RyanCavanaugh Oct 27, 2016

sandersn Oct 27, 2016

RyanCavanaugh Oct 27, 2016

sandersn Oct 27, 2016

sandersn Oct 27, 2016

sandersn Oct 27, 2016

ahejlsberg Oct 28, 2016

sandersn Oct 28, 2016

ahejlsberg commented Oct 28, 2016 •

edited

Loading

sandersn commented Oct 28, 2016

ahejlsberg commented Oct 28, 2016

sandersn commented Oct 28, 2016

ahejlsberg Oct 28, 2016

sandersn Oct 28, 2016

sandersn Oct 28, 2016

ahejlsberg Oct 28, 2016

sandersn Oct 28, 2016

Skip overloads with too short function parameters #11905

Skip overloads with too short function parameters #11905

Conversation

sandersn commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahejlsberg commented Oct 28, 2016 • edited Loading

sandersn commented Oct 28, 2016

ahejlsberg commented Oct 28, 2016

sandersn commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahejlsberg commented Oct 28, 2016 •

edited

Loading