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

Element access with non-literal constant key should be narrowable #36230

Closed
thany opened this issue Jan 16, 2020 · 12 comments · Fixed by #45974
Closed

Element access with non-literal constant key should be narrowable #36230

thany opened this issue Jan 16, 2020 · 12 comments · Fixed by #45974
Labels
Experience Enhancement Noncontroversial enhancements Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript

Comments

@thany
Copy link

thany commented Jan 16, 2020

TypeScript Version: 3.7.4 (also tried vNightly on the playground)

Search Terms: exhaustive type check array

Code

Error ts2532:

type Progress = Array<number | undefined> | undefined;

const progress: Progress = [1, 2, 3];
const index = 1;

if (progress !== undefined && progress[index] !== undefined && progress[index] >= 0) {
  // Do something
}

Workaround, but undesirable:

type Progress = Array<number | undefined> | undefined;

const progress: Progress = [1, 2, 3];
const index = 1;

if (progress !== undefined)
  const p = progress[index];
  if (p !== undefined && p >= 0) {
    // Do something
  }
}

Expected behavior:
The exhaustive type checking mechanism in Typescript should know that neither progress nor progress[index] are undefined inside the if statement, because I'm doing an explicit check for that.

Actual behavior:
On progress[index] >= 0 you'll get ts2532: Object is possibly 'undefined'

Playground Link

Related Issues:
In #34661 the workaround seemed to be to use an intermediate variable, which will fix my code as well, while also making it more hairy.

@andrewbranch
Copy link
Member

If I recall correctly, @sandersn implemented control flow like this for literal element access, so at first glance it seems like it could be expanded to constant element access, but I’m not an expert in this area.

@thany
Copy link
Author

thany commented Feb 14, 2020

Me neither 😀
Literal element access is probably the right keyword. Because if index were a function and I'd be doing progress[index()] (hence it wouldn't be literal element access anymore) the error would be completely correct and understandable. TS wouldn't be able to know (with confidence) if the function returns the same result every time. But as for a constant value, that's fully predictable, if I understand const correctly.

@sandersn
Copy link
Member

sandersn commented Feb 14, 2020

I recall the cost of control flow analysis being much higher since there are many more element accesses with non-literal types than with literal types. But I could be wrong. It's worth making an experimental PR to see what the performance cost is.

@sandersn sandersn added the Experimentation Needed Someone needs to try this out to see what happens label Feb 14, 2020
@sandersn sandersn changed the title Exhaustive typing & array elements Element access should narrow with non-literal types too Feb 15, 2020
@sandersn sandersn added the Experience Enhancement Noncontroversial enhancements label Feb 15, 2020
@andrewbranch andrewbranch added the Suggestion An idea for TypeScript label Feb 15, 2020
@DanielRosenwasser
Copy link
Member

Didn't we talk about this at #32357 and #31478 ?

@sandersn
Copy link
Member

Yes:

Inserting flow nodes only for element accesses with literal access expressions was a performance decision from quite some time ago. As far as I know, we have no reason to revisit that decision.

I made that decision two years ago, so maybe something has changed since then. It's more likely that I was simply wrong and the performance hit is small, or worth it to get this feature.

@RyanCavanaugh RyanCavanaugh changed the title Element access should narrow with non-literal types too Element access with non-constant key should be narrowable Feb 21, 2020
@tadhgmister
Copy link

tadhgmister commented Feb 21, 2020

A particularly compelling case of this is when using const enum properties for discriminated union. Using the same property from the declaration doesn't narrow the type but the number literal does, even though both emit identical javascript code (Playground Link)

const enum X{ key }

interface A { [X.key]: "A"; data_a: number }
interface B { [X.key]: "B"; data_b: number }
interface C { [X.key]: "C"; data_c: number }

 // doesn't work
function getData_WITH_CONST_FROM_DECLARATION(thing: A | B | C) {
    switch (thing[X.key]) {
        case "A": return thing.data_a;
        case "B": return thing.data_b;
        case "C": return thing.data_c;
    }
}

 // works for only string or number literals
function getData_WITH_LITERAL(thing: A | B | C) {
    switch (thing[0]) {
        case "A": return thing.data_a;
        case "B": return thing.data_b;
        case "C": return thing.data_c;
    }
}

Also the title should probably be "allow constant but non literal key to be narrowable", non constant keys probably shouldn't work.

@joshkel
Copy link

joshkel commented Mar 29, 2020

If I'm understanding correctly, the inability to narrow using constant keys is also a significant limitation of symbol properties. These comments from #1863 suggest that using symbols as properties is fully supported, but code such as the following doesn't work:

const Parent: unique symbol = Symbol('Parent');
const Name: unique symbol = Symbol('Name');

interface GenericGroup {
    [Name]?: string;
}
interface Company extends GenericGroup {
}
interface Division extends GenericGroup {
    [Parent]?: Company;
}
interface Team extends GenericGroup {
    [Parent]?: Division;
}
type Group = Company | Division | Team;

function getName(group: GenericGroup): string {
    if (group[Name] == null) {
        return '';
    }
    // Error - "Type 'string | undefined' is not assignable to type 'string'"
    return group[Name];
}

function getParentName(group: Group): string | undefined {
    if (!(Parent in group)) {
        return undefined;
    }
    // Error - "Element implicitly has an 'any' type because expression of type 
    // 'unique symbol' can't be used to index type 'Group'."
    return group[Parent]?.[Name];
}

@lazytype
Copy link

lazytype commented Apr 7, 2020

Should the following behavior fall under the current issue or should it be its own?:

const record: Record<string, number | string> = {};

const key = 4;
const constKey = 4 as const;

let stringValue: string;

record[4] = 'hello';
stringValue = record[4];

// @ts-expect-error - should be error because checker doesn't know what `key` is specifically
record[key] = 'hello';
stringValue = record[key];

record[constKey] = 'hello';
// Proposal: allow `record[constKey]` to be narrowed since `constKey` is a `const` key
stringValue = record[constKey];

// Bonus: hopefully implementing above would make the following work for free
if (key === 4) {
    record[key] = 'hello';
    stringValue = record[key];
}

Playground

@sandersn
Copy link
Member

sandersn commented Apr 8, 2020

@lazytype it's the same; const index = 1 from the OP behaves the same as your const constKey = 4 as const in this case.

@andrewbranch andrewbranch changed the title Element access with non-constant key should be narrowable Element access with non-literal key should be narrowable Sep 3, 2020
@andrewbranch andrewbranch changed the title Element access with non-literal key should be narrowable Element access with non-literal constant key should be narrowable Sep 3, 2020
@mbroadst
Copy link

Hi, I think I'm running into this bug please let me know if I should post elsewhere:

const kSymbol = Symbol('foo');
class WithSymbol {
  [kSymbol]?: number[];
}

function takesWithSymbol(foo: WithSymbol) {
  if (foo[kSymbol] && foo[kSymbol].length) {  // Object is possibly `undefined` ts(2532)
  }  
}

Curiously if the member variable is a primitive type this works just fine:

const kSymbol = Symbol('foo');
class WithSymbol {
  [kSymbol]?: number;
}

function takesWithSymbol(foo: WithSymbol) {
  if (foo[kSymbol] && foo[kSymbol] === 4) {  // works fine
  }  
}

@andrewbranch
Copy link
Member

Yep, that’s this. The second example works because you’re always allowed to compare foo[kSymbol] for equality even if it might be undefined.

@tadhgmister
Copy link

@mbroadst yes that is the same issue, and foo[kSymbol] === 4 is still a valid statement if foo[kSymbol] is undefined so there is no type error, trying to do foo[kSymbol] === [] for array or foo[kSymbol].toFixed() for a number shows it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Experimentation Needed Someone needs to try this out to see what happens Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants