Skip to content
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

Jump to bottom

Improve narrowing logic for instanceof, type predicate functions, and assertion functions #49625

Merged
merged 7 commits into from
Jul 16, 2022
Merged

Improve narrowing logic for instanceof, type predicate functions, and assertion functions #49625

merged 7 commits into from
Jul 16, 2022

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Jun 22, 2022 •
edited
Loading

This PR improves our narrowing logic for instanceof, type predicate functions, and assertion functions. In particular, we now correctly handle situations where the narrowing target type is a union type.

Some examples:

type Falsy = false | 0 | 0n | '' | null | undefined;

declare function isFalsy(value: unknown): value is Falsy;

function fx1(x: string | number | undefined) {
    if (isFalsy(x)) {
        x;  // "" | 0 | undefined, previously undefined
    }
}

function fx2<T>(x: T | undefined) {
    if (isFalsy(x)) {
        x;  // (T & null) | (T & false) | (T & "") | (T & 0) | (T & 0n) | undefined, previously undefined
    }
}

function fx3<T extends string | number>(x: T) {
    if (isFalsy(x)) {
        x;  // T & "" | T & 0, previously T & Falsy
    }
}

declare function isA(obj: unknown): obj is { a: false } | { b: 0 };

function fx4(obj: { b: number }) {
    if (isA(obj)) {
        obj;  // { b: 0 }, previously { b: number } & ({ a: false } | { b: 0 })
    }
}

declare class X { x: string }
declare class XS extends X { xs: string }

declare class Y { y: string }
declare class YS extends Y { ys: string }

declare function isXSorY(obj: unknown): obj is XS | Y;

function fx5<T extends X>(obj: X | YS, c: typeof XS | typeof Y) {
    if (obj instanceof c) {
        obj;  // XS | YS, previously YS
    }
    if (isXSorY(obj)) {
        obj;  // XS | YS, previously YS
    }
}

Previously all of the above examples produced wrong results.

Fixes #31156.
Fixes #35953.
Fixes #37807.
Fixes #38869.
Fixes #39105.
Fixes #40035.
Fixes #41871.
Fixes #42101.
Fixes #43825.
Fixes #44754.
Fixes #46909.
Fixes #49588.

@typescript-bot typescript-bot added Author: Team For Backlog Bug PRs that fix a backlog bug labels Jun 22, 2022
@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot perf test faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at fc81aff. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at fc81aff. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at fc81aff. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at fc81aff. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..49625

Metric main 49625 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 335,317k (± 0.01%) 335,376k (± 0.01%) +59k (+ 0.02%) 335,317k 335,422k
Parse Time 2.09s (± 0.68%) 2.08s (± 0.43%) -0.01s (- 0.62%) 2.06s 2.10s
Bind Time 0.91s (± 0.52%) 0.91s (± 0.75%) -0.00s (- 0.11%) 0.89s 0.92s
Check Time 5.75s (± 0.40%) 5.76s (± 0.55%) +0.01s (+ 0.14%) 5.71s 5.82s
Emit Time 6.42s (± 0.72%) 6.39s (± 0.95%) -0.03s (- 0.50%) 6.30s 6.53s
Total Time 15.17s (± 0.36%) 15.13s (± 0.54%) -0.04s (- 0.24%) 14.99s 15.32s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,554k (± 0.13%) 195,458k (± 0.01%) +2,904k (+ 1.51%) 195,397k 195,515k
Parse Time 0.86s (± 0.69%) 0.85s (± 0.55%) -0.01s (- 0.70%) 0.84s 0.86s
Bind Time 0.58s (± 1.29%) 0.57s (± 0.86%) -0.00s (- 0.52%) 0.57s 0.59s
Check Time 7.73s (± 0.61%) 8.38s (± 1.29%) +0.66s (+ 8.50%) 8.22s 8.76s
Emit Time 2.52s (± 0.55%) 2.52s (± 0.74%) +0.00s (+ 0.16%) 2.48s 2.56s
Total Time 11.68s (± 0.47%) 12.33s (± 0.96%) +0.65s (+ 5.56%) 12.15s 12.73s
Monaco - node (v14.15.1, x64)
Memory used 325,607k (± 0.00%) 325,478k (± 0.01%) -129k (- 0.04%) 325,415k 325,532k
Parse Time 1.59s (± 0.78%) 1.58s (± 0.66%) -0.01s (- 0.63%) 1.56s 1.61s
Bind Time 0.80s (± 0.60%) 0.79s (± 0.92%) -0.00s (- 0.63%) 0.78s 0.81s
Check Time 5.72s (± 0.69%) 5.67s (± 0.30%) -0.05s (- 0.82%) 5.64s 5.72s
Emit Time 3.37s (± 0.73%) 3.35s (± 0.74%) -0.02s (- 0.62%) 3.29s 3.41s
Total Time 11.46s (± 0.52%) 11.39s (± 0.28%) -0.08s (- 0.68%) 11.33s 11.46s
TFS - node (v14.15.1, x64)
Memory used 288,754k (± 0.01%) 288,722k (± 0.01%) -32k (- 0.01%) 288,692k 288,770k
Parse Time 1.34s (± 1.52%) 1.33s (± 1.20%) -0.02s (- 1.12%) 1.30s 1.36s
Bind Time 0.75s (± 0.94%) 0.74s (± 0.64%) -0.01s (- 0.80%) 0.73s 0.75s
Check Time 5.33s (± 0.38%) 5.30s (± 0.39%) -0.03s (- 0.47%) 5.27s 5.35s
Emit Time 3.59s (± 2.15%) 3.61s (± 1.95%) +0.01s (+ 0.39%) 3.45s 3.74s
Total Time 11.02s (± 0.93%) 10.98s (± 0.57%) -0.03s (- 0.29%) 10.84s 11.09s
material-ui - node (v14.15.1, x64)
Memory used 446,332k (± 0.00%) 446,216k (± 0.06%) -116k (- 0.03%) 445,138k 446,373k
Parse Time 1.89s (± 0.50%) 1.88s (± 0.40%) -0.01s (- 0.42%) 1.87s 1.91s
Bind Time 0.72s (± 1.23%) 0.72s (± 1.26%) -0.00s (- 0.41%) 0.70s 0.74s
Check Time 13.17s (± 0.59%) 13.14s (± 0.63%) -0.03s (- 0.22%) 12.96s 13.31s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.79s (± 0.49%) 15.75s (± 0.50%) -0.03s (- 0.22%) 15.56s 15.90s
xstate - node (v14.15.1, x64)
Memory used 541,028k (± 0.00%) 541,672k (± 0.00%) +645k (+ 0.12%) 541,617k 541,714k
Parse Time 2.63s (± 0.49%) 2.63s (± 0.34%) +0.00s (+ 0.04%) 2.62s 2.65s
Bind Time 1.17s (± 0.81%) 1.17s (± 1.15%) +0.00s (+ 0.17%) 1.14s 1.19s
Check Time 1.53s (± 0.52%) 1.53s (± 0.49%) +0.00s (+ 0.07%) 1.52s 1.55s
Emit Time 0.07s (± 4.92%) 0.07s (± 3.14%) 🟩-0.00s (- 4.05%) 0.07s 0.08s
Total Time 5.41s (± 0.30%) 5.41s (± 0.33%) -0.00s (- 0.06%) 5.38s 5.45s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49625 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/49625/merge

@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot perf test faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 37bfa91. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 37bfa91. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 37bfa91. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/49625/merge

@typescript-bot
Copy link
Collaborator

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.

@RyanCavanaugh
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 37bfa91. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..49625

Metric main 49625 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 335,331k (± 0.01%) 335,388k (± 0.01%) +57k (+ 0.02%) 335,295k 335,490k
Parse Time 2.07s (± 0.65%) 2.08s (± 0.46%) +0.01s (+ 0.39%) 2.06s 2.10s
Bind Time 0.90s (± 0.49%) 0.91s (± 0.49%) +0.01s (+ 0.67%) 0.90s 0.92s
Check Time 5.75s (± 0.25%) 5.75s (± 0.39%) -0.00s (- 0.03%) 5.69s 5.80s
Emit Time 6.40s (± 0.91%) 6.44s (± 0.89%) +0.03s (+ 0.52%) 6.29s 6.53s
Total Time 15.13s (± 0.47%) 15.17s (± 0.44%) +0.04s (+ 0.27%) 15.02s 15.28s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,649k (± 0.01%) 195,450k (± 0.01%) +2,801k (+ 1.45%) 195,393k 195,504k
Parse Time 0.85s (± 0.47%) 0.85s (± 0.58%) +0.00s (+ 0.59%) 0.85s 0.87s
Bind Time 0.57s (± 0.87%) 0.58s (± 1.52%) +0.01s (+ 0.87%) 0.57s 0.61s
Check Time 7.63s (± 0.44%) 8.38s (± 0.97%) +0.75s (+ 9.76%) 8.23s 8.58s
Emit Time 2.52s (± 0.68%) 2.54s (± 1.39%) +0.03s (+ 1.03%) 2.48s 2.64s
Total Time 11.57s (± 0.29%) 12.35s (± 0.60%) +0.78s (+ 6.78%) 12.24s 12.51s
Monaco - node (v14.15.1, x64)
Memory used 325,616k (± 0.00%) 325,488k (± 0.01%) -129k (- 0.04%) 325,441k 325,530k
Parse Time 1.57s (± 0.51%) 1.59s (± 0.47%) +0.02s (+ 1.21%) 1.58s 1.61s
Bind Time 0.79s (± 0.28%) 0.79s (± 0.46%) +0.00s (+ 0.63%) 0.79s 0.80s
Check Time 5.67s (± 0.51%) 5.69s (± 0.68%) +0.02s (+ 0.42%) 5.63s 5.78s
Emit Time 3.33s (± 0.76%) 3.36s (± 0.84%) +0.03s (+ 0.87%) 3.32s 3.45s
Total Time 11.36s (± 0.31%) 11.43s (± 0.46%) +0.08s (+ 0.70%) 11.33s 11.54s
TFS - node (v14.15.1, x64)
Memory used 288,773k (± 0.01%) 288,739k (± 0.01%) -34k (- 0.01%) 288,655k 288,826k
Parse Time 1.32s (± 1.18%) 1.33s (± 0.55%) +0.00s (+ 0.23%) 1.31s 1.34s
Bind Time 0.75s (± 0.63%) 0.76s (± 0.90%) +0.01s (+ 0.80%) 0.74s 0.77s
Check Time 5.31s (± 0.34%) 5.31s (± 0.50%) -0.01s (- 0.11%) 5.26s 5.38s
Emit Time 3.57s (± 1.74%) 3.63s (± 1.83%) +0.06s (+ 1.79%) 3.44s 3.69s
Total Time 10.95s (± 0.65%) 11.02s (± 0.59%) +0.07s (+ 0.62%) 10.84s 11.13s
material-ui - node (v14.15.1, x64)
Memory used 446,341k (± 0.00%) 446,338k (± 0.01%) -3k (- 0.00%) 446,300k 446,422k
Parse Time 1.89s (± 0.47%) 1.90s (± 0.56%) +0.02s (+ 0.85%) 1.87s 1.92s
Bind Time 0.72s (± 1.22%) 0.73s (± 1.05%) +0.00s (+ 0.28%) 0.71s 0.74s
Check Time 13.05s (± 0.50%) 13.22s (± 0.67%) +0.16s (+ 1.24%) 13.03s 13.47s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.66s (± 0.41%) 15.84s (± 0.53%) +0.18s (+ 1.12%) 15.66s 16.09s
xstate - node (v14.15.1, x64)
Memory used 541,039k (± 0.01%) 541,040k (± 0.01%) +1k (+ 0.00%) 540,989k 541,111k
Parse Time 2.63s (± 0.49%) 2.65s (± 0.62%) +0.02s (+ 0.72%) 2.62s 2.69s
Bind Time 1.17s (± 0.79%) 1.18s (± 1.24%) +0.01s (+ 1.11%) 1.15s 1.21s
Check Time 1.53s (± 0.78%) 1.54s (± 0.73%) +0.01s (+ 0.59%) 1.52s 1.56s
Emit Time 0.07s (± 4.13%) 0.07s (± 4.92%) +0.00s (+ 2.78%) 0.07s 0.08s
Total Time 5.41s (± 0.35%) 5.44s (± 0.54%) +0.03s (+ 0.55%) 5.37s 5.50s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49625 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2022 •
edited
Loading

Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/128629/artifacts?artifactName=tgz&fileId=9370A02BE9286083A98BB5F91ED39114FDC7E27C11CF7BF17C728DC44CF30EDE02&fileName=/typescript-4.8.0-insiders.20220622.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.8.0-pr-49625-17".;

@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 5, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 960ad11. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 5, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 960ad11. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 5, 2022 •
edited
Loading

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 960ad11. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/49625/merge

@typescript-bot
Copy link
Collaborator

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.

@jakebailey
Copy link
Member

Is this for the 4.8 RC, or is this waiting for 4.9?

(Wondering because I was hoping to make this main's LKG as it might fix a few workarounds I have on my "enable strictFunctionTypes" branch.)

@ahejlsberg ahejlsberg merged commit 2c68ded into main Jul 16, 2022
@ahejlsberg ahejlsberg deleted the fix31156 branch July 16, 2022 00:01
@DanielRosenwasser DanielRosenwasser mentioned this pull request Jul 18, 2022
Closed
@sandersn sandersn mentioned this pull request Jul 21, 2022
Closed
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 21, 2022
@sandersn
Update node types.isMap tests
59e7b9d 
microsoft/TypeScript#49625 improves handling of
unions in type predicates so that unions are correctly preserved. This
breaks types.isMap in node.

For now I just changed the tests' expected type, but the type of isMap doesn't
make much sense to me. It should probably be changed, but that's a much
more complex task.

This break is tracked at
microsoft/TypeScript#49988 although it's
correct, I think,  so not very likely to be reverted.
@sandersn sandersn mentioned this pull request Jul 21, 2022
Merged
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 22, 2022
@sandersn
Update node types.isMap tests (#61389)
87c7553 
* Update node types.isMap tests

microsoft/TypeScript#49625 improves handling of
unions in type predicates so that unions are correctly preserved. This
breaks types.isMap in node.

For now I just changed the tests' expected type, but the type of isMap doesn't
make much sense to me. It should probably be changed, but that's a much
more complex task.

This break is tracked at
microsoft/TypeScript#49988 although it's
correct, I think,  so not very likely to be reverted.

* Change test for isSet too
@DanielRosenwasser DanielRosenwasser mentioned this pull request Jul 22, 2022
Closed
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 22, 2022
@sandersn
Update underscore and weak-napi tests for TS 4.8
399faf6 
TS 4.8 improves narrowing of type predicates when a union is passed in.
This changes the return type of a couple of underscore and weak-napi
functions. For now I just updated the tests, since underscore's change
is pretty minor. And weak-napi's types
probably need to be rewritten substantially, which would probably result
in fixing this change.

See microsoft/TypeScript#49625 for the change.
@sandersn sandersn mentioned this pull request Jul 22, 2022
Merged
sandersn added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 22, 2022
@sandersn
Update underscore and weak-napi tests for TS 4.8 (#61419)
6a6411f 
TS 4.8 improves narrowing of type predicates when a union is passed in.
This changes the return type of a couple of underscore and weak-napi
functions. For now I just updated the tests, since underscore's change
is pretty minor. And weak-napi's types
probably need to be rewritten substantially, which would probably result
in fixing this change.

See microsoft/TypeScript#49625 for the change.
@ahejlsberg ahejlsberg mentioned this pull request Jul 25, 2022
Merged
@Andarist Andarist mentioned this pull request Oct 7, 2022
Closed
5 tasks
@rf-figma rf-figma mentioned this pull request May 5, 2023
Closed
@jakebailey jakebailey mentioned this pull request Oct 1, 2023
Closed
@epmatsw epmatsw mentioned this pull request Oct 2, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
5 participants
@ahejlsberg @typescript-bot @RyanCavanaugh @jakebailey @DanielRosenwasser