From 5544f20f113e59d6789a249dc24df73fdc354fa1 Mon Sep 17 00:00:00 2001 From: Rebecca Stevens Date: Wed, 12 Feb 2025 21:21:46 +1300 Subject: [PATCH] feat: add support for ignoring sync methods from certain locations (#392) Co-authored-by: Sebastian Good <2230835+scagood@users.noreply.github.com> --- docs/rules/no-sync.md | 57 ++++++ lib/rules/no-sync.js | 114 +++++++++++- lib/util/get-full-type-name.js | 47 +++++ lib/util/get-parser-services.js | 24 +++ lib/util/get-type-of-node.js | 21 +++ package.json | 10 +- tests/fixtures/no-sync/base/file.ts | 1 + tests/fixtures/no-sync/base/tsconfig.json | 10 + tests/fixtures/no-sync/file.ts | 1 + tests/fixtures/no-sync/ignore-package/file.ts | 1 + .../node_modules/aaa/index.d.ts | 1 + .../ignore-package/node_modules/aaa/index.js | 0 .../node_modules/aaa/package.json | 6 + .../no-sync/ignore-package/package.json | 7 + .../no-sync/ignore-package/tsconfig.json | 10 + tests/fixtures/no-sync/tsconfig.json | 10 + tests/lib/rules/no-sync.js | 172 +++++++++++++++++- tests/test-helpers.js | 60 ++++++ 18 files changed, 543 insertions(+), 9 deletions(-) create mode 100644 lib/util/get-full-type-name.js create mode 100644 lib/util/get-parser-services.js create mode 100644 lib/util/get-type-of-node.js create mode 100644 tests/fixtures/no-sync/base/file.ts create mode 100644 tests/fixtures/no-sync/base/tsconfig.json create mode 100644 tests/fixtures/no-sync/file.ts create mode 100644 tests/fixtures/no-sync/ignore-package/file.ts create mode 100644 tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.d.ts create mode 100644 tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.js create mode 100644 tests/fixtures/no-sync/ignore-package/node_modules/aaa/package.json create mode 100644 tests/fixtures/no-sync/ignore-package/package.json create mode 100644 tests/fixtures/no-sync/ignore-package/tsconfig.json create mode 100644 tests/fixtures/no-sync/tsconfig.json diff --git a/docs/rules/no-sync.md b/docs/rules/no-sync.md index dc400974..38dea638 100644 --- a/docs/rules/no-sync.md +++ b/docs/rules/no-sync.md @@ -61,6 +61,7 @@ fs.readFileSync(somePath).toString(); #### ignores You can `ignore` specific function names using this option. +Additionally, if you are using TypeScript you can optionally specify where the function is declared. Examples of **incorrect** code for this rule with the `{ ignores: ['readFileSync'] }` option: @@ -78,6 +79,62 @@ Examples of **correct** code for this rule with the `{ ignores: ['readFileSync'] fs.readFileSync(somePath); ``` +##### Advanced (TypeScript only) + +You can provide a list of specifiers to ignore. Specifiers are typed as follows: + +```ts +type Specifier = +| string +| { + from: "file"; + path?: string; + name?: string[]; + } +| { + from: "package"; + package?: string; + name?: string[]; + } +| { + from: "lib"; + name?: string[]; + } +``` + +###### From a file + +Examples of **correct** code for this rule with the ignore file specifier: + +```js +/*eslint n/no-sync: ["error", { ignores: [{ from: 'file', path: './foo.ts' }]}] */ + +import { fooSync } from "./foo" +fooSync() +``` + +###### From a package + +Examples of **correct** code for this rule with the ignore package specifier: + +```js +/*eslint n/no-sync: ["error", { ignores: [{ from: 'package', package: 'effect' }]}] */ + +import { Effect } from "effect" +const value = Effect.runSync(Effect.succeed(42)) +``` + +###### From the TypeScript library + +Examples of **correct** code for this rule with the ignore lib specifier: + +```js +/*eslint n/no-sync: ["error", { ignores: [{ from: 'lib' }]}] */ + +const stylesheet = new CSSStyleSheet() +stylesheet.replaceSync("body { font-size: 1.4em; } p { color: red; }") +``` + ## 🔎 Implementation - [Rule source](../../lib/rules/no-sync.js) diff --git a/lib/rules/no-sync.js b/lib/rules/no-sync.js index fb0de8e6..6e9529c8 100644 --- a/lib/rules/no-sync.js +++ b/lib/rules/no-sync.js @@ -4,6 +4,14 @@ */ "use strict" +const typeMatchesSpecifier = + /** @type {import('ts-declaration-location').default} */ ( + /** @type {unknown} */ (require("ts-declaration-location")) + ) +const getTypeOfNode = require("../util/get-type-of-node") +const getParserServices = require("../util/get-parser-services") +const getFullTypeName = require("../util/get-full-type-name") + const selectors = [ // fs.readFileSync() // readFileSync.call(null, 'path') @@ -32,7 +40,56 @@ module.exports = { }, ignores: { type: "array", - items: { type: "string" }, + items: { + oneOf: [ + { type: "string" }, + { + type: "object", + properties: { + from: { const: "file" }, + path: { + type: "string", + }, + name: { + type: "array", + items: { + type: "string", + }, + }, + }, + additionalProperties: false, + }, + { + type: "object", + properties: { + from: { const: "lib" }, + name: { + type: "array", + items: { + type: "string", + }, + }, + }, + additionalProperties: false, + }, + { + type: "object", + properties: { + from: { const: "package" }, + package: { + type: "string", + }, + name: { + type: "array", + items: { + type: "string", + }, + }, + }, + additionalProperties: false, + }, + ], + }, default: [], }, }, @@ -57,15 +114,64 @@ module.exports = { * @returns {void} */ [selector.join(",")](node) { - if (ignores.includes(node.name)) { - return + const parserServices = getParserServices(context) + + /** + * @type {import('typescript').Type | undefined | null} + */ + let type = undefined + + /** + * @type {string | undefined | null} + */ + let fullName = undefined + + for (const ignore of ignores) { + if (typeof ignore === "string") { + if (ignore === node.name) { + return + } + + continue + } + + if ( + parserServices === null || + parserServices.program === null + ) { + throw new Error( + 'TypeScript parser services not available. Rule "n/no-sync" is configured to use "ignores" option with a non-string value. This requires TypeScript parser services to be available.' + ) + } + + type = + type === undefined + ? getTypeOfNode(node, parserServices) + : type + + fullName = + fullName === undefined + ? getFullTypeName(type) + : fullName + + if ( + typeMatchesSpecifier( + parserServices.program, + ignore, + type + ) && + (ignore.name === undefined || + ignore.name.includes(fullName ?? node.name)) + ) { + return + } } context.report({ node: node.parent, messageId: "noSync", data: { - propertyName: node.name, + propertyName: fullName ?? node.name, }, }) }, diff --git a/lib/util/get-full-type-name.js b/lib/util/get-full-type-name.js new file mode 100644 index 00000000..9e5ee7bf --- /dev/null +++ b/lib/util/get-full-type-name.js @@ -0,0 +1,47 @@ +"use strict" + +const ts = (() => { + try { + // eslint-disable-next-line n/no-unpublished-require + return require("typescript") + } catch { + return null + } +})() + +/** + * @param {import('typescript').Type | null} type + * @returns {string | null} + */ +module.exports = function getFullTypeName(type) { + if (ts === null || type === null) { + return null + } + + /** + * @type {string[]} + */ + let nameParts = [] + let currentSymbol = type.getSymbol() + while (currentSymbol !== undefined) { + if ( + currentSymbol.valueDeclaration?.kind === ts.SyntaxKind.SourceFile || + currentSymbol.valueDeclaration?.kind === + ts.SyntaxKind.ModuleDeclaration + ) { + break + } + + nameParts.unshift(currentSymbol.getName()) + currentSymbol = + /** @type {import('typescript').Symbol & {parent: import('typescript').Symbol | undefined}} */ ( + currentSymbol + ).parent + } + + if (nameParts.length === 0) { + return null + } + + return nameParts.join(".") +} diff --git a/lib/util/get-parser-services.js b/lib/util/get-parser-services.js new file mode 100644 index 00000000..1d0be646 --- /dev/null +++ b/lib/util/get-parser-services.js @@ -0,0 +1,24 @@ +"use strict" + +const { + getParserServices: getParserServicesFromTsEslint, +} = require("@typescript-eslint/utils/eslint-utils") + +/** + * Get the TypeScript parser services. + * If TypeScript isn't present, returns `null`. + * + * @param {import('eslint').Rule.RuleContext} context - rule context + * @returns {import('@typescript-eslint/parser').ParserServices | null} + */ +module.exports = function getParserServices(context) { + // Not using tseslint parser? + if ( + context.sourceCode.parserServices?.esTreeNodeToTSNodeMap == null || + context.sourceCode.parserServices.tsNodeToESTreeNodeMap == null + ) { + return null + } + + return getParserServicesFromTsEslint(/** @type {any} */ (context), true) +} diff --git a/lib/util/get-type-of-node.js b/lib/util/get-type-of-node.js new file mode 100644 index 00000000..ca5050f0 --- /dev/null +++ b/lib/util/get-type-of-node.js @@ -0,0 +1,21 @@ +"use strict" + +/** + * Get the type of a node. + * If TypeScript isn't present, returns `null`. + * + * @param {import('estree').Node} node - A node + * @param {import('@typescript-eslint/parser').ParserServices} parserServices - A parserServices + * @returns {import('typescript').Type | null} + */ +module.exports = function getTypeOfNode(node, parserServices) { + const { esTreeNodeToTSNodeMap, program } = parserServices + if (program === null) { + return null + } + const tsNode = esTreeNodeToTSNodeMap.get(/** @type {any} */ (node)) + const checker = program.getTypeChecker() + const nodeType = checker.getTypeAtLocation(tsNode) + const constrained = checker.getBaseConstraintOfType(nodeType) + return constrained ?? nodeType +} diff --git a/package.json b/package.json index 6d9ecfbf..855647d5 100644 --- a/package.json +++ b/package.json @@ -17,21 +17,23 @@ }, "dependencies": { "@eslint-community/eslint-utils": "^4.4.1", + "@typescript-eslint/utils": "^8.21.0", "enhanced-resolve": "^5.17.1", "eslint-plugin-es-x": "^7.8.0", "get-tsconfig": "^4.8.1", "globals": "^15.11.0", "ignore": "^5.3.2", "minimatch": "^9.0.5", - "semver": "^7.6.3" + "semver": "^7.6.3", + "ts-declaration-location": "^1.0.5" }, "devDependencies": { "@eslint/js": "^9.14.0", "@types/eslint": "^9.6.1", "@types/estree": "^1.0.6", "@types/node": "^20.17.5", - "@typescript-eslint/parser": "^8.12.2", - "@typescript-eslint/typescript-estree": "^8.12.2", + "@typescript-eslint/parser": "^8.21.0", + "@typescript-eslint/typescript-estree": "^8.21.0", "eslint": "^9.14.0", "eslint-config-prettier": "^9.1.0", "eslint-doc-generator": "^1.7.1", @@ -51,7 +53,7 @@ "rimraf": "^5.0.10", "ts-ignore-import": "^4.0.1", "type-fest": "^4.26.1", - "typescript": "~5.6" + "typescript": "^5.7" }, "scripts": { "build": "node scripts/update", diff --git a/tests/fixtures/no-sync/base/file.ts b/tests/fixtures/no-sync/base/file.ts new file mode 100644 index 00000000..182a8503 --- /dev/null +++ b/tests/fixtures/no-sync/base/file.ts @@ -0,0 +1 @@ +// File needs to exists diff --git a/tests/fixtures/no-sync/base/tsconfig.json b/tests/fixtures/no-sync/base/tsconfig.json new file mode 100644 index 00000000..5b217cb9 --- /dev/null +++ b/tests/fixtures/no-sync/base/tsconfig.json @@ -0,0 +1,10 @@ +{ + "compilerOptions": { + "target": "ESNext", + "module": "ESNext", + "moduleResolution": "Bundler", + "strict": true, + "skipLibCheck": true + }, + "include": ["**/*"] +} diff --git a/tests/fixtures/no-sync/file.ts b/tests/fixtures/no-sync/file.ts new file mode 100644 index 00000000..182a8503 --- /dev/null +++ b/tests/fixtures/no-sync/file.ts @@ -0,0 +1 @@ +// File needs to exists diff --git a/tests/fixtures/no-sync/ignore-package/file.ts b/tests/fixtures/no-sync/ignore-package/file.ts new file mode 100644 index 00000000..182a8503 --- /dev/null +++ b/tests/fixtures/no-sync/ignore-package/file.ts @@ -0,0 +1 @@ +// File needs to exists diff --git a/tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.d.ts b/tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.d.ts new file mode 100644 index 00000000..38f18aef --- /dev/null +++ b/tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.d.ts @@ -0,0 +1 @@ +export function fooSync(): void; diff --git a/tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.js b/tests/fixtures/no-sync/ignore-package/node_modules/aaa/index.js new file mode 100644 index 00000000..e69de29b diff --git a/tests/fixtures/no-sync/ignore-package/node_modules/aaa/package.json b/tests/fixtures/no-sync/ignore-package/node_modules/aaa/package.json new file mode 100644 index 00000000..39437d0a --- /dev/null +++ b/tests/fixtures/no-sync/ignore-package/node_modules/aaa/package.json @@ -0,0 +1,6 @@ +{ + "name": "aaa", + "version": "0.0.0", + "main": "./index.js", + "types": "./index.d.ts" +} diff --git a/tests/fixtures/no-sync/ignore-package/package.json b/tests/fixtures/no-sync/ignore-package/package.json new file mode 100644 index 00000000..5c6c2690 --- /dev/null +++ b/tests/fixtures/no-sync/ignore-package/package.json @@ -0,0 +1,7 @@ +{ + "name": "test", + "version": "0.0.0", + "dependencies": { + "aaa": "0.0.0" + } +} diff --git a/tests/fixtures/no-sync/ignore-package/tsconfig.json b/tests/fixtures/no-sync/ignore-package/tsconfig.json new file mode 100644 index 00000000..5b217cb9 --- /dev/null +++ b/tests/fixtures/no-sync/ignore-package/tsconfig.json @@ -0,0 +1,10 @@ +{ + "compilerOptions": { + "target": "ESNext", + "module": "ESNext", + "moduleResolution": "Bundler", + "strict": true, + "skipLibCheck": true + }, + "include": ["**/*"] +} diff --git a/tests/fixtures/no-sync/tsconfig.json b/tests/fixtures/no-sync/tsconfig.json new file mode 100644 index 00000000..5b217cb9 --- /dev/null +++ b/tests/fixtures/no-sync/tsconfig.json @@ -0,0 +1,10 @@ +{ + "compilerOptions": { + "target": "ESNext", + "module": "ESNext", + "moduleResolution": "Bundler", + "strict": true, + "skipLibCheck": true + }, + "include": ["**/*"] +} diff --git a/tests/lib/rules/no-sync.js b/tests/lib/rules/no-sync.js index 0c7141ee..9fb8c33d 100644 --- a/tests/lib/rules/no-sync.js +++ b/tests/lib/rules/no-sync.js @@ -4,7 +4,7 @@ */ "use strict" -const RuleTester = require("#test-helpers").RuleTester +const { RuleTester, TsRuleTester } = require("#test-helpers") const rule = require("../../../lib/rules/no-sync") new RuleTester().run("no-sync", rule, { @@ -149,3 +149,173 @@ new RuleTester().run("no-sync", rule, { }, ], }) + +new (TsRuleTester("no-sync/base").run)("no-sync", rule, { + valid: [ + { + code: ` +declare function fooSync(): void; +fooSync(); +`, + options: [ + { + ignores: [ + { + from: "file", + }, + ], + }, + ], + }, + { + code: ` +declare function fooSync(): void; +fooSync(); +`, + options: [ + { + ignores: [ + { + from: "file", + name: ["fooSync"], + }, + ], + }, + ], + }, + { + code: ` +const stylesheet = new CSSStyleSheet(); +stylesheet.replaceSync("body { font-size: 1.4em; } p { color: red; }"); +`, + options: [ + { + ignores: [ + { + from: "lib", + name: ["CSSStyleSheet.replaceSync"], + }, + ], + }, + ], + }, + ], + invalid: [ + { + code: ` +declare function fooSync(): void; +fooSync(); +`, + options: [ + { + ignores: [ + { + from: "file", + path: "**/bar.ts", + }, + ], + }, + ], + errors: [ + { + messageId: "noSync", + data: { propertyName: "fooSync" }, + type: "CallExpression", + }, + ], + }, + { + code: ` +declare function fooSync(): void; +fooSync(); +`, + options: [ + { + ignores: [ + { + from: "file", + name: ["barSync"], + }, + ], + }, + ], + errors: [ + { + messageId: "noSync", + data: { propertyName: "fooSync" }, + type: "CallExpression", + }, + ], + }, + { + code: ` +const stylesheet = new CSSStyleSheet(); +stylesheet.replaceSync("body { font-size: 1.4em; } p { color: red; }"); +`, + options: [ + { + ignores: [ + { + from: "file", + name: ["CSSStyleSheet.replaceSync"], + }, + ], + }, + ], + errors: [ + { + messageId: "noSync", + data: { propertyName: "CSSStyleSheet.replaceSync" }, + type: "MemberExpression", + }, + ], + }, + ], +}) + +new (TsRuleTester("no-sync/ignore-package").run)("no-sync", rule, { + valid: [ + { + code: ` +import { fooSync } from "aaa"; +fooSync(); +`, + options: [ + { + ignores: [ + { + from: "package", + package: "aaa", + name: ["fooSync"], + }, + ], + }, + ], + }, + ], + invalid: [ + { + code: ` +import { fooSync } from "aaa"; +fooSync(); +`, + options: [ + { + ignores: [ + { + from: "file", + name: ["fooSync"], + }, + ], + }, + ], + errors: [ + { + messageId: "noSync", + data: { propertyName: "fooSync" }, + type: "CallExpression", + }, + ], + }, + ], +}) diff --git a/tests/test-helpers.js b/tests/test-helpers.js index bff30126..69ce7abd 100644 --- a/tests/test-helpers.js +++ b/tests/test-helpers.js @@ -3,12 +3,15 @@ * @author 唯然 */ "use strict" + +const path = require("path") const eslintVersion = require("eslint/package.json").version const { RuleTester } = require("eslint") const { FlatRuleTester } = require("eslint/use-at-your-own-risk") const globals = require("globals") const semverSatisfies = require("semver/functions/satisfies") const os = require("os") +const typescriptParser = require("@typescript-eslint/parser") // greater than or equal to ESLint v9 exports.gteEslintV9 = semverSatisfies(eslintVersion, ">=9", { @@ -33,6 +36,26 @@ const defaultConfig = { globals: { ...globals.es2015, ...globals.node }, }, } + +/** + * @param {string} fixturePath - Path to the fixture directory relative to the fixtures directory + * @returns + */ +function getTsConfig(fixturePath) { + return { + languageOptions: { + parser: typescriptParser, + parserOptions: { + tsconfigRootDir: path.join(__dirname, "fixtures", fixturePath), + projectService: { + // Ensure we're not using the default project + maximumDefaultProjectFileMatchCount_THIS_WILL_SLOW_DOWN_LINTING: 0, + }, + }, + }, + } +} + exports.RuleTester = function (config = defaultConfig) { if (config.languageOptions.env?.node === false) config.languageOptions.globals = config.languageOptions.globals || {} @@ -55,6 +78,31 @@ exports.RuleTester = function (config = defaultConfig) { return ruleTester } +/** + * @param {string | import('eslint').Linter.Config} configOrFixturePath + * @returns + */ +exports.TsRuleTester = function (configOrFixturePath) { + const config = + typeof configOrFixturePath === "object" + ? configOrFixturePath + : getTsConfig(configOrFixturePath) + + const ruleTester = exports.RuleTester.call(this, config) + const $run = ruleTester.run.bind(ruleTester) + ruleTester.run = function (name, rule, tests) { + tests.valid = tests.valid.map(setTsFilename) + tests.invalid = tests.invalid.map(setTsFilename) + + $run(name, rule, tests) + } + return ruleTester +} +Object.setPrototypeOf( + exports.TsRuleTester.prototype, + exports.RuleTester.prototype +) + // support skip in tests function shouldRun(item) { if (typeof item === "string") return true @@ -63,3 +111,15 @@ function shouldRun(item) { delete item.skip return skip === void 0 || skip === false } + +function setTsFilename(item) { + if (typeof item === "string") { + return { + code: item, + filename: "file.ts", + } + } + + item.filename ??= "file.ts" + return item +}