Skip to content

[no-floating-promises] flag .map(async) result #5958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
4 tasks done
rvsit opened this issue Nov 9, 2022 · 6 comments · Fixed by #7897
Closed
4 tasks done

[no-floating-promises] flag .map(async) result #5958

rvsit opened this issue Nov 9, 2022 · 6 comments · Fixed by #7897
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@rvsit
Copy link

rvsit commented Nov 9, 2022

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.4&sourceType=module&code=MYewdgzgLgBAJgQygmBeGBtA5FAptLAXQG4AoUxZAOgFsEAHACgQgE8xgZGBKNAPhgBvUjBgIA7ggCWsMLnEwACgCcQNKRFyNGAfWX4ANDH0ArXqgGaoAFSk1cIAK5RGpowEYADN+7cyAXz9SIA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6aRAR1ko9oEMA7v0r50URNGgB7aJAA04bHiKlkFanQbM2TabQBm8af3zMA5rWIyAtpWQpxkSTLlKAviHdA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Repro Code

const data = ['test'];

data.map(async () => {
  await new Promise((_res, rej) => setTimeout(rej, 1000));
});

ESLint Config

module.exports = {
  "rules": {
    "@typescript-eslint/require-await": "error",
    "@typescript-eslint/no-floating-promises": "error"
  }
}

tsconfig

{
  "compilerOptions": {
    "strictNullChecks": true
  }
}

Expected Result

Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator.

Actual Result

No eslint error, but ERR_UNHANDLED_REJECTION when running

Additional Info

No response

Versions

package version
@typescript-eslint/eslint-plugin 5.42.1
@typescript-eslint/parser 5.42.1
TypeScript 4.8.4
ESLint 8.15.0
node web
@rvsit rvsit added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Nov 9, 2022
@Josh-Cena
Copy link
Member

Technically speaking, this is not a floating promise, because the return type of map is Array<Promise>, not Promise<Array>. This is why the rule is not able to detect that there's a promise at the top level.

This is the same if you wrap the promise in anything:

function getObj(p: Promise<any>) {
  return { p };
}

getObj(Promise.resolve(1));

no-misused-promises can't catch this either, because that's definitely a legitimate thing to write (passing a promise to another function; returning a promise-containing value from a function).

Let's see if others have ideas; I don't think there's a great heuristic to catch these cases unless we special-case object types and array types.

@Josh-Cena Josh-Cena added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Nov 11, 2022
@bradzacher
Copy link
Member

I think this would be a pretty good addition to the lint rule!
We could error on this case and suggest wrapping the expression in await Promise.all(expression) (could also add suggestions for .race and .allSettled).

We'd have to put this behind an option (default false for now) to avoid a breaking change, but i'd hazard a guess that we could turn it on by default eventually. Would be good to audit some codebases to be sure.

WDYT @Josh-Cena?

@Josh-Cena
Copy link
Member

Does that mean we special-case array types and object types? I was talking with @JoshuaKGoldberg and he seemed to dislike special-casing.

@bradzacher
Copy link
Member

Object types - no I don't think so because there is no construct to handle them natively. So our rule would report and then someone would have to refactor their code completely to extract and await the promise.

But array types are different to object types - they have the native promise methods to await them. This means there's no difficult code change to make - as I mentioned the simplest would be expression to await Promise.all(expression) - which isn't too different to the non-array case (await (expression))

To me at least handling the array case isn't a special case - instead it's just a natural extension to the rule.

@JoshuaKGoldberg
Copy link
Member

I'm on the fence here and fine either way. It feels like we're entering "there's no perfect answer" territory. The case of array types being special & built-in makes sense to me but I wouldn't mind either way. 🤷

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed awaiting response Issues waiting for a reply from the OP or another party labels Nov 16, 2022
@ST-DDT
Copy link
Contributor

ST-DDT commented Oct 12, 2023

Is this the issue causing the following code to be marked incorrectly as okay?

Is: array.map(async () => await foo()).join(','); // [object Promise],[object Promise]
Expected: await Promise.all(array.map(() => foo())).join(',') // value1,value2

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed bug Something isn't working labels Nov 13, 2023
@bradzacher bradzacher changed the title Bug: [no-floating-promises] .map(async) does not fail rule [no-floating-promises] flag .map(async) result Nov 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
5 participants