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

Control flow analysis for element access with variable index doesn't account for async race condition #58822

Closed
rixtox opened this issue Jun 10, 2024 · 8 comments
Labels
Duplicate An existing issue was already created

Comments

@rixtox
Copy link

rixtox commented Jun 10, 2024

πŸ”Ž Search Terms

Control flow analysis async race condition

πŸ•— Version & Regression Information

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.5.1-rc#code/IYZwngdgxgBAZgV2gFwJYHsIwOYFNkBywAtrgBQCUAXDAAoBO6xqIuAPCMvahNgHwwA3gCgYYmPXwJ6WAOQB3dPQA2AE1kBuYQF9hw0JFiIUGLHACMZdACMAVjQBKuKEtUcuPbABoYZMgA8aTm5eChgAXgEAN3RUVTCAHxgkVVw4HlxVPh8Aa1wwII9QoVFxVDhfZDAAB1x0CptbAG08sABdCPDwmAAiYyg0TB6wkXExmBcIThgIElwImGB5YFRkHHwiUkotcfFGlvy2slnSCh3xXTH91o7ulLSM1S1dYUnp1oXZbEl8T01XzDTRoLUbiA7tY5zQohbDUGAxOIlXZiN7oZS4AB0ynQ2DIAAMABK4ZTYnwAEkEJ1w2gAhHizqUxLoXgwmCxMcASWQmhYrHZcvkKD5eY0BWAKG0GUA

πŸ’» Code

async function getName(): Promise<string> {
    return 'world';
}

async function f1(obj: Record<string, ((x: string) => void) | undefined>, key: string) {
    if (typeof obj[key] === "function") {
        const name = await getName();
        obj[key](name);
    }
    obj[key] = undefined;
}

const key = 'greeting';
const obj = {
    [key](name: string): void {
        console.log(`Hello, ${name}!`);
    }
}

Promise.all([f1(obj, key), f1(obj, key)]);

πŸ™ Actual behavior

No type error, but runtime failure:

Unhandled Promise Rejection: TypeError: obj[key] is not a function. (In 'obj[key](name)', 'obj[key]' is undefined)

πŸ™‚ Expected behavior

Type checker should account for potential race condition and do not persist the narrowed type after an await statement.

Additional information about the issue

No response

@nmain
Copy link

nmain commented Jun 10, 2024

This is a duplicate of #9998; see also the many other duplicates linked from that issue.

@rixtox
Copy link
Author

rixtox commented Jun 10, 2024

This is a duplicate of #9998; see also the many other duplicates linked from that issue.

But this is a regression as 5.4.5 would not make the optimistic type narrowing on this path.

@Josh-Cena
Copy link
Contributor

How is this relevant to computed keys at all? Your code would have the same unsoundness if you substitute all [key] with .greeting.

@rixtox
Copy link
Author

rixtox commented Jun 10, 2024

How is this relevant to computed keys at all? Your code would have the same unsoundness if you substitute all [key] with .greeting.

It's not about computed keys. I just took the example from this blog and extended a bit. The main concern is that when there's an await in the control flow, you can no longer assume "neither obj nor key are ever mutated".

@MartinJohns
Copy link
Contributor

The main concern is that when there's an await in the control flow, you can no longer assume "neither obj nor key are ever mutated".

Which is exactly what #9998 is about. And it's unrelated to await, a sync function has the same issue.

@fatcerberus
Copy link

The analogy for a synch function would be an intervening function call that causes the same mutation.

This issue is Exhibit A of "why TS team was so hesitant to add narrowing by indexed access for so long". The bottom line is the compiler doesn't know whether any given await (or function call) is going to change types, so it's just a question of whether it should be optimistic or pessimistic when it encounters one. Here TS chose to be optimistic. If it was pessimistic, people would complain about unaffected variables being widened (leading to unsafe casts and/or redundant additional type checks), so neither option is really "better" than the other--it's a tradeoff.

@rixtox
Copy link
Author

rixtox commented Jun 11, 2024

neither option is really "better" than the other--it's a tradeoff.

Maybe worth mentioning the tradeoffs in the blog post. I read the blog post and felt it might not be as reliable as it seems. Which is why I wrote this issue. I don't think I would be the only one with this thought.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jun 14, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

7 participants