No type narrowing for discriminated union type guards on unique symbol properties

[kpdonn]

Note: I know about #10530 but I explain why I think this deserves its own issue in the related issues section below

TypeScript Version: 2.9.0-dev.20180403

Search Terms:
unique symbol, discriminated union, type guard, narrow, narrowing

Code

declare const kind: unique symbol

type Square = {
  readonly [kind]: "square"
  length: number
  width: number
}

type Circle = {
  readonly [kind]: "circle"
  radius: number
}

type Shape = Square | Circle

function doesNotNarrow(shape: Shape) {
  if (shape[kind] === "square") {
    console.log(shape.length, shape.width) // error length does not exist on Shape 
  } else {
    console.log(shape.radius) // error radius does not exist on Shape 
  }
}

Expected behavior:
No errors because the type should be narrowed inside the if statements.

Actual behavior:
Compile errors because typescript does not narrow the type despite the discriminated union.

Playground Link:
Link

Related Issues:

• Discriminant property type guard not applied with bracket notation #10530 Discriminant property type guard not applied with bracket notation

I know this looks like a straight duplicate but I don't think it should be considered one because symbols aren't mentioned anywhere in that issue or in the titles of any of its duplicates, and unique symbol did not exist when that issue was originally considered.

The only reason that #10530 hasn't been fixed is because it caused enough of a performance slowdown that it wasn't considered a worthwhile tradeoff. I'm hoping that limiting this to only unique symbols will have a small enough performance impact to make it worthwhile this time.

Unlike most string keys, symbols can only be used with bracket notation so I think the value proposition here is much higher than it was for string keys. Plus it seems like using a symbol as a discriminant property that doesn't have to worry about conflicting with other property names is one of the most natural use cases for symbols.

[mhegazy]

The only reason that #10530 hasn't been fixed is because it caused enough of a performance slowdown that it wasn't considered a worthwhile tradeoff. I'm hoping that limiting this to only unique symbols will have a small enough performance impact to make it worthwhile this time.

i am afraid the issue is not what you are indexing with. the issue is the binder currently marks places that can "possibly" narrow the type of a symbol, and puts them in a control flow graph. every time the type for a symbol is needed the graph is walked to get the type at that location. currently a[..] is not included as a possible narrowing location. adding it makes walking the control flow graph much more expensive (with all array accesses and such). It does not matter what it is that you are indexing with really.

[mhegazy]

We usually like to keep one ticket per issue. So closing in favor of #10530.

[kpdonn]

Two points: Are you sure that this would be included if #10530 is ever fixed? I could be misinterpreting it but looking at the old PR that originally tried to fix it, especially the test cases for it, it seems to have only been focused on direct string and number literal accesses like obj["kind"] but I’m not seeing clear indications it was ever intended to address cases like

const kind = "kind"
obj[kind]

If it isn’t likely to address that case then it seems possible that even #10530 wouldn’t fix symbols and this issue would still be relevant.

Second, isn’t it at least possible that this could end up being addressed separately? I know you just said the issue isn’t what you are indexing with, which I think I understand: the performance problem is essentially in the binder and the binder doesn’t know what type anything is. But it seems like it could potentially be special cased given how very restricted the places a unique symbol can come from are. Not that special casing it would be desirable, but right now unique symbol is a second class citizen in typescript which isn’t desirable either.

[mhegazy]

Are you sure that this would be included if #10530 is ever fixed?

I do not know. the issue is open still so i would not rule it out. I would recommend subscribing to the issue for further updates.

If it isn’t likely to address that case then it seems possible that even #10530 wouldn’t fix symbols and this issue would still be relevant.

As i noted earlier, to allow narrowing on strings, numbers, symbols in element access expressions (a[exp]) we need to add that to the control flow graph, and that is what is causing the perf regression.

[typescript-bot]

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.