Skip to content

Commit d2b9372

Browse files
JoshuaKGoldbergJoshuaKGoldberg
authored
feat: add no-property-in-node rule (#433)
* 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 * <!-- omit from toc --> * 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
1 parent 6ae0ee6 commit d2b9372

11 files changed

+381
-49
lines changed

‎.eslint-doc-generatorrc.js

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const prettier = require('prettier');
66
module.exports = {
77
ignoreConfig: [
88
'all',
9+
'all-type-checked',
910
'rules',
1011
'rules-recommended',
1112
'tests',

‎README.md

+53-46
Large diffs are not rendered by default.

‎configs/all-type-checked.js

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
'use strict';
2+
3+
const mod = require('../lib/index.js');
4+
5+
module.exports = {
6+
plugins: { 'eslint-plugin': mod },
7+
rules: mod.configs['all-type-checked'].rules,
8+
};

‎docs/rules/no-property-in-node.md

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
# Disallow using `in` to narrow node types instead of looking at properties (`eslint-plugin/no-property-in-node`)
2+
3+
💭 This rule requires type information.
4+
5+
<!-- end auto-generated rule header -->
6+
7+
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.
8+
`'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`.
9+
10+
Instead, checking a node's `type` property is generally considered preferable.
11+
12+
## Rule Details
13+
14+
Examples of **incorrect** code for this rule:
15+
16+
```ts
17+
/* eslint eslint-plugin/no-property-in-node: error */
18+
19+
/** @type {import('eslint').Rule.RuleModule} */
20+
module.exports = {
21+
meta: {
22+
/* ... */
23+
},
24+
create(context) {
25+
return {
26+
'ClassDeclaration, FunctionDeclaration'(node) {
27+
if ('superClass' in node) {
28+
console.log('This is a class declaration:', node);
29+
}
30+
},
31+
};
32+
},
33+
};
34+
```
35+
36+
Examples of **correct** code for this rule:
37+
38+
```ts
39+
/* eslint eslint-plugin/no-property-in-node: error */
40+
41+
/** @type {import('eslint').Rule.RuleModule} */
42+
module.exports = {
43+
meta: {
44+
/* ... */
45+
},
46+
create(context) {
47+
return {
48+
'ClassDeclaration, FunctionDeclaration'(node) {
49+
if (node.type === 'ClassDeclaration') {
50+
console.log('This is a class declaration:', node);
51+
}
52+
},
53+
};
54+
},
55+
};
56+
```

‎lib/index.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ const packageMetadata = require('../package');
1515
const PLUGIN_NAME = packageMetadata.name.replace(/^eslint-plugin-/, '');
1616

1717
const configFilters = {
18-
all: () => true,
18+
all: (rule) => !rule.meta.docs.requiresTypeChecking,
19+
'all-type-checked': () => true,
1920
recommended: (rule) => rule.meta.docs.recommended,
2021
rules: (rule) => rule.meta.docs.category === 'Rules',
2122
tests: (rule) => rule.meta.docs.category === 'Tests',

‎lib/rules/no-property-in-node.js

+86
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
'use strict';
2+
3+
const typedNodeSourceFileTesters = [
4+
/@types[/\\]estree[/\\]index\.d\.ts/,
5+
/@typescript-eslint[/\\]types[/\\]dist[/\\]generated[/\\]ast-spec\.d\.ts/,
6+
];
7+
8+
/**
9+
* Given a TypeScript type, determines whether the type appears to be for a known
10+
* AST type from the typings of @typescript-eslint/types or estree.
11+
* We check based on two rough conditions:
12+
* - The type has a 'kind' property (as AST node types all do)
13+
* - The type is declared in one of those package's .d.ts types
14+
*
15+
* @example
16+
* ```
17+
* module.exports = {
18+
* create(context) {
19+
* BinaryExpression(node) {
20+
* const type = services.getTypeAtLocation(node.right);
21+
* // ^^^^
22+
* // This variable's type will be TSESTree.BinaryExpression
23+
* }
24+
* }
25+
* }
26+
* ```
27+
*
28+
* @param {import('typescript').Type} type
29+
* @returns Whether the type seems to include a known ESTree or TSESTree AST node.
30+
*/
31+
function isAstNodeType(type) {
32+
return (type.types || [type])
33+
.filter((typePart) => typePart.getProperty('type'))
34+
.flatMap(
35+
(typePart) => (typePart.symbol && typePart.symbol.declarations) || []
36+
)
37+
.some((declaration) => {
38+
const fileName = declaration.getSourceFile().fileName;
39+
return (
40+
fileName &&
41+
typedNodeSourceFileTesters.some((tester) => tester.test(fileName))
42+
);
43+
});
44+
}
45+
46+
/** @type {import('eslint').Rule.RuleModule} */
47+
module.exports = {
48+
meta: {
49+
type: 'suggestion',
50+
docs: {
51+
description:
52+
'disallow using `in` to narrow node types instead of looking at properties',
53+
category: 'Rules',
54+
recommended: false,
55+
requiresTypeChecking: true,
56+
url: 'https://github.com/eslint-community/eslint-plugin-eslint-plugin/tree/HEAD/docs/rules/no-property-in-node.md',
57+
},
58+
schema: [],
59+
messages: {
60+
in: 'Prefer checking specific node properties instead of a broad `in`.',
61+
},
62+
},
63+
64+
create(context) {
65+
return {
66+
'BinaryExpression[operator=in]'(node) {
67+
// TODO: Switch this to ESLintUtils.getParserServices with typescript-eslint@>=6
68+
// https://github.com/eslint-community/eslint-plugin-eslint-plugin/issues/269
69+
const services = (context.sourceCode || context).parserServices;
70+
if (!services.program) {
71+
throw new Error(
72+
'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.'
73+
);
74+
}
75+
76+
const checker = services.program.getTypeChecker();
77+
const tsNode = services.esTreeNodeToTSNodeMap.get(node.right);
78+
const type = checker.getTypeAtLocation(tsNode);
79+
80+
if (isAstNodeType(type)) {
81+
context.report({ messageId: 'in', node });
82+
}
83+
},
84+
};
85+
},
86+
};

‎package.json

+5-2
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,10 @@
5555
"@eslint/eslintrc": "^2.0.2",
5656
"@eslint/js": "^8.37.0",
5757
"@release-it/conventional-changelog": "^4.3.0",
58-
"@typescript-eslint/parser": "^5.36.2",
58+
"@types/eslint": "^8.56.2",
59+
"@types/estree": "^1.0.5",
60+
"@typescript-eslint/parser": "^5.62.0",
61+
"@typescript-eslint/utils": "^5.62.0",
5962
"chai": "^4.3.6",
6063
"dirty-chai": "^2.0.1",
6164
"eslint": "^8.23.0",
@@ -81,7 +84,7 @@
8184
"nyc": "^15.1.0",
8285
"prettier": "^2.7.1",
8386
"release-it": "^14.14.3",
84-
"typescript": "^5.0.4"
87+
"typescript": "5.1.3"
8588
},
8689
"peerDependencies": {
8790
"eslint": ">=7.0.0"

‎tests/lib/fixtures/estree.ts

Whitespace-only changes.

‎tests/lib/fixtures/file.ts

Whitespace-only changes.

‎tests/lib/fixtures/tsconfig.json

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"compilerOptions": {
3+
"moduleResolution": "NodeNext"
4+
}
5+
}

‎tests/lib/rules/no-property-in-node.js

+165
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
'use strict';
2+
3+
const RuleTester = require('eslint').RuleTester;
4+
const path = require('path');
5+
const rule = require('../../../lib/rules/no-property-in-node');
6+
7+
const ruleTester = new RuleTester({
8+
parser: require.resolve('@typescript-eslint/parser'),
9+
parserOptions: {
10+
project: './tsconfig.json',
11+
tsconfigRootDir: path.join(__dirname, '../fixtures'),
12+
},
13+
});
14+
15+
ruleTester.run('no-property-in-node', rule, {
16+
valid: [
17+
`'a' in window;`,
18+
`
19+
declare const node: Node;
20+
'a' in node;
21+
`,
22+
`
23+
type Node = { unrelated: true; };
24+
declare const node: Node;
25+
'a' in node;
26+
`,
27+
`
28+
interface Node {
29+
unrelated: true;
30+
};
31+
declare const node: Node;
32+
'a' in node;
33+
`,
34+
`
35+
declare const node: UnresolvedType;
36+
'a' in node;
37+
`,
38+
`
39+
import * as ESTree from 'estree';
40+
declare const loc: ESTree.SourceLocation;
41+
'a' in loc;
42+
`,
43+
`
44+
import * as ESTree from 'estree';
45+
declare const node: ESTree.Node;
46+
a.superClass;
47+
`,
48+
`
49+
import * as ESTree from 'estree';
50+
declare const node: ESTree.Node;
51+
a.type;
52+
`,
53+
`
54+
import * as ESTree from 'estree';
55+
declare const node: ESTree.Node;
56+
a.type === 'ClassDeclaration';
57+
`,
58+
`
59+
import * as ESTree from 'estree';
60+
declare const node: ESTree.ClassDeclaration | ESTree.FunctionDeclaration;
61+
a.type === 'ClassDeclaration';
62+
`,
63+
`
64+
import { TSESTree } from '@typescript-eslint/utils';
65+
declare const node: TSESTree.Node;
66+
node.superClass;
67+
`,
68+
`
69+
import { TSESTree } from '@typescript-eslint/utils';
70+
declare const node: TSESTree.Node;
71+
node.type;
72+
`,
73+
`
74+
import { TSESTree } from '@typescript-eslint/utils';
75+
declare const node: TSESTree.ClassDeclaration | TSESTree.FunctionDeclaration;
76+
node.type === 'ClassDeclaration';
77+
`,
78+
`
79+
import * as eslint from 'eslint';
80+
const listener: eslint.Rule.RuleListener = {
81+
ClassDeclaration(node) {
82+
node.type;
83+
},
84+
};
85+
`,
86+
`
87+
import * as eslint from 'eslint';
88+
const listener: eslint.Rule.RuleListener = {
89+
'ClassDeclaration, FunctionDeclaration'(node) {
90+
node.type === 'ClassDeclaration';
91+
},
92+
};
93+
`,
94+
],
95+
invalid: [
96+
{
97+
code: `
98+
import { TSESTree } from '@typescript-eslint/utils';
99+
declare const node: TSESTree.Node;
100+
'a' in node;
101+
`,
102+
errors: [
103+
{
104+
column: 9,
105+
line: 4,
106+
endColumn: 20,
107+
endLine: 4,
108+
messageId: 'in',
109+
},
110+
],
111+
},
112+
{
113+
code: `
114+
import { TSESTree } from '@typescript-eslint/utils';
115+
type Other = { key: true };
116+
declare const node: TSESTree.Node | Other;
117+
'a' in node;
118+
`,
119+
errors: [
120+
{
121+
column: 9,
122+
line: 5,
123+
endColumn: 20,
124+
endLine: 5,
125+
messageId: 'in',
126+
},
127+
],
128+
},
129+
{
130+
code: `
131+
import * as ESTree from 'estree';
132+
declare const node: ESTree.Node;
133+
'a' in node;
134+
`,
135+
errors: [
136+
{
137+
column: 9,
138+
line: 4,
139+
endColumn: 20,
140+
endLine: 4,
141+
messageId: 'in',
142+
},
143+
],
144+
},
145+
{
146+
code: `
147+
import * as eslint from 'eslint';
148+
const listener: eslint.Rule.RuleListener = {
149+
ClassDeclaration(node) {
150+
'a' in node;
151+
},
152+
};
153+
`,
154+
errors: [
155+
{
156+
column: 13,
157+
line: 5,
158+
endColumn: 24,
159+
endLine: 5,
160+
messageId: 'in',
161+
},
162+
],
163+
},
164+
],
165+
});

0 commit comments

Comments
 (0)