Skip to content

In JS, this assignments in constructors are preferred and nullable initializers become any#22882

Merged
sandersn merged 10 commits into
masterfrom
js/constructor-initializers
Mar 26, 2018
Merged

In JS, this assignments in constructors are preferred and nullable initializers become any#22882
sandersn merged 10 commits into
masterfrom
js/constructor-initializers

Conversation

@sandersn

Copy link
Copy Markdown
Member

In Javascript, the compiler unions the type of all special-property-assignments to determine the type of properties declared this way. For example:

function C () { }
C.prototype.m1 = function() { this.a = 1 }
C.prototype.m2 = function() { this.a = 'hi' }

This results in C having an instance property a: string | number | undefined. However, when there's a this-assignment in the constructor, it should be treated as the definitive declaration:

function C() {
  this.a = 1
}
C.prototype.m1 = function() {
  this.a = 'hi' // error: 'string' is not assignable to 'number'
}

Assignment in the constructor is the most common Javascript pattern for declaring instance variables. This PR therefore implements 2 things:

  1. Prefer this-assignments in the constructor to determine the type of special assignment declarations.
  2. Initializers of type null, undefined and never[] become any, any and any[] respectively. This applies to all initializers, not just special-assignment initializers.

Specifically, when constructing the type for a special-assignment property, if there are this-assignments in the constructor, the types of those this-assignments are used in exclusion of other special-assignments. If, however, these types consist only of null | undefined, then all special-assignments are used to construct the type. For example:

function C() {
  this.ctor = 1
  this.ctor = 'hi'
  this.n = null
  this.empty = [] // no other assignments: this.empty: any[]
  this.useless = undefined
}
function C.prototype.m1() {
  this.ctor = false // error: this.ctor: string | number
  this.n = 1 // ok: this.n: number | null
  this.method = 1
  this.method = 'hi' // ok: this.method: string | number | undefined
  this.useless = null // ok, but this.useless: any
}

Note that

  1. Properties whose this-assignments only appear in methods add undefined to the type since they may not be initialised during construction.
  2. Properties that are assigned nothing but null and undefined will become any and properties that are assigned nothing but [] will become any[].

Fixes #22792
Fixes #22458

I tested this change on the user code in Javascript and it doesn't add any errors that I noticed. I believe this change will provide strictly better typing since the this-assignment-in-constructor pattern is so common in Javascript.

@sandersn sandersn requested review from mhegazy and weswigham March 26, 2018 16:38
@sandersn

Copy link
Copy Markdown
Member Author

@DanielRosenwasser I think you might have opinions about this too.

@weswigham

Copy link
Copy Markdown
Member

Initializers of type null, undefined and never[] become any, any and any[] respectively. This applies to all initializers, not just special-assignment initializers.

Flagged as implicit any errors, I hope, so if that's on people know where any's are leaking into their program from?

@sandersn

Copy link
Copy Markdown
Member Author

@weswigham Not yet! Let me think about it.

@sandersn

Copy link
Copy Markdown
Member Author

@weswigham noImplicitAny seems like a good idea. Here is my thought process so far:

  1. (Bad) Simple uses of null and undefined are still inconvenient and require type annotation. (function f(a = null) { })
  2. (Good) When working with JS, people seem to turn on strictNullChecks before noImplicitAny (at least based on @OliverJAsh's reported experience). Plus, noImplicitAny implies that you want to put in all type annotations, even if it's inconvenient.
  3. (Bad) The compiler wasn't really designed for strictNullChecks on but noImplicitAny off. We don't really know how well it works.
  4. (Good) The compiler comes with free frogurt!

I'm leaning toward putting in the noImplicitAny errors. That's what I'll do next.

@weswigham weswigham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small comments, but looks good.

Comment thread src/compiler/checker.ts
return getWidenedType(addOptionality(type, definedInMethod && !definedInConstructor));
let type = jsDocType;
if (!type) {
const sourceTypes = some(constructorTypes, t => !!(t.flags & ~(TypeFlags.Nullable | TypeFlags.ContainsWideningType))) ? constructorTypes : types;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line deserves a comment explaining it. Why are we checking that there is a constructor type which isn't nullable/widening?

Comment thread src/compiler/checker.ts
type = getUnionType(sourceTypes, UnionReduction.Subtype);
}
const widened = getWidenedType(addOptionality(type, definedInMethod && !definedInConstructor));
if (filterType(widened, t => !!(t.flags & ~TypeFlags.Nullable)) === neverType) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't going to work for generics extending the "nullable" types is it? Eg

// @filename: m.d.ts
type IsNullable<T extends undefined | null> = T;
type AsNullable<T> = [T] extends [IsNullable<infer U>] ? U : never;
declare function foo<T>(x: T, y: (p: T) => AsNullable<T>): AsNullable<T>;
// @filename: file.js
/**
* @template {T}
* @param {T} x
*/
function Thing(x) {
  this.member = foo(x, x => undefined);
}

? (Without generic bounds in js this was the only way I could think to introduce a bounded generic inside js, so maybe this is less of an issue and more of a statement?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No it will not work there, but if you have T extends undefined | null, I think that was intentional enough to not want any here. Weird, but intentional.

@sandersn sandersn merged commit c9ac15a into master Mar 26, 2018
@sandersn sandersn deleted the js/constructor-initializers branch March 26, 2018 20:42

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants