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.

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

Allow narrowing for any left-hand in operand #45152

Merged
merged 3 commits into from
Jul 29, 2021
Merged

Conversation

gabritto
Copy link
Member

Extends a more restrictive fix already in place for #42639.

Previously, narrowing of an in expression happens if the expression satisfy two constraints:

  • In the binder: isNarrowableInOperands is a syntactic constraint that allows narrowing of an expression if the right in operand is a narrowing expression, and the left in operand is a string literal (e.g. if ('prop' in c) { ... }).
  • In the checker: narrowBinaryExpression only calls narrowByInKeyword if the left in operand's type is a string literal type.

An already merged PR (#44893) relaxed the binder constraint to allow narrowing in cases the left in operand is an identifier, to address scenarios such as:

type A = { a: number };
type B = { b: string };
declare c: A | B;

if ("a" in c) {
    c; // c has type `A` here
}

const a = "a";
if (a in c) {
    c; // c should also have type `A` here; previously it had type `A | B`
}

This PR further relaxes the constraint in the binder for narrowing in expressions, by no longer placing any syntactic constraint on the left in operand expression. This means that an in expression is considered narrowable by the binder if the right expression is narrowing:

type A = { a: number };
type B = { b: string };
declare c: A | B;

if ("a" in c) {
    c; // c has type `A` here
}

const a = "a";
if (a in c) {
    c; // c has type `A` here
}

let stringA: string = "a";
if ((stringA as "a") in c) {
    c; // c now has type `A` here too
}

if ((stringA as ("a" | "b")) in c) {
    c; // c has type `A | B`; not narrowed because the left expression has type `"a" | "b"`, i.e. not a string literal type
}

Concerns:
From what I can tell, a possible concern would be performance, since this PR makes it so the checker attempts to narrow more types of in expressions, but the impact may be negligible.
A caveat here (brought up by @andrewbranch) regarding perf tests is that, since the checker currently doesn't narrow those kinds of expressions, real world code is unlikely to have those kinds of expressions.

@gabritto gabritto requested a review from andrewbranch July 22, 2021 18:48
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 22, 2021
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

1 similar comment
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@gabritto gabritto self-assigned this Jul 22, 2021
@gabritto gabritto linked an issue Jul 22, 2021 that may be closed by this pull request
@andrewbranch
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 22, 2021

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 22, 2021

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 22, 2021

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

@andrewbranch
Copy link
Member

@typescript-bot user test this inline

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 22, 2021

Heya @andrewbranch, I've started to run the inline community code test suite on this PR at b4f2f8c. 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..45152

Metric main 45152 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 348,498k (± 0.03%) 348,551k (± 0.02%) +53k (+ 0.02%) 348,389k 348,681k
Parse Time 1.81s (± 0.57%) 1.80s (± 0.26%) -0.01s (- 0.66%) 1.79s 1.81s
Bind Time 0.84s (± 0.90%) 0.84s (± 0.56%) -0.00s (- 0.12%) 0.83s 0.85s
Check Time 5.37s (± 0.72%) 5.36s (± 0.53%) -0.01s (- 0.24%) 5.30s 5.42s
Emit Time 5.85s (± 0.54%) 5.80s (± 0.68%) -0.05s (- 0.89%) 5.73s 5.93s
Total Time 13.88s (± 0.47%) 13.80s (± 0.49%) -0.08s (- 0.58%) 13.68s 14.01s
Compiler-Unions - node (v10.16.3, x64)
Memory used 203,376k (± 0.02%) 203,369k (± 0.03%) -8k (- 0.00%) 203,194k 203,456k
Parse Time 0.78s (± 0.95%) 0.78s (± 0.60%) -0.00s (- 0.38%) 0.77s 0.79s
Bind Time 0.53s (± 1.27%) 0.53s (± 1.13%) -0.00s (- 0.19%) 0.51s 0.54s
Check Time 7.83s (± 0.69%) 7.81s (± 0.77%) -0.02s (- 0.24%) 7.68s 7.99s
Emit Time 2.43s (± 0.51%) 2.43s (± 0.63%) -0.00s (- 0.04%) 2.39s 2.46s
Total Time 11.56s (± 0.51%) 11.54s (± 0.50%) -0.02s (- 0.21%) 11.44s 11.74s
Monaco - node (v10.16.3, x64)
Memory used 340,535k (± 0.03%) 340,559k (± 0.01%) +24k (+ 0.01%) 340,500k 340,636k
Parse Time 1.47s (± 1.01%) 1.47s (± 0.40%) -0.00s (- 0.20%) 1.46s 1.48s
Bind Time 0.75s (± 0.97%) 0.74s (± 0.65%) -0.01s (- 1.47%) 0.73s 0.75s
Check Time 5.38s (± 0.74%) 5.37s (± 0.48%) -0.02s (- 0.37%) 5.30s 5.41s
Emit Time 3.16s (± 0.86%) 3.16s (± 0.78%) -0.00s (- 0.09%) 3.12s 3.23s
Total Time 10.76s (± 0.37%) 10.73s (± 0.43%) -0.04s (- 0.33%) 10.62s 10.83s
TFS - node (v10.16.3, x64)
Memory used 304,022k (± 0.02%) 303,969k (± 0.02%) -52k (- 0.02%) 303,838k 304,132k
Parse Time 1.19s (± 0.52%) 1.19s (± 0.81%) +0.00s (+ 0.17%) 1.17s 1.21s
Bind Time 0.71s (± 0.70%) 0.71s (± 0.78%) -0.00s (- 0.42%) 0.70s 0.72s
Check Time 4.90s (± 0.40%) 4.87s (± 0.51%) -0.03s (- 0.63%) 4.81s 4.92s
Emit Time 3.32s (± 0.79%) 3.30s (± 0.73%) -0.03s (- 0.75%) 3.23s 3.34s
Total Time 10.12s (± 0.34%) 10.07s (± 0.43%) -0.05s (- 0.53%) 9.94s 10.13s
material-ui - node (v10.16.3, x64)
Memory used 469,690k (± 0.01%) 469,634k (± 0.02%) -56k (- 0.01%) 469,413k 469,773k
Parse Time 1.74s (± 0.57%) 1.74s (± 0.61%) -0.01s (- 0.29%) 1.71s 1.76s
Bind Time 0.66s (± 0.79%) 0.66s (± 1.59%) -0.00s (- 0.15%) 0.63s 0.68s
Check Time 14.22s (± 0.78%) 14.14s (± 0.38%) -0.08s (- 0.56%) 14.03s 14.25s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.62s (± 0.69%) 16.54s (± 0.35%) -0.09s (- 0.51%) 16.42s 16.67s
Angular - node (v12.1.0, x64)
Memory used 326,625k (± 0.02%) 326,506k (± 0.09%) -118k (- 0.04%) 325,358k 326,811k
Parse Time 1.80s (± 0.66%) 1.78s (± 0.73%) -0.01s (- 0.78%) 1.75s 1.81s
Bind Time 0.83s (± 1.30%) 0.82s (± 1.00%) -0.01s (- 0.60%) 0.81s 0.85s
Check Time 5.19s (± 0.58%) 5.19s (± 0.47%) -0.01s (- 0.13%) 5.15s 5.27s
Emit Time 6.07s (± 0.41%) 6.08s (± 0.69%) +0.01s (+ 0.25%) 5.98s 6.18s
Total Time 13.89s (± 0.33%) 13.88s (± 0.39%) -0.02s (- 0.12%) 13.73s 13.98s
Compiler-Unions - node (v12.1.0, x64)
Memory used 190,734k (± 0.10%) 190,850k (± 0.03%) +115k (+ 0.06%) 190,734k 190,931k
Parse Time 0.78s (± 0.83%) 0.77s (± 0.52%) -0.01s (- 0.90%) 0.76s 0.78s
Bind Time 0.53s (± 0.75%) 0.53s (± 1.09%) +0.00s (+ 0.38%) 0.52s 0.55s
Check Time 7.33s (± 0.47%) 7.28s (± 0.51%) -0.04s (- 0.55%) 7.18s 7.35s
Emit Time 2.45s (± 1.02%) 2.44s (± 0.63%) -0.01s (- 0.57%) 2.40s 2.48s
Total Time 11.09s (± 0.42%) 11.03s (± 0.43%) -0.06s (- 0.53%) 10.91s 11.13s
Monaco - node (v12.1.0, x64)
Memory used 323,695k (± 0.01%) 323,714k (± 0.02%) +19k (+ 0.01%) 323,555k 323,834k
Parse Time 1.44s (± 0.70%) 1.43s (± 0.77%) -0.00s (- 0.21%) 1.41s 1.46s
Bind Time 0.73s (± 0.82%) 0.72s (± 0.66%) -0.00s (- 0.41%) 0.71s 0.73s
Check Time 5.26s (± 0.54%) 5.25s (± 0.59%) -0.00s (- 0.06%) 5.21s 5.33s
Emit Time 3.20s (± 0.49%) 3.19s (± 0.82%) -0.01s (- 0.25%) 3.14s 3.27s
Total Time 10.62s (± 0.34%) 10.60s (± 0.40%) -0.02s (- 0.17%) 10.50s 10.68s
TFS - node (v12.1.0, x64)
Memory used 288,644k (± 0.03%) 288,668k (± 0.02%) +24k (+ 0.01%) 288,536k 288,812k
Parse Time 1.21s (± 0.49%) 1.21s (± 0.79%) -0.01s (- 0.66%) 1.18s 1.22s
Bind Time 0.70s (± 1.27%) 0.70s (± 0.85%) -0.01s (- 1.28%) 0.68s 0.71s
Check Time 4.84s (± 0.44%) 4.84s (± 0.53%) -0.00s (- 0.06%) 4.79s 4.91s
Emit Time 3.38s (± 1.07%) 3.36s (± 1.00%) -0.02s (- 0.53%) 3.30s 3.47s
Total Time 10.13s (± 0.40%) 10.09s (± 0.53%) -0.04s (- 0.38%) 9.99s 10.27s
material-ui - node (v12.1.0, x64)
Memory used 448,401k (± 0.01%) 448,447k (± 0.01%) +46k (+ 0.01%) 448,329k 448,559k
Parse Time 1.72s (± 0.52%) 1.72s (± 0.82%) -0.00s (- 0.23%) 1.69s 1.76s
Bind Time 0.66s (± 0.57%) 0.65s (± 1.30%) -0.00s (- 0.61%) 0.63s 0.67s
Check Time 12.86s (± 0.53%) 12.72s (± 0.32%) -0.14s (- 1.07%) 12.64s 12.81s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.24s (± 0.48%) 15.09s (± 0.28%) -0.15s (- 0.98%) 15.00s 15.18s
Angular - node (v14.15.1, x64)
Memory used 325,716k (± 0.01%) 325,734k (± 0.01%) +19k (+ 0.01%) 325,644k 325,776k
Parse Time 1.82s (± 0.34%) 1.81s (± 0.90%) -0.01s (- 0.27%) 1.79s 1.87s
Bind Time 0.88s (± 0.96%) 0.86s (± 0.42%) -0.02s (- 1.93%) 0.86s 0.87s
Check Time 5.27s (± 0.43%) 5.23s (± 0.39%) -0.04s (- 0.78%) 5.18s 5.26s
Emit Time 6.21s (± 1.05%) 6.16s (± 0.75%) -0.05s (- 0.76%) 6.08s 6.28s
Total Time 14.17s (± 0.53%) 14.07s (± 0.40%) -0.10s (- 0.73%) 13.97s 14.19s
Compiler-Unions - node (v14.15.1, x64)
Memory used 191,124k (± 0.63%) 191,118k (± 0.63%) -6k (- 0.00%) 189,448k 192,776k
Parse Time 0.81s (± 0.83%) 0.80s (± 1.11%) -0.00s (- 0.37%) 0.79s 0.83s
Bind Time 0.56s (± 0.65%) 0.56s (± 0.72%) -0.00s (- 0.89%) 0.55s 0.57s
Check Time 7.50s (± 0.56%) 7.45s (± 0.58%) -0.05s (- 0.69%) 7.35s 7.56s
Emit Time 2.42s (± 0.73%) 2.42s (± 0.66%) +0.00s (+ 0.04%) 2.38s 2.46s
Total Time 11.30s (± 0.47%) 11.23s (± 0.50%) -0.06s (- 0.55%) 11.10s 11.36s
Monaco - node (v14.15.1, x64)
Memory used 322,488k (± 0.01%) 322,496k (± 0.01%) +8k (+ 0.00%) 322,452k 322,540k
Parse Time 1.50s (± 0.51%) 1.48s (± 0.98%) -0.01s (- 0.80%) 1.46s 1.53s
Bind Time 0.76s (± 0.59%) 0.76s (± 0.81%) +0.00s (+ 0.40%) 0.75s 0.77s
Check Time 5.22s (± 0.50%) 5.17s (± 0.47%) -0.05s (- 1.01%) 5.13s 5.25s
Emit Time 3.24s (± 0.83%) 3.21s (± 0.52%) -0.03s (- 1.05%) 3.17s 3.24s
Total Time 10.71s (± 0.45%) 10.62s (± 0.44%) -0.10s (- 0.89%) 10.56s 10.74s
TFS - node (v14.15.1, x64)
Memory used 287,658k (± 0.01%) 287,656k (± 0.01%) -1k (- 0.00%) 287,615k 287,716k
Parse Time 1.23s (± 1.43%) 1.26s (± 1.33%) +0.02s (+ 2.03%) 1.22s 1.30s
Bind Time 0.75s (± 3.33%) 0.72s (± 0.92%) 🟩-0.03s (- 3.61%) 0.71s 0.74s
Check Time 4.82s (± 0.43%) 4.82s (± 0.54%) +0.00s (+ 0.02%) 4.74s 4.87s
Emit Time 3.45s (± 0.74%) 3.46s (± 0.95%) +0.02s (+ 0.55%) 3.40s 3.58s
Total Time 10.24s (± 0.36%) 10.26s (± 0.46%) +0.02s (+ 0.20%) 10.19s 10.40s
material-ui - node (v14.15.1, x64)
Memory used 446,859k (± 0.00%) 446,855k (± 0.00%) -4k (- 0.00%) 446,802k 446,894k
Parse Time 1.76s (± 0.46%) 1.76s (± 0.43%) -0.01s (- 0.40%) 1.74s 1.77s
Bind Time 0.69s (± 0.68%) 0.69s (± 1.06%) -0.00s (- 0.29%) 0.68s 0.71s
Check Time 13.00s (± 0.88%) 12.86s (± 0.45%) -0.14s (- 1.06%) 12.72s 13.01s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.45s (± 0.76%) 15.30s (± 0.41%) -0.14s (- 0.93%) 15.16s 15.46s
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 45152 10
Baseline main 10

Developer Information:

Download Benchmark

@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

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

Here they are:

Comparison Report - main..refs/pull/45152/merge

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

Approved with the suggested change.

src/compiler/binder.ts Outdated Show resolved Hide resolved
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 26, 2021

A while back we put in place a restriction to ensure that assertion functions had to be fully annotated so that they couldn't trigger further CFA computation when trying to resolve them (#33622).

It's been a while, so @ahejlsberg do you have an example of when this could happen? Just want to make sure there's nothing we're missing here.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Thinking about it some more, I think this is probably fine.

@gabritto gabritto merged commit db0f793 into main Jul 29, 2021
@gabritto gabritto deleted the gabritto/issue42639 branch July 29, 2021 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"in" operator not narrowing the type if used with a const
5 participants