Skip to content

Allow assignments to a narrowable reference to be considered narrowable #39824

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 4 commits into from
Jul 30, 2020
Merged

Allow assignments to a narrowable reference to be considered narrowable #39824

merged 4 commits into from
Jul 30, 2020

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Jul 29, 2020 •
edited
Loading

This modifies control flow to consider an assignment to be narrowable if the assignment target is a narrowable reference.

Given:

type D = { done: true, value: 1 } | { done: false, value: 2 };
declare function f(): D;
// (A)
{
  let o: D;
  if (o = f(), o.done) {
    o.value; // '1'
  }
}
 
// (B)
{
  let o: D;
  if ((o = f()).done) {
      o.value; // '1 | 2'
  }
}

Prior to this fix, o.value in A had the type 1 but o.value in B had the type 1 | 2 because we did not consider (o = f()) to be narrowable.

This also fixes some discrepancies in the control flow graph formatter used for debugging to handle circular references in loops.

Fixes #35484

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 29, 2020
@rbuckton rbuckton requested review from ahejlsberg and weswigham July 29, 2020 23:43
@rbuckton
Copy link
Contributor Author

rbuckton commented Jul 29, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020 •
edited
Loading

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at b9db641. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 29, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..39824

Metric master 39824 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 343,603k (± 0.02%) 343,132k (± 0.03%) -470k (- 0.14%) 342,897k 343,331k
Parse Time 2.02s (± 0.36%) 2.00s (± 0.62%) -0.02s (- 1.24%) 1.98s 2.04s
Bind Time 0.81s (± 0.64%) 0.82s (± 1.94%) +0.01s (+ 1.11%) 0.80s 0.88s
Check Time 4.76s (± 1.08%) 4.75s (± 0.51%) -0.00s (- 0.11%) 4.70s 4.81s
Emit Time 5.19s (± 0.67%) 5.15s (± 0.42%) -0.04s (- 0.81%) 5.11s 5.21s
Total Time 12.78s (± 0.71%) 12.71s (± 0.32%) -0.06s (- 0.50%) 12.63s 12.80s
Monaco - node (v10.16.3, x64)
Memory used 339,090k (± 0.02%) 339,108k (± 0.02%) +19k (+ 0.01%) 338,947k 339,297k
Parse Time 1.58s (± 0.63%) 1.58s (± 0.48%) -0.00s (- 0.25%) 1.56s 1.59s
Bind Time 0.71s (± 0.84%) 0.71s (± 0.51%) +0.01s (+ 1.13%) 0.71s 0.72s
Check Time 4.91s (± 0.67%) 4.92s (± 0.54%) +0.01s (+ 0.16%) 4.88s 5.00s
Emit Time 2.75s (± 0.83%) 2.76s (± 0.98%) +0.01s (+ 0.29%) 2.70s 2.84s
Total Time 9.94s (± 0.47%) 9.96s (± 0.45%) +0.02s (+ 0.19%) 9.86s 10.05s
TFS - node (v10.16.3, x64)
Memory used 301,944k (± 0.02%) 301,981k (± 0.02%) +36k (+ 0.01%) 301,876k 302,202k
Parse Time 1.21s (± 0.48%) 1.20s (± 0.48%) -0.00s (- 0.33%) 1.19s 1.21s
Bind Time 0.67s (± 0.56%) 0.67s (± 0.74%) +0.00s (+ 0.15%) 0.66s 0.68s
Check Time 4.42s (± 0.67%) 4.43s (± 0.70%) +0.01s (+ 0.32%) 4.37s 4.52s
Emit Time 2.89s (± 0.78%) 2.90s (± 1.28%) +0.01s (+ 0.45%) 2.82s 2.99s
Total Time 9.18s (± 0.52%) 9.20s (± 0.54%) +0.02s (+ 0.20%) 9.11s 9.32s
material-ui - node (v10.16.3, x64)
Memory used 458,729k (± 0.01%) 458,381k (± 0.01%) -348k (- 0.08%) 458,194k 458,459k
Parse Time 2.05s (± 0.73%) 2.04s (± 0.41%) -0.01s (- 0.29%) 2.02s 2.06s
Bind Time 0.65s (± 1.39%) 0.64s (± 1.06%) -0.00s (- 0.16%) 0.62s 0.65s
Check Time 12.96s (± 0.89%) 12.83s (± 0.37%) -0.13s (- 1.00%) 12.73s 12.93s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.65s (± 0.76%) 15.51s (± 0.32%) -0.14s (- 0.87%) 15.39s 15.61s
Angular - node (v12.1.0, x64)
Memory used 320,857k (± 0.02%) 320,410k (± 0.02%) -448k (- 0.14%) 320,224k 320,523k
Parse Time 2.00s (± 0.78%) 1.99s (± 0.52%) -0.01s (- 0.65%) 1.97s 2.01s
Bind Time 0.80s (± 0.87%) 0.80s (± 0.77%) -0.00s (- 0.00%) 0.79s 0.81s
Check Time 4.66s (± 0.40%) 4.66s (± 0.30%) +0.00s (+ 0.09%) 4.64s 4.70s
Emit Time 5.38s (± 0.61%) 5.34s (± 0.48%) -0.04s (- 0.67%) 5.28s 5.39s
Total Time 12.84s (± 0.42%) 12.79s (± 0.31%) -0.05s (- 0.38%) 12.69s 12.90s
Monaco - node (v12.1.0, x64)
Memory used 321,569k (± 0.02%) 321,547k (± 0.02%) -23k (- 0.01%) 321,341k 321,623k
Parse Time 1.55s (± 0.78%) 1.53s (± 0.97%) -0.01s (- 0.84%) 1.50s 1.56s
Bind Time 0.68s (± 0.87%) 0.69s (± 1.05%) +0.01s (+ 1.02%) 0.68s 0.71s
Check Time 4.72s (± 0.41%) 4.68s (± 0.53%) -0.03s (- 0.72%) 4.63s 4.74s
Emit Time 2.80s (± 0.46%) 2.79s (± 0.74%) -0.01s (- 0.43%) 2.75s 2.85s
Total Time 9.75s (± 0.28%) 9.70s (± 0.39%) -0.05s (- 0.53%) 9.66s 9.84s
TFS - node (v12.1.0, x64)
Memory used 286,612k (± 0.02%) 286,535k (± 0.02%) -77k (- 0.03%) 286,424k 286,744k
Parse Time 1.23s (± 0.78%) 1.23s (± 0.91%) -0.01s (- 0.41%) 1.21s 1.26s
Bind Time 0.63s (± 0.47%) 0.64s (± 0.81%) +0.01s (+ 1.11%) 0.63s 0.65s
Check Time 4.31s (± 0.40%) 4.32s (± 0.66%) +0.01s (+ 0.21%) 4.26s 4.39s
Emit Time 2.93s (± 0.64%) 2.91s (± 0.79%) -0.02s (- 0.55%) 2.86s 2.97s
Total Time 9.10s (± 0.32%) 9.10s (± 0.49%) -0.00s (- 0.01%) 8.99s 9.20s
material-ui - node (v12.1.0, x64)
Memory used 437,042k (± 0.01%) 436,604k (± 0.06%) -438k (- 0.10%) 435,801k 436,871k
Parse Time 2.03s (± 0.46%) 2.02s (± 0.53%) -0.00s (- 0.15%) 1.99s 2.05s
Bind Time 0.63s (± 0.71%) 0.62s (± 0.72%) -0.01s (- 0.96%) 0.61s 0.63s
Check Time 11.63s (± 1.28%) 11.54s (± 0.37%) -0.09s (- 0.77%) 11.46s 11.65s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.29s (± 1.02%) 14.19s (± 0.31%) -0.10s (- 0.69%) 14.11s 14.30s
Angular - node (v8.9.0, x64)
Memory used 340,209k (± 0.02%) 339,789k (± 0.01%) -420k (- 0.12%) 339,732k 339,851k
Parse Time 2.54s (± 0.50%) 2.53s (± 0.32%) -0.01s (- 0.55%) 2.51s 2.54s
Bind Time 0.85s (± 0.76%) 0.85s (± 1.07%) +0.00s (+ 0.35%) 0.83s 0.87s
Check Time 5.36s (± 0.42%) 5.38s (± 0.80%) +0.02s (+ 0.30%) 5.27s 5.46s
Emit Time 5.93s (± 2.09%) 5.87s (± 1.39%) -0.05s (- 0.91%) 5.69s 6.11s
Total Time 14.68s (± 0.91%) 14.63s (± 0.65%) -0.05s (- 0.31%) 14.43s 14.85s
Monaco - node (v8.9.0, x64)
Memory used 340,451k (± 0.01%) 340,520k (± 0.02%) +68k (+ 0.02%) 340,431k 340,664k
Parse Time 1.88s (± 0.44%) 1.88s (± 0.76%) +0.00s (+ 0.11%) 1.86s 1.93s
Bind Time 0.89s (± 0.56%) 0.88s (± 0.76%) -0.00s (- 0.34%) 0.87s 0.90s
Check Time 5.45s (± 0.63%) 5.44s (± 0.58%) -0.00s (- 0.06%) 5.38s 5.54s
Emit Time 3.23s (± 0.86%) 3.22s (± 0.56%) -0.01s (- 0.28%) 3.17s 3.25s
Total Time 11.43s (± 0.56%) 11.43s (± 0.35%) -0.01s (- 0.08%) 11.35s 11.55s
TFS - node (v8.9.0, x64)
Memory used 303,832k (± 0.03%) 303,791k (± 0.02%) -42k (- 0.01%) 303,662k 303,940k
Parse Time 1.55s (± 0.48%) 1.55s (± 0.45%) +0.00s (+ 0.26%) 1.54s 1.57s
Bind Time 0.67s (± 0.83%) 0.67s (± 0.70%) -0.00s (- 0.30%) 0.66s 0.68s
Check Time 5.14s (± 0.96%) 5.09s (± 1.57%) -0.05s (- 0.97%) 4.91s 5.20s
Emit Time 2.97s (± 2.78%) 3.00s (± 2.96%) +0.03s (+ 0.94%) 2.86s 3.22s
Total Time 10.33s (± 0.49%) 10.31s (± 0.28%) -0.02s (- 0.17%) 10.24s 10.37s
material-ui - node (v8.9.0, x64)
Memory used 462,977k (± 0.01%) 462,685k (± 0.01%) -292k (- 0.06%) 462,585k 462,821k
Parse Time 2.41s (± 0.70%) 2.40s (± 0.75%) -0.00s (- 0.12%) 2.37s 2.45s
Bind Time 0.77s (± 1.44%) 0.77s (± 1.33%) -0.01s (- 0.77%) 0.75s 0.80s
Check Time 17.21s (± 0.96%) 17.14s (± 0.96%) -0.07s (- 0.41%) 16.65s 17.32s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.40s (± 0.85%) 20.31s (± 0.82%) -0.08s (- 0.41%) 19.83s 20.51s
Angular - node (v8.9.0, x86)
Memory used 195,202k (± 0.02%) 194,973k (± 0.02%) -230k (- 0.12%) 194,886k 195,054k
Parse Time 2.46s (± 0.71%) 2.45s (± 0.57%) -0.01s (- 0.49%) 2.43s 2.49s
Bind Time 0.98s (± 0.94%) 0.98s (± 0.90%) +0.00s (+ 0.10%) 0.97s 1.01s
Check Time 4.83s (± 0.34%) 4.85s (± 0.38%) +0.02s (+ 0.44%) 4.80s 4.89s
Emit Time 5.91s (± 0.69%) 5.92s (± 0.70%) +0.01s (+ 0.08%) 5.84s 6.01s
Total Time 14.19s (± 0.37%) 14.20s (± 0.37%) +0.01s (+ 0.06%) 14.09s 14.31s
Monaco - node (v8.9.0, x86)
Memory used 193,539k (± 0.02%) 193,559k (± 0.02%) +20k (+ 0.01%) 193,465k 193,634k
Parse Time 1.91s (± 0.63%) 1.91s (± 1.02%) +0.00s (+ 0.05%) 1.88s 1.98s
Bind Time 0.70s (± 0.47%) 0.70s (± 1.08%) +0.00s (+ 0.57%) 0.69s 0.73s
Check Time 5.53s (± 0.84%) 5.52s (± 1.48%) -0.01s (- 0.23%) 5.23s 5.68s
Emit Time 2.66s (± 0.94%) 2.69s (± 3.03%) +0.02s (+ 0.90%) 2.62s 3.01s
Total Time 10.81s (± 0.46%) 10.82s (± 0.65%) +0.01s (+ 0.10%) 10.68s 11.06s
TFS - node (v8.9.0, x86)
Memory used 173,756k (± 0.02%) 173,747k (± 0.02%) -9k (- 0.01%) 173,640k 173,856k
Parse Time 1.59s (± 0.89%) 1.58s (± 0.96%) -0.00s (- 0.25%) 1.56s 1.63s
Bind Time 0.64s (± 0.75%) 0.64s (± 0.70%) +0.00s (+ 0.16%) 0.63s 0.65s
Check Time 4.66s (± 0.85%) 4.67s (± 0.71%) +0.01s (+ 0.26%) 4.61s 4.77s
Emit Time 2.79s (± 1.35%) 2.76s (± 1.33%) -0.03s (- 0.97%) 2.69s 2.87s
Total Time 9.67s (± 0.63%) 9.65s (± 0.52%) -0.02s (- 0.22%) 9.57s 9.76s
material-ui - node (v8.9.0, x86)
Memory used 262,230k (± 0.01%) 262,100k (± 0.01%) -129k (- 0.05%) 262,068k 262,174k
Parse Time 2.46s (± 0.39%) 2.44s (± 0.50%) -0.02s (- 0.81%) 2.40s 2.47s
Bind Time 0.68s (± 1.86%) 0.67s (± 1.60%) -0.01s (- 1.18%) 0.65s 0.70s
Check Time 15.65s (± 0.50%) 15.60s (± 0.51%) -0.05s (- 0.35%) 15.35s 15.74s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.78s (± 0.45%) 18.70s (± 0.41%) -0.08s (- 0.43%) 18.46s 18.84s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 39824 10
Baseline master 10

@typescript-bot typescript-bot mentioned this pull request Jul 30, 2020
Open
@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@rbuckton
Copy link
Contributor Author

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 30, 2020 •
edited
Loading

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 4c90ba9. You can monitor the build here.

@rbuckton
Copy link
Contributor Author

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 30, 2020 •
edited
Loading

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 21963ce. You can monitor the build here.

ahejlsberg
ahejlsberg reviewed Jul 30, 2020
View reviewed changes
Copy link
Member

@ahejlsberg ahejlsberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I assume the changes in dbg.ts are just orthogonal, yes?

@rbuckton
Copy link
Contributor Author

The changes in dbg.ts were to fix a circular reference issue that I found while I was investigating the control flow graphs of the original issue and aren't directly related to this fix.

@rbuckton
Copy link
Contributor Author

@ahejlsberg any other feedback? I'd like to get this in for 4.0.1 if its an acceptable change as this issue has already been rescheduled several times.

@rbuckton rbuckton merged commit 32934a9 into master Jul 30, 2020
@jakebailey jakebailey deleted the fix35484 branch November 7, 2022 17:37
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

@rbuckton rbuckton

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.

Generator<T, TReturn>['next'] doesn't narrow properly when using assignment expressions or the comma operator
3 participants
@rbuckton @typescript-bot @ahejlsberg