Simplify generic function call error reporting #16439

ahejlsberg · 2017-06-10T22:50:51Z

This PR simplifies the way we report errors related to type inference for generic functions. Previously, type inference could fail when there was no common supertype for the inferences. With this PR type inference always succeeds, but may infer a set of types that don't match the argument list (which in turn causes an error when the call is type checked). Specifically, for a given list of type inference candidates, we now infer the leftmost type for which no type to the right is a supertype. This change leads to simpler error messages that seem to better match programmer intuitions.

Consider:

declare function foo<T>(x: T, y: T): void;
foo(1, "hello");

Previously, we'd report the following error:

foo(1, "hello");
~~~
t.ts(2,1): error TS2453: The type argument for type parameter 'T' cannot be inferred from the usage. Consider specifying the type arguments explicitly.
  Type argument candidate 'number' is not a valid type argument because it is not a supertype of candidate 'string'.

Now, we simply report:

foo(1, "hello");
       ~~~~~~~
t.ts(2,8): error TS2345: Argument of type '"hello"' is not assignable to parameter of type 'number'.

Effectively, we pinpoint the first argument that causes inference to fail instead of abstractly listing the incompatible types with no identification of their origin.

In addition to providing more intuitive error messages, we eliminate 120 lines of complex error reporting code (which has been the source of bugs in the past).

gcnew · 2017-06-11T08:42:58Z

Good work!

I've used the same error reporting trick in my PR #16104. It's nice seeing it applied throughout.

sandersn · 2017-06-12T16:14:44Z

Note that this fixes #15116 by deleting all the offending code. It also obsoletes the fix, but not the side effect, of #15221. The error messages should still widen literals to their base types using the same condition that is found in getInferredType.

For example:

    var result = concat(1, ""); // error
                           ~~
!!! error TS2345: Argument of type '""' is not assignable to parameter of type '1'.

The error should actually be

    var result = concat(1, ""); // error
                           ~~
!!! error TS2345: Argument of type 'string' is not assignable to parameter of type 'number'.

I think it is a simple change to extract widenInferredLiteralTypes from getInferredType and insert it just before calling checkApplicableSignature in resolveCall, or perhaps at the beginning of checkApplicableSignature. Do you mind including this change in this PR? If not I can keep #15221 around and update it to do that after this PR goes in.

I'm still looking at the test changes, but the code in checker looks much improved.

sandersn · 2017-06-12T16:25:08Z

Looking at the baseline changes I see that the old code sometimes widened literals and sometimes didn't, and the new code sometimes widens literal and sometimes doesn't. Strange.

sandersn · 2017-06-12T16:30:45Z

tests/baselines/reference/genericCallWithGenericSignatureArguments.types

+
+var r4 = foo((x: typeof a) => a, (x: typeof b) => b); // typeof a => typeof a
+>r4 : (x: { x: number; y?: number; }) => { x: number; y?: number; }
+>foo((x: typeof a) => a, (x: typeof b) => b) : (x: { x: number; y?: number; }) => { x: number; y?: number; }


This inference from from the first argument is kind of weird. The result will not have z?: number from typeof b. It's arguably better than the error it replaces, but ideally the result would be typeof a & typeof b in this case. I don't think that result can be generalised though, so maybe "surprising success" is the best we can do.

Previously we'd require that one be an exact subtype of the other, which would fail. Now, we allow the call because assignability succeeds even though neither is a subtype of the other. I don't think this corner case much matters, and you can make just as good a case for succeeding as for failing.

sandersn

Looks like a nice improvement to me. A couple of test comments need to be looked at and updated. And a couple of errors now go away and give surprising results.

sandersn · 2017-06-12T16:58:39Z

tests/baselines/reference/genericCallWithOverloadedConstructorTypedArguments.errors.txt

-            return cb;
-        }
-
-        var r8 = foo6(a); // error


update or remove comment

sandersn · 2017-06-12T17:06:22Z

tests/baselines/reference/tsxStatelessFunctionComponentsWithTypeArguments2.errors.txt

@@ -8,13 +8,15 @@ tests/cases/conformance/jsx/file.tsx(20,19): error TS2322: Type '{ func: (a: num
  Type '{ func: (a: number, b: string) => void; }' is not assignable to type '{ func: (arg: number) => void; }'.
    Types of property 'func' are incompatible.
      Type '(a: number, b: string) => void' is not assignable to type '(arg: number) => void'.
-tests/cases/conformance/jsx/file.tsx(31,9): error TS2605: JSX element type 'Element' is not a constructor function for JSX elements.
-  Property 'render' is missing in type 'Element'.


@yuit or @RyanCavanaugh This error is gone in the update basedline. But it looks bogus. Is it?

sandersn · 2017-06-12T17:09:17Z

tests/baselines/reference/typeArgumentInferenceWithConstraintAsCommonRoot.errors.txt

@@ -11,7 +10,6 @@ tests/cases/compiler/typeArgumentInferenceWithConstraintAsCommonRoot.ts(7,1): er
    var g: Giraffe;
    var e: Elephant;
    f(g, e); // valid because both Giraffe and Elephant satisfy the constraint. T is Animal


update comment. looks like it doesn't find a common supertype any more. is it supposed to?

RyanCavanaugh · 2017-06-12T17:25:58Z

tests/baselines/reference/genericCallWithNonSymmetricSubtypes.errors.txt

-    var a: { x: number; y?: number; };
-    var b: { x: number; z?: number; };
-
-    var r = foo(a, b); // { x: number; y?: number; };


This doesn't seem right - there's no error, and the type of r is pretty arbitrary now. It seems like it'd be better to infer from the intersection of the remaining supertypes - this would give r the correct type ({ x: number; y?: number} & { x: number; z?: number }), and the leaf-node error messages will still be good.

I think inferring an intersection of the remaining supertypes would be very non-intuitive. For example, in the simplest of cases we'd be telling you that string isn't assignable to string & number. It would just be a bogus type generator!

ahejlsberg · 2017-06-13T16:39:26Z

@sandersn Regarding your comment on widening, we are reporting the widened types of inferred type arguments, but beyond that we just rely on normal error reporting (and we don't artificially widen types in argument error reporting). The particular concat example you cite isn't a good one because no widening occurs there (and we reported unwidened types previously as well).

* Simplify error reporting for generic functions * Accept new baselines * Fix fourslash tests

ahejlsberg added 3 commits June 10, 2017 14:38

Simplify error reporting for generic functions

a8c648f

Accept new baselines

8cdd7f2

Fix fourslash tests

604c344

msftclas added the cla-not-required label Jun 10, 2017

ahejlsberg changed the title ~~Simplify generic function errors~~ Simplify generic function error reporting Jun 11, 2017

ahejlsberg changed the title ~~Simplify generic function error reporting~~ Simplify generic function call error reporting Jun 11, 2017

sandersn reviewed Jun 12, 2017

View reviewed changes

sandersn approved these changes Jun 12, 2017

View reviewed changes

RyanCavanaugh reviewed Jun 12, 2017

View reviewed changes

mhegazy approved these changes Jun 13, 2017

View reviewed changes

mhegazy merged commit 31f0814 into master Jun 13, 2017

mhegazy deleted the simplifyGenericErrors branch June 13, 2017 18:43

mhegazy pushed a commit that referenced this pull request Jun 13, 2017

Simplify generic function call error reporting (#16439)

5b9f7be

* Simplify error reporting for generic functions * Accept new baselines * Fix fourslash tests

mhegazy mentioned this pull request Jun 13, 2017

Port #16439 #16501

Merged

mhegazy added a commit that referenced this pull request Jun 13, 2017

Simplify generic function call error reporting (#16439) (#16501)

68e2a45

* Simplify error reporting for generic functions * Accept new baselines * Fix fourslash tests

sandersn mentioned this pull request Jun 13, 2017

Assert when type inference can't find a common supertype #15116

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify generic function call error reporting #16439

Simplify generic function call error reporting #16439

ahejlsberg commented Jun 10, 2017

gcnew commented Jun 11, 2017

sandersn commented Jun 12, 2017

sandersn commented Jun 12, 2017

sandersn Jun 12, 2017

ahejlsberg Jun 13, 2017

sandersn left a comment

sandersn Jun 12, 2017

sandersn Jun 12, 2017

sandersn Jun 12, 2017

RyanCavanaugh Jun 12, 2017

ahejlsberg Jun 13, 2017 •

edited

Loading

ahejlsberg commented Jun 13, 2017

Simplify generic function call error reporting #16439

Simplify generic function call error reporting #16439

Conversation

ahejlsberg commented Jun 10, 2017

gcnew commented Jun 11, 2017

sandersn commented Jun 12, 2017

sandersn commented Jun 12, 2017

sandersn Jun 12, 2017

Choose a reason for hiding this comment

ahejlsberg Jun 13, 2017

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jun 12, 2017

Choose a reason for hiding this comment

sandersn Jun 12, 2017

Choose a reason for hiding this comment

sandersn Jun 12, 2017

Choose a reason for hiding this comment

RyanCavanaugh Jun 12, 2017

Choose a reason for hiding this comment

ahejlsberg Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

ahejlsberg commented Jun 13, 2017

ahejlsberg Jun 13, 2017 •

edited

Loading