In JS, this assignments in constructors are preferred and nullable initializers become any#22882
Conversation
Also move this-assignment fixes out of binder. I'm going to put it in the checker instead.
|
@DanielRosenwasser I think you might have opinions about this too. |
Flagged as implicit any errors, I hope, so if that's on people know where any's are leaking into their program from? |
|
@weswigham Not yet! Let me think about it. |
|
@weswigham noImplicitAny seems like a good idea. Here is my thought process so far:
I'm leaning toward putting in the noImplicitAny errors. That's what I'll do next. |
weswigham
left a comment
There was a problem hiding this comment.
Small comments, but looks good.
| return getWidenedType(addOptionality(type, definedInMethod && !definedInConstructor)); | ||
| let type = jsDocType; | ||
| if (!type) { | ||
| const sourceTypes = some(constructorTypes, t => !!(t.flags & ~(TypeFlags.Nullable | TypeFlags.ContainsWideningType))) ? constructorTypes : types; |
There was a problem hiding this comment.
This line deserves a comment explaining it. Why are we checking that there is a constructor type which isn't nullable/widening?
| type = getUnionType(sourceTypes, UnionReduction.Subtype); | ||
| } | ||
| const widened = getWidenedType(addOptionality(type, definedInMethod && !definedInConstructor)); | ||
| if (filterType(widened, t => !!(t.flags & ~TypeFlags.Nullable)) === neverType) { |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
In Javascript, the compiler unions the type of all special-property-assignments to determine the type of properties declared this way. For example:
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:Assignment in the constructor is the most common Javascript pattern for declaring instance variables. This PR therefore implements 2 things:
null,undefinedandnever[]becomeany,anyandany[]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:Note that
nullandundefinedwill becomeanyand properties that are assigned nothing but[]will becomeany[].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.