Rule Change: return await is both faster and allows better error handling

[clshortfuse]

What rule do you want to change?

no-return-await

What change to do you want to make?

Generate fewer warnings

How do you think the change should be implemented?

A new default behavior

Example code

async function do(url) {
  return await fetch(url);
}

What does the rule currently do for this code?

Eslint will say to remove await with the docs saying:

What will the rule do after it's changed?

Suggest return await. (Or at least not misinform authors).

Participation

• I am willing to submit a pull request to implement this change.

Additional comments

The docs state:

You can avoid the extra microtask by not awaiting the return value, with the trade off of the function no longer being a part of the stack trace if an error is thrown asynchronously from the Promise being returned. This can make debugging more difficult.

This is actually false and if that is the reasoning then it should be changed. I have a long explanation of the mechanics as to why return await is faster at this Stackoverflow comment but to keep this brief, you can run this:

async function test(name, fn) {
  let tick = 0;
  const tock = () => tick++;
  Promise.resolve().then(tock).then(tock).then(tock);

  const p = await fn();
  console.assert(p === 42);
  console.log(name, tick);
}

await Promise.all([
  test('nonpromise-sync', () => 42),
  test('nonpromise-async', async () => 42),
  test('nonpromise-async-await', async () => await 42),
  test('promise-sync', () => Promise.resolve(42)),
  test('promise-async', async () => Promise.resolve(42)),
  test('promise-async-await', async () => await Promise.resolve(42)),
]);

setTimeout(() => {}, 100);

And you'll see that returning a Promise (not using return await) is the slowest method.

There is also an added benefit (maybe) that execution (resolving) happens within the function itself, so you can catch errors. If you try to add a catch to a returning Promise, it'll never fire.

function errorThrower () {
  return Promise.reject('fail');
}
function returnNoAwait() {
 try {
     return errorThrower();
 } catch {
   console.log('I want to catch, but I am never executed');
 }
} 
async function returnAwait() {
  try {
      return await errorThrower();
  } catch(e) {
    console.log('I caught an error. I still have e, so I can rethrow.');
  }
}
await returnAwait();
await returnNoAwait();

Yields:

I caught an error. I still have e, so I can rethrow.
Uncaught fail

And note that uncaught fail will crash some environments.

[mdjermanovic]

Hi @clshortfuse, thanks for the issue!

There have been a lot of discussions about this rule.

You can avoid the extra microtask by not awaiting the return value, with the trade off of the function no longer being a part of the stack trace if an error is thrown asynchronously from the Promise being returned. This can make debugging more difficult.

This is actually false and if that is the reasoning then it should be changed. I have a long explanation of the mechanics as to why return await is faster at this Stackoverflow comment but to keep this brief, you can run this

I think this was true at the time when the rule was created but may have become false after tc39/ecma262#1250.

What will the rule do after it's changed?

Suggest return await. (Or at least not misinform authors).

We can't change the no-return-await rule to suggest return await. I think the best past forward for this rule is to update the docs saying that the rationale for the rule is to warn about redundant code.

[fasttime]

We can't change the no-return-await rule to suggest return await. I think the best past forward for this rule is to update the docs saying that the rationale for the rule is to warn about redundant code.

Sounds reasonable. I think such an update would make the rule classify as stylistic?

[clshortfuse]

In addition to the new spec changes (I do mention that in the stackoverflow), it is technically faster if you're not using promises eg: return await 42 is slower than return 42. But placing await in front of non-promise is an anti-pattern.

The real micro-optimization logic is:

• Use return {value} if value is not a promise
• Use return await {value} if value is a promise.

You would need something like typescript-eslint for this though, since natively eslint by itself won't parse the type of value.

[mdjermanovic]

I think such an update would make the rule classify as stylistic?

I think yes, since it reports logical redundancy that no longer negatively impacts performance according to the current state of the ES spec and implementations.

[nzakas]

It seems like there's agreement to update the documentation for this rule. @clshortfuse do you want to submit a PR for that?

[clshortfuse]

Apologies. It seems I misunderstood the intent here. I understood that the rule is now merely of stylistic use to users and no longer something worth recommending.

Bluntly stated, the rule should be removed or changed to better align to ECMAScript spec.

There are no documentation-only "fixes" I see that can be made.

I have a PR that target using return await on literals and undefined because there is no other reason I see for keeping the rule.

[fasttime]

There are no documentation-only "fixes" I see that can be made.

@clshortfuse I think the idea was to replace misleading information in the rule docs (e.g. "avoid the extra microtask", "a likely common performance hazard", etc.), without changing the logic. I'm on your side in arguing that this was not very clear from the previous discussion.

[clshortfuse]

The problem is the entire point of the rule is as follows:

This rule aims to prevent a likely common performance hazard due to a lack of understanding of the semantics of async function.

The raison d'etre (core reason) for the rule is now invalid.

I originally stated that we actually want to encourage return await, though that kinda doesn't make sense for a rule called no-return-await to suggest return await. That's how I interpreted the previous comments.

I'm not sure what documentation-only may exist. If the original motivation of the rule is promote better performance, then the rule should actually promote better performance, and that means it is essentially a rule change/modification. There is a small carve-out where we can still keep promoting this rule, and it's as I scripted in the PR.

Currently, as it stands, the rule, when applies with the suggestions, does the opposite of it's intended purpose. Applying the eslint rule degrades performance.

Sorry if I sound like I'm repeating myself, but essentially I don't see how we can not modify the rule given the "new" spec that's been in place for about 5 years.

[mdjermanovic]

The original motivation for the rule was to report logically redundant code and improve performance. I think the first point is still valid stylistically, and my suggestion was to update the documentation with the new facts about performance.

Currently, as it stands, the rule, when applies with the suggestions, does the opposite of it's intended purpose. Applying the eslint rule degrades performance

I'd be also fine with deprecating this rule.

[nzakas]

I'm fine with deprecating the rule, but we'd still need the documentation updated with that.

[bradzacher]

Note that we have @typescript-eslint/return-await which uses types and thus can enforce the presence or absence of the await.

[clshortfuse]

Here's what I'm thinking:

Because no-return-await was documented and implemented as rule for the reasons for targeting performance and there's no mention about it being used for stylistic reasons we should either:
A: deprecate the rule as-is
B: rewrite the rule to maintain the original purpose, by targeting known performance degradations

Option A IMO implies a breaking changing, since users would need find their own solution if they want to continue targeting performance. I'm okay with this. I just don't know how to draft the deprecation language into the documentation. Does somebody have a commit I can reference?

Option B reduces the effectiveness of the rule putting in the question how useful it is. Without the ability to typecheck, the best we can at a JS level. The best we can do is check for Literal and undefined. I have option B as the current PR as is. It's not the best solution for people who still want to target performance.

I personally use the typescript-eslint rule @bradzacher mentioned with "always". There's more I can do to improve the rule over on that side like ensuring users don't try to try/catch a function that returns a Promise. So deprecating and perhaps referring to use of typescript-eslint to target performance is better.

At the same time, no-return-await has its merits as a stylistic change. People may want that because it is less code. Maybe people did add the rule because it has best of both worlds. But I think that could probably be its own stylistic rule with its own description and users can opt-in to that. I don't have strong opinion about it being recommended. (In other words that can be a new rule for later.)

I lean on deprecation because the nature of await has changed, but I would wonder if @bradzacher is interested in allowing a bit more options for the typescript version.

[nzakas]

@clshortfuse we've already mentioned above that we'd like to deprecate the rule and update the documentation. :)

[clshortfuse]

@nzakas Yeah, that's fine, as I said. But:

I'm okay with this. I just don't know how to draft the deprecation language into the documentation. Does somebody have a commit I can reference?

[mdjermanovic]

@clshortfuse here's an example: 5ed8b9b