Skip to content

Better checking of @param/@property tags #39487

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
Jul 8, 2020
Merged

Better checking of @param/@property tags #39487

merged 2 commits into from
Jul 8, 2020

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jul 7, 2020

  1. Use getWidenedTypeForVariableLikeDeclaration, instead of directly calling tryGetTypeFromEffectiveTypeNode. This requires some changes in the former function since it can't assume that the declaration has an initializer. See below for details.
  2. isOptional now calls isOptionalJSDocPropertyLikeTag.
  3. isOptionalJSDocPropertyLikeTag now handles JSDocPropertyTag (previously it was named isOptionalJSDocParameterTag).

getWidenedTypeForVariableLikeDeclaration now handles @param and @property tags. I switched a couple of places to use type guards and added a call to hasExpressionInitializer. Then, to avoid calling it twice, I moved a js-only check inside that block. I'm not certain it's the right thing -- the code is cleaner now, but I could have left it as-is and added casts instead. If performance is worse it's the first thing I'm going to try undoing.

Fixes #39111

sandersn added 2 commits July 7, 2020 15:27
@sandersn
More consistent checking of @property/@param
3fef517 
1. Use getWidenedTypeForVariableLikeDeclaration, instead of directly
calling tryGetTypeFromEffectiveTypeNode. This requires some changes in
the former function since it can't assume that the declaration has an
initializer.
2. isOptional now calls isOptionalJSDocPropertyLikeTag.
3. isOptionalJSDocPropertyLikeTag now handles JSDocPropertyTag
(previously it was named isOptionalJSDocParameterTag).
@sandersn
rename to isOptionalJSDocPropertyLikeTag
c11528a
@sandersn sandersn requested review from elibarzilay and weswigham July 7, 2020 22:41
@sandersn
Copy link
Member Author

sandersn commented Jul 7, 2020

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2020 •
edited
Loading

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

Update: The results are in!

weswigham
weswigham approved these changes Jul 7, 2020
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Yeah, I agree that the re-sequencing looks nicer. The isInJSFile check should be cheap since it's just a node flag check nowadays, anyway.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..39487

Metric master 39487 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 343,883k (± 0.03%) 343,431k (± 0.02%) -452k (- 0.13%) 343,315k 343,529k
Parse Time 2.00s (± 0.83%) 2.01s (± 0.60%) +0.01s (+ 0.35%) 1.98s 2.03s
Bind Time 0.82s (± 0.61%) 0.82s (± 0.73%) 0.00s ( 0.00%) 0.80s 0.83s
Check Time 4.72s (± 0.63%) 4.73s (± 0.69%) +0.01s (+ 0.23%) 4.66s 4.81s
Emit Time 5.17s (± 0.61%) 5.20s (± 0.78%) +0.03s (+ 0.64%) 5.13s 5.32s
Total Time 12.71s (± 0.42%) 12.76s (± 0.55%) +0.05s (+ 0.39%) 12.63s 12.97s
Monaco - node (v10.16.3, x64)
Memory used 339,128k (± 0.02%) 339,056k (± 0.02%) -71k (- 0.02%) 338,942k 339,240k
Parse Time 1.57s (± 0.42%) 1.58s (± 0.44%) +0.01s (+ 0.57%) 1.56s 1.59s
Bind Time 0.71s (± 1.15%) 0.71s (± 0.63%) -0.00s (- 0.14%) 0.70s 0.72s
Check Time 4.86s (± 0.46%) 4.87s (± 0.37%) +0.01s (+ 0.27%) 4.83s 4.91s
Emit Time 2.73s (± 0.43%) 2.74s (± 0.41%) +0.01s (+ 0.29%) 2.71s 2.76s
Total Time 9.87s (± 0.35%) 9.91s (± 0.18%) +0.03s (+ 0.31%) 9.86s 9.94s
TFS - node (v10.16.3, x64)
Memory used 301,920k (± 0.02%) 302,054k (± 0.03%) +134k (+ 0.04%) 301,720k 302,210k
Parse Time 1.21s (± 1.04%) 1.21s (± 0.62%) +0.00s (+ 0.33%) 1.20s 1.23s
Bind Time 0.67s (± 0.33%) 0.67s (± 0.83%) -0.00s (- 0.15%) 0.66s 0.68s
Check Time 4.37s (± 0.73%) 4.38s (± 0.53%) +0.01s (+ 0.30%) 4.33s 4.45s
Emit Time 2.90s (± 1.05%) 2.88s (± 1.27%) -0.02s (- 0.65%) 2.79s 2.96s
Total Time 9.15s (± 0.49%) 9.14s (± 0.60%) -0.00s (- 0.03%) 9.03s 9.29s
material-ui - node (v10.16.3, x64)
Memory used 459,424k (± 0.01%) 459,105k (± 0.02%) -318k (- 0.07%) 458,958k 459,290k
Parse Time 2.05s (± 0.33%) 2.04s (± 0.67%) -0.01s (- 0.63%) 2.02s 2.07s
Bind Time 0.64s (± 1.57%) 0.65s (± 1.23%) +0.00s (+ 0.47%) 0.63s 0.66s
Check Time 12.86s (± 0.48%) 12.88s (± 0.61%) +0.02s (+ 0.14%) 12.75s 13.10s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.56s (± 0.43%) 15.57s (± 0.57%) +0.01s (+ 0.06%) 15.44s 15.83s
Angular - node (v12.1.0, x64)
Memory used 321,147k (± 0.03%) 320,723k (± 0.03%) -424k (- 0.13%) 320,504k 320,877k
Parse Time 1.99s (± 0.38%) 1.99s (± 0.44%) -0.01s (- 0.30%) 1.97s 2.01s
Bind Time 0.80s (± 0.59%) 0.80s (± 0.74%) +0.00s (+ 0.62%) 0.79s 0.82s
Check Time 4.63s (± 0.44%) 4.63s (± 0.55%) -0.00s (- 0.06%) 4.57s 4.67s
Emit Time 5.35s (± 0.56%) 5.37s (± 0.54%) +0.01s (+ 0.28%) 5.31s 5.45s
Total Time 12.77s (± 0.25%) 12.79s (± 0.26%) +0.01s (+ 0.09%) 12.71s 12.89s
Monaco - node (v12.1.0, x64)
Memory used 321,514k (± 0.02%) 321,596k (± 0.01%) +82k (+ 0.03%) 321,493k 321,714k
Parse Time 1.56s (± 0.88%) 1.54s (± 0.90%) -0.02s (- 1.35%) 1.50s 1.56s
Bind Time 0.69s (± 0.48%) 0.69s (± 0.43%) +0.00s (+ 0.29%) 0.69s 0.70s
Check Time 4.67s (± 0.71%) 4.67s (± 0.56%) +0.00s (+ 0.06%) 4.62s 4.74s
Emit Time 2.78s (± 0.63%) 2.79s (± 0.71%) +0.01s (+ 0.22%) 2.75s 2.85s
Total Time 9.70s (± 0.45%) 9.69s (± 0.46%) -0.01s (- 0.10%) 9.61s 9.83s
TFS - node (v12.1.0, x64)
Memory used 286,564k (± 0.02%) 286,581k (± 0.02%) +16k (+ 0.01%) 286,402k 286,690k
Parse Time 1.23s (± 0.69%) 1.23s (± 0.72%) +0.00s (+ 0.16%) 1.22s 1.26s
Bind Time 0.65s (± 1.47%) 0.65s (± 1.06%) -0.00s (- 0.15%) 0.63s 0.66s
Check Time 4.29s (± 0.52%) 4.29s (± 0.56%) +0.00s (+ 0.02%) 4.25s 4.34s
Emit Time 2.92s (± 0.64%) 2.95s (± 1.03%) +0.03s (+ 1.13%) 2.90s 3.04s
Total Time 9.09s (± 0.42%) 9.13s (± 0.46%) +0.04s (+ 0.43%) 9.03s 9.22s
material-ui - node (v12.1.0, x64)
Memory used 437,757k (± 0.01%) 437,541k (± 0.01%) -216k (- 0.05%) 437,444k 437,612k
Parse Time 2.03s (± 0.51%) 2.02s (± 0.44%) -0.00s (- 0.20%) 2.00s 2.04s
Bind Time 0.63s (± 1.03%) 0.63s (± 0.71%) 0.00s ( 0.00%) 0.62s 0.64s
Check Time 11.55s (± 0.77%) 11.62s (± 0.80%) +0.07s (+ 0.61%) 11.45s 11.91s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.20s (± 0.62%) 14.27s (± 0.64%) +0.06s (+ 0.46%) 14.11s 14.55s
Angular - node (v8.9.0, x64)
Memory used 340,433k (± 0.01%) 340,050k (± 0.02%) -383k (- 0.11%) 339,912k 340,182k
Parse Time 2.54s (± 0.38%) 2.52s (± 0.30%) -0.01s (- 0.43%) 2.50s 2.54s
Bind Time 0.85s (± 0.61%) 0.85s (± 0.65%) +0.00s (+ 0.35%) 0.84s 0.86s
Check Time 5.37s (± 0.69%) 5.38s (± 0.67%) +0.01s (+ 0.19%) 5.32s 5.48s
Emit Time 5.93s (± 1.50%) 5.92s (± 1.03%) -0.00s (- 0.02%) 5.80s 6.06s
Total Time 14.69s (± 0.76%) 14.68s (± 0.52%) -0.00s (- 0.02%) 14.55s 14.89s
Monaco - node (v8.9.0, x64)
Memory used 340,475k (± 0.01%) 340,496k (± 0.02%) +21k (+ 0.01%) 340,331k 340,613k
Parse Time 1.88s (± 0.50%) 1.87s (± 0.51%) -0.00s (- 0.16%) 1.86s 1.90s
Bind Time 0.88s (± 0.66%) 0.88s (± 0.39%) +0.00s (+ 0.00%) 0.88s 0.89s
Check Time 5.39s (± 0.58%) 5.42s (± 0.66%) +0.03s (+ 0.52%) 5.34s 5.52s
Emit Time 3.25s (± 0.72%) 3.25s (± 0.88%) -0.01s (- 0.25%) 3.20s 3.33s
Total Time 11.40s (± 0.31%) 11.42s (± 0.38%) +0.02s (+ 0.18%) 11.32s 11.49s
TFS - node (v8.9.0, x64)
Memory used 303,809k (± 0.03%) 303,809k (± 0.02%) -1k (- 0.00%) 303,711k 303,949k
Parse Time 1.56s (± 2.07%) 1.54s (± 0.58%) -0.02s (- 1.21%) 1.53s 1.57s
Bind Time 0.67s (± 0.55%) 0.67s (± 1.26%) -0.00s (- 0.59%) 0.65s 0.69s
Check Time 5.04s (± 1.77%) 5.07s (± 1.29%) +0.03s (+ 0.63%) 4.95s 5.18s
Emit Time 3.00s (± 3.18%) 3.00s (± 2.78%) -0.00s (- 0.17%) 2.89s 3.21s
Total Time 10.28s (± 0.47%) 10.29s (± 0.47%) +0.00s (+ 0.03%) 10.21s 10.41s
material-ui - node (v8.9.0, x64)
Memory used 463,690k (± 0.01%) 463,395k (± 0.02%) -295k (- 0.06%) 463,211k 463,638k
Parse Time 2.40s (± 0.63%) 2.39s (± 0.30%) -0.01s (- 0.46%) 2.37s 2.41s
Bind Time 0.77s (± 1.04%) 0.77s (± 1.36%) +0.00s (+ 0.13%) 0.75s 0.80s
Check Time 17.13s (± 1.14%) 17.20s (± 0.61%) +0.08s (+ 0.46%) 16.95s 17.43s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.29s (± 0.99%) 20.37s (± 0.53%) +0.07s (+ 0.36%) 20.11s 20.59s
Angular - node (v8.9.0, x86)
Memory used 195,324k (± 0.04%) 195,145k (± 0.02%) -179k (- 0.09%) 195,058k 195,246k
Parse Time 2.46s (± 0.77%) 2.46s (± 0.97%) -0.00s (- 0.04%) 2.40s 2.52s
Bind Time 0.99s (± 0.95%) 0.99s (± 0.71%) +0.00s (+ 0.41%) 0.98s 1.01s
Check Time 4.83s (± 0.53%) 4.83s (± 0.78%) +0.01s (+ 0.10%) 4.77s 4.93s
Emit Time 5.95s (± 1.37%) 5.89s (± 1.07%) -0.06s (- 0.92%) 5.71s 6.04s
Total Time 14.23s (± 0.69%) 14.18s (± 0.74%) -0.05s (- 0.34%) 13.90s 14.40s
Monaco - node (v8.9.0, x86)
Memory used 193,474k (± 0.02%) 193,475k (± 0.03%) +1k (+ 0.00%) 193,391k 193,644k
Parse Time 1.92s (± 1.15%) 1.92s (± 1.36%) -0.00s (- 0.16%) 1.86s 2.00s
Bind Time 0.70s (± 0.88%) 0.70s (± 0.74%) 0.00s ( 0.00%) 0.69s 0.71s
Check Time 5.50s (± 0.50%) 5.46s (± 1.39%) -0.04s (- 0.73%) 5.18s 5.59s
Emit Time 2.67s (± 1.03%) 2.72s (± 2.65%) +0.05s (+ 1.87%) 2.65s 2.99s
Total Time 10.79s (± 0.63%) 10.80s (± 0.47%) +0.01s (+ 0.08%) 10.72s 10.91s
TFS - node (v8.9.0, x86)
Memory used 173,770k (± 0.02%) 173,786k (± 0.03%) +16k (+ 0.01%) 173,709k 173,933k
Parse Time 1.60s (± 0.88%) 1.58s (± 0.70%) -0.01s (- 0.81%) 1.56s 1.61s
Bind Time 0.64s (± 1.35%) 0.64s (± 0.53%) -0.01s (- 0.78%) 0.63s 0.64s
Check Time 4.65s (± 0.54%) 4.65s (± 0.43%) 0.00s ( 0.00%) 4.60s 4.69s
Emit Time 2.80s (± 1.40%) 2.80s (± 1.60%) +0.00s (+ 0.11%) 2.71s 2.92s
Total Time 9.69s (± 0.64%) 9.68s (± 0.43%) -0.02s (- 0.17%) 9.57s 9.76s
material-ui - node (v8.9.0, x86)
Memory used 262,549k (± 0.02%) 262,418k (± 0.02%) -131k (- 0.05%) 262,351k 262,530k
Parse Time 2.46s (± 0.64%) 2.45s (± 0.73%) -0.01s (- 0.28%) 2.42s 2.49s
Bind Time 0.66s (± 1.21%) 0.67s (± 1.26%) +0.01s (+ 1.06%) 0.65s 0.69s
Check Time 15.65s (± 0.88%) 15.62s (± 0.70%) -0.03s (- 0.22%) 15.43s 15.83s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.77s (± 0.78%) 18.74s (± 0.66%) -0.03s (- 0.18%) 18.51s 18.98s
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 39487 10
Baseline master 10

@sandersn
Copy link
Member Author

sandersn commented Jul 8, 2020

Yep, no significant slowdown from the perf tests.

@sandersn sandersn merged commit 007b82f into master Jul 8, 2020
@sandersn sandersn deleted the better-checking-propertylike-tags branch July 8, 2020 20:55
cangSDARM added a commit to cangSDARM/TypeScript that referenced this pull request Jul 9, 2020
@cangSDARM
Merge remote-tracking branch 'upstream/master'
84b6358 
* upstream/master: (75 commits)
  Insert auto-imports in sorted order (microsoft#39394)
  LEGO: check in for master to temporary branch.
  Better checking of @param/@Property tags (microsoft#39487)
  fix(25155): add space before optional parameters/properties (microsoft#38798)
  Add regression test for microsoft#38834 (microsoft#39479)
  Fixes searches for symbols exported using export * as (microsoft#39507)
  fix(39421): omit prefix text for rest binding element (microsoft#39433)
  fix(39440): show QF for abstract classes with methods which include 'this' parameter (microsoft#39465)
  Remove unnecessary assert (microsoft#39483)
  LEGO: check in for master to temporary branch.
  Update user baselines (microsoft#39220)
  Type `this` in more constructor functions (microsoft#39447)
  LEGO: check in for master to temporary branch.
  LEGO: check in for master to temporary branch.
  Properly handle rest parameters in function declarations with @type annotations (microsoft#39473)
  Ensure type/namespaceish statics are included in the list of namespace merge members (microsoft#38920)
  Fix getTypeAtLocation for dotted implements clauses (microsoft#39363)
  Add workflow_dispatch to our nightly publish script. (microsoft#39485)
  Fix crash in decorator metadata calculation when serializing template literal type nodes (microsoft#39481)
  Fix test semantic merge conflict between microsoft#39348 and microsoft#39130 (microsoft#39478)
  ...

# Conflicts:
#	src/compiler/scanner.ts
@mastermatt mastermatt mentioned this pull request Aug 4, 2020
Open
@connorjclark connorjclark mentioned this pull request Nov 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham weswigham approved these changes

@elibarzilay elibarzilay Awaiting requested review from elibarzilay

Assignees

@sandersn sandersn

Labels
Author: Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSDoc optional property
3 participants
@sandersn @typescript-bot @weswigham