Bug: [return-await] unsafe autofixes in error handling code

[kirkwaiblinger]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.4.2&fileType=.ts&code=CYUwxgNghgTiAEYD2A7AzgF3gcxBgMkmANYBc8AFAJTwC8AfPFCgJ4DcAUB1GiymPABmAV34YAlqhx4AqmhAwAkigAOwjNXIAFGEgC24%2BQB5MMcSmyMA3h3jwA9Pfhp9CZKHgYAFlCwgAHuDqIGjwAPq8-F66KEjCaBAsYfDAwmYWnl4IguIwmEyRAoIwUHogtg5OzMDwevFYPioqICjwAEYggkhw8OIYAOShaFCCCBhI8ABEohBExJOZCLMkAHQVyOhY8QrKali0TADuUH3wOvqGICtwLhAAbiAUkwDKrvAqugbymb6IzPCoRLwG5Ie4IEYYBSLeDLYi9UL9GZzEDAfqTKicOxwDBpVrbJSqdScAC%2BXB4fCKojAEikmGEKnEwGeOIZwAAwkhQJozp9LiYMOlLPAbHYIHgYXM6NICHNqJjPDAWMKKnZHCkJi4ys4cYJBD8sHooErvLpDms7Fi8LjpXIdoSNBiKsShOYoBAgSKLbDriAxTxHo67KTiUA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQHYHsBaaRAF1ml0IEMB3agS1PSnwDM3IAacbSAAVIBPAA4oAxtAYjShFPAa5SAehLlKNekxaRE0aPmiReAXxAmgA&tsconfig=&tokens=false

Repro Code

declare const getLock: () => any;

async function getUserInput(): Promise<string> {
  // some code that executes _synchronously_ during the first async frame
  // and must happen before it's safe to "unlock" the lock.
  const userInput = await Promise.resolve("Some promise that can only resolve after the lock is 'unlocked'");
  return userInput;
}

async function stupidStupidCode(): Promise<string> {
  let lock = getLock();
  try {
    // do some stuff that may throw.
    return getUserInput();
  } finally {
    lock.release();
  }
}

ESLint Config

module.exports = {
  "rules": {
    "no-return-await": "off",
    "@typescript-eslint/return-await": "error"
  }
}

tsconfig

undefined

Expected Result

I expect that the line return getUserInput(); should flag and give me a suggested fix of return await getUserInput();

Actual Result

autofixes, rather than suggestion.

Additional Info

The programmatically fixed code results in deadlock :( Adding the await in that context makes the unlocking wait for the user input, but the user input is waiting for the unlocking.

I realize that the interaction between the two functions in the example is very crappy and brittle due to depending on code in an async function to execute synchronously, but... it's 100% real code that I encountered at work, and, even though we only had return-await enabled as a warning, my "autofix on save" editor settings led to this getting autofixed in a file I was working on, and I figured, eh, autofixes are safe, i don't need to think too hard about what that function does, and we released it, causing a very difficult-to-debug bug in production (total showstopper too, lol 😆).

This simply isn't safe, in my opinion, to provide as an autofix rather than a suggestion which needs to be reviewed by a human.

Note that there are things that I think are totally safe to autofix, though! For example, depending on your config for this rule, you may switch either direction between these code examples safely

async function safeToAutofix(): Promise<void> {
    return asyncFunctionCall()
}

async function safeToAutofix(): Promise<void> {
   return await asyncFunctionCall()
}

It's just when error handling stuff comes in that the control flow gets messed up and can lead to very very subtle bugs.

[bradzacher]

I don't think that there's a bug here - instead it seems to me like you have configured a lint rule to use a style that is fundamentally incompatible with your codebase.

Your application logic explicitly relies on the presence or absence of the await in order to function correctly - so enforcing that you "always return await is incorrect in your codebase.

---

It's not possible for us (or typescript, for that matter) to detect this sort of implicit interdependency between things.

It's generally very, very, very rare that this sort of code style is used in JS code given that JS is single-threaded. Most of the time codebases will use the promise themselves as the locking signal to avoid the need to maintain an external lock because an external lock is a manual, complicated, error prone thing.

Evidence of the rareness of this pattern is that we have numerous promise rules that auto-fix things and have existed for >5y (eg this rule is 5y old) and this is the first report ever we've seen of someone using a locking system!

Given the rarity of this I'm inclined to close this as a known, rare edge case. Maybe it could be documented as a caution.

thoughts @typescript-eslint/triage-team?

[kirkwaiblinger]

i mean, sure.... the example that I came up with is minimal reproduction of something that wasn't literally a lock in the original issue, but still something that blocked the getUserInput function. Either way, it certainly was in the wild, and the implementing change to fix it would be very quick and easy (and, also, it's already done to be honest, see https://github.com/kirkwaiblinger/typescript-eslint/compare/main...return-await-safer-autofixes?expand=1), so... might as well?

FWIW my perspective is that the error handling control flow aspect of this rule is very much a central concern, since the default config explicitly flags different code based on whether it affects the exception handling control flow. Thanks to that, it's almost a trivial change to use those existing codepaths to decide between autofix or suggestion.

[kirkwaiblinger]

oh, sorry, one thing to clarify

instead it seems to me like you have configured a lint rule to use a style that is fundamentally incompatible with your codebase.

This is a situation where we inherited a codebase that had never been under lint, and had been turning on lots of linter rules and running autofixes on them. So, that particular code was already crappy, should never have been written that way, and, very much correctly was flagged by the linter! I don't think that the codebase was incompatible with the linter or whatever, i think that the linter was finding the crazy skeletons in the closet :) Just, not every skeleton is safe to autofix, despite very much needing to be fixed.

But, I guess, more generally, I really do think it's a no-brainer to update the rule exercise caution with the following completely standard scenario. Maybe it's a better illustration.

async function functionThatHasErrorHandling() {
    try {
        // would autofix to return await thingThatMaybeRejects();
        return thingThatMaybeRejects();
    } catch (error) {
       // This code has never run in the history of the codebase. But the 
       // authors of this codebase clearly don't know that, since, well,
       // they wrote this dead exception handling block in the first place.

       // Why should we assume it's safe for it to do so now,
       // rather than flagging the line with a suggestion (or custom error message)
       // that alerts the programmer that this previously dead codepath will now
       // potentially start to run?

       exec('sudo rm -rf system32');
    } 
}

It would seem there's precedent for this kind of caution, looking at things like https://typescript-eslint.io/rules/prefer-optional-chain/#allowpotentiallyunsafefixesthatmodifythereturntypeiknowwhatimdoing. Only this seems like a stronger case, since it's something that wouldn't be caught by a Typescript error.

[JoshuaKGoldberg]

I think we need to switch it from a fix to a suggestion. Switching from return promise to return await promise changes the behavior of the code. Although it's almost always better behavior (since it captures the stack better with try semantics), it's still different behavior.

[bradzacher]

This also occurs with the new using keyword as well.

function getLock() {
  const l = lock();
  return { [Symbol.dispose]: () => l.release() };
}

function foo() {
  using foo = getLock();

  // unlocks as soon as the "sync" code finishes
  return getPromise();
  // unlocks once the promise resolves
  return await getPromise();
}

(ofc this new syntax implicitly uses finally under the hood)

[bradzacher]

I think we need to switch it from a fix to a suggestion

I'm not sure I agree - the behaviour should be consistent across really every case aside from the finally and catch cases.

Though I disagree that catching in a different location is a dangerous change in behaviour to exercise code paths that were previously unreachable due to a bug.

I'm okay if we add specific handling for the finally case to make that a suggestion, but I don't think other cases need be a suggestion.
My justification is that if it were a problem to have this as a fixer we would have had at least one issue filed over the last 5 years since the rule was added.

[Josh-Cena]

I'm in agreement with @JoshuaKGoldberg that it should be a suggestion, even if for most users it doesn't matter. Autofix should not change code behavior in a salient way.

[kirkwaiblinger]

It seems this issue hasn't had any activity for a while and opinions still diverge... 🤔

I still think it seems simplest both conceptually and implementation-wise to posit "wherever the in-try-catch setting requires the use of return await ought to be a suggested fix rather than an autofix", thereby deferring all questions of what constitutes a relevant control flow change to previous work, rather than relitigating them here. (I realize I may have unintentionally opened the door to that in the issue report when my intention was to just demonstrate the value proposition).

@bradzacher I don't mean to push too hard, but would that resolution be acceptable?

[JoshuaKGoldberg]

Coming back to this: @kirkwaiblinger and I poked at the control flow nuances and it seems pretty reasonable to me that we'd switch any fixer that can change control flow to a suggestion. Marking as accepting PRs with the note that this shouldn't change all fixers to suggestions. Just the ones that modify control flow. 👍