require-await: allow async function with throw or return?

[ljharb]

require-await as currently written is a terrible rule; there's zero value in requiring that an async function have an await.

However, an async function that has no await, no throw, and no return truly has no purpose, because it can't block on anything.

Would you be open to adding an option (or better, changing the default behavior) to this rule so that it only warns on async functions that lack await+throw+return?

(to be fair, even if it calls a function that throws synchronously, it could still be valuable to have an async function because it ensures a rejected promise)

[not-an-aardvark]

🎉 🎊10000th issue! 🎊 🎉

[not-an-aardvark]

(to be fair, even if it calls a function that throws synchronously, it could still be valuable to have an async function because it ensures a rejected promise)

Yeah, this is my thinking too. I guess this could be useful for catching cases where the user does something like this, where they want the Promise to fulfill after both steps are complete:

async function foo() {
  doTheFirstStep();
  doTheAsyncSecondStep(); // oops, this should have `await`
}

But I'm not sure if that pattern is common enough for it to be worth telling users to make the function synchronous. It seems like if the user wants to ensure a rejected Promise is returned from error handling, then an async function is probably the way to go.

For example, I'm imagining code like this could exist in polymorphic interfaces, where a Promise is returned for consistency (so an async function is used) even when the action wouldn't always need to be asynchronous:

interface File {
  write(text: string): Promise<void>
}

const FileOnDisk: File = {
  async write(text) {
    await util.promisify(fs.writeFile)(path, encode(text));
  }
};

const FileInMemory: File = {
  // this function does not contain `await`, but returns a Promise to conform to the interface
  async write(text) {
    this._contents = encode(text);
  }
};

[ljharb]

This suggestion is mainly trying to turn require-await from a rule that's harmful and provides virtually no value, into one that provides nonzero value.

[not-an-aardvark]

I agree that the current rule has some problems (as you know, some members of the team feel differently). I guess I'm a bit unclear on to what extent this option would be an improvement.

• Using an async function can be useful to avoid a synchronous exception, but synchronous exceptions can come from method calls and not just throw statements. So making the rule check for throw statements seems leaky (e.g. the rule could start triggering an error if a section of code containing throw is refactored into a separate function).
• return statements are different in that they're always local. But a significant number of async functions probably return a value, which would stop the rule from reporting any mistakes from missing await. So the rule's niche with this option enabled would be to report missing await operators on functions that return Promise<void>. Still, some async functions intentionally return Promise<void> without using await, which would cause the rule to report false positives.

Overall it's not clear to me that this would be a net positive -- it seems like it would be removing some of the good cases along with some of the bad cases, which still might not make the rule worthwhile.

[ljharb]

@not-an-aardvark can you elaborate when, in the eyes of any member of the team, this rule has value as-is? In other words, can you provide a concrete class of use cases where ensuring >= 1 awaits in an async function improves the quality of any code?

Perhaps if i can understand how this rule helps anybody (it’s quite different from require-yield because async functions are quite different from generators), i could better understand how i might enhance it for my use cases.

[not-an-aardvark]

I'm basing this on the discussion from when the rule was proposed in #6820.

As I understand it, there are two places the rule can provide value:

• The rule could alert the developer if they intended to await the result of a Promise, but forgot/didn't realize that they needed to use the await keyword.
• The rule could alert the developer if they intended for the function to be synchronous (e.g. if they refactored the code to move asynchronous calls somewhere else) but they forgot to remove the async keyword.

[ljharb]

For the first case; that would only be true if they intended to await the result of one promise. If they forgot to await two promises, this rule would only catch the first one and not successive ones - or if they did use await at least once, they could merrily forget to await a thousand promises and this rule would not complain.

For the second case, I suppose that's fair, but if they intended the function to be synchronous, wouldn't "it's a promise" alert them to that at the other end far more effectively than this rule?

[jednano]

Wouldn't it also be acceptable for an async function to return a Promise?

async function foo() {
    return Promise.all([
        doHomework(),
        doChores(),
    ]);
}

[ljharb]

Absolutely it should be; and because any function or property access could potentially return a thenable, require-await can't possibly statically determine if you made an error here or not.

[jednano]

All really good points! I agree that this rule is pretty much pointless. I'm not even sure I would want it as a warning! I think TypeScript has a similar feature though and it seems to work pretty well. At least, it prevented me from making a mistake a few times.

[fatcerberus]

Note that IIRC, by default the C# compiler produces a warning if you write an async function without an await - so there's at least some precedent for a rule like this. In truth I don't have much of a horse in this race though; I think I'll enable the rule in my project for a bit and see how I feel about it.

[ljharb]

I'm not sure how C# works; but unless async/await works identically in C#, it wouldn't be much of a justification.

[not-an-aardvark]

The require-await rule exists and is unlikely to be removed given our deprecation policy, so I don't think this issue is a good place for general discussion about whether the rule is useful. (If you don't find it useful, you shouldn't enable it.)

Refocusing the discussion: @ljharb Do you still want to add an option to it? If so, we should probably get input from some people who currently use the rule to see if this modification would still fit their use case.

[fatcerberus]

It's pretty much the same as JS, just replace Promise with Task. They're syntax sugar just like in JS.

[ljharb]

@not-an-aardvark an option for the rule would hopefully make it more useful/less harmful, so yes, I'd like to see one.

[not-an-aardvark]

cc @mysticatea since you were in favor of adding the rule awhile ago in #6820.

[not-an-aardvark]

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get consensus from the team, so I'm closing it. We define consensus as having three 👍s from team members, as well as a team member willing to champion the proposal. This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is consensus among the team that they're worth adding.

Since ESLint is pluggable and can load custom rules at runtime, the lack of consensus among the ESLint team doesn't need to be a blocker for you using this in your project, if you'd find it useful. It just means that you would need to implement the rule yourself, rather than using a bundled rule that is packaged with ESLint.