Skip to content
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

Bug: [no-misused-promises] An array of promises passed with spread (...args) syntax is not checked #5744

Closed
4 tasks done
Aaron1011 opened this issue Oct 4, 2022 · 3 comments · Fixed by #8493
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@Aaron1011
Copy link
Contributor

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=GYVwdgxgLglg9mABBBBnEBbApgCgHQED6EAhgDZkBGJEA1qgFyICCATqyQJ4A8OAlIgC8APkQA3ODAAmwvkwnTEAb0QBfAFDqyWKMkqohiANr8hogAqs4GGKix5WWVHDJjcUViCx8ANIlMiiJbWtvaOzq7unt4AugDc6ihg6Nj4BBD6fEA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1oFtLlZkiACa1i0Dr0GoMkRNHHRI4MAF8QKoA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Repro Code

function consume(..._callbacks: Array<() => void>): void { }

let cbs = [() => Promise.resolve(true), () => Promise.resolve(true)];
consume(...cbs)

ESLint Config

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

tsconfig

No response

Expected Result

I expected that the 4th line should report "Promise returned in function argument where a void return was expected.", just as if I manually called the function with each value from the array (consume(cbs[0], consume(cbs[1]))

Actual Result

There was no error reported on the fourth line.

Additional Info

Handling of 'rest' parameters is implemented in #5731 . However, support for an explicit 'spread' argument still needs to be added.

Versions

package version
@typescript-eslint/eslint-plugin 5.39.0
@typescript-eslint/parser 5.39.0
TypeScript 4.8.4
ESLint 8.15.0
node web
@Aaron1011 Aaron1011 added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Oct 4, 2022
@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Oct 4, 2022
@kirkwaiblinger
Copy link
Member

I think this may have been fixed. When I click on the playground link, I do see an error as described in the Expected Result.

@kirkwaiblinger
Copy link
Member

Looking a little deeper, it looks like the test case in the issue is checked because in a scenario like

declare const f: (...args: Array<() => void>);
declare argsList: Array<() => Promise<void>>;
f(...argsList)

the rule will ask "what is the type of ...args" and the response is () => Promise<void>. See

/**
* @returns Whether expression is a function that returns a thenable.
*/
function returnsThenable(checker: ts.TypeChecker, node: ts.Node): boolean {
const type = checker.getApparentType(checker.getTypeAtLocation(node));
if (anySignatureIsThenableType(checker, node, type)) {
return true;
}
return false;
}

So, the test cases

function consume(..._callbacks: Array<() => void>): void { }
// array
let cbs = [() => Promise.resolve(true), () => Promise.resolve(true)];
consume(...cbs)

and

function consume(..._callbacks: Array<() => void>): void { }
// tuple
const cbs = [() => Promise.resolve(true), () => Promise.resolve(true)] as const;
consume(...cbs)

pass just fine.

This does reveal though, the rule isn't too careful about checking unions of call signatures, so something like the following will fail to report, though it would be possible to detect.

function consume(...maybeCallbacks: Array<(() => void) | number>): void { }
const cbs = [() => Promise.resolve(true), 3] as const;
consume(...cbs)

That's not isolated to spread arguments; we can just as easily trick the rule with

function consume(...maybeCallbacks: Array<(() => void) | number>): void { }
declare const cb: (() => Promise<boolean>) | 3
consume(cb)

However, the use of spread args might be a more compelling case to address it than on its own.

Furthermore, while the example in the issue does pass, there is no test case at present to ensure that it passes.

So, I'd see as actionable

  1. Add test case from issue
  2. Address unions of potentially misused promises (might be better to file a new issue for that, rather than include it in this one)
  3. Remove comments in code that refer to this issue and spread elements being unsupported
    // Note - we currently do not support 'spread' arguments - adding support for them
    // is tracked in https://github.com/typescript-eslint/typescript-eslint/issues/5744

Would maintainers agree with these actions?

@JoshuaKGoldberg
Copy link
Member

Yup, thanks for the deep dive @kirkwaiblinger! All sounds good to me. 🚀

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 bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants