Object is possibly undefined` error in TypeScript 4.1.2

[doberkofler]

TypeScript Version: 4.1.2

Search Terms: Object is possibly undefined

Code

function main(values: Array<string>) {
    for (let i = 0; i < values.length; i++) {
        if (typeof values[i] === 'string' && values[i].length > 0) {
            console.log(i);
        }
    }
}

Expected behavior:
No error

Actual behavior:
TypeScript 4.1.2 reports a Object is possibly undefined error in values[i].length but given the first condition the object must be defined as it is of type 'string'

Playground Link: https://www.typescriptlang.org/play?noUncheckedIndexedAccess=true&ts=4.1.0-beta#code/GYVwdgxgLglg9mABAWwIYzACgG6oDYgCmAzgFyICCATlagJ4A8xUVGA5gHwCUiA3gFCIhiYHCqJMeQlEQxEAXkQAGANyzEDRLgIkAdFLBsoACzUwA1OZ4Dht2cAlQ6AB0JwH2osQDaMALoK8ooA5MyshsGIAGRRWvhevn76hIYmiBzK1oJ2ORAIxHBS+nBsmDBcKtk5AL5ViLW1QA

Related Issues: no

[MartinJohns]

Duplicate of #10530. Type narrowing does not occur for indexed access forms e[k] where k is not a literal.

[doberkofler]

@MartinJohns Sorry for the duplicate, but I would not have found it and really looked. I just wanted to add that this specific use case of the problem is the main cause for an immense number of "false errors" when activating the new noUncheckedIndexedAccess flag in my code base.

[RyanCavanaugh]

This was one of the caveats that kept the flag from being implemented for so long. We don't have any way to ascertain whether i or the indexed array has changed between any two arbitrary expressions, so the only "safe" thing to do is always include undefined. You can write

function main(values: Array<string>) {
    for (let i = 0; i < values.length; i++) {
        const el = values[i];
        if (typeof el === 'string' && el.length > 0) {
            console.log(i);
        }
    }
}

[thecotne]

@RyanCavanaugh this should work as well

function main(values: ReadonlyArray<string>) {
    for (let i = 0; i < values.length; i++) {
        if (typeof values[i] === 'string' && values[i].length > 0) {
            console.log(i);
        }
    }
}

since ReadonlyArray can't magically change what indexes are in the set and what are not

[doberkofler]

@thecotne Just to make sure: it should but it does not. Correct?

[thecotne]

@doberkofler sorry for confusion. it should but it does not.

https://www.typescriptlang.org/play?noUncheckedIndexedAccess=true&ts=4.1.0-beta&ssl=1&ssc=31&pln=1&pc=23#code/GYVwdgxgLglg9mABAWwIYzACgG6oDYgCmAzgFyIBKhqAJgngJ4CCATi6gwDzFQsYDmAPgCUiAN4AoRNMTA4LRJjyEoiGIgC8iAAwBuNYk6JcBEgDplYflAAW+mAGoHoyTLdrgiqAwAOhOJ4mRMQA2jAAupoaWgDkPHxWMYgAZMnG+MFh4RaEVraIgjouUu6lEAjEcMoWcPyYMMK6JaUAvs2IbS1AA

[Skillz4Killz]

Have you considered using a for of or forEach loop?

function main(values: Array<string>) {
    for (const value of values){
        if (value) {
            console.log(value);
        }
    }
}

function main(values: Array<string>) {
    values.forEach(value => {
         if (value) {
             console.log(value);
         }
    })
}

[doberkofler]

@Skillz4Killz You are absolutely correct. The code snipped is just an example for the generic problem where the array element is first tested and then used if available.

[saperdadsk]

@doberkofler Consider the following (contrived) example though

function main(values: Array<string>) {
    for (let i = 0; i < values.length; i++) {
        const incrementI = () => {i = i + 1;}
        if (typeof values[i] === 'string' && incrementI() && values[i].length > 0) {
            console.log(i);
        }
    }
}

values[i].length is no longer guaranteed to be defined, and TS has no good way to detecting that change.

[RyanCavanaugh]

since ReadonlyArray can't magically change what indexes are in the set and what are not

This isn't true. A ReadonlyArray might be an alias for an underlying mutable array. e.g.

function fn(cb: (arr: ReadonlyArray<number>, clear: () => boolean) => void) {
  let arr = [1, 2, 3];

  cb(arr, () => {
      while(arr.length) arr.pop();
      return true;
  }) 
};

// Crashes
fn((arr, clear) => {
    let i = 0;
    if (arr[i] !== undefined && clear() && arr[i].toFixed() === "1") {

    }
})

[peterholak]

This was one of the caveats that kept the flag from being implemented for so long. We don't have any way to ascertain whether i or the indexed array has changed between any two arbitrary expressions, so the only "safe" thing to do is always include undefined.

But doesn't narrowing on all object properties work in the "unsafe way" in TypeScript anyway? E.g.

type X = { word: string|undefined }

function bad(obj: X) {
    if (obj.word !== undefined) {
        mutate(obj)
        return obj.word.length
    }
    return undefined
}

function mutate(obj: X) {
    obj.word = undefined
}

console.log(bad({ word: 'hello' }))

Produces a run-time error, but no TypeScript errors. Of course noone should ever write code like this, but it's the same principle.

It's a shame that JS doesn't have better native support for immutable data. The "edge case" of mutability is preventing such a useful feature from working conveniently in the huge percentage of codebases that never mutate most stuff anyway (especially non-local stuff).

[RyanCavanaugh]

The entire point of the flag was to be more safe than TS's normal behavior, so it being more restrictive than the default is a not undesirable aspect.

[yume-chan]

I know TS doesn't recognize this pattern, even for normal optional properties:

interface Foo {
  bar: string | undefined;
}

declare const foo: Foo;

if (foo.bar) {
  foo.bar.toLowerCase(); // OK
}

const bar = 'bar';
if (foo[bar]) {
  foo[bar].toLowerCase(); // Object is possibly 'undefined'.
}

(Playground link)

However I can't understand why:

If it's because a variable (bar in this case) may change between two accesses, since bar is a constant, it's not possible.

If you say the object (foo in this case) may change, then why does accessing with literals work? I expect they (access with literals and variables) both work or not work, and I prefer both not.

[doberkofler]

@RyanCavanaugh

The entire point of the flag was to be more safe than TS's normal behavior, so it being more restrictive than the default is a not undesirable aspect.

I understand that this option is restrictive and the only reason for my SR was to better understand how to take advantage of it. At least in my "real world" scenario, it seems almost impossible using it, as it leads to reject thousands of code lines that are actually valid. I'm still wondering, if maybe just be identifying some common use cases that are legal, the usability of this interesting new option might be dramatically improved.

[radekmie]

@yume-chan It's not enough for bar to be const. Consider this:

const bar = { toString: Math.random };
({ [bar]: 1 }) // { '0.9271219051493171': 1 }
({ [bar]: 1 }) // { '0.07489693725883417': 1 }

It may work for string, number, symbol, and maybe a few others.

[peterholak]

The entire point of the flag was to be more safe than TS's normal behavior, so it being more restrictive than the default is a not undesirable aspect.

I appreciate having that option, but it mixes together two different concerns in an inseparable way

1. safety in the face of mutating data
2. safety for indexed array access (even on immutable data and index)

I think there is a good chunk of people that would like having option 2 without also being forced into option 1 (as it is much more rare, while 2. is very common).

Even so, it is a valuable improvement, just not as useful as it could be.

[MartinJohns]

@peterholak Option two would require a way to represent immutable types in TypeScript. This is currently not possible. It's something I personally really want, but it's nothing in the foreseeable future. Issues to follow would be #14909, #16317, #17181.

[peterholak]

@MartinJohns By option 2 I just meant assuming that the array and index don't change within a block (maybe unless there is an explicit assignment directly within the same block), and doing the narrowing anyway, just like it works for non-indexed access (#41612 (comment)). So not really "immutable", but "not mutated in practice"—which covers a lot of cases and is reasonably easy to manually keep in check by humans (depending on the programming style of the project I guess).

It would be unsafe if the data changes in other ways, but that's a good trade-off for many projects—especially compared to not adding |undefined to indexed access at all like in previous versions.

Basically an option to still add |undefined to indexed access, but also doing the unsafe narrowing after explicit checks. These are kind of two different features, but they are mixed together under the same flag.

[yume-chan]

@radekmie

1. Your code is valid JavaScript, but TS already disallow it.
2. I have already said "bar in this case" so I just want to focus on this case
3. I think your comment is more like "TS shouldn't do anything that's not 100% safe", however in my second theory TS has already accepted something "unsafe", so the point is "what's the possiblility for developers (at all skill levels) to make such a mistake" and "is it suitable to treat all such scenario as an error"

[radekmie]

@yume-chan I just wanted to emphasize that being const-constant is not enough, that's all. I agree with 3 though.

[karlhorky]

@RyanCavanaugh why was this closed? Was there something that resolve this?

Edit: Ah just seeing the "Design Limitation" label on this now - I guess this means that this is by design and TS will not change this behavior?

[peterholak]

For what it's worth, I still think this is 2 separate features mixed under the same flag. One is "stricter narrowing" (but not everywhere) and the other is "adding |undefined to indexed access".