Repository navigation
No type narrowing for discriminated union type guards on unique symbol properties #23135
Description
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.
Activity
mhegazy commentedon Apr 4, 2018
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 commentedon Apr 4, 2018
We usually like to keep one ticket per issue. So closing in favor of #10530.
kpdonn commentedon Apr 4, 2018
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 likeIf 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 symbolcan come from are. Not that special casing it would be desirable, but right nowunique symbolis a second class citizen in typescript which isn’t desirable either.mhegazy commentedon Apr 4, 2018
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.
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 commentedon Apr 19, 2018
Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.