narrow by instanceof: infer type parameters from original type #30161
Conversation
|
Just as a heads up, your commits don't seem to be associated with your GitHub account. While this isn't technically a problem, you might care if you want more appropriate attribution. You can either make sure your GitHub account is associated with the email address you're using for your commits, or rebase and amend your commits to fix the author name and email. |
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at f133aed. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@typescript-bot test this new one |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 6443775. You can monitor the build here. It should now contribute to this PR's status checks. |
|
The bot lies; RWC is clean. @typescript-bot perf test this |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 6443775. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@ahejlsberg take a look? Interesting change |
|
@RyanCavanaugh Here they are:Comparison Report - master..30161
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Will this also fix this case: class Query<T> {}
function f<T>(q: T[] | Query<T>) {
if (Array.isArray(q)) {
q; // T[], so far so good
} else if (q instanceof Query) {
q; // Query<any>, wtpf
}
}In my case there's no inheritance. I'm literally only checking if it's one of the types already named in the union and it goes and widens it from |
|
@fatcerberus I think that it should handle that case, but I'm not sure whether the custom type guard in |
|
The |
|
This does fix the unnecessary widening issue. |
|
I'm going to update the test to better organize the various facets of this feature that are being tested. Here are a few samples (commented annotations are taken mechanically from the generated function dontWidenPointlessly() {
class Query<T> {
uses: T;
}
function f<T>(p: T[] | Query<T>) {
if (Array.isArray(p)) {
p; // p: T[]
} else if (q instanceof Query) {
p; // p: Query<T>
}
}
}I've identified some bugs/limitations that need to be corrected before moving forward: function union() {
class Parent<A> {
a: A;
}
class Child<B> extends Parent<B> {
b: B;
}
function multipleParents(
p: Parent<number> | Parent<string> | Parent<boolean>
) {
if (p instanceof Child) {
p; // p: Child<number> | Child<string> | Child<boolean>
} else {
p; // p: Parent<number> | Parent<string> | Parent<boolean>
}
}
function mixedChildren(p: Parent<number> | Child<string>) {
if (p instanceof Child) {
p; // p: Child<string>
} else {
p; // p: Parent<number>
}
}
function imcompatibleOptions(
p: Parent<number> | Parent<string> | { foo: boolean }
) {
if (p instanceof Child) {
p; // p: Child<any>
} else {
p; // p: Parent<number> | Parent<string> | { foo: boolean; }
}
}
}There are several problems visible here:
|
|
It occurs to me that the theory behind the current implementation is not terribly sound. At the same time, it's not entirely obvious to me how this can be corrected. When we have let x: T;
if (x instanceof K) {
...
}and x: T & (exists<t1 extends C1, t2 extends C2> in K<t1, t2>)That is, we know that it's still a The trick is that in many (but not obviously all) cases we can simplify this to remove the existential, producing a type that can be expressed in TS today, and is functionally the same. For example, On the other hand, const a: {f:1|2} = {f: 1};
const b: {f:number} = a;
b.f = 3;The way this could be done soundly is by annotating every field with a const a: {readonly f : 1 | 2} & {writeonly f: 1 | 2} = {f: 1};
const b: {readonly f : number} & {writeonly f : 1 | 2} = a;
b.f = 3; // error, f is only writeonly with 1 | 2
Moving back to the original problem, we would like to say that What this suggests as an alternative, sound(ish) procedure is to therefore consider the type This partly simplifies the view of what must be done- many easy cases fall out immediately. The main problem are object and intersection types (since they have fields, and those fields must be compared against the existential) whereas all other instances are either |
const a: {f:1|2} = {f: 1};
const b: {f:number} = a;
b.f = 3;This kind of blew my mind. Thinking about it, I guess enforcing the contravariance here might be too restrictive in practice; it would effectively make all objects invariant unless they consisted of nothing but Still thought-provoking, though. |
|
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
|
The TypeScript team hasn't accepted the linked issue #28560. If you can get it accepted, this PR will have a better chance of being reviewed. |
This change fixes #28560 by inferring (where possible) the generic type parameters for a class when it's narrowed to by the
instanceofoperator.The general process is to infer the parameters as though performing an assignability check of
new Child(...)to the super type. This means that it handles e.g.and anything else you might try to throw at it.
This is a breaking change. However, nothing in the test suite broke (the only changes would be in the newly-added test case that covers this behavior).
It therefore might be a good idea to put it behind a new strict flag, e.g.
--stricterInstanceofNarrowing. However, depending on whether anyone out in the world breaks, that might not be needed.Not all cases are fully covered (i.e. they still infer
anyfor some/all type parameters), or otherwise behave in a (potentially) unexpected (albeit correct) manner:the resulting type is the most-specific sound(ish) type. It's unlikely anyone deliberately will encounter this case (it should be the case that the
Childtype is uninhabited in that case).