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

Forgotten 'await' error doesn't account for strictPropertyInitialization #43071

Closed
DanielRosenwasser opened this issue Mar 4, 2021 · 4 comments
Assignees
Labels
Bug A bug in TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 4, 2021

We recently ran into some breaks in #43004 (comment) as a result of #25330; however, I think this might be a bigger change than we anticipate.

For one, we don't check whether the value is used after checking the value. For another, we don't really think about class properties, which aren't reliable unless using strictPropertyInitialization.

// @strictNullChecks: true
// @strictPropertyInitialization: false

class C {
  yadda: Promise<number>;

  async doStuff() {
    this.yadda = Promise.resolve(42);
  }
}

async function f() {
  let obj = new C();
  if (obj.yadda) {
    await obj.yadda;
  }

  let x = new C().yadda;
  if (x) {
    await x;
  }
}

Finally, there's some sort of bug where this is plainly giving false positives.

let x: Promise<string>;

setTimeout(() => {
  x = Promise.resolve("");
})

setTimeout(() => {
  // this should NOT be an error
  if (x) {
    x;
  }
})

I guess the question is: should these be errors?

@RyanCavanaugh RyanCavanaugh added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Mar 9, 2021
@DanielRosenwasser
Copy link
Member Author

Conclusion:

  • Checking the body of the if is unnecessary, don't change behavior on that.
  • This check can only be done if strictPropertyInitialization is true.
  • This check reports false-positives when a closure-captured variable of a Promise is assigned in some functions, and checked for truthiness in another, and that's a bug.

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Mar 12, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.3.0 milestone Mar 12, 2021
@andrewbranch
Copy link
Member

The design meeting notes say

Uncalled functions and unawaited promises should never be an error when the same identifier is referenced in the truthy block

which seems to conflict with

Checking the body of the if is unnecessary, don't change behavior on that

Unless I’m misunderstanding something?

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Apr 7, 2021

Yeah, seems confusing - I think that might've been a rushed conclusion that overlooked the existing discussion.

@andrewbranch
Copy link
Member

Closing in favor of #43514 since we decided on a different fix (#43593)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants