no-return-await: Why?

[magicmark]

What rule do you want to change?

https://eslint.org/docs/rules/no-return-await

What do you want to change

Remove it or add a big warning label

Are you willing to submit a pull request to implement this change?

Yes

Why?

Looks like this is a can of worms that comes up every now and again:

• Documentation of "no-return-await" #11878
• Remove no-return-await rule #12246

tl;dr in newer versions of node/v8 (v7.3), you lose valuable stack trace information. The linked Jake Archibald post, excellent as it is, appears to be slightly out of date when he says:

Note: Outside of try/catch blocks, return await is redundant. There's even an ESLint rule to detect it, but it allows it in try/catch.

In new versions of v8, this is no longer accurate: https://v8.dev/blog/fast-async.

Looking at this documentation rule, it looks like the guidance is to have this on because that's the "best" thing to do.

But it's a tradeoff:

• do you want to incur the cost of an extra microtask (is this is still even relevant?), or;
• do you want to get better stack traces?

It's unclear to me what the real world overhead of the extra microtask is, but unless you're running node for HFT purposes, I would argue favoring better stack traces is the better tradeoff for most folks? (And let perf be an optimization)

[ljharb]

v8 isn't the only engine, and authoring your code based on engine quirks is a terrible idea.

return await is ALWAYS redundant outside of a try block, and it's simply bad/sloppy code to have it, even if, in one engine, it's been optimized away so the bad code isn't slow anymore.

[magicmark]

return await is ALWAYS redundant outside of a try block

https://i.fluffy.cc/cfzM4PPfm33FXdZthNH2CGQRwcC3dG6C.html

Maybe i'm missing something, but I'm not using a try/catch block, and it doesn't look redundant to me?

v8 isn't the only engine

Sure, but a large % of eslint's users are probably using v8 in some capacity (do we have any metrics on that?), so any information related just to v8 is probably still worth surfacing?

[ljharb]

Stack traces aren't part of the language, and thus have no bearing on this conversation.

It is redundant. A return value in an async function is automatically awaited, so awaiting it yourself is unnecessary.

[ljharb]

I think it'd be totally reasonable to make sure the rule docs reflect current implementations; but the rule is still strictly necessary and will forever be. At some point in the future, v8 will hopefully fix their insufficiently helpful stack traces so this redundancy no longer provides a benefit.

[magicmark]

Stack traces aren't part of the language, and thus have no bearing on this conversation.

But it still affects us (folks using modern v8) in the real world. At the end of the day, I still have worse stack traces if I follow this rule.

So I'm unsure why this isn't considered part of the tradeoff, and not part of the conversation? Maybe I'm missing something?

(I get that eslint doesn't want to be bounced around by current market forces and wants to stay true to the spec, but in this specific case, it feels like it's to our detriment as dumb js devs who don't understand the deeper philosophy and haven't done the homework here)

I think it'd be totally reasonable to make sure the rule docs reflect current implementations

I think this would be a great! Meeting js devs where they are and providing some guidance for folks in this situation of using modern v8 would be awesome.

I'll send a PR and we can hammer it out in there. Thanks!

[ljharb]

It's certainly a tradeoff to be made, and a reason to disable the rule.

[nzakas]

We won’t be removing the rule, but the documentation can certainly be updated with more information about the tradeoffs.

[czlowiek488]

It may be a good idea to introduce new rule which work as opposite to no-return-await.
Maybe something like force-return-await?

[ljharb]

@czlowiek488 that would be actively harmful, since it forces an extra microtask, and would thus make your code slower.

[czlowiek488]

@ljharb Imho it is more harmfull if it does not print stack traces, it may make debugging extremely hard if you are not aware of it. Microptimization should not be an excuse to hide those traces.

[nzakas]

Folks, I don’t think this discussion is of benefit to the project. We can’t introduce a core rule that actively prescribed the opposite of another core rule. You can always create your own rule to do exactly what you want.

[CherryDT]

I support the opinion that no-return-await is harmful and instead force-return-await should be recommended.

To understand why exactly, please take a look at this issue in which I explained the reasoning behind this opinion in a very detailed manner: standard/standard#1442

You can also find some tests in regards to performance impacts and other considerations there.

I see the primary benefit here not in stack traces but in eliminating lingering traps for future code modifications. (Most straight-forward example: What will happen if someone wants to add a ?? defaultValue after your return somethingAsync()?)

[ljharb]

Nothing bad? When a promise is returned, it’s awaited implicitly; when a scalar, there’s no need for it.

[CherryDT]

No, in fact the default value will just never be applied because the promise is always truthy and even though the call looks like it's to a synchronous function (no await) it's actually only going to work when await is added at that point.

[ljharb]

Ah, right, i see what you mean. That’s not about return await tho - that’s about using await on a promise when you use the value - iow, that’s not “return await”, that needs (await promise) ?? defaultValue, which is then returned.

[CherryDT]

Exactly, and always awaiting async function calls even when returning their result prevents this trap from being laid, because then from the call site itself it's obvious that the call is to an async function, plus adding ?? defaultValue just works, like it would with a synchronous function call. (the parens are superfluous though because the await has a higher precedence than ??).

This is just one of example of many though, others would be adding a try, assigning the result to a variable, wrapping the result in an object, etc. (see issue linked above)

[ljharb]

If you awaited it and then added the default value, it would either be a syntax error, or, the await’s low precedence would silently cause the very bug you’re concerned about. There’s no way to avoid the “trap” of needing to know which things are promises.

[CherryDT]

return await somethingAsync() ?? defaultValue works just fine. await has one of the highest precedences, the same as typeof or void. (maybe you are confusing it with yield?)

[jhimes144]

Hello,

At the very least the language in the no-return-await documentation is harmful to the community in my opinion.

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

I myself understand the semantics and have been using return await specifically for the stack trace benefit until I read this comment on benchmarks between return await and return typescript-eslint/typescript-eslint#1378 (comment)

Sometimes return await can take up to twice as long as return and for me that nullifies the stack trace advantage. I don't think enough people know just how much of a performance impact this has.

I think the documentation should be updated with benchmarks like this so that developers can actually be informed to make a decision not be told they lack understanding with no commentary.

EDIT: After looking into exactly the time taken per call in the grand scheme of things - I have to walk back on my preference and say that return await is the better choice for me for stack trace visibility.

[mdjermanovic]

At the very least the language in the no-return-await documentation is harmful to the community in my opinion.

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

I don't understand why this sentence is harmful. This rule disallows using return await, and this sentence says that there's a performance hazard if you're using return await. It would be harmful if this rule was actually enforcing return await without stating how much of a performance impact it has.

I think the documentation should be updated with benchmarks like this so that developers can actually be informed to make a decision not be told they lack understanding with no commentary.

I'm not supportive of including benchmarks as the results depend on the current implementations of engines, and that changes all the time.

I completely agree with you that documentation mentions a lack of understanding, but doesn't really explain what should be understood. We should definitely improve that part.

[jhimes144]

I completely agree with you that documentation mentions a lack of understanding, but doesn't really explain what should be understood. We should definitely improve that part.

This is what I view harmful - because it encourages cargo cult thinking. It just says do this and if you don't you lack understanding. While in the fine print it says (except if you value stack traces over the performance impact) while not mentioning the performance impact, or even a ballpark.

I'm not supportive of including benchmarks as the results depend on the current implementations of engines, and that changes all the time.

That is a good point - however I think its worth mentioning a ballpark impact. I think it would be helpful to say something like...

On most engines return await runs 2x slower than return . However, on newer engines its only around >20% slower - Disclaimer - this is subject to change version to version.

This encourages the developer to make an educated decision on this rule and if they believe the ballpark metrics are not sufficient they will perform their own benchmarks/search elsewhere online.

[czlowiek488]

But do we care about performance all the time? I do care about it only when I can not scale easly or my client is complaining about expenses. I believe that at the beginning of development it is better to have clean stack traces then faster execution time.

[mdjermanovic]

On most engines return await runs 2x slower than return . However, on newer engines its only around >20% slower - Disclaimer - this is subject to change version to version.

I really think we shouldn't mention any numbers or term "newer engines" because we'll not run benchmarks and update the documentation every month or so. On the other hand, we could add a better explanation on why return await has a performance impact. In particular, what happens on return something from an async function vs what happens on return await something, and why that await might be redundant. That seems to be missing, and a PR would be welcome. In fact, there was a short explanation in a previous version of this document, but then disappeared after a change.

[mdjermanovic]

As for the original request:

Looking at this documentation rule, it looks like the guidance is to have this on because that's the "best" thing to do.

But it's a tradeoff:

• do you want to incur the cost of an extra microtask (is this is still even relevant?), or;
• do you want to get better stack traces?

That tradeoff is already stated in the opening section of the no-return-await documentation:

Using return await inside an async function keeps the current function in the call stack until the Promise that is being awaited has resolved, at the cost of an extra microtask before resolving the outer Promise. return await can also be used in a try/catch statement to catch errors from another function that returns a Promise.

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.

So I'm not sure if there is anything else that should be done here, and I think we can close this issue.

[CherryDT]

I can't stress enough that it's not just about stack traces but especially about readability and maintainability, avoiding refactoring traps... See my exchange with ljharb above and my comments at standard/standard#1442

[mdjermanovic]

As this issue has been open for more than 6 months, and PR has not been submitted yet, I'm closing this issue.

If there's anything that should be added to the documentation for this rule, please feel free to open a new issue.