Check for strict subtypes and then regular subtypes in getNarrowedType #52984
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 0325f9d. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 0325f9d. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 0325f9d. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 0325f9d. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@typescript-bot perf test this faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 0325f9d. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:Comparison Report - main..52984
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
ahejlsberg
changed the title
ahejlsberg
changed the title
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at d737eee. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at d737eee. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at d737eee. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at d737eee. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at d737eee. You can monitor the build here. |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
|
@ahejlsberg Here they are:Comparison Report - main..52984
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
|
And, the answer is probably no (because it's risky), but, given the 5.0 regressions people are reporting, I'm somewhat curious if this is something we should backport before stable 5.0. But, I assume not. |
|
@jakebailey You can see some of them here. |
|
I think we should consider bringing this one into 5.0. |
Original reporter of #50916 here. I tried out #52282 again in our codebase and while it does address the original issue, it introduced a regression elsewhere so this would still block us from upgrading to TS 5.0. I've tried out the fix in this PR locally and it seems to resolve both issues, so it'd be great if this could make it into TS 5.0. The new regression if curious type DistributedKeyOf<T> = T extends unknown ? keyof T : never;
type NarrowByKeyValue<ObjT, KeyT extends PropertyKey, ValueT> = ObjT extends unknown
? KeyT extends keyof ObjT
? ValueT extends ObjT[KeyT]
? ObjT & Readonly<Record<KeyT, ValueT>>
: never
: never
: never;
type NarrowByDeepValue<ObjT, DeepPathT, ValueT> = DeepPathT extends readonly [
infer Head extends DistributedKeyOf<ObjT>,
]
? NarrowByKeyValue<ObjT, Head, ValueT>
: DeepPathT extends readonly [infer Head extends DistributedKeyOf<ObjT>, ...infer Rest]
? NarrowByKeyValue<ObjT, Head, NarrowByDeepValue<NonNullable<ObjT[Head]>, Rest, ValueT>>
: never;
declare function doesValueAtDeepPathSatisfy<
ObjT extends object,
const DeepPathT extends ReadonlyArray<number | string>,
ValueT,
>(
obj: ObjT,
deepPath: DeepPathT,
predicate: (arg: unknown) => arg is ValueT,
): obj is NarrowByDeepValue<ObjT, DeepPathT, ValueT>;
type Foo = {value: {type: 'A'}; a?: number} | {value: {type: 'B'}; b?: number};
declare function isA(arg: unknown): arg is 'A';
declare function isB(arg: unknown): arg is 'B';
declare function assert(condition: boolean): asserts condition;
function test1(foo: Foo): {value: {type: 'A'}; a?: number} {
assert(doesValueAtDeepPathSatisfy(foo, ['value', 'type'], isA));
return foo;
}
function test2(foo: Foo): {value: {type: 'A'}; a?: number} {
assert(!doesValueAtDeepPathSatisfy(foo, ['value', 'type'], isB));
return foo;
} |
|
@MichaelMitchell-at Good to hear this PR fixes the issues. Meanwhile, would you mind updating the regression code above with the definitions of the missing functions so we can have an additional repro. |
Ok, I'll need to refactor a bit to remove some internal dependencies |
|
This also fixes a regression in Ember's internal type test suite for TS 5.1 (no issue on 5.0) shaped like this: function example(vals: Array<unknown> | MyCollection<unknown>): number {
if (Array.isArray(vals)) {
return vals.length;
// ^^^^ has type `Array<any>`
} else {
// ❌ type error (erroneous!)
return vals.collectionSize;
// ^^^^ has type `Array<unknown> | MyCollection<unknown>`
}
}With this PR, this is back to behaving the way it did in v5.0 and earlier. 👍🏼 |
|
@ahejlsberg updated |
|
@MichaelMitchell-at Much appreciated. Will add as a test case. |
|
@chriskrycho I take it you mean this PR brings it back to behaving like 4.9 and earlier, right? |
|
Additional repro added in latest commit. |
|
@ahejlsberg Possibly I missed up my bisection, but in my testing, I thought I confirmed that the 5.0 RC worked and 5.1 did not. I can dig further (though not till Monday at the earliest). |
|
@typescript-bot cherry-pick this to release-5.0 |
|
Heya @jakebailey, I've started to run the task to cherry-pick this into |
|
Hey @jakebailey, I've opened #53085 for you. |
Component commits: b3d3ec9 Use strictSubtypeRelation in getNarrowedType and narrow only for pure subtypes 0325f9d Accept new baselines 3df807f First check for strict subtypes, then check for regular subtypes d737eee Accept new baselines 9b2d602 Add tests 9ea8a55 Accept new baselines 8bb30e2 Add another repro
…e-5.0 (#53085) Co-authored-by: Anders Hejlsberg <[email protected]>
Experiment to see the effects of first checking for strict subtypes and then checking for regular subtypes in
getNarrowedType, and picking the asserted type only when it is a pure subtype of the original type.First experiment checked only using the strict subtype relationship, but that introduces too many breaking changes (specifically caused by strict checking of signature arity and readonly properties).
Fixes #52827.