From d2b9372279d39b9ca9db2c0874b7dfab6a19c208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Tue, 6 Feb 2024 09:05:03 -0500 Subject: [PATCH] feat: add no-property-in-node rule (#433) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add no-property-in-node rule * Add ☑️ emoji for recommended-type-checked * tests: valid before invalid * Also check for whether the node has a 'type' * Added docs and example to isAstNodeType * Expanded rule details * Add more valid test cases * Fixed test path to fixtures * Use parserOptions.project: true for eslint-remote-tester on TS files * nit: avoid shadowing name for typePart * * Downgraded to typescript-eslint@v5 * Also remove @typescript-eslint/utils * Or rather, make @typescript-eslint/utils a -D * Remove ts-api-utils too * Removed recommended-type-checked * Removed README.md section too * Removed eslint-remote-tester.config.js parserOptions.project too * Redid README.md presets table * Added all-type-checked * Removed file notice --- .eslint-doc-generatorrc.js | 1 + README.md | 99 ++++++++------- configs/all-type-checked.js | 8 ++ docs/rules/no-property-in-node.md | 56 +++++++++ lib/index.js | 3 +- lib/rules/no-property-in-node.js | 86 +++++++++++++ package.json | 7 +- tests/lib/fixtures/estree.ts | 0 tests/lib/fixtures/file.ts | 0 tests/lib/fixtures/tsconfig.json | 5 + tests/lib/rules/no-property-in-node.js | 165 +++++++++++++++++++++++++ 11 files changed, 381 insertions(+), 49 deletions(-) create mode 100644 configs/all-type-checked.js create mode 100644 docs/rules/no-property-in-node.md create mode 100644 lib/rules/no-property-in-node.js create mode 100644 tests/lib/fixtures/estree.ts create mode 100644 tests/lib/fixtures/file.ts create mode 100644 tests/lib/fixtures/tsconfig.json create mode 100644 tests/lib/rules/no-property-in-node.js diff --git a/.eslint-doc-generatorrc.js b/.eslint-doc-generatorrc.js index 3279eca8..47100791 100644 --- a/.eslint-doc-generatorrc.js +++ b/.eslint-doc-generatorrc.js @@ -6,6 +6,7 @@ const prettier = require('prettier'); module.exports = { ignoreConfig: [ 'all', + 'all-type-checked', 'rules', 'rules-recommended', 'tests', diff --git a/README.md b/README.md index 9a7b4b52..d3c7169a 100644 --- a/README.md +++ b/README.md @@ -1,15 +1,19 @@ -# eslint-plugin-eslint-plugin ![CI](https://github.com/eslint-community/eslint-plugin-eslint-plugin/workflows/CI/badge.svg) [![NPM version](https://img.shields.io/npm/v/eslint-plugin-eslint-plugin.svg?style=flat)](https://npmjs.org/package/eslint-plugin-eslint-plugin) [![Conventional Commits](https://img.shields.io/badge/Conventional%20Commits-1.0.0-yellow.svg)](https://conventionalcommits.org) +# eslint-plugin-eslint-plugin ![CI](https://github.com/eslint-community/eslint-plugin-eslint-plugin/workflows/CI/badge.svg) [![NPM version](https://img.shields.io/npm/v/eslint-plugin-eslint-plugin.svg?style=flat)](https://npmjs.org/package/eslint-plugin-eslint-plugin) [![Conventional Commits](https://img.shields.io/badge/Conventional%20Commits-1.0.0-yellow.svg)](https://conventionalcommits.org) An ESLint plugin for linting ESLint plugins. Rules written in CJS, ESM, and TypeScript are all supported. -- [Installation](#Installation) -- [Usage](#Usage) -- [Rules](#Rules) -- [Presets](#Presets) - - [Semantic versioning policy](#Semanticversioningpolicy) - - [Preset usage](#Presetusage) +- [Installation](#installation) +- [Usage](#usage) + - [**.eslintrc.json**](#eslintrcjson) + - [`eslint.config.js` (requires eslint\>=v8.23.0)](#eslintconfigjs-requires-eslintv8230) +- [Rules](#rules) + - [Rules](#rules-1) + - [Tests](#tests) +- [Presets](#presets) + - [Semantic versioning policy](#semantic-versioning-policy) + - [Preset usage](#preset-usage) ## Presets -| | Name | Description | -| :-- | :------------------ | :------------------------------------------------------------------------ | -| ✅ | `recommended` | enables all recommended rules in this plugin | -| | `rules-recommended` | enables all recommended rules that are aimed at linting ESLint rule files | -| | `tests-recommended` | enables all recommended rules that are aimed at linting ESLint test files | -| | `all` | enables all rules in this plugin | -| | `rules` | enables all rules that are aimed at linting ESLint rule files | -| | `tests` | enables all rules that are aimed at linting ESLint test files | +| | Name | Description | +| :-- | :------------------ | :--------------------------------------------------------------------------- | +| ✅ | `recommended` | enables all recommended rules in this plugin | +| | `rules-recommended` | enables all recommended rules that are aimed at linting ESLint rule files | +| | `tests-recommended` | enables all recommended rules that are aimed at linting ESLint test files | +| | `all` | enables all rules in this plugin, including those requiring type information | +| | `all-type-checked` | enables all rules in this plugin, including those requiring type information | +| | `rules` | enables all rules that are aimed at linting ESLint rule files | +| | `tests` | enables all rules that are aimed at linting ESLint test files | ### Semantic versioning policy diff --git a/configs/all-type-checked.js b/configs/all-type-checked.js new file mode 100644 index 00000000..e32d57d2 --- /dev/null +++ b/configs/all-type-checked.js @@ -0,0 +1,8 @@ +'use strict'; + +const mod = require('../lib/index.js'); + +module.exports = { + plugins: { 'eslint-plugin': mod }, + rules: mod.configs['all-type-checked'].rules, +}; diff --git a/docs/rules/no-property-in-node.md b/docs/rules/no-property-in-node.md new file mode 100644 index 00000000..22da26dd --- /dev/null +++ b/docs/rules/no-property-in-node.md @@ -0,0 +1,56 @@ +# Disallow using `in` to narrow node types instead of looking at properties (`eslint-plugin/no-property-in-node`) + +💭 This rule requires type information. + + + +When working with a node of type `ESTree.Node` or `TSESTree.Node`, it can be tempting to use the `'in'` operator to narrow the node's type. +`'in'` narrowing is susceptible to confusing behavior from quirks of ASTs, such as node properties sometimes being omitted from nodes and other times explicitly being set to `null` or `undefined`. + +Instead, checking a node's `type` property is generally considered preferable. + +## Rule Details + +Examples of **incorrect** code for this rule: + +```ts +/* eslint eslint-plugin/no-property-in-node: error */ + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + /* ... */ + }, + create(context) { + return { + 'ClassDeclaration, FunctionDeclaration'(node) { + if ('superClass' in node) { + console.log('This is a class declaration:', node); + } + }, + }; + }, +}; +``` + +Examples of **correct** code for this rule: + +```ts +/* eslint eslint-plugin/no-property-in-node: error */ + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + /* ... */ + }, + create(context) { + return { + 'ClassDeclaration, FunctionDeclaration'(node) { + if (node.type === 'ClassDeclaration') { + console.log('This is a class declaration:', node); + } + }, + }; + }, +}; +``` diff --git a/lib/index.js b/lib/index.js index 4ff4ef76..b09550a2 100644 --- a/lib/index.js +++ b/lib/index.js @@ -15,7 +15,8 @@ const packageMetadata = require('../package'); const PLUGIN_NAME = packageMetadata.name.replace(/^eslint-plugin-/, ''); const configFilters = { - all: () => true, + all: (rule) => !rule.meta.docs.requiresTypeChecking, + 'all-type-checked': () => true, recommended: (rule) => rule.meta.docs.recommended, rules: (rule) => rule.meta.docs.category === 'Rules', tests: (rule) => rule.meta.docs.category === 'Tests', diff --git a/lib/rules/no-property-in-node.js b/lib/rules/no-property-in-node.js new file mode 100644 index 00000000..f3e2e0c8 --- /dev/null +++ b/lib/rules/no-property-in-node.js @@ -0,0 +1,86 @@ +'use strict'; + +const typedNodeSourceFileTesters = [ + /@types[/\\]estree[/\\]index\.d\.ts/, + /@typescript-eslint[/\\]types[/\\]dist[/\\]generated[/\\]ast-spec\.d\.ts/, +]; + +/** + * Given a TypeScript type, determines whether the type appears to be for a known + * AST type from the typings of @typescript-eslint/types or estree. + * We check based on two rough conditions: + * - The type has a 'kind' property (as AST node types all do) + * - The type is declared in one of those package's .d.ts types + * + * @example + * ``` + * module.exports = { + * create(context) { + * BinaryExpression(node) { + * const type = services.getTypeAtLocation(node.right); + * // ^^^^ + * // This variable's type will be TSESTree.BinaryExpression + * } + * } + * } + * ``` + * + * @param {import('typescript').Type} type + * @returns Whether the type seems to include a known ESTree or TSESTree AST node. + */ +function isAstNodeType(type) { + return (type.types || [type]) + .filter((typePart) => typePart.getProperty('type')) + .flatMap( + (typePart) => (typePart.symbol && typePart.symbol.declarations) || [] + ) + .some((declaration) => { + const fileName = declaration.getSourceFile().fileName; + return ( + fileName && + typedNodeSourceFileTesters.some((tester) => tester.test(fileName)) + ); + }); +} + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + meta: { + type: 'suggestion', + docs: { + description: + 'disallow using `in` to narrow node types instead of looking at properties', + category: 'Rules', + recommended: false, + requiresTypeChecking: true, + url: 'https://github.com/eslint-community/eslint-plugin-eslint-plugin/tree/HEAD/docs/rules/no-property-in-node.md', + }, + schema: [], + messages: { + in: 'Prefer checking specific node properties instead of a broad `in`.', + }, + }, + + create(context) { + return { + 'BinaryExpression[operator=in]'(node) { + // TODO: Switch this to ESLintUtils.getParserServices with typescript-eslint@>=6 + // https://github.com/eslint-community/eslint-plugin-eslint-plugin/issues/269 + const services = (context.sourceCode || context).parserServices; + if (!services.program) { + throw new Error( + 'You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.' + ); + } + + const checker = services.program.getTypeChecker(); + const tsNode = services.esTreeNodeToTSNodeMap.get(node.right); + const type = checker.getTypeAtLocation(tsNode); + + if (isAstNodeType(type)) { + context.report({ messageId: 'in', node }); + } + }, + }; + }, +}; diff --git a/package.json b/package.json index 3b2f1b21..6dd21a87 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,10 @@ "@eslint/eslintrc": "^2.0.2", "@eslint/js": "^8.37.0", "@release-it/conventional-changelog": "^4.3.0", - "@typescript-eslint/parser": "^5.36.2", + "@types/eslint": "^8.56.2", + "@types/estree": "^1.0.5", + "@typescript-eslint/parser": "^5.62.0", + "@typescript-eslint/utils": "^5.62.0", "chai": "^4.3.6", "dirty-chai": "^2.0.1", "eslint": "^8.23.0", @@ -81,7 +84,7 @@ "nyc": "^15.1.0", "prettier": "^2.7.1", "release-it": "^14.14.3", - "typescript": "^5.0.4" + "typescript": "5.1.3" }, "peerDependencies": { "eslint": ">=7.0.0" diff --git a/tests/lib/fixtures/estree.ts b/tests/lib/fixtures/estree.ts new file mode 100644 index 00000000..e69de29b diff --git a/tests/lib/fixtures/file.ts b/tests/lib/fixtures/file.ts new file mode 100644 index 00000000..e69de29b diff --git a/tests/lib/fixtures/tsconfig.json b/tests/lib/fixtures/tsconfig.json new file mode 100644 index 00000000..0d505be7 --- /dev/null +++ b/tests/lib/fixtures/tsconfig.json @@ -0,0 +1,5 @@ +{ + "compilerOptions": { + "moduleResolution": "NodeNext" + } +} diff --git a/tests/lib/rules/no-property-in-node.js b/tests/lib/rules/no-property-in-node.js new file mode 100644 index 00000000..39e4587f --- /dev/null +++ b/tests/lib/rules/no-property-in-node.js @@ -0,0 +1,165 @@ +'use strict'; + +const RuleTester = require('eslint').RuleTester; +const path = require('path'); +const rule = require('../../../lib/rules/no-property-in-node'); + +const ruleTester = new RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + project: './tsconfig.json', + tsconfigRootDir: path.join(__dirname, '../fixtures'), + }, +}); + +ruleTester.run('no-property-in-node', rule, { + valid: [ + `'a' in window;`, + ` + declare const node: Node; + 'a' in node; + `, + ` + type Node = { unrelated: true; }; + declare const node: Node; + 'a' in node; + `, + ` + interface Node { + unrelated: true; + }; + declare const node: Node; + 'a' in node; + `, + ` + declare const node: UnresolvedType; + 'a' in node; + `, + ` + import * as ESTree from 'estree'; + declare const loc: ESTree.SourceLocation; + 'a' in loc; + `, + ` + import * as ESTree from 'estree'; + declare const node: ESTree.Node; + a.superClass; + `, + ` + import * as ESTree from 'estree'; + declare const node: ESTree.Node; + a.type; + `, + ` + import * as ESTree from 'estree'; + declare const node: ESTree.Node; + a.type === 'ClassDeclaration'; + `, + ` + import * as ESTree from 'estree'; + declare const node: ESTree.ClassDeclaration | ESTree.FunctionDeclaration; + a.type === 'ClassDeclaration'; + `, + ` + import { TSESTree } from '@typescript-eslint/utils'; + declare const node: TSESTree.Node; + node.superClass; + `, + ` + import { TSESTree } from '@typescript-eslint/utils'; + declare const node: TSESTree.Node; + node.type; + `, + ` + import { TSESTree } from '@typescript-eslint/utils'; + declare const node: TSESTree.ClassDeclaration | TSESTree.FunctionDeclaration; + node.type === 'ClassDeclaration'; + `, + ` + import * as eslint from 'eslint'; + const listener: eslint.Rule.RuleListener = { + ClassDeclaration(node) { + node.type; + }, + }; + `, + ` + import * as eslint from 'eslint'; + const listener: eslint.Rule.RuleListener = { + 'ClassDeclaration, FunctionDeclaration'(node) { + node.type === 'ClassDeclaration'; + }, + }; + `, + ], + invalid: [ + { + code: ` + import { TSESTree } from '@typescript-eslint/utils'; + declare const node: TSESTree.Node; + 'a' in node; + `, + errors: [ + { + column: 9, + line: 4, + endColumn: 20, + endLine: 4, + messageId: 'in', + }, + ], + }, + { + code: ` + import { TSESTree } from '@typescript-eslint/utils'; + type Other = { key: true }; + declare const node: TSESTree.Node | Other; + 'a' in node; + `, + errors: [ + { + column: 9, + line: 5, + endColumn: 20, + endLine: 5, + messageId: 'in', + }, + ], + }, + { + code: ` + import * as ESTree from 'estree'; + declare const node: ESTree.Node; + 'a' in node; + `, + errors: [ + { + column: 9, + line: 4, + endColumn: 20, + endLine: 4, + messageId: 'in', + }, + ], + }, + { + code: ` + import * as eslint from 'eslint'; + const listener: eslint.Rule.RuleListener = { + ClassDeclaration(node) { + 'a' in node; + }, + }; + `, + errors: [ + { + column: 13, + line: 5, + endColumn: 24, + endLine: 5, + messageId: 'in', + }, + ], + }, + ], +});