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

Narrow on element access of literal #26424

Merged
merged 7 commits into from Aug 15, 2018

Conversation

sandersn
Copy link
Member

This means that, for example, the tuple [number, string?] allows its second element to be narrowed with element access:

export function f(pair: [number, string?]): string {
  return pair[1] ? pair[1] : 'nope';
}

Or you can narrow on properties that don't work with property access:

interface Square {
    ["dash-ok"]: "square";
    ["square-size"]: number;
}
 interface Rectangle {
    ["dash-ok"]: "rectangle";
    width: number;
    height: number;
}
 interface Circle {
    ["dash-ok"]: "circle";
    radius: number;
}
 type Shape = Square | Rectangle | Circle;
function area(s: Shape): number {
    switch(s['dash-ok']) {
        case "square": return s['square-size'] * s['square-size'];
        case "rectangle": return s.width * s['height'];
        case "circle": return Math.PI * s['radius'] * s.radius;
    }
}

This only works for actual literal references, not expressions with literal types, to prevent the binder from making the control flow graph much bigger than it is currently. In other words, this doesn't work:

export function f(pair: [number, string?]): string {
  const one = 1
  return pair[one] ? pair[one] : 'nope';
}

Performance is not significantly different from today, even on the Compiler, Unions Edition, which is full of very large unions:

┌─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┐
│ Project     │            Baseline │             Current │             Delta │     Best │    Worst │
╞═════════════╧═════════════════════╧═════════════════════╧═══════════════════╧══════════╧══════════╡
│ Angular - node (v10.7.0, x64)                                                                     │
├─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┤
│ Memory used │ 366,503k (±  0.01%) │ 366,586k (±  0.01%) │   +82k (+  0.02%) │ 366,421k │ 366,712k │
│ Parse Time  │    1.46s (±  0.97%) │    1.47s (±  0.97%) │ +0.01s (+  0.48%) │    1.43s │    1.56s │
│ Bind Time   │    0.66s (±  0.86%) │    0.67s (±  1.05%) │ +0.01s (+  1.29%) │    0.65s │    0.69s │
│ Check Time  │    3.44s (±  0.48%) │    3.47s (±  0.60%) │ +0.02s (+  0.71%) │    3.41s │    3.60s │
│ Emit Time   │    4.85s (±  0.59%) │    4.88s (±  0.52%) │ +0.03s (+  0.55%) │    4.82s │    5.05s │
├─────────────┼─────────────────────┼─────────────────────┼───────────────────┼──────────┼──────────┤
│ Total Time  │   10.41s (±  0.29%) │   10.47s (±  0.35%) │ +0.06s (+  0.62%) │   10.37s │   10.65s │
╞═════════════╧═════════════════════╧═════════════════════╧═══════════════════╧══════════╧══════════╡
│ Compiler - node (v10.7.0, x64)                                                                    │
├─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┤
│ Memory used │ 198,369k (±  0.33%) │ 197,846k (±  0.22%) │  -523k (-  0.26%) │ 197,377k │ 200,636k │
│ Parse Time  │    0.61s (±  1.05%) │    0.61s (±  1.76%) │ +0.01s (+  0.91%) │    0.59s │    0.69s │
│ Bind Time   │    0.67s (±  1.26%) │    0.67s (±  1.29%) │ +0.00s (+  0.15%) │    0.65s │    0.71s │
│ Check Time  │    2.88s (±  0.61%) │    2.89s (±  0.95%) │ +0.01s (+  0.33%) │    2.82s │    3.00s │
│ Emit Time   │    2.00s (±  0.69%) │    2.01s (±  0.74%) │ +0.00s (+  0.07%) │    1.97s │    2.09s │
├─────────────┼─────────────────────┼─────────────────────┼───────────────────┼──────────┼──────────┤
│ Total Time  │    6.15s (±  0.38%) │    6.17s (±  0.56%) │ +0.02s (+  0.29%) │    6.07s │    6.33s │
╞═════════════╧═════════════════════╧═════════════════════╧═══════════════════╧══════════╧══════════╡
│ Compiler - Unions - node (v10.7.0, x64)                                                           │
├─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┤
│ Memory used │ 219,013k (±  0.27%) │ 218,586k (±  0.13%) │  -427k (-  0.20%) │ 218,148k │ 221,204k │
│ Parse Time  │    0.56s (±  1.42%) │    0.56s (±  1.19%) │ -0.00s (-  0.18%) │    0.54s │    0.59s │
│ Bind Time   │    0.62s (±  1.19%) │    0.62s (±  1.09%) │ +0.00s (+  0.65%) │    0.60s │    0.65s │
│ Check Time  │    9.26s (±  0.46%) │    9.27s (±  0.43%) │ +0.01s (+  0.12%) │    9.09s │    9.42s │
│ Emit Time   │    1.92s (±  0.73%) │    1.90s (±  0.91%) │ -0.02s (-  0.81%) │    1.85s │    1.98s │
├─────────────┼─────────────────────┼─────────────────────┼───────────────────┼──────────┼──────────┤
│ Total Time  │   12.36s (±  0.43%) │   12.36s (±  0.34%) │ -0.00s (-  0.00%) │   12.22s │   12.54s │
╞═════════════╧═════════════════════╧═════════════════════╧═══════════════════╧══════════╧══════════╡
│ Monaco - node (v10.7.0, x64)                                                                      │
├─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┤
│ Memory used │ 370,896k (±  0.01%) │ 370,943k (±  0.01%) │   +48k (+  0.01%) │ 370,735k │ 371,128k │
│ Parse Time  │    1.18s (±  1.07%) │    1.18s (±  1.30%) │ +0.01s (+  0.47%) │    1.12s │    1.24s │
│ Bind Time   │    0.60s (±  1.26%) │    0.61s (±  0.93%) │ +0.01s (+  1.00%) │    0.59s │    0.63s │
│ Check Time  │    3.45s (±  0.71%) │    3.47s (±  0.65%) │ +0.02s (+  0.61%) │    3.41s │    3.56s │
│ Emit Time   │    2.43s (±  0.68%) │    2.44s (±  0.55%) │ +0.01s (+  0.39%) │    2.40s │    2.50s │
├─────────────┼─────────────────────┼─────────────────────┼───────────────────┼──────────┼──────────┤
│ Total Time  │    7.65s (±  0.50%) │    7.70s (±  0.35%) │ +0.04s (+  0.59%) │    7.61s │    7.82s │
╞═════════════╧═════════════════════╧═════════════════════╧═══════════════════╧══════════╧══════════╡
│ TFS - node (v10.7.0, x64)                                                                         │
├─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┤
│ Memory used │ 328,758k (±  0.01%) │ 328,830k (±  0.02%) │   +72k (+  0.02%) │ 328,637k │ 328,995k │
│ Parse Time  │    0.88s (±  1.52%) │    0.87s (±  1.05%) │ -0.01s (-  0.68%) │    0.85s │    0.92s │
│ Bind Time   │    0.54s (±  1.69%) │    0.54s (±  0.86%) │ -0.00s (-  0.65%) │    0.52s │    0.55s │
│ Check Time  │    3.46s (±  0.66%) │    3.46s (±  0.82%) │ +0.01s (+  0.16%) │    3.39s │    3.64s │
│ Emit Time   │    2.56s (±  0.69%) │    2.57s (±  0.80%) │ +0.01s (+  0.35%) │    2.49s │    2.65s │
├─────────────┼─────────────────────┼─────────────────────┼───────────────────┼──────────┼──────────┤
│ Total Time  │    7.44s (±  0.51%) │    7.44s (±  0.55%) │ +0.01s (+  0.09%) │    7.34s │    7.66s │
╞═════════════╧═════════════════════╧═════════════════════╧═══════════════════╧══════════╧══════════╡
│ skype4life - node (v10.7.0, x64)                                                                  │
├─────────────┬─────────────────────┬─────────────────────┬───────────────────┬──────────┬──────────┤
│ Memory used │ 821,863k (±  0.00%) │ 822,017k (±  0.00%) │  +154k (+  0.02%) │ 821,873k │ 822,173k │
│ Parse Time  │    3.48s (±  1.03%) │    3.46s (±  0.68%) │ -0.02s (-  0.56%) │    3.38s │    3.58s │
│ Bind Time   │    1.37s (±  1.76%) │    1.37s (±  1.25%) │ -0.00s (-  0.07%) │    1.32s │    1.46s │
│ Check Time  │    9.76s (±  0.33%) │    9.79s (±  0.45%) │ +0.03s (+  0.32%) │    9.66s │   10.02s │
│ Emit Time   │    7.13s (±  0.58%) │    7.18s (±  0.59%) │ +0.05s (+  0.76%) │    7.05s │    7.39s │
├─────────────┼─────────────────────┼─────────────────────┼───────────────────┼──────────┼──────────┤
│ Total Time  │   21.74s (±  0.29%) │   21.80s (±  0.32%) │ +0.06s (+  0.29%) │   21.54s │   22.18s │
└─────────────┴─────────────────────┴─────────────────────┴───────────────────┴──────────┴──────────┘

Finally, unions with index signatures behave slightly differently:

type Urg = { 0: string } | { [n: number]: number }
var u: Urg = { 0: 1 }
const n: number = u[0]
u[0] = 'no'

Which is visible in one baseline change. I'll check the user tests and Definitely Typed, then report back.

Fixes #10530 (and probably a few others that are still open; there are tons of duplicates of this one.)

This means that, for example, the tuple `[number, string?]` allows its
second element to be narrowed with element access:

```ts
export function f(pair: [number, string?]): string {
  return pair[1] ? pair[1] : 'nope';
}
```
@sandersn
Copy link
Member Author

sandersn commented Aug 13, 2018

Webpack breaks with this pattern:

// @Filename: declarations.d.ts
declare function mappy(map: { [s: string]: number }): void;
// @Filename: Compilation.js
export class C {
    constructor() {
        /** @type {{ [assetName: string]: number}} */
        this.assets = {};
    }
    m() {
        // Error: '{}' is not assignable to '{ [assetName: string]: number }'
        mappy(this.assets)
    }
}

Note that this is because of our this-property assignment inference; the equivalent Typescript code doesn't treat the initializer of a class' property declaration as a narrowing assignment.

Edit: Never mind, this is not narrowing, and it repros on master. I'll fix it separately.

@sandersn sandersn added the Breaking Change Would introduce errors in existing code label Aug 13, 2018
@sandersn
Copy link
Member Author

No breaks from Definitely Typed either, so I'm pretty confident that the Breaking Change label on this PR is only theoretical code that we happen to have in our test suite.

@sandersn sandersn merged commit 2bfd919 into master Aug 15, 2018
@RyanCavanaugh RyanCavanaugh deleted the narrow-on-element-access-of-literal branch August 15, 2018 17:39
sandersn added a commit that referenced this pull request Oct 5, 2018
sandersn added a commit that referenced this pull request Oct 5, 2018
@ulrichb
Copy link

ulrichb commented Nov 6, 2018

@sandersn Symbols are still not supported. => Should #23135 be reopened?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants