standard: Remove no-return-await as it sets traps during code refactoring and causes inferior stack traces

What version of this package are you using? 14.3.1

What problem do you want to solve? The no-return-await rule introduced in #695 is the only one with which I continue to disagree, and I think since the arrival of async stack traces in Node, there is now another reason to rethink it.

Therefore I want to open a discussion here. My point is that the no-return-await rule (which disallows “redundant” use of return await promise) has two negative effects:

1) It reduces the usefulness of error stack traces

Node now has zero-cost async stack traces with the --async-stack-traces flag. There, it makes a difference whether return promise or return await promise is used - the former omits a stack frame.

Example:

const delay = require('util').promisify(setTimeout)
const throws = async () => { await delay(100); throw new Error('oh noez') }

const a = async () => { return throws() }
const b = async () => { return await throws() }

async function main () {
    try { await a() } catch (e) { console.error(e) }
    try { await b() } catch (e) { console.error(e) }
}

main()

Result:

david@CHE-X1:~/z/Temp $ node --async-stack-traces example.js
Error: oh noez
    at throws (/mnt/c/Users/david/Temp/example.js:2:54)
    at async main (/mnt/c/Users/david/Temp/example.js:8:8)
Error: oh noez
    at throws (/mnt/c/Users/david/Temp/example.js:2:54)
    at async b (/mnt/c/Users/david/Temp/example.js:5:32)
    at async main (/mnt/c/Users/david/Temp/example.js:9:8)

Note how there is no stack frame for a.

2) It lays out a trap for code refactoring by making it easy to overlook that an async function is being called

Example with subtle unintended behavior:

return somethingAsync()
// refactor to
try {
  return somethingAsync()
} catch (e) {
  // ...
}
// whoops, now suddenly it makes a difference - errors are caught only
// if they happen in the synchronous part of `somethingAsync` - hard to spot!

Example with more obvious unintended behavior:

return somethingAsync()
// refactor to
return somethingAsync() || null
// whoops, that did nothing, because the promise won't be null

Another example:

return somethingAsync()
// refactor to
const value = somethingAsync()
await moreStuff()
return value
// whoops now somethingAsync runs in parallel to moreStuff, which
// may not be immediately obvious and cause a race condition

Or even:

return somethingAsync()
// refactor to
return { value: somethingAsync() }
// whoops now the outer code gets a promise in an unexpected place
// and if it previously was a boolean, it may not even be noticed, but
// conditions will now get always a truthy value...!

There are many more examples like this which don’t immediately crash (it is easy to spot something like somethingAsync().toUpperCase() is not a function, but my examples above are more subtle). If I modify existing code, I may not be familiar with every function that is being called (and I may even use a refactor tool where I just select the returned value and extract to a variable for example). Usually, I’d easily recognize that await somethingAsync() should probably stay this way, but with return somethingAsync() I may not realize that in any place other than the return statement, I’d need an await in front (and even if it stays in a return, other things like the aforementioned try block may suddenly require a change here - and imagine that the return is somewhere inside a larger block of code which I’m now wrapping with try)!

I had all of these issues from my examples happen in real life.

To work around this issue, I manually mark return somethingAsync() with a comment, e.g. return somethingAsync() // async but that seems ugly and much less useful than having the obvious await there (plus, it doesn’t solve the try case).

What do you think?

What do you think is the correct solution to this problem? Remove the no-return-await rule

Are you willing to submit a pull request to implement this change? I guess so…? (but I guess it would be easier for you)

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 9
Comments: 43 (15 by maintainers)

Commits related to this issue

:cop: Disable no-return-await Contrary to ESLint Documentation, returning await has valid use cases. By explicitly marking that the return value needs to be awaited, code is more clear and easier to ... — committed to ricealexander/eslint-config-webtrinkets by ricealexander 4 years ago

Most upvoted comments

There’s already an eslint rule that forbids return await outside of a try, so you don’t have to remember anything

Well, I have to remember that I need to add it inside the try, which is very easy to forget if the linter normally forces you to not add it.

I’ve actually seen this caught in code review more than once, people forgetting to add await in the try, and I think that this could be somewhat mitigated by having the mindset that async values should always be awaited.

I still strongly feel that we should:

Removing the rule completely in the JavaScript part
Enforcing the use of return await in the TypeScript part
- (that is, enforcing return await when you are trying to return a PromiseLike<_> from an async function, the always setting of the @typescript-eslint/return-await rule)

LinusU on Mar 4, 2020

I value your input, @CherryDT.

So, your claim is that the no-return-await rule is bad practice and that the opposite is good practice.

Your examples make sense to me. Personally, when I am using TypeScript, I am protected from that kind of refactoring errors. But that’s just a side note.

Is there discussion in ESLint about the usefulness of no-return-await? Yup. Here it is.

I feel that what @ilyavolodin suggested is reasonable and would allow us in Standard to choose the opposite. Rename the rule to allow the opposite or make a new rule that does the opposite. @CherryDT would you be interested in making that contribution? That would allow you to enjoy your preferred style regardless of what Standard ends up deciding.

And I would appreciate input from more users on this ❤️ .

mightyiam on Oct 21, 2019

[…] is redundant and incorrect and makes your code artificially slower.

Hmm, I don’t really see how it “incorrect”? and it isn’t really “redundant” since the behaviour is different.

If someone wants to incur this cost in exchange for better stack traces in one JS engine, that’s fine! but they should have to disable the rule for that.

This has been brought up in another thread about this, but this isn’t because of one JS engine. I don’t see how one engine could choose to add a function that isn’t running anymore to the stack trace?

It’s an async function, that’s what makes it wait for an async result.

Not really though, if you return a Promise from an async function it doesn’t wait for that result. It immediately returns that promise and the function is no no longer executing. This is why it would also be wrong for a specific engine to keep the function on the stack trace, and why return await is necessary for that.

Something that encourages an incorrect understanding of what the language does is worse for humans, even if it appears to be better for some humans in the short term.

I really don’t understand why this would give an incorrect understanding of the language. To me it seems like the root of the problem here is that Promises flat maps automatically when resolved with another promise.

This is convenient, but this is really what hides what is happening here. Without this feature return a promise without doing await on it from an async function would actually return a Promise<Promise<...>>.

In e.g. Rust this is very nice because you can see this very clearly, and thus it requires you to do their equivalent of return await unless you want to return a future of another future.

However, I don’t think perf actually matters here

I really agree with this, an extra await is not going to be your performance bottleneck, and we are optimising for code readability and correctness firstly.

i think return await is incorrect code, and tools allowing it (or worse, requiring it) will confuse people about how async functions, and await, actually work.

I really don’t think that it will confuse people though 🤔 As I said earlier, I feel that the thing confusing people is really the automagic-flat-mapping…

I still strongly feel that we should remove no-return-await in Standard, and add @typescript-eslint/return-await = always in Standard TS.

LinusU on Feb 28, 2020

Fixed in 14.3.4 :shipit:

LinusU on May 11, 2020

Sure, i agree - and you should use return await x when you need to await the result of x within the function, ie, inside a try block. Otherwise, you’re awaiting for no apparent purpose.

ljharb on Feb 28, 2020

Since we have no implementation of an alternative and since the rule seems to me to be more detriment than use, I would have it removed.

mightyiam on Oct 31, 2019

The better stack traces are a nice side effect but the major issue is that it covers up what the code is actually doing, namely waiting for another asynchronous result. Hence all my examples about how it hinders readability and adds traps for later modifications. My point is that it is bad practice to encourage that. I and my colleagues have wasted several hours of our lives due to traps created by this rule. That can’t be the goal…

There are many other rules following the philosophy of making the code “normalized” and easier to read and less likely to break on changes (to code or environment), so why should this one actually push into the opposite direction? For example, there is no-path-concat, and I’m quite sure that path.join is slower than + by at least an order of magnitude. Or no-throw-literal, I bet throw new Error('xyz') and if (e && e.message === 'xyz') are slower than throw 'xyz' and if (e === 'xyz'). Yet, these rules are worth the (small) speed difference, since they avoid trouble.

(In the path case, it would break on another platform, and in the error case it would break when someone makes the assumption that errors have a property stack or message for example - just like the assumption that turning return getCustomer() into return getCustomer() || defaultCustomer will work as expected…)

Correct me if I’m wrong, but I thought the goal of StandardJS was to eventually save time developing, increase code clarity and avoid common pitfalls (such as these refactoring issues) - not micro-optimize code. For example, the extra parentheses in if ((x = y)) are also redundant for sure, yet they add an important hint to the reader and avoid human errors - just like return await. (I think the opposite rule should exist - always use return await in this case.)

I quote:

This module saves you (and others!) time in three ways:

[…]

Catch style issues & programmer errors early. Save precious code review time by eliminating back-and-forth between reviewer & contributor.

Adopting standard style means ranking the importance of code clarity and community conventions higher than personal style. This might not make sense for 100% of projects and development cultures, however open source can be a hostile place for newbies. Setting up clear, automated contributor expectations makes a project healthier.

(Empasis mine.)

Maybe I should edit this issue to reorder my points, since actually point 2 is the major issue, not point 1. I don’t understand why everyone is arguing about “stack traces for one engine” (here and at eslint) when the main problem is another one.

CherryDT on Feb 28, 2020

@CherryDT what i mean is, return await, outside of a try, is redundant and incorrect and makes your code artificially slower. If someone wants to incur this cost in exchange for better stack traces in one JS engine, that’s fine! but they should have to disable the rule for that.

ljharb on Feb 27, 2020