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

TS2367 must accout for await inside of an async function. #48148

Closed
Giwayume opened this issue Mar 7, 2022 · 23 comments
Closed

TS2367 must accout for await inside of an async function. #48148

Giwayume opened this issue Mar 7, 2022 · 23 comments
Labels
Duplicate An existing issue was already created

Comments

@Giwayume
Copy link

Giwayume commented Mar 7, 2022

Bug Report

πŸ”Ž Search Terms

TS2367 await async this condition will always return since the types have no overlap

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

https://stackoverflow.com/questions/54019195/incorrect-conclusion-that-this-condition-will-always-return-false

πŸ’» Code

public async transitionA() {
    this.state = State.TWO
    await sleep(1000)
    if (this.state === State.ONE) { // <-- TS2367 error
      this.state = State.TWO
    }
}

Typescript thinks that this.state could not possibly be equal to State.ONE inside of the if statement. In reality, anything could happen during the await inside of other methods.

πŸ™ Actual behavior

Static analysis complains that an instance variable could not possibly have been reassigned inside of an asynchronous function, where it has no guarantee that other methods that happen during the await could end up reassigning the variable or not.

πŸ™‚ Expected behavior

Inside of async functions where there is an await between the initial assignment and the variable check, TS2367 should not apply.

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 7, 2022

Another duplicate of #9998. This is even mentioned in the StackOverflow answer to the question you linked.

@Giwayume
Copy link
Author

Giwayume commented Mar 7, 2022

Hm. I don't agree. The buried comment in StackOverflow mentions "this is a similar issue".

That issue does not mention at all await/async in its summary. It is undiscoverable. It's not even clear besides from people complaining in a few replies that it would tackle this.

@nmain
Copy link

nmain commented Mar 7, 2022

Search terms
TS2367 await async this condition will always return since the types have no overlap

Did you put all of that into a single search? Searching just for await async this condition will always return since the types have no overlap returns at least 3 other duplicates specifically about await: #31009 #31429 #42295. Looks like many of the threads don't contain "TS2367" in them, so now there's one that does! πŸŽ‰

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 7, 2022

That issue does not mention at all await/async in its summary.

Because it's irrelevant. The same behaviour applies to sync and async methods.

Let me give you a brief example:

async function doNothing(): Promise<void> {}

class Example {
  private state: string | undefined

  public async example() {
    this.state = "test";
    await doNothing();

    // Error here?
    console.log(this.state.toLocaleLowerCase());
  }
}

Should the line with the console.log statement result in an error, or not? Because what you're asking for in your issue is exactly that: It should result in an error. As #9998 outlines, the only options are either to completely analyse the functions and possible side-effects or revert all narrowed types. The former would impose an immense performance cost, and the latter would be a massive breaking change, and it would just result in people raising bug issues about this new behaviour.

@Giwayume
Copy link
Author

Giwayume commented Mar 7, 2022

No, this is a different problem. The issue isn't that the "doNothing" function itself could modify state, and whether or not to analyze that function and its call chain. The issue is that awaiting "doNothing" can cause a pause in execution, which leads to the fact that any line of code of Javascript loaded on the page, 3rd party (outside of Typescript) or otherwise, that's not even inside of the execution chain of "doNothing", could modify state.

class Example {
  private state: string | undefined

  constructor() {
    setInterval(() => {
      this.state = Math.random() > 0.5 ? 'foo' : undefined;
    }, 100);
  }

  public async example() {
    this.state = "test";
    await doAnything();
    console.log(this.state.toLocaleLowerCase()); // Of course there should be an error here!
  }
}

I don't care what the function that's awaited does, await should always result in Typescript assuming that the "state" variable could be any value as defined by its type. It should work similar to how it works reading a value from inside a closure.

class Example {
  private state: string | undefined

  public async example() {
    this.state = "test";
    setTimeout(() => {
      // This causes an error saying this.state could be undefined, which is a correct assumption.
      // Same should happen after an await.
      console.log(this.state.toLowerCase()); 
    }, 100);
  }
}

To be absolutely clear: the asynchronous scenario is fundamentally different from the synchronous scenario in this case, in that any code can modify a member variable during an await, not just the code inside of the function call. So it should be assumed, the same as in the setTimeout scenario, that after the await/setTimeout the member variable could be any value of its type.

@fatcerberus
Copy link

The async case is not fundamentally different from this one:

class Example {
  private state: string | undefined

  public example() {
    this.state = "test";
    this.foo();
    this.state;  // string - but actually, undefined
  }

  public foo() {
    this.state = undefined;
  }
}

This is a duplicate of #46439, and therefore of #9998.

@Giwayume
Copy link
Author

Giwayume commented Mar 7, 2022

You missed my entire point. I'm not sure how to communicate it clearly without retreading.

The synchronous case is deterministic, you could reasonably expect to be able to predict the behavior if you were willing to dedicate enough computing power.

The asynchronous case is non-deterministic. Depending on how you design your component, you would expect the state to change from external calls. Thus Typescript should not assume the value of the state after asynchronous calls. Typescript could never hope to be able to determine this, even if you had a supercomputer at your disposal. It is ultimately up to the design of the program, as well as user input.

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 7, 2022

I definitely would not expect the state of all properties to change whenever I await on something, and the behaviour you suggest would apply to all properties.

We understand you just fine. We know exactly what issue you're talking about. But this doesn't change the fact that this is not a bug, it's working as intended (and the reasoning is outlined in #9998).

You can work around this issue using a type assertion to not let the property narrow: this.state = State.TWO as State.ONE | State.TWO

And I can guarantee you, if TypeScript were to change this behaviour, there would definitely bug issues about this new behaviour saying it should be the other way around. There is no "right" solution here, there's advantages and disadvantages to both.

@Giwayume
Copy link
Author

Giwayume commented Mar 7, 2022

I definitely would not expect the state of all properties to change whenever I await on something

Typescript is meant to be a guarantee that your production code does not break due to mistyped variables at runtime. From a methodology standpoint, it should work the way I'm saying. Even if it's a 5.0 release.

@MartinJohns
Copy link
Contributor

Typescript is meant to be a guarantee that your production code does not break due to mistyped variables at runtime.

This is not correct. The Design Goals clearly state this as a non-goal:

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

@Giwayume
Copy link
Author

Giwayume commented Mar 7, 2022

Well, this is hindering my productivity :)

(and it's not correct)

@MartinJohns
Copy link
Contributor

And the opposite would hinder the productivity of many other, when the variables that just have been narrowed suddenly require narrowing again. Again: There is no right solution. And I provided you a valid workaround.

Anyway, I'll unsubscribe from this issue now. Duplicates have been linked and reasons for this behaviour have been given. I expect the TypeScript team to close this issue or let the bot close it.

@Giwayume
Copy link
Author

Giwayume commented Mar 7, 2022

The crux of this issue is that the asynchronous case is fundamentally different from the synchronous case. Similar to how function closures reset narrowing. I'll find some practical examples to the team can decide.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Mar 8, 2022
@RyanCavanaugh
Copy link
Member

The sync case and the async case are not fundamentally different. Async functions may or may not have side effects, and sync functions may or may not have side effects. If you assume that 100% of functions have side effects, most programs a person would write will have incorrect type errors. Assuming that 100% of async functions and 0% of sync functions have side effects is perhaps the weirdest stance we could take, since both sync and async functions are equally capable of having side effects.

@fatcerberus
Copy link

fatcerberus commented Mar 8, 2022

Devil's advocate just for completeness: The OP's argument seemed to be that during await, other in-flight asynchronous operations may have side effects on the one that's paused (i.e. causally unrelated to the call being awaited), which can't happen in the sync case.

In other words, that await doNothing() may still have side effects, simply as a matter of using await.

@Giwayume
Copy link
Author

Giwayume commented Mar 8, 2022

Thank you, pretty much the only person who read what I was saying. It is not about the function that's called or its effects.

Even user input is something that can modify state during the await.

During development of an advanced application that makes a ton of use of simultaneous async calls, Typescript is actively hindering by assuming the state couldn't possibly have changed, when many times that is the case or even an intended side effect.

Even if it affects a ton of people, resetting the narrowing during async is going to give a more realistic expectation of how applications using async logic will behave. It will lead to better code with more realistic expectations.

@Giwayume
Copy link
Author

Giwayume commented Mar 8, 2022

Why would a syntactic sugar for promises have different expectations from using promises?

image

image

@RyanCavanaugh
Copy link
Member

On a weekly basis we get a "bug report" that code inside a callback doesn't get narrowing from its parent scope; see inbound links on #11498. What you're saying is that we should get more of those reports -- I don't find that compelling.

We thought about this plenty when we designed control flow / async and are satisfied with the trade-offs the current approach takes.

@Giwayume
Copy link
Author

Giwayume commented Mar 8, 2022

So you can see how the current design is inconsistent.

Since this seems to be such a hot topic, would probably make sense to make it a configurable part of the compiler.

@opengraphica
Copy link

I would tend to agree. The one size fits all approach never works out for large popular projects like typescript. Many companies will want to lean on the side of safety in their code rather than convenience.

@RyanCavanaugh
Copy link
Member

We don't think this makes sense to be configurable because in the vast majority of situations, pessimistically resetting narrowings causes many more problems than it solves, and any given codebase would be extremely unlikely to change such a configuration away from the default. It doesn't make sense to do a large amount of work to enable that setting just for effectively no one to turn it on.

@Giwayume
Copy link
Author

Giwayume commented Mar 8, 2022

Given you've just stated you get weekly issues about this, and this topic I've opened has already been opened several times, I think you're mistaken in thinking that no one will use this configuration (either turning on or off narrowing for various scenarios: sync call, async call, closures).

Would you accept a PR?

@typescript-bot
Copy link
Collaborator

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

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