Bug: [no-floating-promises] Incorrect handling of finally calls

[aleclarson]

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=4.8.3&sourceType=module&code=MYewdgzgLgBAJgUwLYgGIFczCgS3ALhgAocwAHdKQgIxBABsEBDMAShgF4A+GABQCcQSHBAQAeaP1IBzHh2KkKVGLQbM2nHgG8AUDBg4AZgvKV2u-fv4Io6fmD6DhogHTWIDAG4IiAcgAWCPT0IDAA7iD89HC+rADcejAAvjBBojAWlta29o5CIghuCABWCNh+ipThTBAwhkz0orEJ+kk6bTqIKBhYuOBEUPzoCKwuUIFgRETs3BmJoJBqLiHSfgCiYFAI1nAw4whgze2jhqQN9ACeUzPa8+AejMsgq74bWzt1Z8EXR0nxQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1oDN4OBDfMwDmtYtA4BbSshTooiaBOiRwYAL4h1QA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Repro Code

demoFunction(true).then(() => {
  console.log('Entered then');
}).finally(() => {
  console.log('Entered finally');
});

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/no-floating-promises": ["error"],
  },
};

tsconfig

No response

Expected Result

There is no .catch call, and the rule should report it.

Actual Result

Nothing reported

Additional Info

No response

Versions

package	version
@typescript-eslint/eslint-plugin	5.31.0
@typescript-eslint/parser	5.31.0
TypeScript	4.7.4
ESLint	8.20.0
node	18.7.0

[JoshuaKGoldberg]

Hmm, we'd explicitly made this the behavior in #1603 -> #1620. But now I'm not so sure this was the right move. .finally() creates a new Promise, and the code inside it might throw an error.

console.log("a");
Promise.resolve().finally(() => {
  throw new Error("Finally!");
});
console.log("b");

a
b
Uncaught (in promise) Error: Finally!
    at <anonymous>:3:9
    at <anonymous>

cc @a-tarasyuk @bradzacher for historical context. Perhaps we should make the ignoring of .finally(...) calls behind an option for the rule?

[aleclarson]

Floating promises can't be entirely avoided, since .catch handlers can throw. That said, the real issue here is that .finally handlers are basically just sugar for…

p.finally(handle)
// …same as…
p.then(
  async (result) => {
    await handle()
    return result
  },
  async (error) => {
    await handle()
    throw error
  }
)

…and so they should be treated the same as .then calls by the no-floating-promises rule.

[aleclarson]

Another case that should probably be fixed:

// Nothing reported by linter.
demoFunction(true).then(() => {
  console.log('Entered then');
}, (error) => {
  console.log('Entered catch');
});

In this case (perhaps unintuitively), errors thrown in the success handler aren't caught by the rejection handler.

The above code is equivalent to this monstrosity…

const catchReturn = Symbol.for('promise.catchReturn');
demoFunction(true).catch((error) => {
  console.log('Entered catch');
  // Throw to skip the success handler. Use a symbol to identify our throw.
  throw { [catchReturn]: undefined };
}).then(() => {
  console.log('Entered then');
}).catch((error) => {
  if (error && error[catchReturn]) {
    return error[catchReturn];
  }
  throw error;
});

…which should be (but isn't) reported by the no-floating-promises rule. The reason I say it should be reported is that the last .catch handler contains a throw statement, which isn't handled anywhere.

We can get an equivalent implementation of my last two examples to be reported by the no-floating-promises rule by changing it to be the following…

const caught = Symbol.for('promise.caught');
demoFunction(true).catch(() => {
  console.log('Entered catch');
  // Return a symbol to signify an error was caught.
  return caught;
}).then((result) => {
  if (result !== caught) {
    console.log('Entered then');
  }
});

…so I think it stands to reason that the first 2 examples should also be reported.

[bradzacher]

In #1620 it was written that .finally should be treated like a .catch.
In hindsight this isn't correct, because .finally is "transparent" in regards to the promise chain - the value returned from .finally does not change the settled value of the promise unless the .finally handler throws an error.

So what should the handling be?

I think that .finally should essentially be completely ignored by the rule - it shouldn't end a chain, it shouldn't start a new chain - it should just be skipped over and the rule should pretend like it wasn't there.

I.e.

// BAD
prom;
prom.then(f); 
prom.finally(f);
prom.then(f).finally(f);
prom.finally(f).then(f).finally(f);

// GOOD
prom.then(f, f).finally(f);
prom.catch(f).finally(f);
prom.then(f).catch(f).finally(f);
prom.then(f).finally(f).catch(f).finally(f);
prom.catch(f).finally(f).finally(f).finally(f).finally(f).finally(f).finally(f).finally(f);

---

The last caveat mentioned above is something to consider - should a .finally be guarded by a .catch considering that .finally could throw and change a "resolved" promise to a "rejectec" one?
In an absolutely strictest sense - yes! But in the general devx friendly sense - no.
Perhaps it could be an option? But I doubt anyone would want to enforce it cos it would force you to write code like this:

prom.catch(f).finally(f).catch(f);

Which is really cumbersome.

[kirkwaiblinger]

I am working on a PR (#6881) which forced me to think about this a bit... For the purposes of that PR I regarded .finally as different from the rejection handlers in .then and .catch, though I noticed that the existing code treated them the same.

I'd be happy to look into putting up a PR for this issue as well.

To build off of @bradzacher's suggestions:

I also noticed that from the point of view of avoiding literal unhandled rejections, finally can only make an unhandled promise worse. However, the more I think about it, I can also understand the argument that adding a .finally means you are, well, handling the rejection. Not in the sense of preventing a rejection from bubbling up, but in the sense of you, the developer, are acknowledging a rejection could occur, and taking some mitigating action. It's similar to being able to mark floating promises as ignorable with the 'void' operator - you know that they can reject, but you're not worried about it.

My preference for this rule would be to make a .finally totally transparent, regardless of whether it has a handler or not.

asyncFunction().finally() // Currently bad, Propose: still bad
asyncFunction().finally(f) // Currently good, Propose: bad
asyncFuncion().catch(c).finally() // Currently bad, Propose: good
asyncFunction().catch(c).finally(f) // Currently good, Propose: still good

JS/TS is not an exception-safe language, so I think it would be overbearing to consider the possibility of the finally handler throwing. Actually, I think that a .catch handler is possibly more likely to throw than a .finally handler in practice, since:

// This is common in synchronous code.
try {
    mightThrow()
} catch (e) {
    if (isExpectedError(e)) {
        // handle error
    }
    throw error;
}

// It's not crazy to me that this would be present in async code
mightReject().catch(e => {
   if (isExpectedError(e)) {
       // handle
   } 
   throw e;
})

So let's just take the fact that .finally is transparent to the promise status at face value.

One more thought, just to take this argument ad absurdum, by turning to synchronous equivalent again:

try {
    mightThrow()
} catch (e) { 
   // handle error
} finally {
   // have you ever seen anyone put exception-safe code here ???
   try {
       // do the cleanup work
   } catch (e) {
       // handle finally handler error 
       return;
   }
}

finally handlers are never truly safe - that's just a limitation of JS. The linter can't compensate for this.

[aleclarson]

My attempt to reduce the desired behavior (IMO) to its clearest and simplest form is as follows:

• Any .then call that is neither returned nor awaited must have a .catch call somewhere after it for the linter to be satisfied

• Ending a promise chain with .then(onResult, onError) should not satisfy the linter, because errors in onResult won't be caught by the onError function

• Ending a promise chain with .then(onResult).finally(onResolve) should not satisfy the linter, because a rejected promise from .then will pass through .finally and still be an unhandled rejection

• Possibility of an uncaught error in a .catch handler, a .finally handler, or the onError argument of .then(onResult, onError) are considered an unavoidable hazard (therefore, the linter should ignore them unless an explicit throw statement is found inside)

Does that clear things up?

[bradzacher]

Ending a promise chain with .then(onResult, onError) should not satisfy the linter, because errors in onResult won't be caught by the onError function

Strongly disagree with this because you're forcing users to write .then(onResult, onError).catch(onError) at which point you're really just enforcing that they should do .then(onResult).catch(onError). That's a perfectly valid viewpoint - but should be a separate lint rule.

Capturing developer intent is important here - .then(f, f) is intended by the developer to be "handle the success or failure of the promise" - which means the rule should accept the code. We can't prove that the success handler won't throw any more than we can prove that the error handler won't throw - so we should rely on the intent of the code and assume the best.

The fix for this issue is to just ignore .finally entirely as both (a) it doesn't handle errors and (b) the developer intent behind it isn't to handle errors - thus it leaves a floating promise in all cases.