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

Add errors when providing type arguments for intrinsic JSX tags #40293

Merged
merged 1 commit into from
Sep 8, 2020
Merged

Add errors when providing type arguments for intrinsic JSX tags #40293

merged 1 commit into from
Sep 8, 2020

Conversation

weswigham
Copy link
Member

I was looking over #38515 and came up with a test case that'd fail if it was missing, and discovered that we were also missing an error on having type arguments on intrinsic elements, in both the self-closing and non-self-closing tag cases. Since that case was also the only one where the type arguments weren't checked, by doing the type argument check inline there, I can remove the call from the deferred tag check (which should reduce duplication of work between resolveCall and resolveErrorCall). Now, unfortunately, the exhaustiveness of the type parameter checking isn't guaranteed in any way, so in theory it'd be easy to introduce a branch in the future where the type parameters go unchecked now... but hopefully that won't happen.

So you could say this superceeds #38515 and fixes an unreported bug where we failed to report errors when you passed type parameters to jsx intrinsic tags.

@weswigham weswigham requested a review from sandersn August 27, 2020 22:02
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 27, 2020
@weswigham
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 27, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 27, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 27, 2020 •
edited
Loading

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

Update: The results are in!

@weswigham weswigham mentioned this pull request Aug 27, 2020
Closed
@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..40293

Metric master 40293 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 344,439k (± 0.02%) 344,523k (± 0.02%) +84k (+ 0.02%) 344,328k 344,613k
Parse Time 1.99s (± 0.80%) 1.99s (± 0.56%) +0.00s (+ 0.10%) 1.98s 2.02s
Bind Time 0.82s (± 0.54%) 0.82s (± 0.97%) 0.00s ( 0.00%) 0.81s 0.84s
Check Time 4.76s (± 0.27%) 4.77s (± 0.63%) +0.01s (+ 0.27%) 4.72s 4.86s
Emit Time 5.18s (± 0.68%) 5.17s (± 0.44%) -0.01s (- 0.19%) 5.13s 5.23s
Total Time 12.75s (± 0.37%) 12.75s (± 0.43%) +0.00s (+ 0.03%) 12.68s 12.87s
Monaco - node (v10.16.3, x64)
Memory used 339,281k (± 0.03%) 339,323k (± 0.03%) +42k (+ 0.01%) 339,104k 339,635k
Parse Time 1.55s (± 0.47%) 1.55s (± 0.50%) +0.00s (+ 0.19%) 1.54s 1.57s
Bind Time 0.71s (± 1.19%) 0.71s (± 0.66%) -0.00s (- 0.14%) 0.70s 0.72s
Check Time 4.95s (± 0.55%) 4.96s (± 0.43%) +0.01s (+ 0.24%) 4.92s 5.02s
Emit Time 2.74s (± 0.82%) 2.76s (± 1.04%) +0.03s (+ 0.95%) 2.73s 2.87s
Total Time 9.95s (± 0.45%) 9.99s (± 0.31%) +0.04s (+ 0.39%) 9.93s 10.04s
TFS - node (v10.16.3, x64)
Memory used 302,243k (± 0.02%) 302,294k (± 0.02%) +51k (+ 0.02%) 302,133k 302,483k
Parse Time 1.21s (± 0.78%) 1.21s (± 0.70%) +0.00s (+ 0.17%) 1.19s 1.23s
Bind Time 0.68s (± 0.89%) 0.67s (± 1.32%) -0.01s (- 1.19%) 0.65s 0.69s
Check Time 4.45s (± 0.44%) 4.46s (± 0.75%) +0.01s (+ 0.11%) 4.39s 4.56s
Emit Time 2.90s (± 1.13%) 2.90s (± 0.76%) +0.00s (+ 0.17%) 2.86s 2.94s
Total Time 9.23s (± 0.46%) 9.23s (± 0.51%) +0.00s (+ 0.01%) 9.15s 9.37s
material-ui - node (v10.16.3, x64)
Memory used 460,811k (± 0.02%) 460,584k (± 0.01%) -227k (- 0.05%) 460,440k 460,706k
Parse Time 1.95s (± 0.49%) 1.95s (± 0.30%) -0.00s (- 0.21%) 1.94s 1.96s
Bind Time 0.66s (± 1.16%) 0.66s (± 1.17%) +0.01s (+ 0.92%) 0.64s 0.67s
Check Time 13.46s (± 1.13%) 13.23s (± 0.72%) -0.23s (- 1.71%) 13.01s 13.45s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.07s (± 0.94%) 15.83s (± 0.64%) -0.23s (- 1.45%) 15.61s 16.09s
Angular - node (v12.1.0, x64)
Memory used 321,730k (± 0.03%) 321,707k (± 0.04%) -22k (- 0.01%) 321,506k 322,033k
Parse Time 1.99s (± 0.70%) 1.98s (± 0.60%) -0.01s (- 0.45%) 1.96s 2.01s
Bind Time 0.81s (± 1.11%) 0.81s (± 0.46%) 0.00s ( 0.00%) 0.80s 0.81s
Check Time 4.67s (± 0.65%) 4.66s (± 0.44%) -0.01s (- 0.28%) 4.61s 4.72s
Emit Time 5.34s (± 0.37%) 5.33s (± 0.75%) -0.00s (- 0.07%) 5.28s 5.44s
Total Time 12.80s (± 0.32%) 12.77s (± 0.40%) -0.03s (- 0.22%) 12.69s 12.89s
Monaco - node (v12.1.0, x64)
Memory used 321,606k (± 0.01%) 321,605k (± 0.02%) -1k (- 0.00%) 321,499k 321,833k
Parse Time 1.53s (± 0.65%) 1.53s (± 0.90%) -0.00s (- 0.20%) 1.50s 1.56s
Bind Time 0.69s (± 1.08%) 0.69s (± 0.43%) -0.00s (- 0.14%) 0.69s 0.70s
Check Time 4.76s (± 0.57%) 4.76s (± 0.32%) +0.00s (+ 0.08%) 4.73s 4.80s
Emit Time 2.80s (± 0.49%) 2.80s (± 0.66%) +0.00s (+ 0.14%) 2.77s 2.85s
Total Time 9.78s (± 0.45%) 9.78s (± 0.38%) +0.00s (+ 0.03%) 9.72s 9.86s
TFS - node (v12.1.0, x64)
Memory used 286,588k (± 0.02%) 286,633k (± 0.02%) +45k (+ 0.02%) 286,497k 286,822k
Parse Time 1.22s (± 0.86%) 1.22s (± 1.09%) +0.00s (+ 0.08%) 1.20s 1.25s
Bind Time 0.64s (± 0.77%) 0.64s (± 1.32%) -0.00s (- 0.62%) 0.63s 0.67s
Check Time 4.36s (± 0.35%) 4.35s (± 0.36%) -0.01s (- 0.34%) 4.31s 4.37s
Emit Time 2.93s (± 0.78%) 2.93s (± 1.25%) -0.00s (- 0.07%) 2.85s 3.04s
Total Time 9.16s (± 0.44%) 9.14s (± 0.43%) -0.02s (- 0.24%) 9.07s 9.25s
material-ui - node (v12.1.0, x64)
Memory used 439,202k (± 0.05%) 438,893k (± 0.05%) -309k (- 0.07%) 438,044k 439,175k
Parse Time 1.98s (± 0.83%) 1.97s (± 0.29%) -0.01s (- 0.30%) 1.96s 1.98s
Bind Time 0.63s (± 0.76%) 0.63s (± 1.11%) +0.00s (+ 0.32%) 0.62s 0.65s
Check Time 12.04s (± 1.21%) 11.72s (± 0.56%) -0.32s (- 2.63%) 11.59s 11.92s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.64s (± 1.06%) 14.32s (± 0.43%) -0.32s (- 2.19%) 14.21s 14.49s
Angular - node (v8.9.0, x64)
Memory used 341,022k (± 0.02%) 341,047k (± 0.02%) +25k (+ 0.01%) 340,901k 341,146k
Parse Time 2.54s (± 0.49%) 2.53s (± 0.46%) -0.01s (- 0.31%) 2.50s 2.55s
Bind Time 0.85s (± 0.52%) 0.85s (± 0.65%) 0.00s ( 0.00%) 0.84s 0.86s
Check Time 5.41s (± 0.44%) 5.41s (± 0.47%) +0.01s (+ 0.17%) 5.36s 5.48s
Emit Time 6.06s (± 1.60%) 5.95s (± 2.13%) -0.11s (- 1.80%) 5.66s 6.18s
Total Time 14.86s (± 0.74%) 14.76s (± 1.04%) -0.11s (- 0.71%) 14.39s 15.05s
Monaco - node (v8.9.0, x64)
Memory used 340,628k (± 0.01%) 340,607k (± 0.02%) -21k (- 0.01%) 340,509k 340,734k
Parse Time 1.88s (± 0.32%) 1.87s (± 0.39%) -0.00s (- 0.21%) 1.86s 1.89s
Bind Time 0.89s (± 0.56%) 0.89s (± 0.41%) 0.00s ( 0.00%) 0.88s 0.89s
Check Time 5.49s (± 0.44%) 5.50s (± 0.54%) +0.01s (+ 0.15%) 5.42s 5.58s
Emit Time 3.23s (± 0.76%) 3.21s (± 0.52%) -0.02s (- 0.59%) 3.18s 3.25s
Total Time 11.48s (± 0.38%) 11.46s (± 0.35%) -0.02s (- 0.17%) 11.37s 11.54s
TFS - node (v8.9.0, x64)
Memory used 303,879k (± 0.02%) 303,902k (± 0.01%) +23k (+ 0.01%) 303,855k 303,979k
Parse Time 1.55s (± 0.37%) 1.55s (± 0.43%) 0.00s ( 0.00%) 1.53s 1.56s
Bind Time 0.67s (± 0.99%) 0.67s (± 0.73%) +0.00s (+ 0.15%) 0.66s 0.68s
Check Time 5.20s (± 0.51%) 5.19s (± 0.46%) -0.01s (- 0.25%) 5.13s 5.23s
Emit Time 2.93s (± 0.67%) 2.93s (± 0.98%) +0.01s (+ 0.20%) 2.84s 2.98s
Total Time 10.35s (± 0.33%) 10.34s (± 0.37%) -0.01s (- 0.05%) 10.26s 10.43s
material-ui - node (v8.9.0, x64)
Memory used 465,167k (± 0.01%) 465,040k (± 0.01%) -128k (- 0.03%) 464,989k 465,104k
Parse Time 2.39s (± 0.55%) 2.39s (± 0.50%) -0.00s (- 0.17%) 2.36s 2.42s
Bind Time 0.79s (± 1.28%) 0.78s (± 1.50%) -0.01s (- 0.64%) 0.76s 0.82s
Check Time 18.05s (± 0.74%) 17.85s (± 0.56%) -0.20s (- 1.11%) 17.53s 17.97s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 21.23s (± 0.66%) 21.02s (± 0.46%) -0.21s (- 0.97%) 20.69s 21.12s
Angular - node (v8.9.0, x86)
Memory used 195,717k (± 0.02%) 195,649k (± 0.04%) -68k (- 0.03%) 195,518k 195,841k
Parse Time 2.45s (± 0.56%) 2.45s (± 0.44%) -0.00s (- 0.20%) 2.43s 2.48s
Bind Time 0.99s (± 0.40%) 0.99s (± 0.45%) -0.00s (- 0.10%) 0.98s 1.00s
Check Time 4.87s (± 0.33%) 4.88s (± 0.78%) +0.01s (+ 0.23%) 4.82s 4.99s
Emit Time 5.92s (± 0.36%) 5.93s (± 1.28%) +0.01s (+ 0.10%) 5.76s 6.13s
Total Time 14.24s (± 0.25%) 14.25s (± 0.65%) +0.01s (+ 0.07%) 14.13s 14.47s
Monaco - node (v8.9.0, x86)
Memory used 193,624k (± 0.01%) 193,605k (± 0.02%) -20k (- 0.01%) 193,505k 193,673k
Parse Time 1.91s (± 0.91%) 1.89s (± 0.62%) -0.01s (- 0.79%) 1.87s 1.92s
Bind Time 0.70s (± 0.68%) 0.70s (± 0.49%) -0.01s (- 0.85%) 0.69s 0.70s
Check Time 5.54s (± 1.24%) 5.57s (± 0.60%) +0.03s (+ 0.61%) 5.48s 5.63s
Emit Time 2.71s (± 3.21%) 2.66s (± 0.53%) -0.05s (- 1.92%) 2.63s 2.69s
Total Time 10.85s (± 0.46%) 10.82s (± 0.42%) -0.04s (- 0.37%) 10.68s 10.88s
TFS - node (v8.9.0, x86)
Memory used 173,833k (± 0.03%) 173,849k (± 0.03%) +16k (+ 0.01%) 173,735k 173,981k
Parse Time 1.59s (± 1.22%) 1.60s (± 1.38%) +0.01s (+ 0.38%) 1.57s 1.67s
Bind Time 0.64s (± 0.90%) 0.64s (± 0.52%) -0.00s (- 0.47%) 0.63s 0.65s
Check Time 4.72s (± 0.64%) 4.69s (± 0.35%) -0.03s (- 0.66%) 4.66s 4.72s
Emit Time 2.80s (± 0.58%) 2.80s (± 1.23%) +0.00s (+ 0.04%) 2.69s 2.88s
Total Time 9.75s (± 0.45%) 9.73s (± 0.47%) -0.02s (- 0.24%) 9.64s 9.86s
material-ui - node (v8.9.0, x86)
Memory used 263,394k (± 0.01%) 263,320k (± 0.01%) -73k (- 0.03%) 263,232k 263,395k
Parse Time 2.44s (± 0.82%) 2.44s (± 1.08%) 0.00s ( 0.00%) 2.40s 2.52s
Bind Time 0.68s (± 1.79%) 0.68s (± 1.79%) +0.00s (+ 0.59%) 0.65s 0.70s
Check Time 16.44s (± 0.97%) 16.23s (± 0.49%) -0.21s (- 1.27%) 16.09s 16.51s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.56s (± 0.83%) 19.36s (± 0.45%) -0.21s (- 1.06%) 19.19s 19.63s
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 40293 10
Baseline master 10

@weswigham
Copy link
Member Author

@sandersn is looks like the perf with this change is pretty good for material-ui - does ant-design see a similar improvement?

sandersn
sandersn approved these changes Sep 8, 2020
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Code looks good; a quick test on ant-design (that is, not enough runs for significance) shows a 5% speedup, which I think makes this a clear win.

@weswigham weswigham merged commit 7838b01 into microsoft:master Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

Assignees

@weswigham weswigham

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@weswigham @typescript-bot @sandersn