-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Contextually type parameters in super calls using type arguments on the base class. #1816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
93dbcf0
4646d63
8ee09db
40796b2
9d8319d
0ffa722
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5978,11 +5978,36 @@ module ts { | |
| return args; | ||
| } | ||
|
|
||
| // In a 'super' call, type arguments are not provided within the CallExpression node itself. | ||
| // Instead, they must be fetched from the class declaration's base type node. | ||
| function getEffectiveTypeArguments(callExpression: CallExpression): TypeNode[] { | ||
| if (callExpression.expression.kind === SyntaxKind.SuperKeyword) { | ||
| // TODO (drosen): 1) Discuss if checking needs to be done at this point. | ||
| // 2) Have a test where type arguments are not provided on the base class. | ||
| // 3) Have a test where the base class is not generic. | ||
| var containingClass = <ClassDeclaration>getAncestor(callExpression, SyntaxKind.ClassDeclaration); | ||
| var baseClassTypeNode = getClassBaseTypeNode(containingClass); | ||
| return baseClassTypeNode.typeArguments; | ||
| } | ||
| else { | ||
| // Ordinary case - simple function invocation. | ||
| return (<CallExpression>callExpression).typeArguments; | ||
| } | ||
| } | ||
|
|
||
| function resolveCall(node: CallLikeExpression, signatures: Signature[], candidatesOutArray: Signature[]): Signature { | ||
| var isTaggedTemplate = node.kind === SyntaxKind.TaggedTemplateExpression; | ||
|
|
||
| var typeArguments = isTaggedTemplate ? undefined : (<CallExpression>node).typeArguments; | ||
| forEach(typeArguments, checkSourceElement); | ||
| var typeArguments: TypeNode[]; | ||
|
|
||
| if (!isTaggedTemplate) { | ||
| typeArguments = getEffectiveTypeArguments(<CallExpression>node); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine, but one alternative you might consider is for resolveCall to take a parameter for the type arguments, and have the caller determine what to pass. So whichever caller handles super calls would get the type arguments from the base type reference. But I admit it's a bit weird to pass the entire CallLikeExpression, with the the type arguments alongside it. It's like having meat and potatoes, with a side of potatoes.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly! It's like if you gave me corn alongside corn-off-the-cob - it's kind of odd to give both.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do this without passing the type arguments. But I think we'll leave it for another PR. |
||
|
|
||
| // We already perform checking on the type arguments on the class declaration itself. | ||
| if ((<CallExpression>node).expression.kind !== SyntaxKind.SuperKeyword) { | ||
| forEach(typeArguments, checkSourceElement); | ||
| } | ||
| } | ||
|
|
||
| var candidates = candidatesOutArray || []; | ||
| // collectCandidates fills up the candidates array directly | ||
|
|
@@ -6248,7 +6273,7 @@ module ts { | |
| // Another error has already been reported | ||
| return resolveErrorCall(node); | ||
| } | ||
|
|
||
| // Technically, this signatures list may be incomplete. We are taking the apparent type, | ||
| // but we are not including call signatures that may have been added to the Object or | ||
| // Function interface, since they have none by default. This is a bit of a leap of faith | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| tests/cases/compiler/superCallArgsMustMatch.ts(17,15): error TS2345: Argument of type 'string' is not assignable to parameter of type 'number'. | ||
|
|
||
|
|
||
| ==== tests/cases/compiler/superCallArgsMustMatch.ts (1 errors) ==== | ||
| class T5<T>{ | ||
|
|
||
| public foo: T; | ||
|
|
||
| constructor(public bar: T) { } | ||
|
|
||
| } | ||
|
|
||
|
|
||
|
|
||
| class T6 extends T5<number>{ | ||
|
|
||
| constructor() { | ||
|
|
||
| // Should error; base constructor has type T for first arg, | ||
| // which is instantiated with 'number' in the extends clause | ||
| super("hi"); | ||
| ~~~~ | ||
| !!! error TS2345: Argument of type 'string' is not assignable to parameter of type 'number'. | ||
|
|
||
| var x: number = this.foo; | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
|
|
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| //// [superCallParameterContextualTyping1.ts] | ||
|
|
||
| class A<T1, T2> { | ||
| constructor(private map: (value: T1) => T2) { | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class B extends A<number, string> { | ||
| // Ensure 'value' is of type 'number (and not '{}') by using its 'toExponential()' method. | ||
| constructor() { super(value => String(value.toExponential())); } | ||
| } | ||
|
|
||
|
|
||
| //// [superCallParameterContextualTyping1.js] | ||
| var __extends = this.__extends || function (d, b) { | ||
| for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; | ||
| function __() { this.constructor = d; } | ||
| __.prototype = b.prototype; | ||
| d.prototype = new __(); | ||
| }; | ||
| var A = (function () { | ||
| function A(map) { | ||
| this.map = map; | ||
| } | ||
| return A; | ||
| })(); | ||
| var B = (function (_super) { | ||
| __extends(B, _super); | ||
| // Ensure 'value' is of type 'number (and not '{}') by using its 'toExponential()' method. | ||
| function B() { | ||
| _super.call(this, function (value) { return String(value.toExponential()); }); | ||
| } | ||
| return B; | ||
| })(A); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| === tests/cases/conformance/expressions/contextualTyping/superCallParameterContextualTyping1.ts === | ||
|
|
||
| class A<T1, T2> { | ||
| >A : A<T1, T2> | ||
| >T1 : T1 | ||
| >T2 : T2 | ||
|
|
||
| constructor(private map: (value: T1) => T2) { | ||
| >map : (value: T1) => T2 | ||
| >value : T1 | ||
| >T1 : T1 | ||
| >T2 : T2 | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class B extends A<number, string> { | ||
| >B : B | ||
| >A : A<T1, T2> | ||
|
|
||
| // Ensure 'value' is of type 'number (and not '{}') by using its 'toExponential()' method. | ||
| constructor() { super(value => String(value.toExponential())); } | ||
| >super(value => String(value.toExponential())) : void | ||
| >super : typeof A | ||
| >value => String(value.toExponential()) : (value: number) => string | ||
| >value : number | ||
| >String(value.toExponential()) : string | ||
| >String : StringConstructor | ||
| >value.toExponential() : string | ||
| >value.toExponential : (fractionDigits?: number) => string | ||
| >value : number | ||
| >toExponential : (fractionDigits?: number) => string | ||
| } | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| tests/cases/conformance/expressions/contextualTyping/superCallParameterContextualTyping2.ts(10,43): error TS2349: Cannot invoke an expression whose type lacks a call signature. | ||
|
|
||
|
|
||
| ==== tests/cases/conformance/expressions/contextualTyping/superCallParameterContextualTyping2.ts (1 errors) ==== | ||
|
|
||
| class A<T1, T2> { | ||
| constructor(private map: (value: T1) => T2) { | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class C extends A<number, string> { | ||
| // Ensure 'value' is not of type 'any' by invoking it with type arguments. | ||
| constructor() { super(value => String(value<string>())); } | ||
| ~~~~~~~~~~~~~~~ | ||
| !!! error TS2349: Cannot invoke an expression whose type lacks a call signature. | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| //// [superCallParameterContextualTyping2.ts] | ||
|
|
||
| class A<T1, T2> { | ||
| constructor(private map: (value: T1) => T2) { | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class C extends A<number, string> { | ||
| // Ensure 'value' is not of type 'any' by invoking it with type arguments. | ||
| constructor() { super(value => String(value<string>())); } | ||
| } | ||
|
|
||
| //// [superCallParameterContextualTyping2.js] | ||
| var __extends = this.__extends || function (d, b) { | ||
| for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; | ||
| function __() { this.constructor = d; } | ||
| __.prototype = b.prototype; | ||
| d.prototype = new __(); | ||
| }; | ||
| var A = (function () { | ||
| function A(map) { | ||
| this.map = map; | ||
| } | ||
| return A; | ||
| })(); | ||
| var C = (function (_super) { | ||
| __extends(C, _super); | ||
| // Ensure 'value' is not of type 'any' by invoking it with type arguments. | ||
| function C() { | ||
| _super.call(this, function (value) { return String(value()); }); | ||
| } | ||
| return C; | ||
| })(A); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| | ||
| class A<T1, T2> { | ||
| constructor(private map: (value: T1) => T2) { | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class B extends A<number, string> { | ||
| // Ensure 'value' is of type 'number (and not '{}') by using its 'toExponential()' method. | ||
| constructor() { super(value => String(value.toExponential())); } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| | ||
| class A<T1, T2> { | ||
| constructor(private map: (value: T1) => T2) { | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class C extends A<number, string> { | ||
| // Ensure 'value' is not of type 'any' by invoking it with type arguments. | ||
| constructor() { super(value => String(value<string>())); } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /// <reference path='fourslash.ts'/> | ||
|
|
||
| ////class A<T1, T2> { | ||
| //// constructor(private map: (value: T1) => T2) { | ||
| //// | ||
| //// } | ||
| ////} | ||
| //// | ||
| ////class B extends A<number, string> { | ||
| //// constructor() { super(va/*1*/lue => String(va/*2*/lue.toExpone/*3*/ntial())); } | ||
| ////} | ||
|
|
||
| goTo.marker('1'); | ||
| verify.quickInfoIs('(parameter) value: number'); | ||
|
|
||
| goTo.marker('2'); | ||
| verify.quickInfoIs('(parameter) value: number'); | ||
|
|
||
| goTo.marker('3'); | ||
| verify.quickInfoIs('(method) Number.toExponential(fractionDigits?: number): string'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #2 and #3 todo would be good to added in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1