TypeScript: Callbacks can use uninitialized variables that aren't defined during callback execution

Bug Report

🔎 Search Terms

Callback, undefined, null-pointer exception

🕗 Version & Regression Information

This is a crash

⏯ Playground Link

💻 Code

interface User {
    uid: string
}

interface FirestoreTransaction {
    read: () => Promise<User>
    update: (x: any) => Promise<boolean>
}

type TransactionCallback = (t: FirestoreTransaction) => any

async function runTransaction(callback: TransactionCallback) {
    // Placeholder Firestore transaction
    const transaction: FirestoreTransaction = {
        read: async () => ({uid: 'placeholder'}),
        update: async (x: any) => true,
    }
    return Promise.resolve(await callback(transaction))
}

async function main() {
    const user = await runTransaction(async t => {
        try {
            const {uid} = user
            console.log('uid' + uid)
        } catch (e) {
            console.error('Cannot read user ID', e)
            throw e
        }
        return await t.read()
    })
    console.log('user' + JSON.stringify(user))
}

main()

🙁 Actual behavior

I have a project that fetches user details from a Firestore database, which uses a callback pattern to define transactional events. In one of these functions I ran into a runtime error because I was trying to access user within the transaction accidentally and received an undefined error.

[ERR]: "Cannot read user ID",  Cannot access 'user' before initialization

This in turn caused the rest of the logic to fail and I was surprised by this error. While I had written this a bit hastily, I thought that the type checker in Typescript would certainly catch that this value would be undefined before trying to access. My tsconfig.json has "strictNullChecks": true.

It seems like a callback is allowed to access a variable that is defined outside of the callback. This seems like a poor assumption, particularly when I am using async/await to halt further execution and the definition of these variables. I also notice that Typescript is fine if the variable name is defined after the callback flow entirely.

    const user2 = await runTransaction(async t => {
        try {
            const {uid} = user
            console.log('uid' + uid)
        } catch (e) {
            console.error('Cannot read user ID', e)
            throw e
        }
        return await t.read()
    })
    const user = {uid: 'placeholder2'}

🙂 Expected behavior

While callbacks may be able to run at any time, the use of await may give a stronger signal that the return variable and later variable names might not actually exist at the execution time of the callback. I would like there to be some extra warning that the variable might not exist. An error, particularly during strictNullChecks would be useful in catching errors of this class going forward.

About this issue

Original URL
State: closed
Created 3 years ago
Reactions: 3
Comments: 23 (4 by maintainers)

Most upvoted comments

I proposed this being an error under the strictNullChecks flag. If this variable was seen as possibly undefined, then you could use the exclamation operator to override that check. Because technically the variable should be considered possibly undefined.

Fleker on Sep 30, 2021

@fatcerberus That’s exactly what I’m trying to demonstrate. In Dispatch.cancel(jobToken), jobToken should be handled as Possibly undefined. All inside the expression following = should handle jobToken as Possibly undefined

FilipeBeck on Oct 3, 2021

See my example above for a counterexample. There are plenty of valid cases where the callback is not called immediately, and TS has no way to know that.

fatcerberus on Oct 3, 2021

Is the same that:

But it’s not, as it has been mentioned several times already…

MartinJohns on Oct 3, 2021

// Currently, OK. Should be ERROR: Object is possibly ‘undefined’.(2532)

You can argument in both ways: Why should it be possibly undefined, when it’s clearly always assigned a value?

IMO the proper solution would be #11498 to indicate whether the callback is invoked immediately (meaning it’s always undefined) or deferred (it’s never undefined).

MartinJohns on Oct 3, 2021

The compiler could handle as Possibly undefined for the whole expression after =

FilipeBeck on Sep 30, 2021

The point I’m trying to make here is that adding | undefined to the type doesn’t actually solve the problem you want to solve because it doesn’t reflect what actually happens at runtime:

function doIt(callback) {
    callback();
    return 42;
}

const x: number = doIt(() => {
    if (x !== undefined)  // ReferenceError: Cannot access 'x' before initialization
        console.log(x);
});

The error TS would need to produce here would necessarily be one that couldn’t be silenced by simply writing x! and so would break cases where this pattern is actually valid.

fatcerberus on Oct 4, 2021

The compiler handles let v: number = $expression as defined inside the expression. It is not because the expression. It is because v is a number and not a number | undefined. A immediate does not solve this problem.

FilipeBeck on Oct 3, 2021

In that case even checking whether it’s undefined or not will throw

I did not understand. jobToken can be undefined, no? If you know it’s not be undefined, a ! would solves:

const jobToken = Dispatch.onUpdate(async () => {
    // do stuff each frame, maybe an await here and there...
    if (amIFinished) {
        Dispatch.cancel(jobToken!); // Just use a `!`
    }
});

You don’t need refactors the code, etc… Just use a !

FilipeBeck on Oct 3, 2021

Assuming I accept that premise, that wouldn’t be enough because in that case accessing the variable at all is a runtime error. In that case even checking whether it’s undefined or not will throw, and your argument becomes one where TS rejects this pattern outright. This is why we’d need the immediate annotation.

fatcerberus on Oct 3, 2021

In this example, yes. Simply because x is not assigned in x.toString(). First, the expression, after the assignment

FilipeBeck on Oct 3, 2021

You can argument in both ways: Why should it be possibly undefined, when it’s clearly always assigned a value?

No, because the value has not been assigned yet during the access within the callback. In console.log(y.toString()), y is clearly undefined. After return 10, y is defined. A immediate notation described in https://github.com/microsoft/TypeScript/issues/11498 makes it more evident. As described by the Playground example, if declare y as number | undefined, the compiler already understand that the value is not assigned within the callback and is assigned after de assignment statement (after y = doIt(...).

Use a Possibly undefined is more consistent than Variable ‘x’ is used before being assigned because the callback can have deferred code. In the following example, console.log(y.toString()) works because y is already assigned within the setTimeout callback.

let y: number = doIt(() => {
    setTimeout(() => console.log(y.toString()), 10)
    return 10
})

I think the idea here is just to prevent the accidental access that the issue describes. I think the compiler handle y as number | undefined in all code within the assignment expression is simple and sufficient for these scenarios. Again, this is exactly the behaviour when declares y explictly as number | undefined.

FilipeBeck on Oct 3, 2021