explicit-length-check: Check unsafe LogicalExpressions (#952)

Co-authored-by: Sindre Sorhus <[email protected]>

Author: fisker

docs/rules/explicit-length-check.md

@@ -2,7 +2,7 @@
 
 Enforce explicitly checking the length of an object and enforce the comparison style.
 
-This rule is fixable.
+This rule is fixable, unless it's [unsafe to fix](#unsafe-to-fix-case).
 
 ## Zero comparisons
 
@@ -141,3 +141,25 @@ The `non-zero` option can be configured with one of the following:
 	- Enforces non-zero to be checked with: `foo.length !== 0`
 - `greater-than-or-equal`
 	- Enforces non-zero to be checked with: `foo.length >= 1`
+
+## Unsafe to fix case
+
+`.length` check inside `LogicalExpression`s are not safe to fix.
+
+Example:
+
+```js
+const bothNotEmpty = (a, b) => a.length && b.length;
+
+if (bothNotEmpty(foo, bar)) {}
+```
+
+In this case, the `bothNotEmpty` function returns a `number`, but it will most likely be used as a `boolean`. The rule will still report this as an error, but without an auto-fix. You can apply a [suggestion](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions) in your editor, which will fix it to:
+
+```js
+const bothNotEmpty = (a, b) => a.length > 0 && b.length > 0;
+
+if (bothNotEmpty(foo, bar)) {}
+```
+
+The rule is smart enough to know some `LogicalExpression`s are safe to fix, like when it's inside `if`, `while`, etc.

readme.md

@@ -112,7 +112,7 @@ Configure it in `package.json`.
 - [error-message](docs/rules/error-message.md) - Enforce passing a `message` value when creating a built-in error.
 - [escape-case](docs/rules/escape-case.md) - Require escape sequences to use uppercase values. *(fixable)*
 - [expiring-todo-comments](docs/rules/expiring-todo-comments.md) - Add expiration conditions to TODO comments.
-- [explicit-length-check](docs/rules/explicit-length-check.md) - Enforce explicitly comparing the `length` property of a value. *(fixable)*
+- [explicit-length-check](docs/rules/explicit-length-check.md) - Enforce explicitly comparing the `length` property of a value. *(partly fixable)*
 - [filename-case](docs/rules/filename-case.md) - Enforce a case style for filenames.
 - [import-index](docs/rules/import-index.md) - Enforce importing index files with `.`. *(fixable)*
 - [import-style](docs/rules/import-style.md) - Enforce specific import styles per module.

rules/explicit-length-check.js

@@ -5,9 +5,11 @@ const isLiteralValue = require('./utils/is-literal-value');
 
 const TYPE_NON_ZERO = 'non-zero';
 const TYPE_ZERO = 'zero';
+const MESSAGE_ID_SUGGESTION = 'suggestion';
 const messages = {
 	[TYPE_NON_ZERO]: 'Use `.length {{code}}` when checking length is not zero.',
-	[TYPE_ZERO]: 'Use `.length {{code}}` when checking length is zero.'
+	[TYPE_ZERO]: 'Use `.length {{code}}` when checking length is zero.',
+	[MESSAGE_ID_SUGGESTION]: 'Replace `.length` with `.length {{code}}`.'
 };
 
 const isLogicNot = node =>
@@ -172,7 +174,7 @@ function create(context) {
 	const nonZeroStyle = nonZeroStyles.get(options['non-zero']);
 	const sourceCode = context.getSourceCode();
 
-	function reportProblem({node, isZeroLengthCheck, lengthNode}) {
+	function reportProblem({node, isZeroLengthCheck, lengthNode, autoFix}) {
 		const {code, test} = isZeroLengthCheck ? zeroStyle : nonZeroStyle;
 		if (test(node)) {
 			return;
@@ -187,17 +189,33 @@ function create(context) {
 			fixed = `(${fixed})`;
 		}
 
-		context.report({
+		const fix = fixer => fixer.replaceText(node, fixed);
+
+		const problem = {
 			node,
 			messageId: isZeroLengthCheck ? TYPE_ZERO : TYPE_NON_ZERO,
-			data: {code},
-			fix: fixer => fixer.replaceText(node, fixed)
-		});
+			data: {code}
+		};
+
+		if (autoFix) {
+			problem.fix = fix;
+		} else {
+			problem.suggest = [
+				{
+					messageId: MESSAGE_ID_SUGGESTION,
+					data: {code},
+					fix
+				}
+			];
+		}
+
+		context.report(problem);
 	}
 
 	return {
 		[lengthSelector](lengthNode) {
 			let node;
+			let autoFix = true;
 
 			let {isZeroLengthCheck, node: lengthCheckNode} = getLengthCheckNode(lengthNode);
 			if (lengthCheckNode) {
@@ -211,11 +229,15 @@ function create(context) {
 				if (isBooleanNode(ancestor)) {
 					isZeroLengthCheck = isNegative;
 					node = ancestor;
+				} else if (lengthNode.parent.type === 'LogicalExpression') {
+					isZeroLengthCheck = isNegative;
+					node = lengthNode;
+					autoFix = false;
 				}
 			}
 
 			if (node) {
-				reportProblem({node, isZeroLengthCheck, lengthNode});
+				reportProblem({node, isZeroLengthCheck, lengthNode, autoFix});
 			}
 		}
 	};

test/explicit-length-check.js

@@ -1,6 +1,22 @@
 import {outdent} from 'outdent';
 import {test} from './utils/test';
 
+const suggestionCase = ({code, output, desc, options = []}) => {
+	const suggestion = {output};
+	if (desc) {
+		suggestion.desc = desc;
+	}
+
+	return {
+		code,
+		output: code,
+		options,
+		errors: [
+			{suggestions: [suggestion]}
+		]
+	};
+};
+
 const nonZeroCases = [
 	'foo.length',
 	'!!foo.length',
@@ -82,7 +98,33 @@ test({
 		'if (foo.length > 1) {}',
 		'if (foo.length < 2) {}'
 	],
-	invalid: []
+	invalid: [
+		suggestionCase({
+			code: 'const x = foo.length || bar()',
+			output: 'const x = foo.length > 0 || bar()',
+			desc: 'Replace `.length` with `.length > 0`.'
+		}),
+		suggestionCase({
+			code: 'const x = foo.length || bar()',
+			output: 'const x = foo.length !== 0 || bar()',
+			desc: 'Replace `.length` with `.length !== 0`.',
+			options: [{'non-zero': 'not-equal'}]
+		}),
+		suggestionCase({
+			code: 'const x = foo.length || bar()',
+			output: 'const x = foo.length >= 1 || bar()',
+			desc: 'Replace `.length` with `.length >= 1`.',
+			options: [{'non-zero': 'greater-than-or-equal'}]
+		}),
+		suggestionCase({
+			code: '() => foo.length && bar()',
+			output: '() => foo.length > 0 && bar()'
+		}),
+		suggestionCase({
+			code: 'alert(foo.length && bar())',
+			output: 'alert(foo.length > 0 && bar())'
+		})
+	]
 });
 
 test.visualize([

test/snapshots/explicit-length-check.js.md

@@ -871,9 +871,12 @@ Generated by [AVA](https://avajs.dev).
     Output:␊
       1 | bar(foo.length === 0 || foo.length)␊
     ␊
-    Error 1/1:␊
+    Error 1/2:␊
     > 1 | bar(!foo.length || foo.length)␊
         |     ^^^^^^^^^^^ Use `.length === 0` when checking length is zero.␊
+    Error 2/2:␊
+    > 1 | bar(!foo.length || foo.length)␊
+        |                    ^^^^^^^^^^ Use `.length > 0` when checking length is not zero.␊
     `
 
 ## explicit-length-check - #14