-
-
Notifications
You must be signed in to change notification settings - Fork 33
feat: add no-property-in-node rule #433
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
Changes from 1 commit
b9d6c28
8cfaf6d
4d163e1
d62539d
3575644
e42c075
2a94dcb
4d5d332
755cc08
d76b08a
5913127
d45695d
532925d
6b60ab0
89aafda
1d570e8
1ec0c2a
de370b0
18b205a
8d68dc9
d8dc1a0
d8cc468
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| 'use strict'; | ||
|
|
||
| const mod = require('../lib/index.js'); | ||
|
|
||
| module.exports = { | ||
| plugins: { 'eslint-plugin': mod }, | ||
| rules: mod.configs['recommended-type-checked'].rules, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Disallow using `in` to narrow node types instead of looking at properties (`eslint-plugin/no-property-in-node`) | ||
|
|
||
| 💼 This rule is enabled in the `recommended-type-checked` [config](https://github.com/eslint-community/eslint-plugin-eslint-plugin#presets). | ||
|
|
||
| 💭 This rule requires type information. | ||
|
|
||
| <!-- end auto-generated rule header --> | ||
|
|
||
| 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`. | ||
|
|
||
| Using direct property checks is generally considered preferable. | ||
|
|
||
| ## Rule Details | ||
|
|
||
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```ts | ||
| /* eslint eslint-plugin/no-property-in-node: error */ | ||
|
|
||
| declare const node: TSESTree.Parameter; | ||
|
|
||
| if ('optional' in node) { | ||
| node.optional; | ||
| } | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```ts | ||
| /* eslint eslint-plugin/no-property-in-node: error */ | ||
|
|
||
| declare const node: TSESTree.Parameter; | ||
|
|
||
| if (node.type !== TSESTree.AST_NODE_TYPES.TSParameterProperty) { | ||
| node.optional; | ||
| } | ||
JoshuaKGoldberg marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ``` | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would you like to do here? The first ideas that come to mind for me are:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I slightly prefer this. just like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the rule is also
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah 😓 ... another possibility for flat configs specifically could be to make the configs be a function with properties set on it. So you could do either of: export default [
eslintPlugin.configs.recommended,
eslintPlugin.configs.recommended({ typeChecked: true }),
]The only prior art I know of this strategy is azat-io/eslint-plugin-perfectionist#90.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
maybe just adding
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicating every config when we have so many is not great. What about just providing a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heh, that's what we used to do in typescript-eslint. But then we found that most folks only enabled one of the configs. And it was a bit irksome to have to enable two. typescript-eslint/typescript-eslint#5204 -> https://typescript-eslint.io/blog/announcing-typescript-eslint-v6#reworked-configuration-names is what we ended up with.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing discussion in: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| 'use strict'; | ||
|
|
||
| const { ESLintUtils } = require('@typescript-eslint/utils'); | ||
| const tsutils = require('ts-api-utils'); | ||
|
|
||
| const typedNodeSourceFileTesters = [ | ||
| /@types[/\\]estree[/\\]index\.d\.ts/, | ||
| /@typescript-eslint[/\\]types[/\\]dist[/\\]generated[/\\]ast-spec\.d\.ts/, | ||
| ]; | ||
|
|
||
| /** | ||
| * @param {import('typescript').Type} type | ||
| * @returns Whether the type seems to include a known ESTree or TSESTree AST node. | ||
| */ | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| function isAstNodeType(type) { | ||
| return tsutils | ||
| .typeParts(type) | ||
| .flatMap((typePart) => 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: true, | ||
| 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) { | ||
| const services = ESLintUtils.getParserServices(context); | ||
| const type = services.getTypeAtLocation(node.right); | ||
|
|
||
| if (isAstNodeType(type)) { | ||
| context.report({ messageId: 'in', node }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "compilerOptions": { | ||
| "moduleResolution": "NodeNext" | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.