Skip to content

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

Closed
@CherryDT

Description

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)

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions