Skip to content

Commit

Permalink
Add consistent-existence-index-check rule (#2425)
Browse files Browse the repository at this point in the history
Co-authored-by: fisker <lionkay@gmail.com>
  • Loading branch information
axetroy and fisker authored Sep 9, 2024
1 parent 461b01c commit d3e4b80
Show file tree
Hide file tree
Showing 11 changed files with 764 additions and 4 deletions.
76 changes: 76 additions & 0 deletions docs/rules/consistent-existence-index-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# Enforce consistent style for element existence checks with `indexOf()`, `lastIndexOf()`, `findIndex()`, and `findLastIndex()`

💼 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).

<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

Enforce consistent style for element existence checks with `indexOf()`, `lastIndexOf()`, `findIndex()`, and `findLastIndex()`.

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) {}

//
if (index === -1) {}
```

```js
const index = foo.indexOf('bar');

//
if (index >= 0) {}

//
if (index !== -1) {}
```

```js
const index = foo.indexOf('bar');

//
if (index > -1) {}

//
if (index !== -1) {}
```

```js
const index = foo.lastIndexOf('bar');

//
if (index >= 0) {}

//
if (index !== -1) {}
```

```js
const index = array.findIndex(element => element > 10);

//
if (index < 0) {}

//
if (index === -1) {}
```

```js
const index = array.findLastIndex(element => element > 10);

//
if (index < 0) {}

//
if (index === -1) {}
```
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c
| [catch-error-name](docs/rules/catch-error-name.md) | Enforce a specific parameter name in catch clauses. || 🔧 | |
| [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-existence-index-check](docs/rules/consistent-existence-index-check.md) | Enforce consistent style for element existence checks with `indexOf()`, `lastIndexOf()`, `findIndex()`, and `findLastIndex()`. || 🔧 | |
| [consistent-function-scoping](docs/rules/consistent-function-scoping.md) | Move function definitions to the highest possible scope. || | |
| [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. || 🔧 | |
Expand Down
1 change: 1 addition & 0 deletions rules/ast/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ module.exports = {
isFunction: require('./is-function.js'),
isMemberExpression: require('./is-member-expression.js'),
isMethodCall: require('./is-method-call.js'),
isNegativeOne: require('./is-negative-one.js'),
isNewExpression,
isReferenceIdentifier: require('./is-reference-identifier.js'),
isStaticRequire: require('./is-static-require.js'),
Expand Down
12 changes: 12 additions & 0 deletions rules/ast/is-negative-one.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';

const {isNumberLiteral} = require('./literal.js');

function isNegativeOne(node) {
return node?.type === 'UnaryExpression'
&& node.operator === '-'
&& isNumberLiteral(node.argument)
&& node.argument.value === 1;
}

module.exports = isNegativeOne;
133 changes: 133 additions & 0 deletions rules/consistent-existence-index-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
'use strict';
const toLocation = require('./utils/to-location.js');
const {isMethodCall, isNegativeOne, isNumberLiteral} = require('./ast/index.js');

const MESSAGE_ID = 'consistent-existence-index-check';
const messages = {
[MESSAGE_ID]: 'Prefer `{{replacementOperator}} {{replacementValue}}` over `{{originalOperator}} {{originalValue}}` to check {{existenceOrNonExistence}}.',
};

const isZero = node => isNumberLiteral(node) && node.value === 0;

/**
@param {parent: import('estree').BinaryExpression} binaryExpression
@returns {{
replacementOperator: string,
replacementValue: string,
originalOperator: string,
originalValue: string,
} | undefined}
*/
function getReplacement(binaryExpression) {
const {operator, right} = binaryExpression;

if (operator === '<' && isZero(right)) {
return {
replacementOperator: '===',
replacementValue: '-1',
originalOperator: operator,
originalValue: '0',
};
}

if (operator === '>' && isNegativeOne(right)) {
return {
replacementOperator: '!==',
replacementValue: '-1',
originalOperator: operator,
originalValue: '-1',
};
}

if (operator === '>=' && isZero(right)) {
return {
replacementOperator: '!==',
replacementValue: '-1',
originalOperator: operator,
originalValue: '0',
};
}
}

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => ({
/** @param {import('estree').VariableDeclarator} variableDeclarator */
* VariableDeclarator(variableDeclarator) {
if (!(
variableDeclarator.parent.type === 'VariableDeclaration'
&& variableDeclarator.parent.kind === 'const'
&& variableDeclarator.id.type === 'Identifier'
&& isMethodCall(variableDeclarator.init, {methods: ['indexOf', 'lastIndexOf', 'findIndex', 'findLastIndex']})
)) {
return;
}

const variableIdentifier = variableDeclarator.id;
const variables = context.sourceCode.getDeclaredVariables(variableDeclarator);
const [variable] = variables;

// Just for safety
if (
variables.length !== 1
|| variable.identifiers.length !== 1
|| variable.identifiers[0] !== variableIdentifier
) {
return;
}

for (const {identifier} of variable.references) {
/** @type {{parent: import('estree').BinaryExpression}} */
const binaryExpression = identifier.parent;

if (binaryExpression.type !== 'BinaryExpression' || binaryExpression.left !== identifier) {
continue;
}

const replacement = getReplacement(binaryExpression);

if (!replacement) {
return;
}

const {left, operator, right} = binaryExpression;
const {sourceCode} = context;

const operatorToken = sourceCode.getTokenAfter(
left,
token => token.type === 'Punctuator' && token.value === operator,
);

yield {
node: binaryExpression,
loc: toLocation([operatorToken.range[0], right.range[1]], sourceCode),
messageId: MESSAGE_ID,
data: {
...replacement,
existenceOrNonExistence: `${replacement.replacementOperator === '===' ? 'non-' : ''}existence`,
},
* fix(fixer) {
yield fixer.replaceText(operatorToken, replacement.replacementOperator);

if (replacement.replacementValue !== replacement.originalValue) {
yield fixer.replaceText(right, replacement.replacementValue);
}
},
};
}
},
});

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'problem',
docs: {
description:
'Enforce consistent style for element existence checks with `indexOf()`, `lastIndexOf()`, `findIndex()`, and `findLastIndex()`.',
recommended: true,
},
fixable: 'code',
messages,
},
};
3 changes: 1 addition & 2 deletions rules/prefer-includes.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';
const isMethodNamed = require('./utils/is-method-named.js');
const simpleArraySearchRule = require('./shared/simple-array-search-rule.js');
const {isLiteral} = require('./ast/index.js');
const {isLiteral, isNegativeOne} = require('./ast/index.js');

const MESSAGE_ID = 'prefer-includes';
const messages = {
Expand All @@ -10,7 +10,6 @@ const messages = {
// Ignore `{_,lodash,underscore}.{indexOf,lastIndexOf}`
const ignoredVariables = new Set(['_', 'lodash', 'underscore']);
const isIgnoredTarget = node => node.type === 'Identifier' && ignoredVariables.has(node.name);
const isNegativeOne = node => node.type === 'UnaryExpression' && node.operator === '-' && node.argument && node.argument.type === 'Literal' && node.argument.value === 1;
const isLiteralZero = node => isLiteral(node, 0);
const isNegativeResult = node => ['===', '==', '<'].includes(node.operator);

Expand Down
4 changes: 2 additions & 2 deletions rules/utils/resolve-variable-name.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit d3e4b80

Please sign in to comment.