Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 2, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7868c3
Still instantiate the extends clause even when theres a noimplicitany…
weswigham Nov 22, 2017
e22b325
Only be permissive for JS
weswigham Nov 22, 2017
509511e
In JS, instantiate classes even when they have too many type argument…
weswigham Nov 23, 2017
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
3 changes: 1 addition & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7195,12 +7195,11 @@ 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;
}
// In a type reference, the outer type parameters of the referenced class or interface are automatically
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 •
edited
Loading

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.

// 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
19 changes: 19 additions & 0 deletions tests/baselines/reference/jsNoImplicitAnyNoCascadingReferenceErrors.errors.txt
View file
Open in desktop
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;
}
}

40 changes: 40 additions & 0 deletions tests/baselines/reference/jsNoImplicitAnyNoCascadingReferenceErrors.js
View file
Open in desktop
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));
28 changes: 28 additions & 0 deletions tests/baselines/reference/jsNoImplicitAnyNoCascadingReferenceErrors.symbols
View file
Open in desktop
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, --, --))
}
}

33 changes: 33 additions & 0 deletions tests/baselines/reference/jsNoImplicitAnyNoCascadingReferenceErrors.types
View file
Open in desktop
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
}
}

17 changes: 17 additions & 0 deletions tests/cases/compiler/jsNoImplicitAnyNoCascadingReferenceErrors.ts
View file
Open in desktop
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;
}
}