Bug: [no-misused-promises] versus Promise.finally

[dmurvihill]

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.1.6&fileType=.tsx&code=IYZwngdgxgBAZgV2gFwJYHsLwgCgJQwDeAsAFAwzADuwqyMACgE7oC2qIApgHROcBWnKMhwBmPNzioIwADaywOUJFj4YAXgB8RAL54A3GR1A&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1oFtLlZkiACa1i0Dr0GoMkRNHHRI4MAF8QKoA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

async function fn() {
  await Promise.reject(3).finally(async () => {});
}

ESLint Config

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

tsconfig

{
	"exclude": ["**/node_modules"],
	"compilerOptions": {
		"target": "ES2022",
		"useDefineForClassFields": true,
		"module": "ES2022",
		"moduleResolution": "node",
		"resolveJsonModule": true,
		"allowJs": true,
		"checkJs": true,
		"declaration": true,
		"declarationMap": true,
		"sourceMap": true,
		"removeComments": true,
		"importHelpers": true,
		"downlevelIteration": true,
		"newLine": "LF",
		"isolatedModules": true,
		"verbatimModuleSyntax": true,
		"allowSyntheticDefaultImports": true,
		"esModuleInterop": true,
		"forceConsistentCasingInFileNames": true,
		"strict": true,
		"noUnusedLocals": false,
		"noUnusedParameters": false,
		"exactOptionalPropertyTypes": true,
		"noImplicitReturns": true,
		"noFallthroughCasesInSwitch": true,
		"noUncheckedIndexedAccess": true,
		"noImplicitOverride": true,
		"noPropertyAccessFromIndexSignature": true,
		"allowUnusedLabels": false,
		"allowUnreachableCode": false,
		"skipLibCheck": true
	}
}

Expected Result

OK

Actual Result

 Promise returned in function argument where a void return was expected  @typescript-eslint/no-misused-promises

Additional Info

This is a request to re-consider #7276.

The issue was closed with reference to the type definition of the onFinally callback:

interface Promise<T> {
  finally(onfinally?: (() => void) | undefined | null): Promise<T>
}

However, the type definition is erroneous and Microsoft has declined to fix it.

Based on the type definition, @bradzacher closed the issue because,

The rule is simply warning you that you're passing an async function into a place that won't handle the promise.

But the place in question does handle the promise:

onFinally

A function to asynchronously execute when this promise becomes settled. Its return value is ignored unless the returned value is a rejected promise. The function is called with no arguments.

It's unfortunate this problem has come to typescript-eslint from upstream, but please reopen #7276 until a solution can be found.

[kirkwaiblinger]

This is indeed unfortunate that the type definitions don't naturally indicate that a provided promise will be handled. I'm +0.1 on reopening/considering a special-case.

• (against) special-casing is generally to be avoided. We can't generalize this since one of the main points of the rule is to flag () => Promise<void> provided to () => void.
• (in favor) .finally() is a part of a fundamental set of special Promise operations, so it's already special anyway.
• (in favor) the linter report gives the wrong impression to users (including us!) of what .finally() actually does when given a promise (though - this is again because the type is suspect)
• (in favor) the TS type definitions aren't 100% unambiguously wrong, even though I agree () => Promise<void> | void would be preferable
• (against) async/await with try...catch...finally is basically universally better anyway than .finally(), and presumably has much wider usage, so it's not clear this addresses a widespread use case

@typescript-eslint/triage-team

[fregante]

It's ok to discuss this and all, but it seems to me that the plugin is marking a perfectly valid expression as invalid, regardless of whose fault this is.

This is 100% correct code but typescript-eslint is reporting on it:

While I agree that try/catch/finally is better, that's not what this rule is about. I'm concerned that actual bugs — rather, incorrect behavior whether intentional or not — are being discarded here for the sole purpose of keeping the number of issues low.

[JoshuaKGoldberg]

Agreed on treating this as a bug and accepting PRs. It is unfortunate that TS can't update the default lib definitions. We normally wouldn't want to hardcode for external types, but the built-in TS ones are under unique restrictions and uniquely refer to built-in constructs.

ACK that this is much less common of a pattern. But it's still something folks do, and is a reasonable use case of the code constructs in play.

Vote: 👍 from me

---

This is 100% correct code

All code that this plugin reports on is "100% correct" from a certain point of view. Linting is for the inherently subjective parts of code analysis.

Image: promise.finally method that is called with an async function that rejects

Please paste code blocks rather than screenshots of code. It's hard to copy & paste code from raw images even when they have alt text filled out properly (this one doesn't).

Here's the equivalent tseslint playground link: https://typescript-eslint.io/play/#ts=5.8.2&fileType=.tsx&code=FDCGHdQSwFwAgGYFcB2BjGUD2LEoBQCUcA3sHHBNPAAoBOWAtlAM4CmAdHWwFZsb4AzIQ4IoKUABtJAT3ygWM9HCJwAvAD5S5CnBgALBuDgo2xgKJ0GdfAHJuAB0mg0bACa3COgL5fvINgAPByw6eBJvIA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1oFtLlZkiACa1i0Dr0GoMkRNHHRI4MAF8QKoA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEk8sAJgK4YVTolHoAeekANOBqQSeTMmh4Aci1QBhABbpoAazKUxTdIJpQ8AQ0wBzdH0qQOXXpCEBfELaA&tokens=false

await function fn() {
  await Promise.reject(3).finally(async () => {
    throw new Error('replaced')
  })
}

for the sole purpose of keeping the number of issues low.

That is incorrect and a little hurtful 😛. #7276 was closed as a technical decision. This issue is requesting to revisit that decision with a new discussion. kirkwaiblinger engaged this issue last week and tagged the team to continue discussion. We're doing what we can to treat this issue & the people in it with respect and hope everyone does the same.

[kirkwaiblinger]

opening for PRs with resolution being to special-case the behavior around .finally() handler and not report when a promise-returning function is provided as an argument. 🚀