Skip to content

fix narrowTypeByDiscriminant for non null expression access #52136

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
Merged
merged 3 commits into from
Jan 10, 2023
Merged

fix narrowTypeByDiscriminant for non null expression access #52136

merged 3 commits into from
Jan 10, 2023

Conversation

gabritto
Copy link
Member

@gabritto gabritto commented Jan 6, 2023

Fixes #52118.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 6, 2023
@gabritto
Copy link
Member Author

gabritto commented Jan 6, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 6, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 6, 2023 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 6, 2023 •
edited
Loading

Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 366f92a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@gabritto Here are the results of running the user test suite comparing main and refs/pull/52136/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..52136

Metric main 52136 Delta Best Worst
Angular - node (v16.17.1, x64)
Memory used 372,009k (± 0.01%) 372,016k (± 0.01%) +7k (+ 0.00%) 371,944k 372,053k
Parse Time 4.17s (± 0.37%) 4.14s (± 0.41%) -0.03s (- 0.73%) 4.11s 4.16s
Bind Time 1.27s (± 0.67%) 1.27s (± 0.76%) -0.00s (- 0.32%) 1.26s 1.28s
Check Time 9.30s (± 0.48%) 9.29s (± 0.38%) -0.02s (- 0.18%) 9.25s 9.33s
Emit Time 7.97s (± 0.66%) 7.98s (± 0.76%) +0.00s (+ 0.03%) 7.90s 8.07s
Total Time 22.71s (± 0.43%) 22.67s (± 0.44%) -0.04s (- 0.20%) 22.57s 22.84s
Compiler-Unions - node (v16.17.1, x64)
Memory used 199,966k (± 0.03%) 200,539k (± 0.63%) +573k (+ 0.29%) 199,966k 203,137k
Parse Time 1.81s (± 0.42%) 1.82s (± 0.64%) +0.00s (+ 0.13%) 1.80s 1.83s
Bind Time 0.84s (± 1.06%) 0.84s (± 0.47%) -0.00s (- 0.26%) 0.84s 0.85s
Check Time 10.10s (± 0.87%) 10.08s (± 0.93%) -0.02s (- 0.25%) 9.92s 10.18s
Emit Time 2.99s (± 1.03%) 3.03s (± 4.84%) +0.04s (+ 1.36%) 2.95s 3.33s
Total Time 15.76s (± 0.63%) 15.77s (± 1.35%) +0.01s (+ 0.09%) 15.54s 16.17s
Monaco - node (v16.17.1, x64)
Memory used 353,439k (± 0.01%) 353,410k (± 0.01%) -29k (- 0.01%) 353,349k 353,457k
Parse Time 3.19s (± 1.47%) 3.18s (± 0.69%) -0.00s (- 0.09%) 3.14s 3.21s
Bind Time 1.13s (± 1.36%) 1.12s (± 1.05%) -0.01s (- 0.77%) 1.10s 1.13s
Check Time 7.87s (± 0.53%) 7.85s (± 0.73%) -0.02s (- 0.24%) 7.77s 7.94s
Emit Time 4.47s (± 0.82%) 4.47s (± 0.52%) +0.01s (+ 0.15%) 4.45s 4.51s
Total Time 16.65s (± 0.55%) 16.62s (± 0.57%) -0.03s (- 0.18%) 16.52s 16.78s
TFS - node (v16.17.1, x64)
Memory used 309,375k (± 0.00%) 309,380k (± 0.00%) +5k (+ 0.00%) 309,369k 309,393k
Parse Time 2.61s (± 1.76%) 2.61s (± 1.23%) -0.00s (- 0.12%) 2.58s 2.66s
Bind Time 1.06s (± 2.13%) 1.06s (± 1.56%) +0.00s (+ 0.45%) 1.05s 1.09s
Check Time 7.42s (± 0.72%) 7.39s (± 0.25%) -0.04s (- 0.47%) 7.37s 7.41s
Emit Time 4.22s (± 0.80%) 4.22s (± 0.64%) -0.00s (- 0.03%) 4.17s 4.25s
Total Time 15.32s (± 0.55%) 15.28s (± 0.33%) -0.04s (- 0.26%) 15.22s 15.35s
material-ui - node (v16.17.1, x64)
Memory used 484,517k (± 0.01%) 484,503k (± 0.00%) -14k (- 0.00%) 484,483k 484,537k
Parse Time 3.68s (± 0.43%) 3.68s (± 0.47%) -0.00s (- 0.07%) 3.66s 3.71s
Bind Time 1.01s (± 0.76%) 1.01s (± 0.60%) -0.00s (- 0.43%) 1.00s 1.02s
Check Time 18.08s (± 1.31%) 17.83s (± 0.22%) -0.25s (- 1.37%) 17.77s 17.89s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 22.77s (± 1.09%) 22.52s (± 0.22%) -0.26s (- 1.12%) 22.45s 22.58s
xstate - node (v16.17.1, x64)
Memory used 567,830k (± 0.02%) 567,881k (± 0.02%) +51k (+ 0.01%) 567,721k 568,053k
Parse Time 4.76s (± 0.20%) 4.76s (± 0.58%) -0.00s (- 0.07%) 4.72s 4.79s
Bind Time 1.66s (± 0.41%) 1.65s (± 0.56%) -0.00s (- 0.20%) 1.64s 1.66s
Check Time 2.83s (± 0.24%) 2.84s (± 0.35%) +0.01s (+ 0.47%) 2.83s 2.85s
Emit Time 0.09s (± 4.59%) 0.09s (± 4.59%) -0.00s (- 0.00%) 0.08s 0.09s
Total Time 9.34s (± 0.20%) 9.34s (± 0.31%) -0.01s (- 0.07%) 9.29s 9.37s
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 52136 6
Baseline main 6

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

@gabritto Here are the results of running the top-repos suite comparing main and refs/pull/52136/merge:

Everything looks good!

@gabritto gabritto marked this pull request as ready for review January 7, 2023 00:07
ahejlsberg
ahejlsberg reviewed Jan 9, 2023
View reviewed changes
src/compiler/checker.ts
@@ -26299,7 +26300,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (propName === undefined) {
return type;
}
const removeNullable = strictNullChecks && isOptionalChain(access) && maybeTypeOfKind(type, TypeFlags.Nullable);
const removeNullable = strictNullChecks && (isOptionalChain(access) || isNonNullAccess(access)) && maybeTypeOfKind(type, TypeFlags.Nullable);
Copy link
Member

Choose a reason for hiding this comment

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

Here we remove null or undefined from the type, but then a few lines below we add back only an undefined. That reflects what happens in an optional chain, but the ! operator never changes one to the other. It may not matter, but doesn't quite seem right.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, I had missed that. So I guess we shouldn't call getOptionalType below if it's a ! operator, since ! actually gets rid of undefined and null, unlike an optional chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ahejlsberg I pushed the above change, let me know if it's not right or if there's something else missing.

@gabritto gabritto merged commit 2082ef2 into main Jan 10, 2023
@gabritto gabritto deleted the gabritto/issue52118 branch January 10, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahejlsberg ahejlsberg ahejlsberg approved these changes

@weswigham weswigham Awaiting requested review from weswigham

Assignees

@gabritto gabritto

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Minimum of 10 subtypes in union for correct type inferring with non-null assertion
3 participants
@gabritto @typescript-bot @ahejlsberg