Skip to content

Commit

Permalink
explicit-length-check: Make rule work in many more cases (#943)
Browse files Browse the repository at this point in the history
  • Loading branch information
@fisker
fisker authored Dec 21, 2020
1 parent 5c7ea92 commit f3bc798
Show file tree
Hide file tree
Showing 5 changed files with 212 additions and 80 deletions.
64 changes: 40 additions & 24 deletions docs/rules/explicit-length-check.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,42 +11,38 @@ Enforce comparison with `=== 0` when checking for zero length.
### Fail

```js
if (!foo.length) {}
const isEmpty = !foo.length;
```

```js
while (foo.length == 0) {}
const isEmpty = foo.length == 0;
```

```js
do {} while (foo.length < 1);
const isEmpty = foo.length < 1;
```

```js
if (; 0 === foo.length;) {}
const isEmpty = 0 === foo.length;
```

```js
const unicorn = 0 == foo.length ? 1 : 2;
const isEmpty = 0 == foo.length;
```

```js
if (1 > foo.length) {}
const isEmpty = 1 > foo.length;
```

```js
// Negative style is forbid too
if (!(foo.length > 0)) {}
// Negative style is forbidden too
const isEmpty = !(foo.length > 0);
```

### Pass

```js
if (foo.length === 0) {}
```

```js
const unicorn = foo.length === 0 ? 1 : 2;
const isEmpty = foo.length === 0;
```

## Non-zero comparisons
Expand All @@ -56,46 +52,66 @@ Enforce comparison with `> 0` when checking for non-zero length.
### Fail

```js
if (foo.length !== 0) {}
const isNotEmpty = foo.length !== 0;
```

```js
const isNotEmpty = foo.length != 0;
```

```js
const isNotEmpty = foo.length >= 1;
```

```js
const isNotEmpty = 0 !== foo.length;
```

```js
const isNotEmpty = 0 != foo.length;
```

```js
const isNotEmpty = 0 < foo.length;
```

```js
while (foo.length != 0) {}
const isNotEmpty = 1 <= foo.length;
```

```js
do {} while (foo.length >= 1);
// Negative style is forbidden too
const isNotEmpty = !(foo.length === 0);
```

```js
for (; 0 !== foo.length; ) {}
if (foo.length || bar.length) {}
```

```js
const unicorn = 0 != foo.length ? 1 : 2;
const unicorn = foo.length ? 1 : 2;
```

```js
if (0 < foo.length) {}
while (foo.length) {}
```

```js
if (1 <= foo.length) {}
do {} while (foo.length);
```

```js
// Negative style is forbid too
if (!(foo.length === 0)) {}
for (; foo.length; ) {};
```

### Pass

```js
if (foo.length > 0) {}
const isNotEmpty = foo.length > 0;
```

```js
const unicorn = foo.length > 0 ? 1 : 2;
if (foo.length > 0 || bar.length > 0) {}
```

### Options
Expand Down
150 changes: 98 additions & 52 deletions rules/explicit-length-check.js
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
'use strict';
const {isParenthesized} = require('eslint-utils');
const getDocumentationUrl = require('./utils/get-documentation-url');

const TYPE_NON_ZERO = 'non-zero';
const TYPE_ZERO = 'zero';
const messages = {
'non-zero': 'Use `.length {{code}}` when checking length is not zero.',
zero: 'Use `.length {{code}}` when checking length is zero.'
[TYPE_NON_ZERO]: 'Use `.length {{code}}` when checking length is not zero.',
[TYPE_ZERO]: 'Use `.length {{code}}` when checking length is zero.'
};

const isLengthProperty = node =>
Expand Down Expand Up @@ -56,12 +59,17 @@ const zeroStyle = {
test: node => isCompareRight(node, '===', 0)
};

function getNonZeroLengthNode(node) {
// `foo.length`
if (isLengthProperty(node)) {
return node;
const cache = new WeakMap();
function getCheckTypeAndLengthNode(node) {
if (!cache.has(node)) {
cache.set(node, getCheckTypeAndLengthNodeWithoutCache(node));
}

return cache.get(node);
}

function getCheckTypeAndLengthNodeWithoutCache(node) {
// Non-Zero length check
if (
// `foo.length !== 0`
isCompareRight(node, '!==', 0) ||
Expand All @@ -72,7 +80,7 @@ function getNonZeroLengthNode(node) {
// `foo.length >= 1`
isCompareRight(node, '>=', 1)
) {
return node.left;
return {type: TYPE_NON_ZERO, node, lengthNode: node.left};
}

if (
Expand All @@ -85,11 +93,10 @@ function getNonZeroLengthNode(node) {
// `1 <= foo.length`
isCompareLeft(node, '<=', 1)
) {
return node.right;
return {type: TYPE_NON_ZERO, node, lengthNode: node.right};
}
}

function getZeroLengthNode(node) {
// Zero length check
if (
// `foo.length === 0`
isCompareRight(node, '===', 0) ||
Expand All @@ -98,7 +105,7 @@ function getZeroLengthNode(node) {
// `foo.length < 1`
isCompareRight(node, '<', 1)
) {
return node.left;
return {type: TYPE_ZERO, node, lengthNode: node.left};
}

if (
Expand All @@ -109,11 +116,12 @@ function getZeroLengthNode(node) {
// `1 > foo.length`
isCompareLeft(node, '>', 1)
) {
return node.right;
return {type: TYPE_ZERO, node, lengthNode: node.right};
}
}

const selector = `:matches(${
// TODO: check other `LogicalExpression`s
const booleanNodeSelector = `:matches(${
[
'IfStatement',
'ConditionalExpression',
Expand All @@ -123,65 +131,103 @@ const selector = `:matches(${
].join(', ')
}) > *.test`;

const create = context => {
function create(context) {
const options = {
'non-zero': 'greater-than',
...context.options[0]
};
const nonZeroStyle = nonZeroStyles.get(options['non-zero']);
const sourceCode = context.getSourceCode();
const reportedBinaryExpressions = new Set();

function checkExpression(node) {
// Is matched style
if (nonZeroStyle.test(node) || zeroStyle.test(node)) {
return;
}

let isNegative = false;
let expression = node;
while (isLogicNot(expression)) {
isNegative = !isNegative;
expression = expression.argument;
}

if (expression.type === 'LogicalExpression') {
checkExpression(expression.left);
checkExpression(expression.right);
return;
function reportProblem({node, type, lengthNode}, isNegative) {
if (isNegative) {
type = type === TYPE_NON_ZERO ? TYPE_ZERO : TYPE_NON_ZERO;
}

let lengthNode;
let isCheckingZero = isNegative;

const zeroLengthNode = getZeroLengthNode(expression);
if (zeroLengthNode) {
lengthNode = zeroLengthNode;
isCheckingZero = !isCheckingZero;
} else {
const nonZeroLengthNode = getNonZeroLengthNode(expression);
if (nonZeroLengthNode) {
lengthNode = nonZeroLengthNode;
} else {
return;
}
const {code} = type === TYPE_NON_ZERO ? nonZeroStyle : zeroStyle;
let fixed = `${sourceCode.getText(lengthNode)} ${code}`;
if (
!isParenthesized(node, sourceCode) &&
node.type === 'UnaryExpression' &&
node.parent.type === 'UnaryExpression'
) {
fixed = `(${fixed})`;
}

const {code} = isCheckingZero ? zeroStyle : nonZeroStyle;
const messageId = isCheckingZero ? 'zero' : 'non-zero';
context.report({
node,
messageId,
messageId: type,
data: {code},
fix: fixer => fixer.replaceText(node, `${sourceCode.getText(lengthNode)} ${code}`)
fix: fixer => fixer.replaceText(node, fixed)
});
}

function checkBooleanNode(node) {
if (node.type === 'LogicalExpression') {
checkBooleanNode(node.left);
checkBooleanNode(node.right);
return;
}

if (isLengthProperty(node)) {
reportProblem({node, type: TYPE_NON_ZERO, lengthNode: node});
}
}

const binaryExpressions = [];
return {
[selector](node) {
checkExpression(node);
// The outer `!` expression
'UnaryExpression[operator="!"]:not(UnaryExpression[operator="!"] > .argument)'(node) {
let isNegative = false;
let expression = node;
while (isLogicNot(expression)) {
isNegative = !isNegative;
expression = expression.argument;
}

if (expression.type === 'LogicalExpression') {
checkBooleanNode(expression);
return;
}

if (isLengthProperty(expression)) {
reportProblem({type: TYPE_NON_ZERO, node, lengthNode: expression}, isNegative);
return;
}

const result = getCheckTypeAndLengthNode(expression);
if (result) {
reportProblem({...result, node}, isNegative);
reportedBinaryExpressions.add(result.lengthNode);
}
},
[booleanNodeSelector](node) {
checkBooleanNode(node);
},
BinaryExpression(node) {
// Delay check on this, so we don't need take two steps for this case
// `const isEmpty = !(foo.length >= 1);`
binaryExpressions.push(node);
},
'Program:exit'() {
for (const node of binaryExpressions) {
if (
reportedBinaryExpressions.has(node) ||
zeroStyle.test(node) ||
nonZeroStyle.test(node)
) {
continue;
}

const result = getCheckTypeAndLengthNode(node);
if (result) {
reportProblem(result);
}
}
}
};
};
}

const schema = [
{
Expand Down
14 changes: 10 additions & 4 deletions test/explicit-length-check.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@ test({
'if (foo[length]) {}',
'if (foo["length"]) {}',

// Not in `IfStatement` or `ConditionalExpression`
'foo.length',
// Already in wanted style
'foo.length === 0',
'foo.length !== 0',
'foo.length > 0',

// Not boolean
'const bar = foo.length',
'const bar = +foo.length',

// Checking 'non-zero'
'if (foo.length > 0) {}',
{
Expand Down Expand Up @@ -113,5 +115,9 @@ test.visualize([
'if (!(foo.length === 0)) {}',
'while (foo.length >= 1) {}',
'do {} while (foo.length);',
'for (let i = 0; (bar && !foo.length); i ++) {}'
'for (let i = 0; (bar && !foo.length); i ++) {}',
'const isEmpty = foo.length < 1;',
'bar(foo.length >= 1)',
'bar(!foo.length || foo.length)',
'const bar = void !foo.length;'
]);
Loading

0 comments on commit f3bc798

Please sign in to comment.