Skip to content

Fix constant reference check in CFA #44762

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 5 commits into from
Jun 28, 2021
Merged

Fix constant reference check in CFA #44762

merged 5 commits into from
Jun 28, 2021

Conversation

ahejlsberg
Copy link
Member

This PR is a follow up fix to #44730. The PR implements a more thorough check to ensure references are non-mutable when they are narrowed by aliased conditional expressions. For example:

function test(obj: { readonly x: string | number }) {
    const isString = typeof obj.x === 'string';
    obj = { x: 42 };
    if (isString) {
        let s: string = obj.x;  // Not narrowed because obj is assigned in function body
    }
}

Previously we'd just check that x is readonly. Now we also check that obj isn't assigned in the function body.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 26, 2021
@ahejlsberg
Copy link
Member Author

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 26, 2021 •
edited
Loading

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

Update: The results are in!

@ahejlsberg ahejlsberg added this to the TypeScript 4.4.1 (RC) milestone Jun 26, 2021
@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..44762

Metric main 44762 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 344,297k (± 0.02%) 344,310k (± 0.01%) +13k (+ 0.00%) 344,207k 344,404k
Parse Time 1.80s (± 0.38%) 1.81s (± 0.50%) +0.01s (+ 0.56%) 1.79s 1.82s
Bind Time 0.85s (± 0.68%) 0.85s (± 0.70%) -0.00s (- 0.12%) 0.83s 0.86s
Check Time 5.44s (± 0.57%) 5.35s (± 0.43%) -0.10s (- 1.76%) 5.31s 5.41s
Emit Time 5.82s (± 0.51%) 5.82s (± 0.67%) -0.00s (- 0.02%) 5.73s 5.91s
Total Time 13.90s (± 0.41%) 13.82s (± 0.45%) -0.09s (- 0.63%) 13.66s 13.98s
Compiler-Unions - node (v10.16.3, x64)
Memory used 201,520k (± 0.03%) 201,510k (± 0.03%) -10k (- 0.01%) 201,347k 201,602k
Parse Time 0.78s (± 0.79%) 0.78s (± 0.74%) +0.00s (+ 0.26%) 0.77s 0.80s
Bind Time 0.53s (± 1.29%) 0.53s (± 1.09%) +0.01s (+ 1.33%) 0.52s 0.54s
Check Time 7.83s (± 0.57%) 7.82s (± 0.67%) -0.01s (- 0.17%) 7.71s 7.92s
Emit Time 2.44s (± 0.80%) 2.45s (± 0.96%) +0.01s (+ 0.49%) 2.40s 2.50s
Total Time 11.58s (± 0.40%) 11.58s (± 0.57%) +0.01s (+ 0.06%) 11.46s 11.74s
Monaco - node (v10.16.3, x64)
Memory used 340,669k (± 0.01%) 340,668k (± 0.03%) -1k (- 0.00%) 340,492k 340,993k
Parse Time 1.45s (± 0.68%) 1.46s (± 0.92%) +0.01s (+ 0.76%) 1.44s 1.49s
Bind Time 0.74s (± 0.80%) 0.75s (± 0.69%) +0.01s (+ 0.67%) 0.74s 0.76s
Check Time 5.44s (± 0.46%) 5.37s (± 0.92%) -0.06s (- 1.14%) 5.23s 5.45s
Emit Time 3.16s (± 0.57%) 3.16s (± 0.72%) -0.01s (- 0.19%) 3.12s 3.21s
Total Time 10.79s (± 0.33%) 10.74s (± 0.53%) -0.05s (- 0.43%) 10.64s 10.86s
TFS - node (v10.16.3, x64)
Memory used 303,980k (± 0.02%) 303,925k (± 0.02%) -54k (- 0.02%) 303,827k 304,160k
Parse Time 1.18s (± 0.80%) 1.19s (± 0.71%) +0.01s (+ 0.76%) 1.17s 1.21s
Bind Time 0.71s (± 0.66%) 0.71s (± 1.02%) +0.00s (+ 0.28%) 0.69s 0.72s
Check Time 4.97s (± 0.51%) 4.89s (± 0.46%) -0.09s (- 1.77%) 4.85s 4.94s
Emit Time 3.34s (± 1.90%) 3.34s (± 1.36%) +0.00s (+ 0.03%) 3.24s 3.47s
Total Time 10.20s (± 0.77%) 10.13s (± 0.59%) -0.08s (- 0.75%) 10.02s 10.28s
material-ui - node (v10.16.3, x64)
Memory used 471,018k (± 0.01%) 470,992k (± 0.02%) -26k (- 0.01%) 470,811k 471,222k
Parse Time 1.72s (± 0.35%) 1.72s (± 0.49%) +0.01s (+ 0.35%) 1.70s 1.74s
Bind Time 0.66s (± 0.74%) 0.67s (± 1.02%) +0.00s (+ 0.30%) 0.65s 0.68s
Check Time 14.17s (± 0.51%) 14.22s (± 0.37%) +0.05s (+ 0.34%) 14.12s 14.34s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.55s (± 0.43%) 16.61s (± 0.32%) +0.06s (+ 0.34%) 16.49s 16.72s
Angular - node (v12.1.0, x64)
Memory used 322,582k (± 0.02%) 322,494k (± 0.09%) -88k (- 0.03%) 321,385k 322,841k
Parse Time 1.77s (± 0.54%) 1.79s (± 0.85%) +0.01s (+ 0.73%) 1.75s 1.82s
Bind Time 0.83s (± 1.23%) 0.83s (± 1.21%) +0.01s (+ 0.72%) 0.82s 0.86s
Check Time 5.22s (± 0.58%) 5.16s (± 0.76%) -0.06s (- 1.22%) 5.09s 5.28s
Emit Time 6.01s (± 0.38%) 6.03s (± 0.88%) +0.02s (+ 0.33%) 5.96s 6.19s
Total Time 13.84s (± 0.23%) 13.81s (± 0.65%) -0.02s (- 0.17%) 13.67s 14.09s
Compiler-Unions - node (v12.1.0, x64)
Memory used 188,919k (± 0.11%) 189,190k (± 0.24%) +272k (+ 0.14%) 188,877k 191,003k
Parse Time 0.78s (± 0.88%) 0.78s (± 0.48%) -0.00s (- 0.13%) 0.77s 0.78s
Bind Time 0.53s (± 0.84%) 0.53s (± 0.89%) -0.00s (- 0.38%) 0.52s 0.54s
Check Time 7.34s (± 0.61%) 7.30s (± 0.60%) -0.05s (- 0.64%) 7.19s 7.42s
Emit Time 2.45s (± 1.20%) 2.44s (± 1.17%) -0.01s (- 0.49%) 2.38s 2.52s
Total Time 11.11s (± 0.63%) 11.05s (± 0.63%) -0.06s (- 0.56%) 10.88s 11.25s
Monaco - node (v12.1.0, x64)
Memory used 323,805k (± 0.02%) 323,756k (± 0.01%) -49k (- 0.02%) 323,599k 323,841k
Parse Time 1.41s (± 0.60%) 1.42s (± 0.63%) +0.01s (+ 0.92%) 1.40s 1.44s
Bind Time 0.72s (± 0.66%) 0.73s (± 0.94%) +0.01s (+ 1.26%) 0.71s 0.74s
Check Time 5.30s (± 0.41%) 5.24s (± 0.59%) -0.06s (- 1.17%) 5.17s 5.28s
Emit Time 3.16s (± 1.02%) 3.19s (± 1.08%) +0.03s (+ 0.92%) 3.13s 3.28s
Total Time 10.59s (± 0.41%) 10.58s (± 0.51%) -0.01s (- 0.08%) 10.45s 10.69s
TFS - node (v12.1.0, x64)
Memory used 288,654k (± 0.02%) 288,525k (± 0.01%) -129k (- 0.04%) 288,447k 288,596k
Parse Time 1.19s (± 0.65%) 1.19s (± 0.52%) +0.00s (+ 0.17%) 1.18s 1.20s
Bind Time 0.69s (± 0.84%) 0.70s (± 1.04%) +0.01s (+ 0.72%) 0.68s 0.71s
Check Time 4.88s (± 0.40%) 4.79s (± 0.40%) -0.09s (- 1.74%) 4.73s 4.83s
Emit Time 3.33s (± 0.81%) 3.33s (± 0.68%) -0.01s (- 0.18%) 3.28s 3.37s
Total Time 10.09s (± 0.37%) 10.01s (± 0.40%) -0.08s (- 0.81%) 9.92s 10.10s
material-ui - node (v12.1.0, x64)
Memory used 449,700k (± 0.06%) 449,688k (± 0.06%) -12k (- 0.00%) 448,681k 449,911k
Parse Time 1.70s (± 0.52%) 1.71s (± 0.55%) +0.01s (+ 0.65%) 1.69s 1.73s
Bind Time 0.65s (± 0.92%) 0.65s (± 0.76%) 0.00s ( 0.00%) 0.64s 0.66s
Check Time 12.77s (± 0.56%) 12.74s (± 1.21%) -0.03s (- 0.27%) 12.53s 13.25s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.12s (± 0.47%) 15.10s (± 1.00%) -0.02s (- 0.11%) 14.89s 15.59s
Angular - node (v14.15.1, x64)
Memory used 321,258k (± 0.01%) 321,181k (± 0.01%) -76k (- 0.02%) 321,116k 321,233k
Parse Time 1.79s (± 0.64%) 1.81s (± 0.51%) +0.02s (+ 1.01%) 1.79s 1.83s
Bind Time 0.88s (± 0.57%) 0.88s (± 0.68%) +0.00s (+ 0.11%) 0.87s 0.89s
Check Time 5.27s (± 0.50%) 5.15s (± 0.48%) -0.12s (- 2.28%) 5.11s 5.22s
Emit Time 6.09s (± 0.85%) 6.14s (± 0.58%) +0.04s (+ 0.74%) 6.06s 6.22s
Total Time 14.02s (± 0.48%) 13.96s (± 0.40%) -0.06s (- 0.42%) 13.87s 14.08s
Compiler-Unions - node (v14.15.1, x64)
Memory used 188,346k (± 0.50%) 187,738k (± 0.02%) -607k (- 0.32%) 187,634k 187,787k
Parse Time 0.80s (± 0.91%) 0.81s (± 0.74%) +0.00s (+ 0.50%) 0.79s 0.82s
Bind Time 0.56s (± 0.61%) 0.57s (± 1.06%) +0.01s (+ 1.44%) 0.55s 0.58s
Check Time 7.48s (± 0.76%) 7.49s (± 0.85%) +0.01s (+ 0.15%) 7.37s 7.65s
Emit Time 2.42s (± 0.84%) 2.44s (± 0.73%) +0.01s (+ 0.58%) 2.41s 2.50s
Total Time 11.26s (± 0.58%) 11.30s (± 0.72%) +0.04s (+ 0.34%) 11.14s 11.53s
Monaco - node (v14.15.1, x64)
Memory used 322,579k (± 0.00%) 322,554k (± 0.00%) -24k (- 0.01%) 322,533k 322,584k
Parse Time 1.48s (± 0.67%) 1.48s (± 0.60%) +0.01s (+ 0.41%) 1.46s 1.50s
Bind Time 0.75s (± 0.63%) 0.75s (± 0.40%) +0.00s (+ 0.27%) 0.75s 0.76s
Check Time 5.25s (± 0.53%) 5.19s (± 0.55%) -0.06s (- 1.09%) 5.13s 5.27s
Emit Time 3.22s (± 0.64%) 3.23s (± 0.67%) +0.01s (+ 0.31%) 3.19s 3.28s
Total Time 10.70s (± 0.44%) 10.66s (± 0.50%) -0.04s (- 0.37%) 10.55s 10.81s
TFS - node (v14.15.1, x64)
Memory used 287,642k (± 0.00%) 287,609k (± 0.00%) -33k (- 0.01%) 287,590k 287,638k
Parse Time 1.26s (± 1.81%) 1.25s (± 1.31%) -0.01s (- 0.87%) 1.20s 1.27s
Bind Time 0.71s (± 0.51%) 0.72s (± 1.03%) +0.01s (+ 1.26%) 0.71s 0.74s
Check Time 4.91s (± 0.54%) 4.83s (± 0.28%) -0.09s (- 1.75%) 4.81s 4.87s
Emit Time 3.42s (± 0.67%) 3.43s (± 0.70%) +0.01s (+ 0.29%) 3.38s 3.50s
Total Time 10.31s (± 0.37%) 10.23s (± 0.22%) -0.08s (- 0.80%) 10.16s 10.28s
material-ui - node (v14.15.1, x64)
Memory used 447,966k (± 0.06%) 448,187k (± 0.01%) +220k (+ 0.05%) 448,123k 448,230k
Parse Time 1.75s (± 0.35%) 1.76s (± 0.71%) +0.01s (+ 0.34%) 1.73s 1.78s
Bind Time 0.69s (± 0.50%) 0.69s (± 0.65%) +0.00s (+ 0.15%) 0.68s 0.70s
Check Time 12.97s (± 0.52%) 12.91s (± 0.46%) -0.07s (- 0.51%) 12.79s 13.10s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.41s (± 0.46%) 15.35s (± 0.36%) -0.06s (- 0.40%) 15.24s 15.52s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-206-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 (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 44762 10
Baseline main 10

Developer Information:

Download Benchmark

@ahejlsberg
Copy link
Member Author

The slight performance improvement is likely from deferring more calls to isParameterAssigned (which can result in an expensive tree walk).

@RyanCavanaugh Would be nice it we could cherry pick this fix into the 4.4 beta.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 28, 2021 •
edited
Loading

If we merge this in, I'll sync the release-4.4 branch and rebuild later this afternoon with @uniqueiniquity since we have 1-2 other PRs that we want in the beta too.

DanielRosenwasser
DanielRosenwasser reviewed Jun 28, 2021
View reviewed changes
src/compiler/checker.ts
return isConstVariable(symbol) || !!symbol.valueDeclaration && getRootDeclaration(symbol.valueDeclaration).kind === SyntaxKind.Parameter && !isParameterAssigned(symbol);
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ElementAccessExpression:
// The resolvedSymbol property is initialized by checkPropertyAccess or checkElementAccess before we get here.
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if there was a way to assert this easily.

@DanielRosenwasser
Copy link
Member

I added the tests case I mentioned in 3b12c04

Kind of surprised we don't narrow this.x, but not worth blocking on.

@DanielRosenwasser DanielRosenwasser merged commit 5f8a9e5 into main Jun 28, 2021
@DanielRosenwasser DanielRosenwasser deleted the fixConstantReferenceCheck branch June 28, 2021 23:41
@mkubilayk mkubilayk mentioned this pull request Jul 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser left review comments

@RyanCavanaugh RyanCavanaugh Awaiting requested review from RyanCavanaugh

@weswigham weswigham Awaiting requested review from weswigham

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
TypeScript 4.4.1 (RC)
Development

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