From 5ff5b8a6e1b1d736ffad390871c25f56a3cc94a6 Mon Sep 17 00:00:00 2001 From: Axetroy Date: Mon, 19 Aug 2024 11:35:13 +0800 Subject: [PATCH] Add `consistent-indexof-check` --- docs/rules/consistent-indexof-check.md | 36 ++++ readme.md | 1 + rules/consistent-indexof-check.js | 175 ++++++++++++++++++ rules/utils/resolve-variable-name.js | 4 +- test/consistent-non-exists-index-check.mjs | 64 +++++++ test/package.mjs | 1 + .../snapshots/consistent-indexof-check.mjs.md | 149 +++++++++++++++ .../consistent-indexof-check.mjs.snap | Bin 0 -> 556 bytes 8 files changed, 428 insertions(+), 2 deletions(-) create mode 100644 docs/rules/consistent-indexof-check.md create mode 100644 rules/consistent-indexof-check.js create mode 100644 test/consistent-non-exists-index-check.mjs create mode 100644 test/snapshots/consistent-indexof-check.mjs.md create mode 100644 test/snapshots/consistent-indexof-check.mjs.snap diff --git a/docs/rules/consistent-indexof-check.md b/docs/rules/consistent-indexof-check.md new file mode 100644 index 0000000000..b587abcefa --- /dev/null +++ b/docs/rules/consistent-indexof-check.md @@ -0,0 +1,36 @@ +# Enforce consistent styling when checking for element existence using `indexOf()` + +💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs-eslintconfigjs). + +🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix). + + + + +Enforce consistent style when checking for element existence with `indexOf()`. + +Prefer using `index === -1` to check if an element does not exist and `index !== -1` to check if an element does exist. + +Similar to the [`explicit-length-check`](explicit-length-check.md) rule. + +## Examples + +```js +// ❌ +const index = foo.indexOf('bar'); +if (index < 0) {} + +// ✅ +const index = foo.indexOf('bar'); +if (index === -1) {} +``` + +```js +// ❌ +const index = foo.indexOf('bar'); +if (index >= 0) {} + +// ✅ +const index = foo.indexOf('bar'); +if (index !== -1) {} +``` diff --git a/readme.md b/readme.md index df459ba791..066e18852f 100644 --- a/readme.md +++ b/readme.md @@ -115,6 +115,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c | [consistent-destructuring](docs/rules/consistent-destructuring.md) | Use destructured variables over properties. | | 🔧 | 💡 | | [consistent-empty-array-spread](docs/rules/consistent-empty-array-spread.md) | Prefer consistent types when spreading a ternary in an array literal. | ✅ | 🔧 | | | [consistent-function-scoping](docs/rules/consistent-function-scoping.md) | Move function definitions to the highest possible scope. | ✅ | | | +| [consistent-indexof-check](docs/rules/consistent-indexof-check.md) | Enforce consistent styling when checking for element existence using `indexOf()` | ✅ | 🔧 | | | [custom-error-definition](docs/rules/custom-error-definition.md) | Enforce correct `Error` subclassing. | | 🔧 | | | [empty-brace-spaces](docs/rules/empty-brace-spaces.md) | Enforce no spaces between braces. | ✅ | 🔧 | | | [error-message](docs/rules/error-message.md) | Enforce passing a `message` value when creating a built-in error. | ✅ | | | diff --git a/rules/consistent-indexof-check.js b/rules/consistent-indexof-check.js new file mode 100644 index 0000000000..39e4267646 --- /dev/null +++ b/rules/consistent-indexof-check.js @@ -0,0 +1,175 @@ +'use strict'; +const resolveVariableName = require('./utils/resolve-variable-name.js'); + +const MESSAGE_ID = 'consistent-indexof-check'; +const messages = { + [MESSAGE_ID]: 'Prefer `{{replacement}}` over `{{value}}`.', +}; + +/** +Check if the node is a call expression of `indexOf` method. + +@param {import('estree').Node} node +@returns {node is import('estree').CallExpression} +*/ +function isIndexOfCallExpression(node) { + return ( + node + && node.type === 'CallExpression' + && node.callee.type === 'MemberExpression' + && node.callee.property.name === 'indexOf' + && node.arguments.length === 1 + ); +} + +/** +Check if the operator is `<` or `<=` and the value is `0` or `-1`. + +@param {string} operator +@param {string} value +@returns {boolean} +*/ +function isCheckNotExists(operator, value) { + return ( + (operator === '<' && value <= 0) + || (operator === '<=' && value <= -1) + ); +} + +/** +Check if the operator is `>` or `>=` and the value is `-1` or `0`. + +@param {string} operator +@param {number} value +@returns {boolean} +*/ +function isCheckExists(operator, value) { + return ( + (operator === '>' && value === -1) + || (operator === '>=' && value === 0) + ); +} + +/** +Reverse the operator. + +@param {string} operator +@returns {string} +*/ +function reverseOperator(operator) { + switch (operator) { + case '<': { + return '>'; + } + + case '<=': { + return '>='; + } + + case '>': { + return '<'; + } + + case '>=': { + return '<='; + } + + default: { + return operator; + } + } +} + +/** @param {import('eslint').Rule.RuleContext} context */ +const create = context => ({ + /** @param {import('estree').IfStatement} node */ + IfStatement(node) { + if (node.test.type !== 'BinaryExpression') { + return; + } + + /** @type {import('estree').BinaryExpression | undefined} */ + let testNode; + let variableName = ''; + let literalValue = 0; + let operator = ''; + + if ( + // Match case: index === -1 + node.test.left.type === 'Identifier' + && node.test.right.type === 'Literal' + ) { + testNode = node.test; + variableName = testNode.left.name; + literalValue = testNode.right.value; + operator = testNode.operator; + } else if ( + // Match case: -1 === index + node.test.right.type === 'Identifier' + && node.test.left.type === 'Literal' + ) { + testNode = node.test; + variableName = testNode.right.name; + literalValue = testNode.left.value; + operator = reverseOperator(testNode.operator); + } + + if (!testNode) { + return; + } + + let replacement = ''; + + // For better performance, early checking of operators can avoid looking up variables in scope. + if (isCheckNotExists(operator, literalValue)) { + replacement = `${variableName} === -1`; + } else if (isCheckExists(operator, literalValue)) { + replacement = `${variableName} !== -1`; + } + + if (!replacement) { + return; + } + + const variable = resolveVariableName( + variableName, + context.sourceCode.getScope(node), + ); + + if (!variable) { + return; + } + + for (const definition of variable.defs) { + if (definition.type === 'Variable') { + if (!isIndexOfCallExpression(definition.node.init)) { + break; + } + + context.report({ + node: testNode, + messageId: MESSAGE_ID, + data: { + value: context.sourceCode.getText(testNode), + replacement, + }, + fix: fixer => fixer.replaceText(testNode, replacement), + }); + } + } + }, +}); + +/** @type {import('eslint').Rule.RuleModule} */ +module.exports = { + create, + meta: { + type: 'problem', + docs: { + description: 'Enforce consistent styling when checking for element existence using `indexOf()`', + recommended: true, + }, + fixable: 'code', + messages, + }, +}; diff --git a/rules/utils/resolve-variable-name.js b/rules/utils/resolve-variable-name.js index f5b50d126a..4d2c34cc16 100644 --- a/rules/utils/resolve-variable-name.js +++ b/rules/utils/resolve-variable-name.js @@ -4,8 +4,8 @@ Finds a variable named `name` in the scope `scope` (or it's parents). @param {string} name - The variable name to be resolve. -@param {Scope} scope - The scope to look for the variable in. -@returns {Variable?} - The found variable, if any. +@param {import('eslint').Scope.Scope} scope - The scope to look for the variable in. +@returns {import('eslint').Scope.Variable | void} - The found variable, if any. */ module.exports = function resolveVariableName(name, scope) { while (scope) { diff --git a/test/consistent-non-exists-index-check.mjs b/test/consistent-non-exists-index-check.mjs new file mode 100644 index 0000000000..bf2c3e8918 --- /dev/null +++ b/test/consistent-non-exists-index-check.mjs @@ -0,0 +1,64 @@ +import outdent from 'outdent'; +import {getTester} from './utils/test.mjs'; + +const {test} = getTester(import.meta); + +test.snapshot({ + valid: [ + outdent` + const index = foo.indexOf('bar'); + + if (index === -1) {} + `, + outdent` + const index = foo.indexOf('bar'); + + if (-1 === index) {} + `, + outdent` + const index = foo.indexOf('bar'); + + if (index !== -1) {} + `, + outdent` + const index = foo.indexOf('bar'); + + if (-1 !== index) {} + `, + outdent` + const index = 0; // index not from indexOf + + if (index < 0) {} + `, + ], + invalid: [ + outdent` + const index = foo.indexOf('bar'); + + if (index < 0) {} + `, + outdent` + const index = foo.indexOf('bar'); + + if (0 > index) {} + `, + outdent` + const index = foo.indexOf('bar'); + + if (index >= 0) {} + `, + outdent` + const index = foo.indexOf('bar'); + + if (0 <= index) {} + `, + outdent` + const index = foo.indexOf('bar'); + + function foo () { + // It will search the scope chain for 'index' and find the 'index' variable declared above. + if (index < 0) {} + } + `, + ], +}); diff --git a/test/package.mjs b/test/package.mjs index 96bfcb1351..dec37ab749 100644 --- a/test/package.mjs +++ b/test/package.mjs @@ -30,6 +30,7 @@ const RULES_WITHOUT_PASS_FAIL_SECTIONS = new Set([ 'filename-case', // Intended to not use `pass`/`fail` section in this rule. 'prefer-modern-math-apis', + 'consistent-indexof-check', ]); test('Every rule is defined in index file in alphabetical order', t => { diff --git a/test/snapshots/consistent-indexof-check.mjs.md b/test/snapshots/consistent-indexof-check.mjs.md new file mode 100644 index 0000000000..2a6d53422a --- /dev/null +++ b/test/snapshots/consistent-indexof-check.mjs.md @@ -0,0 +1,149 @@ +# Snapshot report for `test/consistent-indexof-check.mjs` + +The actual snapshot is saved in `consistent-indexof-check.mjs.snap`. + +Generated by [AVA](https://avajs.dev). + +## invalid(1): const index = foo.indexOf('bar'); if (index < 0) {} + +> Input + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (index < 0) {}␊ + ` + +> Output + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (index === -1) {}␊ + ` + +> Error 1/1 + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + > 3 | if (index < 0) {}␊ + | ^^^^^^^^^ Prefer \`index === -1\` over \`index < 0\`.␊ + ` + +## invalid(2): const index = foo.indexOf('bar'); if (0 > index) {} + +> Input + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (0 > index) {}␊ + ` + +> Output + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (index === -1) {}␊ + ` + +> Error 1/1 + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + > 3 | if (0 > index) {}␊ + | ^^^^^^^^^ Prefer \`index === -1\` over \`0 > index\`.␊ + ` + +## invalid(3): const index = foo.indexOf('bar'); if (index >= 0) {} + +> Input + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (index >= 0) {}␊ + ` + +> Output + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (index !== -1) {}␊ + ` + +> Error 1/1 + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + > 3 | if (index >= 0) {}␊ + | ^^^^^^^^^^ Prefer \`index !== -1\` over \`index >= 0\`.␊ + ` + +## invalid(4): const index = foo.indexOf('bar'); if (0 <= index) {} + +> Input + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (0 <= index) {}␊ + ` + +> Output + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | if (index !== -1) {}␊ + ` + +> Error 1/1 + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + > 3 | if (0 <= index) {}␊ + | ^^^^^^^^^^ Prefer \`index !== -1\` over \`0 <= index\`.␊ + ` + +## invalid(5): const index = foo.indexOf('bar'); function foo () { // It will search the scope chain for 'index' and find the 'index' variable declared above. if (index < 0) {} } + +> Input + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | function foo () {␊ + 4 | // It will search the scope chain for 'index' and find the 'index' variable declared above.␊ + 5 | if (index < 0) {}␊ + 6 | }␊ + ` + +> Output + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | function foo () {␊ + 4 | // It will search the scope chain for 'index' and find the 'index' variable declared above.␊ + 5 | if (index === -1) {}␊ + 6 | }␊ + ` + +> Error 1/1 + + `␊ + 1 | const index = foo.indexOf('bar');␊ + 2 |␊ + 3 | function foo () {␊ + 4 | // It will search the scope chain for 'index' and find the 'index' variable declared above.␊ + > 5 | if (index < 0) {}␊ + | ^^^^^^^^^ Prefer \`index === -1\` over \`index < 0\`.␊ + 6 | }␊ + ` diff --git a/test/snapshots/consistent-indexof-check.mjs.snap b/test/snapshots/consistent-indexof-check.mjs.snap new file mode 100644 index 0000000000000000000000000000000000000000..392b19750b1f40b83c8f7ae15ca6e3ed7c511a69 GIT binary patch literal 556 zcmV+{0@M9LRzV9 zVl)$tCjkj}h1vsD>H*3HI0zC4K&^TB=4t#daJlr(!e7@^ zGtKG64ig>1nP0R^Tkxa^HJI?}1snuYmb>2vL9yTxRVZzP1t|9VYp9lR`ICi$w8Ey; z;nlV86HSf}=W`3W_{o3DE$6>kfQ{(or7d6;kp-+F8kVsrCobi5Pqf9^HTZImCk$-XKj z73_*VgmiUcq*c^=V`hugkoh=BOh|mNBJuXH-m&j8cD}veb#r?4hoV zhx*rm`g_`(KKV+a`W)*@N>bzblW<+n5Y?J9E~{_F)ztYoT%Y;M%(Xv%4A+~4>&8S} zgSPNAlOpa!v4l%hD%jIFXPhIXM0riA2um}xX8{|rMA}`ui3q7j`M=@z u?mfF5xb#fJMxIXcpAE~4`S)YFJ{oYG<)zpCO_{~&NBRM0Yg%YB3jhFtP!Mte literal 0 HcmV?d00001