Skip to content
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

Always instantiate the extends clause, even in the presence of an error #20232

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5804,7 +5804,7 @@ namespace ts {
for (const baseSig of baseSignatures) {
const minTypeArgumentCount = getMinTypeArgumentCount(baseSig.typeParameters);
const typeParamCount = length(baseSig.typeParameters);
if ((isJavaScript || typeArgCount >= minTypeArgumentCount) && typeArgCount <= typeParamCount) {
if (isJavaScript || (typeArgCount >= minTypeArgumentCount && typeArgCount <= typeParamCount)) {
const sig = typeParamCount ? createSignatureInstantiation(baseSig, fillMissingTypeArguments(typeArguments, baseSig.typeParameters, minTypeArgumentCount, isJavaScript)) : cloneSignature(baseSig);
sig.typeParameters = classType.localTypeParameters;
sig.resolvedReturnType = classType;
Expand Down Expand Up @@ -6705,7 +6705,7 @@ namespace ts {
const numTypeParameters = length(typeParameters);
if (numTypeParameters) {
const numTypeArguments = length(typeArguments);
if ((isJavaScriptImplicitAny || numTypeArguments >= minTypeArgumentCount) && numTypeArguments <= numTypeParameters) {
if (isJavaScriptImplicitAny || (numTypeArguments >= minTypeArgumentCount && numTypeArguments <= numTypeParameters)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should change the name of this parameter now to be just isJavaScript

if (!typeArguments) {
typeArguments = [];
}
Expand All @@ -6724,6 +6724,7 @@ namespace ts {
}
typeArguments[i] = defaultType ? instantiateType(defaultType, mapper) : getDefaultTypeArgumentType(isJavaScriptImplicitAny);
}
typeArguments.length = typeParameters.length;
}
}
return typeArguments;
Expand Down Expand Up @@ -7195,12 +7196,15 @@ namespace ts {
: Diagnostics.Generic_type_0_requires_between_1_and_2_type_arguments;
const typeStr = typeToString(type, /*enclosingDeclaration*/ undefined, TypeFormatFlags.WriteArrayAsGenericType);
error(node, diag, typeStr, minTypeArgumentCount, typeParameters.length);
return unknownType;
if (!isJs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what broke with the permissive behavior in TS?

Copy link
Member Author

@weswigham weswigham Nov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is "broken" once all the arity-based signature removals are excised and arity verification is moved into fillMissingTypeArguments - just observable behavior changes to follow-on error scenarios (which cause a bunch of baseline churn - and in TS it is not uniformly better behavior, since we fill with {} instead of any - we could change that if there's an arity mismatch); I'll open a PR on my PR to show you, here: weswigham#2

I can merge it into this PR and iterate on it if you think it's a good idea (or do a second PR after this one). I just didn't want to add it in (it is more complicated, but also likely more complete) without mentioning it to someone first. It just amounts to better error recovery, so even if it does change a bunch of any's to more specific types in the baselines (as expected), I don't think it'd even be a breaking change, actually.

...That did take longer than I expected to fully flesh out, though - partially because I had to work around our inconsistent handling of generics on psuedoconstructors (like ArrayConstructor). Namely you can write:

interface BadBehavingCtor {
  new <T>(x: T): number;
  new <T, U>(x: T, y: U): string; 
}

and those ctor return types are in no way compatable, but we don't issue an error because we don't look at the different arities' signatures simultaneously right now. It's the reason for all the complexity around getInstantiatedConstructorsForTypeArguments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the clarification. i think i understand now. i am not sure i understand the change in weswigham#2, but we should talk about that one on Monday.

// TODO: Adopt same permissive behavior in TS as in JS to reduce follow-on editing experience failures (requires editing fillMissingTypeArguments)
return unknownType;
}
}
// In a type reference, the outer type parameters of the referenced class or interface are automatically
// supplied as type arguments and the type reference only specifies arguments for the local type parameters
// of the class or interface.
const typeArguments = concatenate(type.outerTypeParameters, fillMissingTypeArguments(typeArgs, typeParameters, minTypeArgumentCount, isJsImplicitAny));
const typeArguments = concatenate(type.outerTypeParameters, fillMissingTypeArguments(typeArgs, typeParameters, minTypeArgumentCount, isJs));
return createTypeReference(<GenericType>type, typeArguments);
}
if (node.typeArguments) {
Expand Down
16 changes: 12 additions & 4 deletions tests/baselines/reference/jsExtendsImplicitAny.errors.txt
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
/b.js(1,17): error TS8026: Expected A<T> type arguments; provide these with an '@extends' tag.
/b.js(3,15): error TS2314: Generic type 'A<T>' requires 1 type argument(s).
/b.js(4,15): error TS2314: Generic type 'A<T>' requires 1 type argument(s).
/b.js(8,15): error TS2314: Generic type 'A<T>' requires 1 type argument(s).


==== /a.d.ts (0 errors) ====
declare class A<T> { x: T; }

==== /b.js (2 errors) ====
==== /b.js (3 errors) ====
class B extends A {}
~
!!! error TS8026: Expected A<T> type arguments; provide these with an '@extends' tag.
new B().x;

/** @augments A */
~
!!! error TS2314: Generic type 'A<T>' requires 1 type argument(s).
class C { }

class C extends A { }
new C().x;

/** @augments A<number, number, number> */
~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2314: Generic type 'A<T>' requires 1 type argument(s).
class D extends A {}
new D().x;
25 changes: 23 additions & 2 deletions tests/baselines/reference/jsExtendsImplicitAny.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,28 @@ class B extends A {}
>B : Symbol(B, Decl(b.js, 0, 0))
>A : Symbol(A, Decl(a.d.ts, 0, 0))

new B().x;
>new B().x : Symbol(A.x, Decl(a.d.ts, 0, 20))
>B : Symbol(B, Decl(b.js, 0, 0))
>x : Symbol(A.x, Decl(a.d.ts, 0, 20))

/** @augments A */
class C { }
>C : Symbol(C, Decl(b.js, 0, 20))
class C extends A { }
>C : Symbol(C, Decl(b.js, 1, 10))
>A : Symbol(A, Decl(a.d.ts, 0, 0))

new C().x;
>new C().x : Symbol(A.x, Decl(a.d.ts, 0, 20))
>C : Symbol(C, Decl(b.js, 1, 10))
>x : Symbol(A.x, Decl(a.d.ts, 0, 20))

/** @augments A<number, number, number> */
class D extends A {}
>D : Symbol(D, Decl(b.js, 5, 10))
>A : Symbol(A, Decl(a.d.ts, 0, 0))

new D().x;
>new D().x : Symbol(A.x, Decl(a.d.ts, 0, 20))
>D : Symbol(D, Decl(b.js, 5, 10))
>x : Symbol(A.x, Decl(a.d.ts, 0, 20))

28 changes: 26 additions & 2 deletions tests/baselines/reference/jsExtendsImplicitAny.types
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,33 @@ declare class A<T> { x: T; }
=== /b.js ===
class B extends A {}
>B : B
>A : typeof A
>A : A<any>

new B().x;
>new B().x : any
>new B() : B
>B : typeof B
>x : any

/** @augments A */
class C { }
class C extends A { }
>C : C
>A : A<any>

new C().x;
>new C().x : any
>new C() : C
>C : typeof C
>x : any

/** @augments A<number, number, number> */
class D extends A {}
>D : D
>A : A<number>

new D().x;
>new D().x : number
>new D() : D
>D : typeof D
>x : number

Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
tests/cases/compiler/index.js(3,21): error TS8026: Expected Foo<T> type arguments; provide these with an '@extends' tag.


==== tests/cases/compiler/somelib.d.ts (0 errors) ====
export declare class Foo<T> {
prop: T;
}
==== tests/cases/compiler/index.js (1 errors) ====
import {Foo} from "./somelib";

class MyFoo extends Foo {
~~~
!!! error TS8026: Expected Foo<T> type arguments; provide these with an '@extends' tag.
constructor() {
super();
this.prop.alpha = 12;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
//// [tests/cases/compiler/jsNoImplicitAnyNoCascadingReferenceErrors.ts] ////

//// [somelib.d.ts]
export declare class Foo<T> {
prop: T;
}
//// [index.js]
import {Foo} from "./somelib";

class MyFoo extends Foo {
constructor() {
super();
this.prop.alpha = 12;
}
}


//// [index.js]
"use strict";
var __extends = (this && this.__extends) || (function () {
var extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
return function (d, b) {
extendStatics(d, b);
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
})();
exports.__esModule = true;
var somelib_1 = require("./somelib");
var MyFoo = /** @class */ (function (_super) {
__extends(MyFoo, _super);
function MyFoo() {
var _this = _super.call(this) || this;
_this.prop.alpha = 12;
return _this;
}
return MyFoo;
}(somelib_1.Foo));
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
=== tests/cases/compiler/somelib.d.ts ===
export declare class Foo<T> {
>Foo : Symbol(Foo, Decl(somelib.d.ts, --, --))
>T : Symbol(T, Decl(somelib.d.ts, --, --))

prop: T;
>prop : Symbol(Foo.prop, Decl(somelib.d.ts, --, --))
>T : Symbol(T, Decl(somelib.d.ts, --, --))
}
=== tests/cases/compiler/index.js ===
import {Foo} from "./somelib";
>Foo : Symbol(Foo, Decl(index.js, 0, 8))

class MyFoo extends Foo {
>MyFoo : Symbol(MyFoo, Decl(index.js, 0, 30))
>Foo : Symbol(Foo, Decl(index.js, 0, 8))

constructor() {
super();
>super : Symbol(Foo, Decl(somelib.d.ts, --, --))

this.prop.alpha = 12;
>this.prop : Symbol(Foo.prop, Decl(somelib.d.ts, --, --))
>this : Symbol(MyFoo, Decl(index.js, 0, 30))
>prop : Symbol(Foo.prop, Decl(somelib.d.ts, --, --))
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
=== tests/cases/compiler/somelib.d.ts ===
export declare class Foo<T> {
>Foo : Foo<T>
>T : T

prop: T;
>prop : T
>T : T
}
=== tests/cases/compiler/index.js ===
import {Foo} from "./somelib";
>Foo : typeof Foo

class MyFoo extends Foo {
>MyFoo : MyFoo
>Foo : Foo<any>

constructor() {
super();
>super() : void
>super : typeof Foo

this.prop.alpha = 12;
>this.prop.alpha = 12 : 12
>this.prop.alpha : any
>this.prop : any
>this : this
>prop : any
>alpha : any
>12 : 12
}
}

8 changes: 7 additions & 1 deletion tests/cases/compiler/jsExtendsImplicitAny.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ declare class A<T> { x: T; }

// @Filename: /b.js
class B extends A {}
new B().x;

/** @augments A */
class C { }
class C extends A { }
new C().x;

/** @augments A<number, number, number> */
class D extends A {}
new D().x;
17 changes: 17 additions & 0 deletions tests/cases/compiler/jsNoImplicitAnyNoCascadingReferenceErrors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// @allowJs: true
// @checkJs: true
// @noImplicitAny: true
// @outDir: ./built
// @filename: somelib.d.ts
export declare class Foo<T> {
prop: T;
}
// @filename: index.js
import {Foo} from "./somelib";

class MyFoo extends Foo {
constructor() {
super();
this.prop.alpha = 12;
}
}