Bug: n/no-sync incorrectly applies to method definitions

[benjie]

Environment

Node version: 20.12.1
npm version: pnpm 9
ESLint version: 8.57.0
eslint-plugin-n version: 17.11.1
Operating System: Ubuntu

What rule do you want to report?

n/no-sync

Link to Minimal Reproducible Example

https://eslint-online-playground.netlify.app/#eNp9kTFvwyAQhf/KiamNHHt3lU5dO3UsVYXss0uLDwTnyFHk/14wdtJKURY40ON7946zCL6pcFKDM1h+B1GLxlJgUHCAsyQAj8GaI/pQ5zPA68iKtaXLBYAKJ2qAvzT14S2WD58FKN+HAiKNceLHqxag2kFZlrCrtqu52KrAY9clQr0yb5AOz/dhuVi2uMyiEBiMJi4jodN9TqkHZz0DQeftAHKV7J0Ze017kuJJkiScFlWLnRoNw3tirt5ZmaYSIVsAPxq8Dgoilyqy+5REijrZeG+9FP9b/IhesUunmh/Vp1+wFDtcIFLwyWF+O9g24qVYvKRo8fiCDqlFajSGpFl9tzT5mVGMIdZrizei/lVdpjb/AupTp8w=

What did you expect to happen?

The following code does not do any synchronous calls, but it does define two methods that end in Sync:

const a = {
  resolvers: {
    Mutation: {
      async thingsSync(_, args, context) {
        /* ... */
      },
      stuffSync: async (_, args, context) => {
        /* ... */
      }
    }
  }
}

The validation rule should only fire for function calls / method execution, not definition.

Participation

• I am willing to submit a pull request for this issue.

Additional comments

No response

[scagood]

This sounds simiular to #354 🤔

[benjie]

I almost filed a comment there, but then determined that it was sufficiently different to justify its own issue. This issue is about a general issue that no-sync should only apply to actual calls where *Sync(...) is executed; whereas #354 is effectively asking for an allow-list of *Sync functions to allow. I'd love to see both features implemented 👍

[benjie]

Also seems to flag interfaces and generic parameters:

https://eslint-online-playground.netlify.app/#eNqFj81qwzAQhF9l0cUJ+Odu2lBK36C5VT0IeZ06dVZiJZcE43fvWmobCi05CLTsNzszswpsGzybkx+xjkG1aqCI3BuL8Diat+cLWZgXTf1ENg6OoHfubr/bmBb2W3kwM8aJCYxA1lGIYOA+Ud/63WZetqpUNYZRrrOt7XF1OrluElc8e8cxiGjWBOANB+QWiod48RgsDz5WWdnkXVEmbpwOA4UWXgoqyr/w4jWBLCaCpeMAWlFDrgqSS6tWRmR2rNW6XYSXFhLVG/tuDlgfgyNJmrRarQ5ZlKNrlQy06vDjCT1Sh2QHDCvzY5ezZNloIgb5J9l1WeUuFf1DXZvdBL6q/+ZSN6mmlk9cvKaa

interface BlahSync {}
function foo<T>(a: T): T {return a}
const a = foo<BlahSync>({})

[scagood]

This is because the only test is a selector:

eslint-plugin-n/lib/rules/no-sync.js

Lines 7 to 23 in 23d0e84

	const allowedAtRootLevelSelector = [
	// fs.readFileSync()
	":function MemberExpression > Identifier[name=/Sync$/]",
	// readFileSync.call(null, 'path')
	":function MemberExpression > Identifier[name=/Sync$/]",
	// readFileSync()
	":function :not(MemberExpression) > Identifier[name=/Sync$/]",
	].join(",")
	const disallowedAtRootLevelSelector = [
	// fs.readFileSync()
	"MemberExpression > Identifier[name=/Sync$/]",
	// readFileSync.call(null, 'path')
	"MemberExpression > Identifier[name=/Sync$/]",
	// readFileSync()
	":not(MemberExpression) > Identifier[name=/Sync$/]",
	].join(",")

We could restrict this to CallExpression nodes and check for somethingSync.apply, somethingSync.bind, and somethingSync.call 🤔

[benjie]

Are there common cases where .apply, .call or .bind are used with sync calls? I don't think you'd generally want to override this and splat syntax can be used for passing multiple args (callback(...args)) which covers most of the use cases for .apply and .call at least.