Skip to content

Use control flow analysis to check 'super(...)' call before 'this' access #38612

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
May 17, 2020
Merged

Use control flow analysis to check 'super(...)' call before 'this' access #38612

merged 4 commits into from
May 17, 2020

Conversation

ahejlsberg
Copy link
Member

With this PR we use control flow analysis to check that a super(...) call precedes every this or super access in the constructor of a derived class. Previously we only checked that a this or super access is lexically preceded by a super(...) call, but that isn't correct in code with branches:

class A {
    x = 1;
}

class B extends A {
    constructor(b: boolean) {
        if (b) {
            super();
            this.x = 2;
        }
        else {
            this.x = 3;  // Error, but previously wasn't
        }
    }
}

Our previous lexical check extended even to super property accesses within arrow functions, but, strangely, not to this property accesses. That was inconsistent and overly restrictive. We now permit such accesses (but it is beyond our capabilities to check that the arrow function isn't called before the call to super(...)).

class A {
    x = 1;
}

class C extends A {
    constructor() {
        let f = () => super.x;  // Was error, now isn't
        super();
        f();
    }
}

Fixes #38512.

weswigham
weswigham approved these changes May 16, 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.

I see you also removed some unused flow node types 👍

@ahejlsberg
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 17, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 17, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 17, 2020 •
edited
Loading

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

Update: The results are in!

@typescript-bot typescript-bot mentioned this pull request May 17, 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.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..38612

Metric master 38612 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 328,320k (± 0.03%) 327,751k (± 0.04%) -569k (- 0.17%) 327,484k 328,030k
Parse Time 1.65s (± 0.45%) 1.64s (± 0.58%) -0.00s (- 0.18%) 1.62s 1.66s
Bind Time 0.91s (± 0.75%) 0.91s (± 1.17%) -0.00s (- 0.11%) 0.88s 0.92s
Check Time 4.85s (± 0.36%) 4.87s (± 0.60%) +0.02s (+ 0.43%) 4.81s 4.93s
Emit Time 5.40s (± 0.64%) 5.41s (± 0.65%) +0.01s (+ 0.24%) 5.34s 5.47s
Total Time 12.80s (± 0.24%) 12.83s (± 0.43%) +0.03s (+ 0.26%) 12.71s 12.97s
Monaco - node (v10.16.3, x64)
Memory used 327,392k (± 0.02%) 327,284k (± 0.01%) -108k (- 0.03%) 327,181k 327,392k
Parse Time 1.27s (± 0.44%) 1.28s (± 0.43%) +0.01s (+ 0.47%) 1.27s 1.29s
Bind Time 0.79s (± 0.63%) 0.79s (± 0.47%) 0.00s ( 0.00%) 0.78s 0.79s
Check Time 4.88s (± 0.26%) 4.88s (± 0.49%) -0.00s (- 0.08%) 4.82s 4.92s
Emit Time 2.94s (± 0.70%) 2.95s (± 0.63%) +0.01s (+ 0.48%) 2.92s 2.98s
Total Time 9.88s (± 0.26%) 9.89s (± 0.34%) +0.02s (+ 0.17%) 9.80s 9.97s
TFS - node (v10.16.3, x64)
Memory used 292,393k (± 0.01%) 292,406k (± 0.02%) +12k (+ 0.00%) 292,311k 292,523k
Parse Time 0.96s (± 0.71%) 0.97s (± 0.91%) +0.00s (+ 0.31%) 0.94s 0.98s
Bind Time 0.76s (± 1.18%) 0.75s (± 0.45%) -0.00s (- 0.40%) 0.75s 0.76s
Check Time 4.40s (± 0.49%) 4.39s (± 0.60%) -0.01s (- 0.30%) 4.35s 4.47s
Emit Time 3.09s (± 0.83%) 3.10s (± 0.79%) +0.00s (+ 0.13%) 3.06s 3.16s
Total Time 9.21s (± 0.52%) 9.21s (± 0.45%) -0.01s (- 0.10%) 9.15s 9.33s
material-ui - node (v10.16.3, x64)
Memory used 450,264k (± 0.02%) 449,950k (± 0.01%) -314k (- 0.07%) 449,838k 450,082k
Parse Time 1.79s (± 0.27%) 1.78s (± 0.42%) -0.00s (- 0.22%) 1.77s 1.80s
Bind Time 0.71s (± 1.18%) 0.70s (± 0.85%) -0.01s (- 1.28%) 0.68s 0.71s
Check Time 12.86s (± 0.62%) 12.82s (± 0.39%) -0.04s (- 0.29%) 12.71s 12.97s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.35s (± 0.51%) 15.31s (± 0.37%) -0.05s (- 0.31%) 15.17s 15.47s
Angular - node (v12.1.0, x64)
Memory used 303,911k (± 0.02%) 303,308k (± 0.03%) -603k (- 0.20%) 303,093k 303,516k
Parse Time 1.60s (± 0.82%) 1.59s (± 0.79%) -0.01s (- 0.31%) 1.58s 1.63s
Bind Time 0.89s (± 0.75%) 0.89s (± 0.73%) +0.00s (+ 0.11%) 0.87s 0.90s
Check Time 4.74s (± 0.56%) 4.74s (± 0.45%) -0.00s (- 0.04%) 4.70s 4.80s
Emit Time 5.57s (± 0.60%) 5.53s (± 0.61%) -0.04s (- 0.66%) 5.47s 5.63s
Total Time 12.79s (± 0.33%) 12.75s (± 0.33%) -0.04s (- 0.31%) 12.67s 12.86s
Monaco - node (v12.1.0, x64)
Memory used 307,256k (± 0.02%) 307,237k (± 0.02%) -19k (- 0.01%) 307,023k 307,381k
Parse Time 1.22s (± 0.57%) 1.22s (± 0.64%) +0.00s (+ 0.00%) 1.20s 1.24s
Bind Time 0.77s (± 0.89%) 0.76s (± 0.44%) -0.01s (- 0.78%) 0.75s 0.77s
Check Time 4.68s (± 0.64%) 4.68s (± 0.53%) +0.00s (+ 0.04%) 4.64s 4.76s
Emit Time 2.99s (± 0.95%) 3.01s (± 1.15%) +0.02s (+ 0.50%) 2.94s 3.11s
Total Time 9.66s (± 0.57%) 9.68s (± 0.45%) +0.01s (+ 0.14%) 9.57s 9.77s
TFS - node (v12.1.0, x64)
Memory used 274,699k (± 0.02%) 274,688k (± 0.02%) -11k (- 0.00%) 274,521k 274,782k
Parse Time 0.93s (± 0.51%) 0.94s (± 0.85%) +0.01s (+ 0.96%) 0.92s 0.96s
Bind Time 0.73s (± 0.89%) 0.73s (± 1.11%) +0.00s (+ 0.27%) 0.71s 0.75s
Check Time 4.31s (± 0.29%) 4.30s (± 0.28%) -0.01s (- 0.14%) 4.28s 4.33s
Emit Time 3.13s (± 1.00%) 3.11s (± 1.56%) -0.02s (- 0.58%) 3.01s 3.26s
Total Time 9.10s (± 0.48%) 9.09s (± 0.59%) -0.01s (- 0.12%) 9.00s 9.26s
material-ui - node (v12.1.0, x64)
Memory used 427,586k (± 0.04%) 427,353k (± 0.05%) -234k (- 0.05%) 426,539k 427,588k
Parse Time 1.76s (± 0.63%) 1.77s (± 0.48%) +0.00s (+ 0.28%) 1.76s 1.79s
Bind Time 0.65s (± 0.95%) 0.65s (± 1.29%) -0.00s (- 0.62%) 0.63s 0.67s
Check Time 11.42s (± 0.89%) 11.42s (± 0.57%) +0.00s (+ 0.04%) 11.27s 11.54s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 13.82s (± 0.79%) 13.83s (± 0.51%) +0.01s (+ 0.05%) 13.65s 13.96s
Angular - node (v8.9.0, x64)
Memory used 323,236k (± 0.01%) 322,734k (± 0.01%) -502k (- 0.16%) 322,644k 322,808k
Parse Time 2.13s (± 0.29%) 2.12s (± 0.29%) -0.01s (- 0.47%) 2.11s 2.14s
Bind Time 0.94s (± 0.85%) 0.94s (± 0.87%) +0.00s (+ 0.11%) 0.93s 0.97s
Check Time 5.48s (± 1.73%) 5.48s (± 1.49%) +0.00s (+ 0.09%) 5.31s 5.63s
Emit Time 6.39s (± 1.64%) 6.32s (± 1.54%) -0.07s (- 1.10%) 6.03s 6.55s
Total Time 14.94s (± 0.60%) 14.86s (± 0.45%) -0.07s (- 0.50%) 14.63s 14.96s
Monaco - node (v8.9.0, x64)
Memory used 325,846k (± 0.01%) 325,876k (± 0.01%) +30k (+ 0.01%) 325,811k 325,927k
Parse Time 1.56s (± 0.80%) 1.56s (± 0.39%) -0.00s (- 0.13%) 1.54s 1.57s
Bind Time 0.91s (± 1.03%) 0.91s (± 0.65%) +0.00s (+ 0.11%) 0.90s 0.93s
Check Time 5.47s (± 0.41%) 5.46s (± 0.44%) -0.01s (- 0.27%) 5.42s 5.51s
Emit Time 3.52s (± 0.47%) 3.50s (± 0.69%) -0.02s (- 0.51%) 3.45s 3.56s
Total Time 11.46s (± 0.32%) 11.42s (± 0.24%) -0.03s (- 0.29%) 11.37s 11.47s
TFS - node (v8.9.0, x64)
Memory used 291,918k (± 0.01%) 291,932k (± 0.01%) +14k (+ 0.00%) 291,853k 292,025k
Parse Time 1.26s (± 0.53%) 1.26s (± 0.32%) +0.00s (+ 0.08%) 1.25s 1.27s
Bind Time 0.76s (± 0.88%) 0.76s (± 0.96%) +0.00s (+ 0.13%) 0.75s 0.78s
Check Time 5.06s (± 1.55%) 5.10s (± 1.27%) +0.04s (+ 0.89%) 4.96s 5.20s
Emit Time 3.27s (± 2.66%) 3.21s (± 3.40%) -0.05s (- 1.56%) 3.02s 3.44s
Total Time 10.34s (± 0.51%) 10.34s (± 0.53%) -0.00s (- 0.01%) 10.24s 10.48s
material-ui - node (v8.9.0, x64)
Memory used 453,143k (± 0.01%) 452,896k (± 0.01%) -247k (- 0.05%) 452,767k 453,036k
Parse Time 2.12s (± 0.49%) 2.13s (± 0.65%) +0.01s (+ 0.33%) 2.10s 2.15s
Bind Time 0.82s (± 0.70%) 0.82s (± 0.60%) -0.00s (- 0.49%) 0.80s 0.82s
Check Time 16.73s (± 0.97%) 16.81s (± 0.72%) +0.08s (+ 0.50%) 16.60s 17.12s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.67s (± 0.83%) 19.75s (± 0.59%) +0.08s (+ 0.43%) 19.52s 20.05s
Angular - node (v8.9.0, x86)
Memory used 186,162k (± 0.02%) 185,911k (± 0.02%) -252k (- 0.14%) 185,833k 185,988k
Parse Time 2.07s (± 0.77%) 2.07s (± 0.82%) +0.01s (+ 0.39%) 2.02s 2.10s
Bind Time 1.09s (± 0.51%) 1.09s (± 1.36%) +0.00s (+ 0.00%) 1.07s 1.14s
Check Time 5.05s (± 0.46%) 5.06s (± 0.29%) +0.02s (+ 0.32%) 5.03s 5.09s
Emit Time 6.10s (± 0.84%) 6.09s (± 0.67%) -0.00s (- 0.03%) 6.00s 6.17s
Total Time 14.30s (± 0.35%) 14.32s (± 0.45%) +0.02s (+ 0.17%) 14.19s 14.47s
Monaco - node (v8.9.0, x86)
Memory used 185,682k (± 0.03%) 185,681k (± 0.02%) -1k (- 0.00%) 185,590k 185,782k
Parse Time 1.60s (± 0.74%) 1.60s (± 0.47%) -0.00s (- 0.06%) 1.58s 1.62s
Bind Time 0.77s (± 0.80%) 0.77s (± 1.23%) +0.00s (+ 0.39%) 0.76s 0.80s
Check Time 5.50s (± 0.60%) 5.50s (± 0.27%) -0.00s (- 0.02%) 5.47s 5.54s
Emit Time 2.87s (± 1.16%) 2.89s (± 0.61%) +0.02s (+ 0.52%) 2.85s 2.93s
Total Time 10.74s (± 0.52%) 10.76s (± 0.27%) +0.02s (+ 0.15%) 10.70s 10.82s
TFS - node (v8.9.0, x86)
Memory used 167,270k (± 0.02%) 167,274k (± 0.02%) +5k (+ 0.00%) 167,186k 167,387k
Parse Time 1.29s (± 0.34%) 1.30s (± 1.02%) +0.01s (+ 0.46%) 1.27s 1.33s
Bind Time 0.72s (± 1.01%) 0.72s (± 1.08%) +0.00s (+ 0.42%) 0.71s 0.74s
Check Time 4.72s (± 0.53%) 4.71s (± 0.53%) -0.01s (- 0.25%) 4.66s 4.76s
Emit Time 2.97s (± 1.10%) 2.97s (± 1.10%) +0.01s (+ 0.27%) 2.90s 3.06s
Total Time 9.70s (± 0.43%) 9.71s (± 0.44%) +0.01s (+ 0.08%) 9.64s 9.86s
material-ui - node (v8.9.0, x86)
Memory used 256,674k (± 0.01%) 256,589k (± 0.02%) -85k (- 0.03%) 256,519k 256,681k
Parse Time 2.19s (± 0.68%) 2.19s (± 0.54%) +0.00s (+ 0.09%) 2.17s 2.22s
Bind Time 0.69s (± 0.71%) 0.70s (± 0.88%) +0.01s (+ 0.72%) 0.69s 0.72s
Check Time 15.44s (± 0.53%) 15.37s (± 0.56%) -0.07s (- 0.45%) 15.13s 15.55s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.32s (± 0.47%) 18.26s (± 0.50%) -0.06s (- 0.34%) 18.00s 18.46s
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 38612 10
Baseline master 10

@ahejlsberg ahejlsberg merged commit 3c1f37e into master May 17, 2020
cangSDARM added a commit to cangSDARM/TypeScript that referenced this pull request May 18, 2020
@cangSDARM
Merge remote-tracking branch 'upstream/master'
0fbfa63 
* upstream/master:
  Use control flow analysis to check 'super(...)' call before 'this' access (microsoft#38612)
  LEGO: check in for master to temporary branch.
  Make `processTaggedTemplateExpression` visit a returned node
  goToDefinition: find only the value if it's the RHS of an assignment
  Fix regression organize imports duplicates comments (microsoft#38599)
  Fix crash in JS declaration emit (microsoft#38508)
@jakebailey jakebailey deleted the fix38512 branch November 7, 2022 17:36
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

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Execution branches not evaluated for super/this calls
3 participants
@ahejlsberg @typescript-bot @weswigham