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

Fix crash when serializing the return type of a generic call to Array.prototype.flat #38904

Merged
merged 4 commits into from
Jun 15, 2020
Merged

Fix crash when serializing the return type of a generic call to Array.prototype.flat #38904

merged 4 commits into from
Jun 15, 2020

Conversation

weswigham
Copy link
Member

Fixes #38298

This fix is two-pronged:

  • First, node serialization now has a guard during conditional type serialization that issues an error if it attempts to serialize a union within itself (and therefore will repeat endlessly).
  • Second, to reduce the likelihood of such a scenario occurring in the first place, we now preserve alias symbols on generic indexed accesses, and on the resulting unions or intersections instantiating those indexed accesses may produce. This creates a number of improvements to our .types and error message baselines where many complex types got reduced to SomeAlias<T> or the like. That should be an improvement for many situations outside of the crashing one.

@weswigham weswigham requested a review from sandersn June 2, 2020 21:12
@weswigham
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2020 •
edited
Loading

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

Update: The results are in!

@typescript-bot typescript-bot mentioned this pull request Jun 2, 2020
Closed
@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.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..38904

Metric master 38904 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 328,367k (± 0.03%) 327,951k (± 0.02%) -416k (- 0.13%) 327,773k 328,079k
Parse Time 1.65s (± 0.30%) 1.65s (± 0.71%) +0.01s (+ 0.36%) 1.62s 1.67s
Bind Time 0.90s (± 0.81%) 0.91s (± 1.09%) +0.01s (+ 0.67%) 0.88s 0.93s
Check Time 4.86s (± 0.33%) 4.87s (± 0.36%) +0.01s (+ 0.23%) 4.83s 4.90s
Emit Time 5.43s (± 0.60%) 5.40s (± 0.45%) -0.03s (- 0.59%) 5.33s 5.45s
Total Time 12.84s (± 0.35%) 12.83s (± 0.37%) -0.01s (- 0.06%) 12.72s 12.91s
Monaco - node (v10.16.3, x64)
Memory used 327,355k (± 0.02%) 327,389k (± 0.02%) +34k (+ 0.01%) 327,269k 327,516k
Parse Time 1.27s (± 0.65%) 1.28s (± 0.73%) +0.01s (+ 0.71%) 1.26s 1.30s
Bind Time 0.79s (± 0.66%) 0.79s (± 0.70%) +0.00s (+ 0.38%) 0.78s 0.80s
Check Time 4.88s (± 0.54%) 4.91s (± 0.60%) +0.04s (+ 0.72%) 4.86s 4.99s
Emit Time 2.95s (± 0.68%) 2.97s (± 1.29%) +0.02s (+ 0.81%) 2.89s 3.08s
Total Time 9.89s (± 0.43%) 9.96s (± 0.61%) +0.07s (+ 0.73%) 9.86s 10.09s
TFS - node (v10.16.3, x64)
Memory used 292,454k (± 0.02%) 292,477k (± 0.01%) +22k (+ 0.01%) 292,406k 292,542k
Parse Time 0.96s (± 0.62%) 0.97s (± 0.62%) +0.00s (+ 0.00%) 0.95s 0.98s
Bind Time 0.76s (± 0.39%) 0.76s (± 0.85%) +0.00s (+ 0.53%) 0.75s 0.78s
Check Time 4.40s (± 0.58%) 4.42s (± 0.46%) +0.01s (+ 0.32%) 4.37s 4.46s
Emit Time 3.10s (± 0.50%) 3.08s (± 0.85%) -0.02s (- 0.58%) 3.00s 3.12s
Total Time 9.23s (± 0.34%) 9.23s (± 0.36%) +0.00s (+ 0.03%) 9.12s 9.29s
material-ui - node (v10.16.3, x64)
Memory used 449,999k (± 0.01%) 449,729k (± 0.01%) -270k (- 0.06%) 449,578k 449,905k
Parse Time 1.79s (± 0.34%) 1.79s (± 0.37%) +0.00s (+ 0.22%) 1.78s 1.81s
Bind Time 0.70s (± 0.57%) 0.70s (± 0.63%) +0.00s (+ 0.14%) 0.69s 0.71s
Check Time 12.91s (± 0.97%) 12.94s (± 1.10%) +0.03s (+ 0.19%) 12.73s 13.23s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.40s (± 0.80%) 15.43s (± 0.94%) +0.03s (+ 0.19%) 15.23s 15.74s
Angular - node (v12.1.0, x64)
Memory used 303,913k (± 0.03%) 303,470k (± 0.03%) -443k (- 0.15%) 303,281k 303,652k
Parse Time 1.60s (± 0.62%) 1.60s (± 0.86%) +0.01s (+ 0.38%) 1.57s 1.63s
Bind Time 0.88s (± 0.41%) 0.89s (± 1.01%) +0.00s (+ 0.23%) 0.86s 0.90s
Check Time 4.73s (± 0.70%) 4.73s (± 0.52%) -0.01s (- 0.13%) 4.69s 4.78s
Emit Time 5.56s (± 1.03%) 5.56s (± 0.59%) -0.01s (- 0.11%) 5.50s 5.65s
Total Time 12.78s (± 0.61%) 12.78s (± 0.43%) -0.01s (- 0.04%) 12.69s 12.88s
Monaco - node (v12.1.0, x64)
Memory used 307,272k (± 0.01%) 307,323k (± 0.02%) +52k (+ 0.02%) 307,156k 307,433k
Parse Time 1.23s (± 0.71%) 1.22s (± 0.49%) -0.00s (- 0.33%) 1.21s 1.24s
Bind Time 0.76s (± 1.05%) 0.76s (± 0.79%) -0.01s (- 0.79%) 0.74s 0.77s
Check Time 4.69s (± 0.44%) 4.71s (± 0.63%) +0.01s (+ 0.32%) 4.64s 4.78s
Emit Time 2.99s (± 0.96%) 3.00s (± 0.94%) +0.01s (+ 0.30%) 2.95s 3.08s
Total Time 9.68s (± 0.45%) 9.69s (± 0.59%) +0.01s (+ 0.13%) 9.55s 9.82s
TFS - node (v12.1.0, x64)
Memory used 274,696k (± 0.02%) 274,779k (± 0.01%) +83k (+ 0.03%) 274,696k 274,822k
Parse Time 0.93s (± 0.60%) 0.94s (± 0.79%) +0.01s (+ 0.86%) 0.92s 0.95s
Bind Time 0.74s (± 0.90%) 0.74s (± 2.50%) -0.00s (- 0.27%) 0.72s 0.81s
Check Time 4.30s (± 0.41%) 4.32s (± 0.46%) +0.02s (+ 0.54%) 4.27s 4.38s
Emit Time 3.10s (± 1.08%) 3.11s (± 0.82%) +0.01s (+ 0.29%) 3.04s 3.15s
Total Time 9.07s (± 0.52%) 9.11s (± 0.49%) +0.04s (+ 0.46%) 9.03s 9.23s
material-ui - node (v12.1.0, x64)
Memory used 427,320k (± 0.05%) 427,075k (± 0.05%) -245k (- 0.06%) 426,279k 427,447k
Parse Time 1.76s (± 0.43%) 1.77s (± 0.81%) +0.01s (+ 0.34%) 1.74s 1.80s
Bind Time 0.64s (± 1.06%) 0.65s (± 0.51%) +0.01s (+ 0.93%) 0.64s 0.66s
Check Time 11.44s (± 0.84%) 11.48s (± 0.62%) +0.04s (+ 0.38%) 11.33s 11.64s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 13.84s (± 0.67%) 13.90s (± 0.55%) +0.06s (+ 0.40%) 13.73s 14.08s
Angular - node (v8.9.0, x64)
Memory used 323,207k (± 0.02%) 322,856k (± 0.02%) -351k (- 0.11%) 322,672k 323,051k
Parse Time 2.12s (± 0.46%) 2.13s (± 0.63%) +0.01s (+ 0.38%) 2.10s 2.16s
Bind Time 0.94s (± 0.96%) 0.94s (± 1.22%) -0.00s (- 0.32%) 0.93s 0.98s
Check Time 5.53s (± 1.02%) 5.46s (± 1.64%) -0.08s (- 1.41%) 5.29s 5.63s
Emit Time 6.31s (± 1.14%) 6.35s (± 0.99%) +0.05s (+ 0.76%) 6.27s 6.55s
Total Time 14.90s (± 0.40%) 14.88s (± 0.50%) -0.03s (- 0.17%) 14.69s 15.05s
Monaco - node (v8.9.0, x64)
Memory used 325,943k (± 0.01%) 326,019k (± 0.01%) +76k (+ 0.02%) 325,949k 326,075k
Parse Time 1.56s (± 0.49%) 1.56s (± 0.68%) +0.01s (+ 0.51%) 1.55s 1.59s
Bind Time 0.92s (± 1.35%) 0.92s (± 1.31%) -0.01s (- 0.65%) 0.90s 0.96s
Check Time 5.49s (± 0.70%) 5.49s (± 0.62%) +0.00s (+ 0.09%) 5.40s 5.57s
Emit Time 3.50s (± 0.65%) 3.47s (± 0.70%) -0.03s (- 0.83%) 3.40s 3.52s
Total Time 11.47s (± 0.47%) 11.44s (± 0.45%) -0.03s (- 0.23%) 11.33s 11.56s
TFS - node (v8.9.0, x64)
Memory used 292,052k (± 0.02%) 292,101k (± 0.01%) +49k (+ 0.02%) 292,024k 292,197k
Parse Time 1.26s (± 0.67%) 1.25s (± 0.47%) -0.01s (- 0.56%) 1.24s 1.27s
Bind Time 0.75s (± 0.79%) 0.76s (± 1.18%) +0.00s (+ 0.27%) 0.74s 0.78s
Check Time 5.01s (± 1.60%) 5.12s (± 1.39%) +0.11s (+ 2.14%) 4.93s 5.21s
Emit Time 3.28s (± 2.49%) 3.20s (± 2.39%) -0.08s (- 2.41%) 3.12s 3.40s
Total Time 10.30s (± 0.50%) 10.33s (± 0.41%) +0.03s (+ 0.27%) 10.27s 10.43s
material-ui - node (v8.9.0, x64)
Memory used 452,949k (± 0.01%) 452,700k (± 0.02%) -248k (- 0.05%) 452,475k 452,918k
Parse Time 2.12s (± 0.50%) 2.12s (± 0.46%) -0.00s (- 0.09%) 2.11s 2.15s
Bind Time 0.82s (± 0.81%) 0.82s (± 1.24%) +0.00s (+ 0.12%) 0.80s 0.85s
Check Time 16.86s (± 1.01%) 16.85s (± 0.53%) -0.00s (- 0.02%) 16.60s 17.02s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.80s (± 0.86%) 19.79s (± 0.44%) -0.01s (- 0.04%) 19.58s 19.96s
Angular - node (v8.9.0, x86)
Memory used 186,241k (± 0.02%) 186,064k (± 0.03%) -177k (- 0.10%) 185,962k 186,185k
Parse Time 2.06s (± 0.67%) 2.07s (± 0.45%) +0.01s (+ 0.39%) 2.06s 2.10s
Bind Time 1.09s (± 1.12%) 1.09s (± 0.77%) +0.00s (+ 0.18%) 1.08s 1.11s
Check Time 5.06s (± 0.58%) 5.07s (± 0.40%) +0.01s (+ 0.30%) 5.02s 5.12s
Emit Time 6.07s (± 0.68%) 6.08s (± 0.84%) +0.01s (+ 0.15%) 5.92s 6.16s
Total Time 14.28s (± 0.49%) 14.32s (± 0.39%) +0.03s (+ 0.24%) 14.18s 14.41s
Monaco - node (v8.9.0, x86)
Memory used 185,806k (± 0.02%) 185,787k (± 0.02%) -19k (- 0.01%) 185,715k 185,892k
Parse Time 1.60s (± 0.64%) 1.60s (± 0.47%) -0.00s (- 0.13%) 1.58s 1.62s
Bind Time 0.77s (± 0.75%) 0.77s (± 0.52%) -0.00s (- 0.26%) 0.76s 0.78s
Check Time 5.48s (± 0.39%) 5.51s (± 0.47%) +0.04s (+ 0.64%) 5.46s 5.59s
Emit Time 2.90s (± 0.68%) 2.89s (± 0.79%) -0.00s (- 0.14%) 2.84s 2.95s
Total Time 10.75s (± 0.36%) 10.78s (± 0.22%) +0.03s (+ 0.26%) 10.72s 10.83s
TFS - node (v8.9.0, x86)
Memory used 167,389k (± 0.02%) 167,390k (± 0.02%) +2k (+ 0.00%) 167,318k 167,455k
Parse Time 1.29s (± 0.68%) 1.29s (± 0.45%) 0.00s ( 0.00%) 1.28s 1.30s
Bind Time 0.72s (± 1.11%) 0.72s (± 0.72%) -0.00s (- 0.14%) 0.71s 0.73s
Check Time 4.71s (± 0.59%) 4.72s (± 0.77%) +0.02s (+ 0.32%) 4.65s 4.82s
Emit Time 2.98s (± 0.78%) 2.99s (± 2.62%) +0.01s (+ 0.34%) 2.89s 3.25s
Total Time 9.70s (± 0.40%) 9.72s (± 0.85%) +0.02s (+ 0.24%) 9.60s 9.98s
material-ui - node (v8.9.0, x86)
Memory used 256,662k (± 0.01%) 256,526k (± 0.02%) -135k (- 0.05%) 256,422k 256,628k
Parse Time 2.18s (± 0.58%) 2.18s (± 0.60%) 0.00s ( 0.00%) 2.16s 2.21s
Bind Time 0.70s (± 1.63%) 0.70s (± 1.58%) +0.01s (+ 0.86%) 0.68s 0.74s
Check Time 15.55s (± 0.89%) 15.52s (± 0.74%) -0.03s (- 0.19%) 15.24s 15.80s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.43s (± 0.76%) 18.41s (± 0.64%) -0.02s (- 0.12%) 18.11s 18.67s
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 38904 10
Baseline master 10

@weswigham
Copy link
Member Author

RWC and user all look good. Perf is fine. DT needs some text changes to webpack-config-utils, but otherwise is unaffected. Everything looks good. @RyanCavanaugh since this was a crash bug, did we want to backport this for the next servicing release?

andrewbranch
andrewbranch reviewed Jun 3, 2020
View reviewed changes
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
@weswigham weswigham requested a review from andrewbranch June 3, 2020 20:05
@weswigham weswigham mentioned this pull request Jun 8, 2020
Closed
@weswigham weswigham merged commit 6a777ff into microsoft:master Jun 15, 2020
@weswigham
Copy link
Member Author

@typescript-bot cherry-pick this to release-3.9

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 15, 2020 •
edited
Loading

Heya @weswigham, I've started to run the task to cherry-pick this into release-3.9 on this PR at e6ccbbd. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @weswigham, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-3.9 manually.

weswigham added a commit to weswigham/TypeScript that referenced this pull request Jun 15, 2020
@weswigham
Fix crash when serializing the return type of a generic call to Array…
d82279e 
….prototype.flat (microsoft#38904)

* Add declaration emit error and checking for circularly referential unions produced by recursive conditionals

* Allow indexed accesses to produce alias symbols on types

* Add test that still triggers the declaration emit error

* Fix spelling
@weswigham weswigham mentioned this pull request Jun 15, 2020
Merged
weswigham added a commit that referenced this pull request Jun 15, 2020
@weswigham
Fix crash when serializing the return type of a generic call to Array…
986e9dd 
….prototype.flat (#38904) (#39079)

* Add declaration emit error and checking for circularly referential unions produced by recursive conditionals

* Allow indexed accesses to produce alias symbols on types

* Add test that still triggers the declaration emit error

* Fix spelling
amcasey added a commit to amcasey/TypeScript that referenced this pull request Jul 7, 2020
@amcasey
Revert "Fix crash when serializing the return type of a generic call …
5400152 
…to Array.prototype.flat (microsoft#38904) (microsoft#39079)"

This reverts commit 986e9dd.
@amcasey amcasey mentioned this pull request Jul 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanCavanaugh RyanCavanaugh RyanCavanaugh approved these changes

@andrewbranch andrewbranch andrewbranch approved these changes

@sandersn sandersn Awaiting requested review from sandersn

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

Assignees

@weswigham weswigham

Labels
Author: Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RangeError when trying to infer type for Array.prototype.flat()
4 participants
@weswigham @typescript-bot @andrewbranch @RyanCavanaugh