-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: add no-property-in-node rule #433
Merged
aladdin-add
merged 22 commits into
eslint-community:main
from
JoshuaKGoldberg:no-property-in-node
Feb 6, 2024
Merged
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
b9d6c28
feat: add no-property-in-node rule
JoshuaKGoldberg 8cfaf6d
Add ☑️ emoji for recommended-type-checked
JoshuaKGoldberg 4d163e1
tests: valid before invalid
JoshuaKGoldberg d62539d
Also check for whether the node has a 'type'
JoshuaKGoldberg 3575644
Added docs and example to isAstNodeType
JoshuaKGoldberg e42c075
Expanded rule details
JoshuaKGoldberg 2a94dcb
Add more valid test cases
JoshuaKGoldberg 4d5d332
Merge branch 'main'
JoshuaKGoldberg 755cc08
Fixed test path to fixtures
JoshuaKGoldberg d76b08a
Use parserOptions.project: true for eslint-remote-tester on TS files
JoshuaKGoldberg 5913127
nit: avoid shadowing name for typePart
JoshuaKGoldberg d45695d
<!-- omit from toc -->
JoshuaKGoldberg 532925d
Downgraded to typescript-eslint@v5
JoshuaKGoldberg 6b60ab0
Also remove @typescript-eslint/utils
JoshuaKGoldberg 89aafda
Or rather, make @typescript-eslint/utils a -D
JoshuaKGoldberg 1d570e8
Remove ts-api-utils too
JoshuaKGoldberg 1ec0c2a
Removed recommended-type-checked
JoshuaKGoldberg de370b0
Removed README.md section too
JoshuaKGoldberg 18b205a
Removed eslint-remote-tester.config.js parserOptions.project too
JoshuaKGoldberg 8d68dc9
Redid README.md presets table
JoshuaKGoldberg d8dc1a0
Added all-type-checked
JoshuaKGoldberg d8cc468
Removed file notice
JoshuaKGoldberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
# 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`. | ||
|
||
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); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
'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/, | ||
]; | ||
|
||
/** | ||
* 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 tsutils | ||
.typeParts(type) | ||
.filter((typePart) => typePart.getProperty('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 }); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"compilerOptions": { | ||
"moduleResolution": "NodeNext" | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://github.com/eslint-community/eslint-plugin-eslint-plugin/actions/runs/7794182409/job/21255096639?pr=433:
What would you like to do here? The first ideas that come to mind for me are:
all
& add anall-type-checked
config?no-property-in-node
for specificallyeslint/eslint
, since it's the only one without a TSConfig?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly prefer this. just like
recommended
andrecommended-type-checked
. @bmish wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rule is also
rules-recommended
? So also needrules-recommended-type-checked
...., so many presets doesn't seem like a good idea.😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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:
The only prior art I know of this strategy is azat-io/eslint-plugin-perfectionist#90.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recommended
config follows the eslint's semantic-versioning-policy, whileall
,rules-all
,tests-all
does not.maybe just adding
recommended: false
in this PR. let's consider adding the configs in another PR. thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
type-checked
config with just this single rule that you can mix and match with the others?There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing discussion in: