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

Control flow analysis of aliased conditional expressions and discriminants #44730

Merged
merged 5 commits into from
Jun 25, 2021

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Jun 24, 2021

With this PR we support control flow analysis of conditional expressions and discriminant property accesses indirectly referenced through const variables. For example:

function f1(x: unknown) {
    const isString = typeof x === 'string';
    if (isString) {
        x.length;  // Ok
    }
}

function f2(obj: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
    const isFoo = obj.kind === 'foo';
    if (isFoo) {
        obj.foo;  // Ok
    }
    else {
        obj.bar;  // Ok
    }
}

function f3(obj: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
    const { kind } = obj;
    if (kind === 'foo') {
        obj.foo;  // Ok
    }
    else {
        obj.bar;  // Ok
    }
}

function f4(obj: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
    const k = obj.kind;
    if (k === 'foo') {
        obj.foo;  // Ok
    }
    else {
        obj.bar;  // Ok
    }
}

Intuitively, indirect references to conditional expressions or discriminant property accesses behave exactly as if the expressions were written in-line.

Narrowing through indirect references occurs only when the conditional expression or discriminant property access is declared in a const variable declaration with no type annotation, and the reference being narrowed is a const variable, a readonly property, or a parameter for which there are no assignments in the function body.

Some examples where narrowing does not occur (but it would had the indirectly referenced conditions been written in-line):

function f5(x: unknown) {
    let isString = typeof x === 'string';
    if (isString) {
        x.length;  // Error, no narrowing because isString isn't 'const'
    }
}

function f6(obj: { kind: 'foo', foo: string } | { kind: 'bar', bar: number }) {
    const isFoo = obj.kind === 'foo';
    obj = obj;
    if (isFoo) {
        obj.foo;  // Error, no narrowing because 'obj' is assigned in function body
    }
}

Up to five levels of indirection are analyzed in conditional expressions. For example, two levels of indirections are analyzed in the following code:

function f7(x: string | number | boolean) {
    const isString = typeof x === "string";
    const isNumber = typeof x === "number";
    const isStringOrNumber = isString || isNumber;
    if (isStringOrNumber) {
        x;  // string | number
    }
    else {
        x;  // boolean
    }
}

It is possible to mix indirect conditional expressions and directly specified conditions:

function f8(value: number | undefined) : number {
    const isNumber = typeof value == "number";
    if (isNumber && value > 0) {
        return value * 10;
    }
    return 0;
}

This PR fixes most of the long list of issues that were closed as duplicates of #12184, but not all. In particular, the pattern of destructuring a discriminant property and a payload property into two local variables and expecting a coupling between the two is not supported as the control flow analyzer doesn't "see" the connection. For example:

type Data = { kind: 'str', payload: string } | { kind: 'num', payload: number };

function foo({ kind, payload }: Data) {
    if (kind === 'str') {
        payload.length;  // Error, payload not narrowed to string
    }
}

We may be able to support that pattern later, but likely not in this PR.

Fixes #12184.
Fixes #19421.
Fixes #19943.
Fixes #24865.
Fixes #26804.
Fixes #31037.
Fixes #31059.
Fixes #31291.
Fixes #31344.
Fixes #31870.
Fixes #33192.
Fixes #34535.
Fixes #35603.
Fixes #36510.
Fixes #37638.
Fixes #37855.
Fixes #39657.
Fixes #39996.
Fixes #40201.
Fixes #40341.
Fixes #41266.
Fixes #41870.
Fixes #43333.

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

@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 Jun 24, 2021

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

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

@Kingwl
Copy link
Contributor

Kingwl commented Jun 24, 2021

Cooooooooooooooool!!!!!! Great work!!!!!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..44730

Metric main 44730 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 344,407k (± 0.02%) 344,459k (± 0.02%) +52k (+ 0.01%) 344,275k 344,590k
Parse Time 1.79s (± 0.57%) 1.80s (± 0.59%) +0.01s (+ 0.73%) 1.78s 1.84s
Bind Time 0.84s (± 0.48%) 0.84s (± 0.66%) +0.00s (+ 0.36%) 0.83s 0.85s
Check Time 5.19s (± 0.50%) 5.34s (± 0.60%) +0.15s (+ 2.89%) 5.27s 5.41s
Emit Time 5.67s (± 1.49%) 5.61s (± 0.61%) -0.06s (- 0.99%) 5.55s 5.70s
Total Time 13.49s (± 0.83%) 13.60s (± 0.45%) +0.11s (+ 0.79%) 13.49s 13.73s
Compiler-Unions - node (v10.16.3, x64)
Memory used 201,023k (± 0.03%) 201,308k (± 0.06%) +286k (+ 0.14%) 200,914k 201,504k
Parse Time 0.78s (± 0.71%) 0.79s (± 0.63%) +0.00s (+ 0.38%) 0.78s 0.80s
Bind Time 0.53s (± 1.10%) 0.53s (± 1.23%) +0.00s (+ 0.19%) 0.51s 0.54s
Check Time 7.55s (± 0.59%) 7.88s (± 0.74%) +0.33s (+ 4.33%) 7.80s 8.07s
Emit Time 2.29s (± 1.01%) 2.28s (± 1.50%) -0.01s (- 0.26%) 2.23s 2.38s
Total Time 11.15s (± 0.48%) 11.48s (± 0.71%) +0.33s (+ 2.92%) 11.36s 11.73s
Monaco - node (v10.16.3, x64)
Memory used 340,445k (± 0.01%) 340,467k (± 0.02%) +22k (+ 0.01%) 340,317k 340,638k
Parse Time 1.46s (± 0.51%) 1.46s (± 0.65%) -0.00s (- 0.07%) 1.43s 1.47s
Bind Time 0.74s (± 0.81%) 0.74s (± 1.10%) +0.01s (+ 1.09%) 0.72s 0.76s
Check Time 5.33s (± 0.49%) 5.45s (± 0.85%) +0.12s (+ 2.21%) 5.34s 5.53s
Emit Time 3.02s (± 1.01%) 2.99s (± 0.55%) -0.03s (- 0.99%) 2.95s 3.03s
Total Time 10.54s (± 0.29%) 10.63s (± 0.54%) +0.10s (+ 0.93%) 10.52s 10.75s
TFS - node (v10.16.3, x64)
Memory used 303,727k (± 0.02%) 303,726k (± 0.02%) -1k (- 0.00%) 303,586k 303,809k
Parse Time 1.17s (± 0.51%) 1.19s (± 0.59%) +0.01s (+ 1.19%) 1.18s 1.21s
Bind Time 0.71s (± 0.71%) 0.71s (± 0.47%) +0.00s (+ 0.71%) 0.70s 0.72s
Check Time 4.85s (± 0.57%) 4.97s (± 0.49%) +0.12s (+ 2.50%) 4.91s 5.03s
Emit Time 3.14s (± 1.57%) 3.14s (± 1.52%) +0.01s (+ 0.29%) 3.01s 3.23s
Total Time 9.86s (± 0.58%) 10.02s (± 0.58%) +0.15s (+ 1.52%) 9.87s 10.15s
material-ui - node (v10.16.3, x64)
Memory used 471,419k (± 0.01%) 471,361k (± 0.01%) -59k (- 0.01%) 471,261k 471,470k
Parse Time 1.72s (± 0.42%) 1.73s (± 0.74%) +0.01s (+ 0.81%) 1.70s 1.76s
Bind Time 0.66s (± 0.61%) 0.66s (± 1.06%) +0.00s (+ 0.30%) 0.64s 0.67s
Check Time 14.14s (± 0.47%) 14.12s (± 0.51%) -0.02s (- 0.13%) 13.97s 14.33s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.51s (± 0.39%) 16.51s (± 0.48%) -0.00s (- 0.02%) 16.33s 16.72s
Angular - node (v12.1.0, x64)
Memory used 322,463k (± 0.11%) 322,767k (± 0.03%) +304k (+ 0.09%) 322,599k 323,001k
Parse Time 1.78s (± 0.67%) 1.78s (± 0.49%) +0.00s (+ 0.28%) 1.76s 1.80s
Bind Time 0.83s (± 0.58%) 0.83s (± 0.80%) +0.00s (+ 0.36%) 0.82s 0.85s
Check Time 5.11s (± 0.61%) 5.23s (± 0.40%) +0.12s (+ 2.33%) 5.18s 5.28s
Emit Time 5.85s (± 1.07%) 5.88s (± 0.78%) +0.02s (+ 0.41%) 5.79s 6.04s
Total Time 13.57s (± 0.58%) 13.72s (± 0.47%) +0.15s (+ 1.13%) 13.64s 13.95s
Compiler-Unions - node (v12.1.0, x64)
Memory used 188,537k (± 0.07%) 188,783k (± 0.10%) +246k (+ 0.13%) 188,275k 189,034k
Parse Time 0.77s (± 0.84%) 0.77s (± 0.77%) +0.01s (+ 0.78%) 0.77s 0.79s
Bind Time 0.53s (± 0.42%) 0.53s (± 0.92%) +0.00s (+ 0.95%) 0.52s 0.54s
Check Time 7.05s (± 0.43%) 7.36s (± 0.75%) +0.31s (+ 4.35%) 7.27s 7.51s
Emit Time 2.33s (± 0.81%) 2.35s (± 1.26%) +0.03s (+ 1.20%) 2.29s 2.42s
Total Time 10.67s (± 0.31%) 11.03s (± 0.48%) +0.35s (+ 3.30%) 10.94s 11.21s
Monaco - node (v12.1.0, x64)
Memory used 323,543k (± 0.01%) 323,528k (± 0.06%) -15k (- 0.00%) 322,733k 323,711k
Parse Time 1.41s (± 0.66%) 1.42s (± 0.65%) +0.01s (+ 0.99%) 1.40s 1.44s
Bind Time 0.71s (± 0.63%) 0.72s (± 0.31%) +0.01s (+ 0.98%) 0.71s 0.72s
Check Time 5.17s (± 0.27%) 5.30s (± 0.66%) +0.14s (+ 2.65%) 5.24s 5.42s
Emit Time 3.07s (± 0.93%) 3.08s (± 0.61%) +0.01s (+ 0.33%) 3.05s 3.14s
Total Time 10.35s (± 0.29%) 10.52s (± 0.42%) +0.17s (+ 1.64%) 10.44s 10.65s
TFS - node (v12.1.0, x64)
Memory used 288,402k (± 0.01%) 288,418k (± 0.02%) +16k (+ 0.01%) 288,304k 288,547k
Parse Time 1.19s (± 0.69%) 1.19s (± 0.73%) +0.00s (+ 0.08%) 1.17s 1.21s
Bind Time 0.69s (± 0.53%) 0.69s (± 0.89%) +0.01s (+ 0.73%) 0.68s 0.70s
Check Time 4.76s (± 0.58%) 4.90s (± 0.55%) +0.13s (+ 2.81%) 4.86s 4.96s
Emit Time 3.19s (± 1.09%) 3.19s (± 0.87%) -0.00s (- 0.03%) 3.14s 3.24s
Total Time 9.83s (± 0.54%) 9.96s (± 0.55%) +0.13s (+ 1.37%) 9.87s 10.08s
material-ui - node (v12.1.0, x64)
Memory used 449,982k (± 0.06%) 449,993k (± 0.06%) +10k (+ 0.00%) 448,924k 450,201k
Parse Time 1.71s (± 0.35%) 1.71s (± 0.63%) -0.00s (- 0.18%) 1.69s 1.74s
Bind Time 0.64s (± 1.16%) 0.64s (± 1.06%) +0.00s (+ 0.62%) 0.63s 0.66s
Check Time 12.64s (± 0.39%) 12.77s (± 0.54%) +0.13s (+ 1.03%) 12.62s 12.90s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.99s (± 0.34%) 15.13s (± 0.48%) +0.13s (+ 0.88%) 14.99s 15.28s
Angular - node (v14.15.1, x64)
Memory used 321,281k (± 0.01%) 321,351k (± 0.01%) +70k (+ 0.02%) 321,278k 321,394k
Parse Time 1.79s (± 0.56%) 1.81s (± 0.74%) +0.02s (+ 1.34%) 1.78s 1.84s
Bind Time 0.87s (± 0.57%) 0.88s (± 0.86%) +0.01s (+ 1.15%) 0.87s 0.90s
Check Time 5.08s (± 0.32%) 5.22s (± 0.43%) +0.14s (+ 2.77%) 5.19s 5.28s
Emit Time 5.88s (± 0.57%) 5.91s (± 0.53%) +0.03s (+ 0.58%) 5.85s 5.96s
Total Time 13.62s (± 0.35%) 13.82s (± 0.36%) +0.20s (+ 1.48%) 13.73s 13.96s
Compiler-Unions - node (v14.15.1, x64)
Memory used 189,136k (± 0.62%) 187,846k (± 0.38%) -1,290k (- 0.68%) 187,466k 190,729k
Parse Time 0.80s (± 0.50%) 0.80s (± 0.74%) +0.00s (+ 0.50%) 0.79s 0.82s
Bind Time 0.56s (± 0.61%) 0.56s (± 0.85%) 0.00s ( 0.00%) 0.55s 0.57s
Check Time 7.17s (± 0.46%) 7.50s (± 0.95%) +0.33s (+ 4.63%) 7.39s 7.71s
Emit Time 2.29s (± 0.72%) 2.31s (± 0.69%) +0.03s (+ 1.09%) 2.28s 2.35s
Total Time 10.81s (± 0.35%) 11.18s (± 0.78%) +0.37s (+ 3.40%) 11.02s 11.44s
Monaco - node (v14.15.1, x64)
Memory used 322,490k (± 0.01%) 322,526k (± 0.01%) +36k (+ 0.01%) 322,489k 322,599k
Parse Time 1.47s (± 0.39%) 1.48s (± 0.75%) +0.01s (+ 0.75%) 1.47s 1.52s
Bind Time 0.75s (± 1.02%) 0.75s (± 0.50%) -0.00s (- 0.13%) 0.74s 0.75s
Check Time 5.14s (± 0.67%) 5.27s (± 0.33%) +0.12s (+ 2.37%) 5.23s 5.31s
Emit Time 3.11s (± 1.03%) 3.11s (± 0.63%) +0.00s (+ 0.06%) 3.08s 3.16s
Total Time 10.47s (± 0.54%) 10.61s (± 0.31%) +0.14s (+ 1.31%) 10.56s 10.69s
TFS - node (v14.15.1, x64)
Memory used 287,388k (± 0.00%) 287,420k (± 0.01%) +32k (+ 0.01%) 287,363k 287,465k
Parse Time 1.25s (± 0.79%) 1.26s (± 0.95%) +0.01s (+ 0.64%) 1.24s 1.29s
Bind Time 0.71s (± 0.42%) 0.71s (± 0.42%) 0.00s ( 0.00%) 0.71s 0.72s
Check Time 4.79s (± 0.32%) 4.89s (± 0.52%) +0.10s (+ 2.04%) 4.85s 4.96s
Emit Time 3.26s (± 0.62%) 3.28s (± 0.61%) +0.01s (+ 0.43%) 3.23s 3.32s
Total Time 10.03s (± 0.30%) 10.14s (± 0.29%) +0.12s (+ 1.19%) 10.07s 10.21s
material-ui - node (v14.15.1, x64)
Memory used 448,227k (± 0.06%) 448,276k (± 0.03%) +49k (+ 0.01%) 447,664k 448,438k
Parse Time 1.75s (± 0.44%) 1.75s (± 0.42%) -0.00s (- 0.06%) 1.73s 1.77s
Bind Time 0.69s (± 0.48%) 0.69s (± 0.58%) +0.00s (+ 0.14%) 0.68s 0.70s
Check Time 12.83s (± 0.67%) 12.86s (± 0.43%) +0.03s (+ 0.20%) 12.75s 12.96s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.27s (± 0.56%) 15.30s (± 0.32%) +0.03s (+ 0.20%) 15.20s 15.39s
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 44730 10
Baseline main 10

Developer Information:

Download Benchmark

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.

Implementation-wise, this looks OK (is there a reason for specifically limiting the nesting analysis to two levels?); but I think the new tests have been elided, since you have a bunch of examples in the OP, but there's no new tests showing the functionality. (I'm interested in some with readonly props, both static and instance, since those don't have examples in your OP, but are referenced in some comments, even though later there's a check for exactly VariableDeclarations)

@@ -23553,6 +23581,17 @@ namespace ts {
break;
case SyntaxKind.CommaToken:
return narrowType(type, expr.right, assumeTrue);
// Ordinarily we won't see && and || expressions in control flow analysis because the Binder breaks those
Copy link
Member

Choose a reason for hiding this comment

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

What about ternary expressions?

Copy link
Member Author

Choose a reason for hiding this comment

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

We narrow within the branches of ternary expressions, but not based on the result of ternary expressions, so I don't think there is anything to do there.

@ahejlsberg
Copy link
Member Author

is there a reason for specifically limiting the nesting analysis to two levels?

I'd like to have a fixed limit so we can avoid maintaining a stack of inlined symbols that would otherwise be needed (to catch accidentally recursive definitions). Two levels is rather arbitrary, could make it three or even five.

I think the new tests have been elided

Yeah, haven't added tests yet.

@RyanCavanaugh
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

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

@ahejlsberg
Copy link
Member Author

I'm interested in some with readonly props, both static and instance, since those don't have examples in your OP, but are referenced in some comments, even though later there's a check for exactly VariableDeclarations

The isVariableDeclaration checks are all related to the declaration of the aliased condition or discriminant access, and are unrelated to how the referenced being narrowed was declared. But sure, we should have tests for all different kinds.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

Hey @RyanCavanaugh, 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/105495/artifacts?artifactName=tgz&fileId=94FC84B182999B7D6DCA47117848030F812A9A364841F767F760CB8D5DBAAF0402&fileName=/typescript-4.4.0-insiders.20210624.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.4.0-pr-44730-11".;

@IllusionMH
Copy link
Contributor

IllusionMH commented Jun 24, 2021

Is it possible to also add narrowing for constants from same destructurization?

function test(data: { type: "num", value: number } | { type: "str", value: string }) {
  const { type, value } = data;
  if (type === "num") {
    value.toFixed(); // currently error
    data.value.toFixed(); // ok
  } else {
    value.charAt(0); // currently error
    data.value.charAt(0); // ok
  }
}

UPD. As mentioned in edit I've missed - maybe later, but not in this PR.

@noseratio
Copy link

@ahejlsberg, would that be possible to extend this for classes as well, e.g.:

class T {
    s = "";
}

function f1(x: unknown) {
    const isT = x instanceof T;
    if (isT) {
        x.s;  // Ok
    }
}

Also, maybe to include some other type checks, e.g., const isString = x?.constructor === String, which works for both primitive string values and instances of String class.

@acutmore
Copy link
Contributor

acutmore commented Jun 25, 2021

@ahejlsberg, would that be possible to extend this for classes as well, e.g.:

class T {
    s = "";
}

function f1(x: unknown) {
    const isT = x instanceof T;
    if (isT) {
        x.s;  // Ok
    }
}

Already working https://www.staging-typescript.org/play?ts=4.4.0-pr-44730-11#code/MYGwhgzhAEAq0G8BQ1XRgXmgImwbiQF8kkAzAVwDtgAXASwHtLpSBGACgA8AuaKga0oMA7pQCUiFGmBMINaHQjwsnBZTlhqAUwak4BNAr3tFsCckOHOAOgh5UAegfQA8vympihIA ❤️

@noseratio
Copy link

noseratio commented Jun 25, 2021

Already working

Very cool! Interestingly, I can use object instead of unknown and it still works :)

@fatcerberus
Copy link

Narrowing through indirect references occurs only when the conditional expression or discriminant property access is declared in a const variable declaration with no type annotation, and the reference being narrowed is a const variable, a readonly property, or a parameter for which there are no assignments in the function body.

Should I take this mean narrowing won't work on non-readonly properties? That limits the utility of this IMO; I pretty much never use the readonly keyword in my interfaces.

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.

It would still be nice to have tests documenting our behavior for other const-ish variable types (eg, static read-only fields) which alias a guard, if only to say we don't currently support it.

@ahejlsberg
Copy link
Member Author

Should I take this mean narrowing won't work on non-readonly properties? That limits the utility of this IMO; I pretty much never use the readonly keyword in my interfaces.

Correct. For example, the following fails if data isn't marked readonly:

function test(obj: { readonly data: string | number }) {
    const isString = typeof obj.data === 'string';
    if (isString) {
        let s: string = obj.data;
    }
}

The rationale here is that we can't know for certain that data hasn't been modified since the condition was evaluated. We'd effectively have to check that there are no assignments to obj.data between the declaration of isString and the reference to isString in the if statement. This adds complexity and could be prohibitively expensive.

It does of course work if you copy the mutable property into a const local:

function test(obj: { data: string | number }) {
    const { data } = obj;
    const isString = typeof data === 'string';
    if (isString) {
        let s: string = data;
    }
}

@jcalz
Copy link
Contributor

jcalz commented Aug 10, 2021

In particular, the pattern of destructuring a discriminant property and a payload property into two local variables and expecting a coupling between the two is not supported as the control flow analyzer doesn't "see" the connection. For example:

type Data = { kind: 'str', payload: string } | { kind: 'num', payload: number };

function foo({ kind, payload }: Data) {
    if (kind === 'str') {
        payload.length;  // Error, payload not narrowed to string
    }
}

We may be able to support that pattern later, but likely not in this PR.

Cross-linking to #30581.

@karlhorky
Copy link
Contributor

Cross-linking to #30581.

Thanks @jcalz ! Correlated unions sound cool!

@ahejlsberg would this be the correct issue to watch for this feature of "coupling" between destructured properties / elements?

@JeanMeche
Copy link
Contributor

@ahejlsberg
Could it maybe possible to reflect on the type that is hold that predicate behaviour ?

That would be to prevent that kind of weird behaviour !

case SyntaxKind.AmpersandAmpersandToken:
return assumeTrue ?
narrowType(narrowType(type, expr.left, /*assumeTrue*/ true), expr.right, /*assumeTrue*/ true) :
getUnionType([narrowType(type, expr.left, /*assumeTrue*/ false), narrowType(type, expr.right, /*assumeTrue*/ false)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

When assumeTrue is false, theoretically expr.right is reachable if and only if expr.left is truthy?

-getUnionType([narrowType(type, expr.left, /*assumeTrue*/ false), narrowType(type, expr.right, /*assumeTrue*/ false)])
+getUnionType([narrowType(type, expr.left, /*assumeTrue*/ false), narrowType(narrowType(type, expr.left, /*assumeTrue*/ true), expr.right, /*assumeTrue*/ false)])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment