Invalid optional promotion to non null

[gottfired]

Bug Report

🔎 Search Terms

• optional
• promotion

🕗 Version & Regression Information

• This issues exists since optional were introduced
• This is the behavior in every version I tried, and I reviewed the FAQ for entries about "type system behaviour"

⏯ Playground Link

Playground link with relevant code

💻 Code

class DoSomething {
    do() {
        console.log("do something");
    }
}

class TestStore {
    _member: DoSomething | null = new DoSomething();

    get member() {
        const ret = this._member;
        this._member = null;
        return ret;
    }
}

function test() {
    const a = new TestStore();
    if (a.member != null) {
        a.member.do(); // <-- ### BUG: TS thinks that no "!" needed here
    }
}

test();

🙁 Actual behavior

When performing a non null/undefined check for an optional variable that is a getter, the TS parser promotes the variable to being not null. The variable can be accessed without non-null-assert via !.

This is a bug, since getters are not idempotent and can return null/undefined on the next call.

🙂 Expected behavior

TS should enforce the use of non-null-assert and inform the user that the variable access can still be null/undefined. This is handled corectly by dart, where you have to either store the variable in a local variable, or use ! to promote the variable.

Correct use with local variable in TS

Same sample in Dart. See that there you have to promote via !. DartPad link

Also see info on same issue for dart: https://dart.dev/tools/non-promotion-reasons#property-or-this or dart-lang/sdk#44327

[MartinJohns]

This surely falls under #9998.

[QbDesu]

While I do agree with @MartinJohns my argument why this shouldn't really be considered a bug would be on a much more fundamental level.

There is not much typescript can do about this without breaking it's Non-Goal No 3:

Apply a sound or "provably correct" type system. Instead, strike a balance between correctness and productivity.

The only alternative would be treating properties that are known to be getters the same as function calls, which is not what you want in most cases, they are almost always supposed to be treated like regular properties. This kind of check is quite common check to prevent check if a value is not null so flagging it based on the signature alone would be annoying to most people that dont have such edge case uses as they'd have to do the non-null assertion even if it's not really needed.

So yes the only way you could detect it without troubling people when it mostly isn't necessary is by analyzing the flow in depth, which isn't worth it for such an edge case in my opinion but the issue above has more discussion on that.

On top of that a simple way to avoid this using modern ECMAScript features is a.member?.do().

[gottfired]

On top of that a simple way to avoid this using modern ECMAScript features is a.member?.do().

That would fix this sample. But it wouldn't fix the case where you want to e.g. store only non null values.

If you had a function persist(entry: DoSomething) {...} and did if (a.member != null) { persist(a.member); } you'd save null even though persist is typed to only take non null parameters.

I agree that this issue maybe shouldn't be fixed or at least be toggleable via compiler flag, because it would be quite inconvenient.

Originally I encountered it when working on a Flutter project and was quite surprised that I had to qualify variables that I had checked for != null the line before. When I read the explanation I noticed that TS let's this through whereas Dart is strict.

[fatcerberus]

TypeScript doesn't differentiate between accessors and regular properties at the type level in general:

const obj = {
    get foo() { return Math.random() > 0.5 ? 42 : 812; }
}
type Foo = { foo: number };
const x: Foo = obj;  // this is fine

So the compiler actually doesn't know which properties are getters or not just by examining the types, and tracking that would likely be a large architectural change with many attendant breaking changes. The current behavior seems unlikely to change.

[gottfired]

@fatcerberus Thanks, that's interesting, I never noticed that. Then the only "correct" solution would be going the same route as Dart and not allowing promotion of members at all, only of local variables. (Dart has a similar problem because child classes allow overriding member properties with getters which is forbidden in TS, so Dart does not auto promote member null checks).

I've modified your sample a bit to illustrate member vs local promotion

type A = {
    foo?: number;
}

class B {
    counter = 0;

    get foo():number|undefined {
        return (this.counter++&1) === 0 ? 1 : undefined;
    }

    set foo(v: number|undefined) {}
}  


function test(v: A) {
    if (v.foo) {
        console.log("member", v.foo); // <-- undefined: this is not safe
    }
    
    
    const local = v.foo;
    if (local) {
        console.log("local", local); // 1: <-- this is safe
    }
}

test(new B());

The problem with the current behaviour would be the following scenario: you write a library that defines type A and test(). Then someone using your library could break your code by passing something like B into your function. So you would always have to go via local variables to make sure you null checks are correct. So not allowing auto promotion for members and only for locals would be the safer route which is what Dart opted for, forcing you to think about this.

The current TS behaviour is a lot more convenient though and covers 99% of use cases I assume.

[gottfired]

Not sure if I'm paranoid, but that could be a potential security issue for libs as well. This code looks fine but would store undefined as PW due to wrong promotion. (I know the sample is very constructed and won't be handled like that in real life, but I think it shows the potential where smth could go wrong).

type Credentials = {
    email: string;
    password?: string;
}

function storeNewPassword(password: string) {
    // Store PW
}

function handlePwChange(credentials: Credentials) {
    if (credentials.password) {
        storeNewPassword(credentials.password); // Code would go here and store undefined
    } else {
        handlePasswordReset();
    }
} 

[fatcerberus]

So not allowing auto promotion for members and only for locals would be the safer route which is what Dart opted for, forcing you to think about this.

For what it’s worth: disabling narrowing based on member access across the board would break discriminated unions. It would need to be more nuanced.

[andrewbranch]

Duplicate of #12070 (which falls under #9998)