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

Jump to bottom

Don't error if binding pattern type is never in control flow analysis for destructured discriminated union #47927

Merged
merged 1 commit into from Feb 23, 2022
Merged

Don't error if binding pattern type is never in control flow analysis for destructured discriminated union #47927

merged 1 commit into from Feb 23, 2022

Conversation

gabritto
Copy link
Member

Fixes #47857.

Error explanation

The checker code introduced by #46266, more precisely getNarrowedTypeOfSymbol, is the one that causes the error.
In the bug example:

export type Expression = BooleanLogicExpression | 'true' | 'false';
export type BooleanLogicExpression = ['and', ...Expression[]] | ['not', Expression];

export function evaluate(expression: Expression): boolean {
  if (Array.isArray(expression)) {
    const [operator, ...operands] = expression;
    switch (operator) {
      case 'and': {
        return operands.every((child) => evaluate(child));
      }
      case 'not': {
        return !evaluate(operands[0]);
      }
      default: {
        throw new Error(`${operator} is not a supported operator`);
      }
    }
  } else {
    return expression === 'true';
  }
}

when we inside the default branch and are getting the narrowed type for operator's symbol, we narrow expression's type to never (because operator is a discriminant and it has type never). And then we try to get the type of operator knowing that expression has type never, by calling getBindingElementTypeFromParentType. Eventually, inside getBindingElementTypeFromParentType, we issue the error, because we can't figure out array/tuple element type for something of type never.

Fix

This PR fixes that by avoiding calling getBindingElementTypeFromParentType when the narrowed parent type is never.
I think the intuition is that this prevents us from issuing errors in destructuring expressions that are "outside" the part of the code where things are narrowed to never, but I might be wrong.
There's a test case (arrayDestructuringInSwitch2.ts) that shows this change doesn't prevent us from issuing never destructuring errors inside the branch where something has type never.

My only concern would be the possibility that we are missing out on erroring in some programs, but this whole narrowing code was added for 4.6 and didn't exist before anyways.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 17, 2022
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this
@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2022 •
edited

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2022 •
edited

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2022 •
edited

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2022 •
edited

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at c4aa31b. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2022 •
edited

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at c4aa31b. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-4.6 and LKG

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2022 •
edited

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.6 on this PR at c4aa31b. You can monitor the build here.

@typescript-bot typescript-bot mentioned this pull request Feb 22, 2022
Merged
@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #48005 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Feb 22, 2022
@gabritto
Cherry-pick PR microsoft#47927 into release-4.6
9ff5248 
Component commits:
c4aa31b early return if pattern type is never
@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/120694/artifacts?artifactName=tgz&fileId=CBB19DCDDDC33046879191D4402B846591692B9AAF0C6112C0F37425030A91D902&fileName=/typescript-4.7.0-insiders.20220222.tgz"
    }
}

and then running npm install.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..47927

Metric main 47927 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 356,800k (± 0.02%) 356,801k (± 0.02%) +1k (+ 0.00%) 356,647k 356,934k
Parse Time 2.03s (± 0.65%) 2.04s (± 0.41%) +0.00s (+ 0.15%) 2.02s 2.05s
Bind Time 0.86s (± 0.69%) 0.86s (± 0.77%) -0.00s (- 0.12%) 0.85s 0.88s
Check Time 5.72s (± 0.35%) 5.67s (± 0.38%) -0.04s (- 0.72%) 5.63s 5.72s
Emit Time 5.96s (± 0.85%) 5.91s (± 0.53%) -0.04s (- 0.74%) 5.87s 6.02s
Total Time 14.57s (± 0.46%) 14.49s (± 0.25%) -0.08s (- 0.57%) 14.39s 14.57s
Compiler-Unions - node (v10.16.3, x64)
Memory used 205,556k (± 0.02%) 205,511k (± 0.03%) -45k (- 0.02%) 205,300k 205,645k
Parse Time 0.83s (± 0.78%) 0.83s (± 0.60%) +0.00s (+ 0.36%) 0.82s 0.84s
Bind Time 0.52s (± 1.28%) 0.53s (± 0.71%) +0.01s (+ 0.96%) 0.52s 0.53s
Check Time 7.83s (± 0.49%) 7.85s (± 0.67%) +0.02s (+ 0.22%) 7.71s 7.95s
Emit Time 2.52s (± 1.55%) 2.50s (± 1.16%) -0.02s (- 0.67%) 2.46s 2.60s
Total Time 11.70s (± 0.57%) 11.71s (± 0.56%) +0.01s (+ 0.09%) 11.55s 11.82s
Monaco - node (v10.16.3, x64)
Memory used 343,234k (± 0.02%) 343,280k (± 0.01%) +46k (+ 0.01%) 343,209k 343,379k
Parse Time 1.56s (± 0.43%) 1.56s (± 0.36%) +0.00s (+ 0.13%) 1.55s 1.58s
Bind Time 0.76s (± 0.53%) 0.76s (± 0.39%) +0.00s (+ 0.13%) 0.76s 0.77s
Check Time 5.59s (± 0.50%) 5.61s (± 0.65%) +0.02s (+ 0.34%) 5.53s 5.70s
Emit Time 3.24s (± 0.93%) 3.21s (± 0.65%) -0.03s (- 0.99%) 3.17s 3.27s
Total Time 11.16s (± 0.36%) 11.15s (± 0.41%) -0.01s (- 0.08%) 11.05s 11.25s
TFS - node (v10.16.3, x64)
Memory used 305,075k (± 0.03%) 305,054k (± 0.01%) -21k (- 0.01%) 304,987k 305,145k
Parse Time 1.26s (± 0.39%) 1.27s (± 0.18%) +0.00s (+ 0.32%) 1.26s 1.27s
Bind Time 0.71s (± 0.69%) 0.72s (± 0.81%) +0.00s (+ 0.42%) 0.70s 0.73s
Check Time 5.14s (± 0.52%) 5.15s (± 0.53%) +0.01s (+ 0.23%) 5.10s 5.20s
Emit Time 3.38s (± 1.03%) 3.42s (± 1.02%) +0.04s (+ 1.06%) 3.35s 3.49s
Total Time 10.50s (± 0.40%) 10.55s (± 0.34%) +0.05s (+ 0.52%) 10.45s 10.64s
material-ui - node (v10.16.3, x64)
Memory used 468,836k (± 0.01%) 468,856k (± 0.01%) +21k (+ 0.00%) 468,744k 468,972k
Parse Time 1.81s (± 0.58%) 1.80s (± 0.37%) -0.01s (- 0.61%) 1.78s 1.81s
Bind Time 0.67s (± 1.02%) 0.67s (± 1.12%) +0.00s (+ 0.15%) 0.65s 0.68s
Check Time 14.18s (± 0.56%) 14.04s (± 0.31%) -0.14s (- 1.02%) 13.96s 14.15s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.66s (± 0.49%) 16.51s (± 0.26%) -0.16s (- 0.94%) 16.42s 16.61s
xstate - node (v10.16.3, x64)
Memory used 574,278k (± 1.37%) 570,758k (± 0.02%) -3,520k (- 0.61%) 570,602k 571,034k
Parse Time 2.57s (± 0.47%) 2.57s (± 0.24%) +0.00s (+ 0.16%) 2.56s 2.58s
Bind Time 1.02s (± 0.78%) 1.02s (± 0.60%) -0.00s (- 0.10%) 1.01s 1.04s
Check Time 1.50s (± 0.67%) 1.49s (± 0.56%) -0.00s (- 0.13%) 1.47s 1.51s
Emit Time 0.07s (± 4.13%) 0.07s (± 0.00%) -0.00s (- 2.78%) 0.07s 0.07s
Total Time 5.16s (± 0.32%) 5.15s (± 0.24%) -0.00s (- 0.06%) 5.13s 5.18s
Angular - node (v12.1.0, x64)
Memory used 334,532k (± 0.03%) 334,419k (± 0.08%) -113k (- 0.03%) 333,335k 334,690k
Parse Time 2.03s (± 0.80%) 2.04s (± 0.71%) +0.01s (+ 0.30%) 2.01s 2.08s
Bind Time 0.85s (± 1.12%) 0.84s (± 1.19%) -0.01s (- 0.71%) 0.82s 0.87s
Check Time 5.49s (± 0.60%) 5.49s (± 0.53%) +0.00s (+ 0.02%) 5.44s 5.55s
Emit Time 6.18s (± 0.55%) 6.13s (± 0.56%) -0.05s (- 0.74%) 6.07s 6.24s
Total Time 14.54s (± 0.41%) 14.50s (± 0.32%) -0.04s (- 0.29%) 14.39s 14.63s
Compiler-Unions - node (v12.1.0, x64)
Memory used 192,972k (± 0.10%) 193,071k (± 0.07%) +98k (+ 0.05%) 192,567k 193,251k
Parse Time 0.83s (± 0.98%) 0.83s (± 1.19%) -0.01s (- 0.60%) 0.81s 0.86s
Bind Time 0.53s (± 0.84%) 0.54s (± 0.89%) +0.01s (+ 0.94%) 0.53s 0.55s
Check Time 7.35s (± 0.60%) 7.35s (± 0.63%) -0.01s (- 0.08%) 7.23s 7.47s
Emit Time 2.49s (± 1.02%) 2.50s (± 0.97%) +0.01s (+ 0.28%) 2.44s 2.55s
Total Time 11.21s (± 0.45%) 11.21s (± 0.57%) +0.00s (+ 0.00%) 11.04s 11.34s
Monaco - node (v12.1.0, x64)
Memory used 326,185k (± 0.01%) 326,105k (± 0.05%) -80k (- 0.02%) 325,437k 326,334k
Parse Time 1.54s (± 0.63%) 1.54s (± 0.79%) -0.01s (- 0.39%) 1.51s 1.56s
Bind Time 0.74s (± 0.67%) 0.74s (± 0.83%) +0.00s (+ 0.41%) 0.73s 0.75s
Check Time 5.47s (± 0.55%) 5.47s (± 0.50%) -0.00s (- 0.00%) 5.42s 5.54s
Emit Time 3.21s (± 0.69%) 3.23s (± 0.89%) +0.02s (+ 0.72%) 3.18s 3.30s
Total Time 10.96s (± 0.24%) 10.98s (± 0.43%) +0.02s (+ 0.20%) 10.91s 11.09s
TFS - node (v12.1.0, x64)
Memory used 289,768k (± 0.02%) 289,744k (± 0.02%) -24k (- 0.01%) 289,600k 289,864k
Parse Time 1.28s (± 0.89%) 1.28s (± 0.73%) +0.00s (+ 0.16%) 1.26s 1.30s
Bind Time 0.71s (± 1.17%) 0.71s (± 1.78%) +0.01s (+ 0.71%) 0.69s 0.74s
Check Time 5.07s (± 0.43%) 5.10s (± 0.42%) +0.03s (+ 0.55%) 5.06s 5.16s
Emit Time 3.44s (± 0.85%) 3.46s (± 0.63%) +0.02s (+ 0.70%) 3.42s 3.53s
Total Time 10.50s (± 0.48%) 10.56s (± 0.40%) +0.06s (+ 0.55%) 10.46s 10.66s
material-ui - node (v12.1.0, x64)
Memory used 447,958k (± 0.02%) 447,898k (± 0.05%) -60k (- 0.01%) 446,955k 448,095k
Parse Time 1.80s (± 0.38%) 1.81s (± 0.70%) +0.01s (+ 0.39%) 1.79s 1.84s
Bind Time 0.64s (± 1.01%) 0.64s (± 0.87%) +0.00s (+ 0.00%) 0.63s 0.65s
Check Time 12.79s (± 0.55%) 12.73s (± 0.46%) -0.06s (- 0.47%) 12.61s 12.85s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.24s (± 0.50%) 15.18s (± 0.41%) -0.06s (- 0.37%) 15.04s 15.30s
xstate - node (v12.1.0, x64)
Memory used 536,802k (± 0.02%) 536,813k (± 0.02%) +12k (+ 0.00%) 536,670k 537,103k
Parse Time 2.53s (± 0.62%) 2.53s (± 0.42%) -0.01s (- 0.28%) 2.50s 2.55s
Bind Time 1.04s (± 0.36%) 1.05s (± 0.55%) +0.01s (+ 0.48%) 1.04s 1.06s
Check Time 1.47s (± 0.89%) 1.46s (± 0.52%) -0.01s (- 0.48%) 1.45s 1.48s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.12s (± 0.51%) 5.11s (± 0.35%) -0.01s (- 0.10%) 5.07s 5.15s
Angular - node (v14.15.1, x64)
Memory used 332,870k (± 0.01%) 332,862k (± 0.01%) -8k (- 0.00%) 332,787k 332,939k
Parse Time 2.02s (± 0.44%) 2.01s (± 0.34%) -0.01s (- 0.25%) 2.00s 2.03s
Bind Time 0.90s (± 1.24%) 0.89s (± 0.53%) -0.01s (- 0.56%) 0.88s 0.90s
Check Time 5.55s (± 0.45%) 5.50s (± 0.46%) -0.05s (- 0.88%) 5.45s 5.56s
Emit Time 6.23s (± 0.64%) 6.21s (± 0.72%) -0.02s (- 0.40%) 6.12s 6.32s
Total Time 14.70s (± 0.35%) 14.61s (± 0.44%) -0.09s (- 0.59%) 14.47s 14.75s
Compiler-Unions - node (v14.15.1, x64)
Memory used 193,704k (± 0.60%) 193,685k (± 0.61%) -19k (- 0.01%) 191,709k 195,025k
Parse Time 0.86s (± 0.78%) 0.85s (± 0.55%) -0.01s (- 0.82%) 0.84s 0.86s
Bind Time 0.57s (± 0.66%) 0.57s (± 0.39%) +0.00s (+ 0.71%) 0.56s 0.57s
Check Time 7.41s (± 0.70%) 7.38s (± 0.38%) -0.03s (- 0.43%) 7.32s 7.43s
Emit Time 2.47s (± 0.87%) 2.47s (± 0.74%) -0.00s (- 0.12%) 2.44s 2.51s
Total Time 11.31s (± 0.62%) 11.27s (± 0.39%) -0.04s (- 0.32%) 11.17s 11.35s
Monaco - node (v14.15.1, x64)
Memory used 325,014k (± 0.00%) 325,011k (± 0.01%) -3k (- 0.00%) 324,955k 325,062k
Parse Time 1.56s (± 0.64%) 1.56s (± 0.61%) -0.00s (- 0.06%) 1.53s 1.57s
Bind Time 0.77s (± 0.75%) 0.77s (± 0.67%) -0.00s (- 0.26%) 0.76s 0.79s
Check Time 5.38s (± 0.50%) 5.38s (± 0.44%) -0.01s (- 0.11%) 5.32s 5.43s
Emit Time 3.28s (± 0.94%) 3.27s (± 0.90%) -0.01s (- 0.18%) 3.21s 3.34s
Total Time 10.99s (± 0.48%) 10.98s (± 0.49%) -0.01s (- 0.10%) 10.86s 11.10s
TFS - node (v14.15.1, x64)
Memory used 288,587k (± 0.01%) 288,582k (± 0.01%) -6k (- 0.00%) 288,536k 288,633k
Parse Time 1.30s (± 1.49%) 1.30s (± 1.55%) +0.01s (+ 0.46%) 1.27s 1.35s
Bind Time 0.74s (± 1.67%) 0.73s (± 0.99%) -0.01s (- 0.81%) 0.71s 0.74s
Check Time 5.07s (± 0.59%) 5.07s (± 0.41%) +0.00s (+ 0.02%) 5.02s 5.12s
Emit Time 3.54s (± 1.42%) 3.54s (± 1.36%) -0.01s (- 0.14%) 3.39s 3.60s
Total Time 10.65s (± 0.63%) 10.65s (± 0.45%) +0.00s (+ 0.01%) 10.50s 10.74s
material-ui - node (v14.15.1, x64)
Memory used 446,234k (± 0.00%) 446,091k (± 0.06%) -144k (- 0.03%) 444,980k 446,253k
Parse Time 1.85s (± 0.70%) 1.85s (± 0.46%) -0.01s (- 0.32%) 1.83s 1.87s
Bind Time 0.69s (± 0.68%) 0.69s (± 0.94%) +0.00s (+ 0.29%) 0.68s 0.71s
Check Time 12.78s (± 0.56%) 12.90s (± 0.73%) +0.12s (+ 0.95%) 12.69s 13.14s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.32s (± 0.50%) 15.44s (± 0.57%) +0.12s (+ 0.78%) 15.25s 15.67s
xstate - node (v14.15.1, x64)
Memory used 534,562k (± 0.01%) 534,565k (± 0.01%) +2k (+ 0.00%) 534,450k 534,673k
Parse Time 2.57s (± 0.26%) 2.57s (± 0.53%) +0.00s (+ 0.08%) 2.54s 2.61s
Bind Time 1.16s (± 1.02%) 1.16s (± 0.72%) -0.01s (- 0.52%) 1.14s 1.18s
Check Time 1.50s (± 0.45%) 1.50s (± 0.59%) -0.01s (- 0.47%) 1.48s 1.52s
Emit Time 0.07s (± 4.92%) 0.07s (± 4.13%) -0.00s (- 2.70%) 0.07s 0.08s
Total Time 5.31s (± 0.30%) 5.31s (± 0.43%) -0.00s (- 0.02%) 5.25s 5.36s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory5 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)
  • xstate - node (v10.16.3, x64)
  • xstate - node (v12.1.0, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 47927 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot typescript-bot mentioned this pull request Feb 23, 2022
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.

DanielRosenwasser
DanielRosenwasser approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment •
edited

Choose a reason for hiding this comment

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

I think the scary thing out of this is that there may be other fragile cases like this where narrowing can create a less-capable type - but perhaps you can't get to them via property narrowing. I think fixing the never case is probably worth it for now until we can see if anything else triggers something like this.

@DanielRosenwasser DanielRosenwasser merged commit 78818e0 into main Feb 23, 2022
@DanielRosenwasser DanielRosenwasser deleted the gabritto/issue47857 branch February 23, 2022 01:34
DanielRosenwasser pushed a commit that referenced this pull request Feb 23, 2022
@typescript-bot @gabritto
🤖 Pick PR #47927 (Don't error if binding pattern type...) into releas…
78ee4ca 
…e-4.6 (#48005)

* Cherry-pick PR #47927 into release-4.6

Component commits:
c4aa31b early return if pattern type is never

* Update LKG

Co-authored-by: Gabriela Araujo Britto <gabrielaa@microsoft.com>
Co-authored-by: typescript-bot <typescript@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@RyanCavanaugh RyanCavanaugh RyanCavanaugh approved these changes

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

Assignees

@gabritto gabritto

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeScript 4.6 sometimes treats tuple union types as "never" when switch-case has a "default" clause
4 participants
@gabritto @DanielRosenwasser @typescript-bot @RyanCavanaugh