Skip to content

Fix stack overflow in JSX discriminated union logic #46354

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 2 commits into from
Oct 14, 2021
Merged

Fix stack overflow in JSX discriminated union logic #46354

merged 2 commits into from
Oct 14, 2021

Conversation

ahejlsberg
Copy link
Member

Fixes #46021.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Oct 14, 2021
@ahejlsberg
Copy link
Member Author

I suspect this also fixes #45632, though we don't have a repro for that one.

@andrewbranch
Copy link
Member

Just out of curiosity @typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 14, 2021 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..46354

Metric main 46354 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 354,001k (± 0.02%) 353,938k (± 0.02%) -63k (- 0.02%) 353,732k 354,079k
Parse Time 1.95s (± 0.31%) 1.96s (± 0.66%) +0.01s (+ 0.57%) 1.92s 1.98s
Bind Time 0.84s (± 0.40%) 0.84s (± 0.73%) +0.00s (+ 0.12%) 0.83s 0.86s
Check Time 5.45s (± 0.48%) 5.45s (± 0.29%) -0.00s (- 0.07%) 5.42s 5.49s
Emit Time 5.82s (± 0.48%) 5.85s (± 0.54%) +0.03s (+ 0.57%) 5.78s 5.92s
Total Time 14.05s (± 0.37%) 14.09s (± 0.37%) +0.04s (+ 0.26%) 13.98s 14.20s
Compiler-Unions - node (v10.16.3, x64)
Memory used 203,900k (± 0.03%) 203,875k (± 0.03%) -25k (- 0.01%) 203,722k 204,011k
Parse Time 0.78s (± 0.61%) 0.78s (± 0.97%) +0.00s (+ 0.13%) 0.77s 0.80s
Bind Time 0.52s (± 1.18%) 0.52s (± 1.28%) -0.00s (- 0.19%) 0.51s 0.53s
Check Time 7.93s (± 0.62%) 7.90s (± 0.42%) -0.03s (- 0.42%) 7.82s 7.97s
Emit Time 2.45s (± 2.50%) 2.44s (± 1.06%) -0.00s (- 0.12%) 2.40s 2.52s
Total Time 11.68s (± 0.63%) 11.65s (± 0.25%) -0.03s (- 0.28%) 11.59s 11.71s
Monaco - node (v10.16.3, x64)
Memory used 342,142k (± 0.03%) 342,136k (± 0.02%) -6k (- 0.00%) 341,994k 342,268k
Parse Time 1.48s (± 0.33%) 1.49s (± 0.68%) +0.00s (+ 0.07%) 1.46s 1.51s
Bind Time 0.75s (± 0.82%) 0.75s (± 1.02%) -0.00s (- 0.40%) 0.73s 0.76s
Check Time 5.48s (± 0.45%) 5.45s (± 0.52%) -0.02s (- 0.42%) 5.35s 5.50s
Emit Time 3.15s (± 0.51%) 3.16s (± 0.86%) +0.01s (+ 0.38%) 3.12s 3.24s
Total Time 10.86s (± 0.21%) 10.85s (± 0.25%) -0.01s (- 0.13%) 10.79s 10.90s
TFS - node (v10.16.3, x64)
Memory used 304,811k (± 0.04%) 304,835k (± 0.02%) +24k (+ 0.01%) 304,714k 305,013k
Parse Time 1.19s (± 0.57%) 1.20s (± 0.37%) +0.01s (+ 0.67%) 1.19s 1.21s
Bind Time 0.71s (± 0.95%) 0.71s (± 0.81%) +0.00s (+ 0.42%) 0.70s 0.72s
Check Time 4.96s (± 0.37%) 4.97s (± 0.51%) +0.01s (+ 0.20%) 4.92s 5.02s
Emit Time 3.31s (± 1.87%) 3.34s (± 0.91%) +0.02s (+ 0.72%) 3.28s 3.40s
Total Time 10.17s (± 0.63%) 10.21s (± 0.45%) +0.04s (+ 0.41%) 10.12s 10.29s
material-ui - node (v10.16.3, x64)
Memory used 471,131k (± 0.02%) 471,024k (± 0.01%) -107k (- 0.02%) 470,916k 471,133k
Parse Time 1.77s (± 0.53%) 1.77s (± 0.28%) -0.01s (- 0.39%) 1.76s 1.78s
Bind Time 0.66s (± 1.27%) 0.66s (± 0.79%) +0.00s (+ 0.61%) 0.65s 0.67s
Check Time 14.36s (± 0.44%) 14.35s (± 0.34%) -0.01s (- 0.10%) 14.21s 14.44s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.79s (± 0.36%) 16.78s (± 0.31%) -0.01s (- 0.08%) 16.63s 16.88s
Angular - node (v12.1.0, x64)
Memory used 331,881k (± 0.01%) 331,577k (± 0.12%) -304k (- 0.09%) 330,483k 331,936k
Parse Time 1.93s (± 1.02%) 1.94s (± 0.58%) +0.00s (+ 0.05%) 1.91s 1.96s
Bind Time 0.81s (± 0.76%) 0.82s (± 1.08%) +0.01s (+ 0.74%) 0.80s 0.84s
Check Time 5.29s (± 0.40%) 5.33s (± 0.79%) +0.05s (+ 0.87%) 5.26s 5.46s
Emit Time 6.12s (± 0.64%) 6.10s (± 0.50%) -0.02s (- 0.28%) 6.06s 6.21s
Total Time 14.15s (± 0.34%) 14.19s (± 0.49%) +0.04s (+ 0.28%) 14.05s 14.36s
Compiler-Unions - node (v12.1.0, x64)
Memory used 191,419k (± 0.05%) 191,359k (± 0.03%) -61k (- 0.03%) 191,220k 191,502k
Parse Time 0.78s (± 0.67%) 0.78s (± 1.08%) 0.00s ( 0.00%) 0.77s 0.80s
Bind Time 0.53s (± 0.71%) 0.53s (± 1.10%) +0.00s (+ 0.38%) 0.51s 0.54s
Check Time 7.42s (± 0.65%) 7.40s (± 0.65%) -0.02s (- 0.24%) 7.30s 7.51s
Emit Time 2.44s (± 0.54%) 2.46s (± 0.68%) +0.02s (+ 0.70%) 2.43s 2.50s
Total Time 11.17s (± 0.42%) 11.16s (± 0.55%) -0.00s (- 0.01%) 11.04s 11.27s
Monaco - node (v12.1.0, x64)
Memory used 325,193k (± 0.02%) 325,126k (± 0.02%) -66k (- 0.02%) 325,001k 325,240k
Parse Time 1.46s (± 0.72%) 1.47s (± 0.57%) +0.01s (+ 0.34%) 1.45s 1.49s
Bind Time 0.73s (± 0.61%) 0.73s (± 0.71%) +0.00s (+ 0.14%) 0.72s 0.74s
Check Time 5.33s (± 0.42%) 5.33s (± 0.30%) -0.01s (- 0.15%) 5.28s 5.35s
Emit Time 3.19s (± 0.74%) 3.19s (± 0.90%) +0.00s (+ 0.06%) 3.14s 3.26s
Total Time 10.71s (± 0.40%) 10.72s (± 0.42%) +0.00s (+ 0.04%) 10.64s 10.80s
TFS - node (v12.1.0, x64)
Memory used 289,492k (± 0.02%) 289,428k (± 0.02%) -64k (- 0.02%) 289,285k 289,557k
Parse Time 1.21s (± 0.51%) 1.21s (± 0.60%) -0.00s (- 0.25%) 1.19s 1.22s
Bind Time 0.69s (± 0.81%) 0.69s (± 0.64%) +0.00s (+ 0.58%) 0.68s 0.70s
Check Time 4.90s (± 0.50%) 4.91s (± 0.39%) +0.01s (+ 0.18%) 4.87s 4.95s
Emit Time 3.38s (± 0.86%) 3.36s (± 1.23%) -0.02s (- 0.65%) 3.28s 3.49s
Total Time 10.18s (± 0.42%) 10.17s (± 0.47%) -0.01s (- 0.09%) 10.09s 10.33s
material-ui - node (v12.1.0, x64)
Memory used 449,663k (± 0.05%) 449,804k (± 0.01%) +141k (+ 0.03%) 449,664k 449,904k
Parse Time 1.78s (± 0.69%) 1.77s (± 0.59%) -0.00s (- 0.28%) 1.76s 1.81s
Bind Time 0.64s (± 1.30%) 0.64s (± 0.91%) +0.00s (+ 0.16%) 0.63s 0.65s
Check Time 13.01s (± 1.10%) 12.90s (± 0.61%) -0.12s (- 0.89%) 12.71s 13.10s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.43s (± 0.95%) 15.31s (± 0.54%) -0.12s (- 0.77%) 15.12s 15.51s
Angular - node (v14.15.1, x64)
Memory used 330,231k (± 0.00%) 330,243k (± 0.01%) +12k (+ 0.00%) 330,199k 330,291k
Parse Time 1.94s (± 0.36%) 1.94s (± 0.57%) +0.01s (+ 0.31%) 1.93s 1.97s
Bind Time 0.86s (± 0.40%) 0.86s (± 0.43%) -0.00s (- 0.23%) 0.85s 0.86s
Check Time 5.31s (± 0.29%) 5.34s (± 0.38%) +0.03s (+ 0.55%) 5.30s 5.39s
Emit Time 6.20s (± 2.86%) 6.12s (± 0.62%) -0.09s (- 1.39%) 6.03s 6.20s
Total Time 14.31s (± 1.25%) 14.26s (± 0.34%) -0.05s (- 0.35%) 14.17s 14.37s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,892k (± 0.36%) 193,181k (± 0.02%) +290k (+ 0.15%) 193,099k 193,269k
Parse Time 0.81s (± 0.58%) 0.81s (± 0.80%) +0.00s (+ 0.25%) 0.80s 0.83s
Bind Time 0.55s (± 0.66%) 0.55s (± 0.62%) -0.00s (- 0.18%) 0.55s 0.56s
Check Time 7.51s (± 0.59%) 7.51s (± 0.40%) +0.00s (+ 0.03%) 7.45s 7.59s
Emit Time 2.45s (± 0.81%) 2.44s (± 0.45%) -0.01s (- 0.29%) 2.41s 2.46s
Total Time 11.32s (± 0.53%) 11.31s (± 0.38%) -0.00s (- 0.03%) 11.23s 11.43s
Monaco - node (v14.15.1, x64)
Memory used 324,002k (± 0.00%) 323,995k (± 0.01%) -7k (- 0.00%) 323,956k 324,040k
Parse Time 1.50s (± 0.50%) 1.50s (± 0.50%) 0.00s ( 0.00%) 1.49s 1.52s
Bind Time 0.76s (± 0.63%) 0.76s (± 0.48%) -0.00s (- 0.13%) 0.75s 0.76s
Check Time 5.32s (± 0.44%) 5.29s (± 0.46%) -0.03s (- 0.58%) 5.25s 5.37s
Emit Time 3.23s (± 0.98%) 3.24s (± 0.70%) +0.01s (+ 0.28%) 3.19s 3.29s
Total Time 10.81s (± 0.48%) 10.79s (± 0.36%) -0.02s (- 0.22%) 10.71s 10.88s
TFS - node (v14.15.1, x64)
Memory used 288,353k (± 0.01%) 288,341k (± 0.01%) -11k (- 0.00%) 288,301k 288,391k
Parse Time 1.23s (± 0.89%) 1.23s (± 0.84%) -0.00s (- 0.00%) 1.21s 1.26s
Bind Time 0.73s (± 0.81%) 0.73s (± 0.79%) -0.00s (- 0.54%) 0.72s 0.74s
Check Time 4.91s (± 0.44%) 4.90s (± 0.34%) -0.01s (- 0.29%) 4.87s 4.94s
Emit Time 3.46s (± 0.70%) 3.46s (± 0.42%) -0.00s (- 0.14%) 3.43s 3.50s
Total Time 10.34s (± 0.39%) 10.32s (± 0.22%) -0.02s (- 0.18%) 10.29s 10.39s
material-ui - node (v14.15.1, x64)
Memory used 448,058k (± 0.00%) 447,962k (± 0.06%) -96k (- 0.02%) 446,819k 448,130k
Parse Time 1.82s (± 0.71%) 1.82s (± 0.48%) +0.00s (+ 0.17%) 1.80s 1.84s
Bind Time 0.67s (± 0.55%) 0.68s (± 0.44%) +0.00s (+ 0.44%) 0.67s 0.68s
Check Time 13.10s (± 0.73%) 13.08s (± 0.96%) -0.03s (- 0.21%) 12.90s 13.52s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.60s (± 0.67%) 15.58s (± 0.77%) -0.02s (- 0.13%) 15.40s 16.00s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory6 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
Benchmark Name Iterations
Current 46354 10
Baseline main 10

Developer Information:

Download Benchmark

@andrewbranch
Copy link
Member

What exactly was happening here?

@ahejlsberg
Copy link
Member Author

It was a case of the contextual type for an expression depending on the expression itself--which is never supposed to happen. The circularity only happened for a JSX element with a discriminated union type.

@andrewbranch
Copy link
Member

I got that from the code change, but I couldn’t quite grok the circular dependency in the test case.

@ahejlsberg
Copy link
Member Author

In return <ListItem variant={listItemVariant}/>, the type of ListItem is a union type. This causes us to attempt to narrow the union type by a discriminant, and variant is a discriminant property. We then try to get the type of listItemVariant, which in getNarrowableTypeFromReference qualifies for a check of the contextual type to see if we should lift its constraint to enable narrowing by CFA. But the attempt to get the contextual type eventually gets us back to needing the type of ListItem. And around it goes. With the switch to getContextFreeTypeOfExpression, we break the circularity because getNarrowableTypeFromReference will no longer see a contextual type.

andrewbranch
andrewbranch approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation!

@ahejlsberg ahejlsberg merged commit 8718df3 into main Oct 14, 2021
@ahejlsberg ahejlsberg deleted the fix46021 branch October 14, 2021 17:11
@ahejlsberg ahejlsberg mentioned this pull request Oct 14, 2021
Closed
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@ahejlsberg @mprobst
Fix stack overflow in JSX discriminated union logic (microsoft#46354)
420c788 
* Use getContextFreeTypeOfExpression to avoid circularities

* Add regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewbranch andrewbranch andrewbranch approved these changes

Assignees

@ahejlsberg ahejlsberg

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.

Maximum call stack size exceeded when compiling tsx
3 participants
@ahejlsberg @andrewbranch @typescript-bot