Reconsidering return-await

[KilianKilmister]

Summary

Making return await mandatory can't be reasonably achieved in some coding styles.

Clipnotes

• depending on project architecture, this rule enforces considerable drag and can actively work against clean error handeling
• lengthy discussion has clearly shown that this isn't a clear cut choice link to upstream
• the original eslint rule is in place to enforce the exact opposite
• the feature was accepted neglecting valid concerns inspite there being a generally agreeable middleground (PR)

Pros and Cons of return-await

It basically boils down to two arguments:

• pro: issues can be handeled in the function itself and it is denoted in the stack-trace
• con: in requires an extra micro task for the promise to resolve and it increases the call-stack

I'm not going to argue about what is usually the better practice. if you would like to read/share opinions about it i would like to refer you to this issue.

Contexts in which return-await is non-sensical

Function wrapping and Partial-Application

convenience wrappers for async functions where one of it's aruments is known ahead of time is a common pattern. You not only don't need that in a stack-trace, it would just get in the way. All you want is to relay that function call 1 to 1.

this is a method from one of my classes and @typescript-eslint/return-await: always wants me to await that. I implicitly don't want that addition to my call-stack.

  async readFile (options?: Encoding<{ flag?: OpenMode} >) {
    return fs.readFile(this.path, options)
  }

Recursive function calls

When you return with a function call, your call-stack doesn't increase. This is an amating feature when working with a recursive-oriented project architecture, as without this, you run the risk to exceed the maximum stack size. This is also where the aditional micro-task can become a real burden, as your architecture is disigned around chopping up larger processes into small recursive functions which configure themselfes based on a single input and then simply adjusting the recursive call as needed and in case of a malfunction they will pass the error back down the chain pinpointing the exact place where it took place. Anything in between will largely be irrellevant to the cause, as recursion is so predictable. When your dealing with regular chains of hundreds or more of these recursive calls, this addition to the call-stack and the added micro-task become a real issue

return value transformatio, error decorating and fail-saves

It's a common practice to decorate or transform a value in some manner. These interrim steps do not need to be aware of any details, and since they are built to handle one specific shape only, any issues happening inside of these steps or at the reciever is guaranteed to be caused outside of them.

In a module of mine, i have a recursive file-system operation able to fork as needed. It's vital for an error occuring in one branch to stop every other branch dead in its tracks to avoid the system crashing down in an avalanche of errors. Each branch will need to do it's own cleanup in such a case, and the original error needs to flow down to the root to pinpoint exactly where the error occured. As the entire point of this structure is to be stateless, safeties and closures are used to handle everything in a decentralized manner. Indexing and eventlisteners are other ways to handle this, but as elements are never used again, the constant addition and subsequent cleanup will cause considerable overhead.

If the chain breaks down, we want to decorate the error appropriatly so we need to do this inside the offending cycle as it is the only one who knows the cause at that point. Every stage needs to be able to do that, but if the stage isn't a a leaf, it needs to pass it on as fast as possible and initialize it's cleanup after that.

These are the magic lines and @typescript-eslint/return-await: always wants me to await both onReject and onFinally.

return Promise.all(promises)
    .then(
      // not much point in having an array of `void`, so were reducing it to one `void`
      () => {},
      // if no root is declared on the error, declare yourself the root
      // and let the error propagate down the chain
      (err) => { err.errorRoot ??= src; return Promise.reject(err) }) // <-- the entire point of this one is to pass the rejection down as fast as possible. and i am now supposed to await an explicit rejection?
    // safety for closing the dir
    .finally(() => dir.close()) // <-- i coudln't care less what's going to happen here. so why should i await it?
}

At the root of this is the exposed function, and here comes the kicker: As everything is handeled automatically from the first call of the recursive copyDirInternal, i'm actively resetting the stack-trace to point at my API surface. the internal workings of the recursion are so predictable and errors are intercepted so early, the stack trace can't contain any information that's not allready known beforehand, or found in the error itself. The error will have been caused outside and independent of this function. and it's internal stacktrace will be irrelevant.

  return copyDirInternal(src.toString(), dest.toString(), mode, window)
    .catch(async (err) => {
      // if an error is reported we engage the safety
      isSafe = false
      // adjust stack-trace to point to the function-call instead of our internal code
      Error.captureStackTrace(err, copyDir)
      // and pass on
      return Promise.reject(err) // <-- i'm supposed to await this apparently
    })

Why was this decision made despite very reasonable objections AND the existence of a middle ground that both sides agreed upon?

I would really like to have an explanation on this. I only explored eslint-config-standard-with-typescript after having been a long user of standard(x). And the fact that this rule was added very much suprised me.

I do feel that this project has made descicions that were very one-sided and against the best judgement.

Suggested Course of Action

Change the Rule Configuration from always to in-try-catch

Enforcing this rule in its current configuration is unreasonable in various scenarios and lengthy discussion has clearly shown opinion about it to be very split.

The Original ESLint Rule

the original eslint rule actively enforces not to return-await. It does however exclude return-await inside try-catch blocks, as there it does make sense. The try-catch block is in place to handle rejections locally. @typescript-eslint/return-await has an option to enforce it only inside try-catch-finally, and i believe i made a strong case for changing the config accordingly.

Concluding

I would like to quote @bradzacher statement which summarizes it very well.

Ultimately, I guess the question is, "how much is that extra stack frame worth to you?"

Would you rather take the perf impact of a return await to give you the added debuggability? Or would you rather structure your code in such a way that this extra stack frame isn't required (i.e. eliminate multiple calls to the same function where possible my splitting code into multiple functions, etc.

There's no "right" answer here, I don't think there's a true "hard and fast rule" which should say "always always do this". However I do think that always return await is wrong, though I concede that in certain cases, it has its uses. As with all linting - there are always exceptions, which is why the eslint-disable class of comments exist, so you can knowingly work around a lint rule, when you have a good reason.

My recommendation is still to use our return-await rule with the in-try-catch option, and know that you should still stop and think if you might want a return await in a specific case.

[LinusU]

So, I've read thru your post a few times now, and I still feel that having always is the best approach for standard. I also feel that you are being a bit hostile, saying that we were "neglecting valid concerns", etc. Remember that everyone is volunteering their time to this project, and you are under no obligation to use it...

I will try to address the different points you bought up:

• Function wrapping and Partial-Application

  You not only don't need that in a stack-trace, it would just get in the way.

  I feel that this statement is a bit weird. If we were talking about regular (non-async) functions, would you still want these functions not to show up in the stack trace? For me, I want the stack trace to show every function call, so that I can follow along in the code. Even if some of these functions is just small helpers and doesn't really do anything, I think that they don't get in the way by printing one extra line in the stack trace. And the few times that the problem lies in the small wrapper function, one is very happy that they were there.

• Recursive function calls

  I think that I would need some more examples here to understand your problem. JavaScript doesn't have TCO so ev en with normal functions it adds to the stack trace if you return by calling yourself.

• return value transformatio, error decorating and fail-saves

  Again, I'm having a hard time seeing how an extra micro-task would affect anything here. IO is not performed in micro task so any micro task will happen before the next IO callbacks are fired. Even so, your example can be very easily resolved without scheduling a micro task:

   return Promise.all(promises)
       .then(
         // not much point in having an array of `void`, so were reducing it to one `void`
         () => {},
         // if no root is declared on the error, declare yourself the root
         // and let the error propagate down the chain
  -      (err) => { err.errorRoot ??= src; return Promise.reject(err) }) // <-- the entire point of this one is to pass the rejection down as fast as possible. and i am now supposed to await an explicit rejection?
  +      (err) => { err.errorRoot ??= src; throw err }) // <-- the entire point of this one is to pass the rejection down as fast as possible. and i am now supposed to await an explicit rejection?
       // safety for closing the dir
       .finally(() => dir.close()) // <-- i coudln't care less what's going to happen here. so why should i await it?
   }

  With dir.close() I'm not exactly sure what you want. If you don't want finally to wait for dir.close(), simply make the functions have braces to remove the implicit return: () => { dir.close() }.

  I think that one thing that comes into play here is that this style guide is optimised for using async/await, but you seem to be using plain promises instead. How about writing that code as such:

  try {
    await Promise.all(promises)
  } catch (err) {
    err.errorRoot ??= src
    throw err
  } finally {
    dir.close()
  }

  and with your second example:

   return copyDirInternal(src.toString(), dest.toString(), mode, window)
  -  .catch(async (err) => {
  +  .catch((err) => {
       // if an error is reported we engage the safety
       isSafe = false
       // adjust stack-trace to point to the function-call instead of our internal code
       Error.captureStackTrace(err, copyDir)
       // and pass on
  -    return Promise.reject(err) // <-- i'm supposed to await this apparently
  +    throw err
     })

  which I think is better written as:

  try {
    return await copyDirInternal(src.toString(), dest.toString(), mode, window)
  } catch (err) {
    // if an error is reported we engage the safety
    isSafe = false
    // adjust stack-trace to point to the function-call instead of our internal code
    Error.captureStackTrace(err, copyDir)
    // and pass on
    throw err
  }

I would also like to know if you have had any measurable performance regression by changing return to return await. We have deployed this in three production workloads, and haven't been able to see any performance regression at all. We have however seen much better stack traces when things go wrong, which have been invaluable for us.

[KilianKilmister]

I also feel that you are being a bit hostile, saying that we were "neglecting valid concerns"m etc.

Any supposed hostility is based on interpratation, as i discribed my reasoning for that in detail.

Remember that everyone is volunteering their time to this project, and you are under no obligation to use it...

1. this is irrelevant to the point being discussed and in no way adresses the stated issue
   • if for example @typescript-eslint was suddenly removing the feature to share configs and would reply to protests by with that quote, that wold not be seen as justified
2. this apears to support my statement about a percieved onesidedness that disregards the needs of the community. Quoting my line:

   I do feel that this project has made descicions that were very one-sided and against the best judgement.

I will try to address the different points you bought up:

and

I would also like to know if you have had any measurable performance regression by changing return to return await. We have deployed this in three production workloads, and haven't been able to see any performance regression at all. We have however seen much better stack traces when things go wrong, which have been invaluable for us.

These two points very much disregard the core of my issue:

• I'm not going to argue about what is usually the better practice.

• • lengthy discussion has clearly shown that this isn't a clear cut choice link to upstream

Addressing specific points.

Function wrapping and Partial-Application

I feel that this statement is a bit weird. If we were talking about regular (non-async) functions, would you still want these functions not to show up in the stack trace?

Nothing this frame of the stack-trace can show me adds any aditional information, so it's inclusion is very much irrellevant to project expressiveness. I know which wrapper was at the root of the problem, and the parameter that's applied inside of it is known at call-time. in places where this isn't the case by defaut, i will have made preparations to include the nescessary information, giving me an error which is expressive enough and in contrast to the stack-trace, has the nescessary information conzentrated in a neat package, making both personal debugging work, and error-reporting from others much easier.

I mean for example how often do you actually use stack-traces of NodeJS internal code? I use them regularly, but it still only makes up a tiny fraction of the of things i use the stack-trace for. and if i do, i can just set relevant break-points. debuggers usually hide stack frames of that internalI code by default anyways, exactly because it does not matter for the wast majority of use-cases and all it would do is throw a large amount of data at the a user who very likely doesn't have to know any of it. Worse than that, it might just obfuscate the actual source of the error and cause unnecessary cofusion. If i have to use that information, it is usually a much better aproach to simply set a break-point.

Recursive function calls

1. I'm primarily talking about call-stack size, not stack-trace information.
2. As with the previous point, all information provided in a stack-trace of such recursion is known at call-time

Take a look at how an unadjusted stacktrace of such a structure looks and how utterly useless any of it is:

file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:13
    return someCall(param)
           ^

Error: something went wrong
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:13:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)
    at doSomething (file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:11:12)

This stack-trace could not be less irrellevant, not just for the developer, but consumer of such a package will likely not have a sinlge clue about what's going on and what he is supposed to report. This recursive operation is 100% static, so if the input is known, the exact progression of the recursion is knwon.

This is what a stack-trace should look like. it points directly to the cause of the issue:

await init ('some wrong param')
^

Error: something went wrong
    at async file:///Users/kilianhefti/dev/GitHub/init-package-json/packages/utils/dist/test.js:25:1

Everyone knows where it was caused, and if a consumer thinks this is a bug, he will know exactly what to report

Don't you agree that the additional stack frame is null and void in this context?

return value transformatio, error decorating and fail-saves

Even so, your example can be very easily resolved without scheduling a micro task:

With dir.close() I'm not exactly sure what you want. If you don't want finally to wait for dir.close(), simply make the functions have braces to remove the implicit return: () => { dir.close() }.

and with your second example:

Irrelevant. this doesn't provide any statement on the merit of each method. Adding // eslint-ignore-line return-await is functionally eqvivalent. The result being having to modify both functional and expressive code with zero benefit.

I think that one thing that comes into play here is that this style guide is optimised for using async/await, but you seem to be using plain promises instead. How about writing that code as such:

This doesn't make sense. async/await is just sugar for a promised-based structure. This distinction between them is arbitrary in this context.

I would also like to know if you have had any measurable performance regression by changing return to return await. We have deployed this in three production workloads, and haven't been able to see any performance regression at all. We have however seen much better stack traces when things go wrong, which have been invaluable for us.

If and to what extent there is a performance decrease is of no relevance to the point being made here.

This has been a very unsatisfying response in that it did not adress anything i had to say.

[mightyiam]

@rostislav-simonik and I have been researching this. There's clearly a problem with always. And not just extraneous micro-tasks, but also for semantic reasons.

The only part of the rule where it seems consistently useful and therefore appropriate to enforce in this package is in try-catch-finally. But the rule has no option that addresses only the try-catch-finally context and disregards elsewhere.

We are not yet up-to-date on the discussion here. We're on a ~2 hours per week schedule.

[LinusU]

@mightyiam I'm not sure that I can clearly see the problem with always, would you mind elaborating?

I personally feel that enforcing return await is very useful for us.

[mightyiam]

I surely will elaborate and discuss, @LinusU. Don't worry.

[mightyiam]

@rostislav-simonik and I feel that we have read enough to join the discussion.

We do acknowledge that at least in some cases, such as @KilianKilmister provided, forcing return await is not useful and even detrimental at least by resulting in unnecessarily long stack traces.

We could really use input from more users, here. The questions we'd like to ask everyone, including @KilianKilmister and @LinusU is this:

1. How often do you return promise instead of the return await promise that is currently enforced? Can you provide an estimated ratio?
2. Can you explain your choices? Do you understand the tradeoffs? Do you merely follow the current enforcement?

Also tagging @haltcase and @jirutka.

[LinusU]

1) We always use return await promise, and have yet to find a single instance when we would want to do just return promise.

2) I would say that I understand the tradeoffs since I've spent a lot of time researching this and commenting on this and other threads about it. I still think that having better better debuggability is worth more than some potential performance hit that I still have yet to see anyone posted some actual measurements/benchmarks on.

[rostislav-simonik]

We simply don't have enough feedback to make a change, we are going to close this. If more feedback is obtained we will reopen.