no-array-for-each: Handle optional chaining (#1753)

Co-authored-by: fisker Cheung <[email protected]>

docs/rules/no-array-for-each.md

@@ -4,7 +4,7 @@
 <!-- RULE_NOTICE -->
 ✅ *This rule is part of the [recommended](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config) config.*
 
-🔧 *This rule is [auto-fixable](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems).*
+🔧💡 *This rule is [auto-fixable](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) and provides [suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).*
 <!-- /RULE_NOTICE -->
 
 Benefits of [`for…of` statement](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of) over [`Array#forEach(…)`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach) can include:
@@ -21,6 +21,12 @@ array.forEach(element => {
 });
 ```
 
+```js
+array?.forEach(element => {
+	bar(element);
+});
+```
+
 ```js
 array.forEach((element, index) => {
 	bar(element, index);

readme.md

@@ -69,7 +69,7 @@ Each rule has emojis denoting:
 | [new-for-builtins](docs/rules/new-for-builtins.md) | Enforce the use of `new` for all builtins, except `String`, `Number`, `Boolean`, `Symbol` and `BigInt`. | ✅ | 🔧 |  |
 | [no-abusive-eslint-disable](docs/rules/no-abusive-eslint-disable.md) | Enforce specifying rules to disable in `eslint-disable` comments. | ✅ |  |  |
 | [no-array-callback-reference](docs/rules/no-array-callback-reference.md) | Prevent passing a function reference directly to iterator methods. | ✅ |  | 💡 |
-| [no-array-for-each](docs/rules/no-array-for-each.md) | Prefer `for…of` over `Array#forEach(…)`. | ✅ | 🔧 |  |
+| [no-array-for-each](docs/rules/no-array-for-each.md) | Prefer `for…of` over `Array#forEach(…)`. | ✅ | 🔧 | 💡 |
 | [no-array-method-this-argument](docs/rules/no-array-method-this-argument.md) | Disallow using the `this` argument in array methods. | ✅ | 🔧 | 💡 |
 | [no-array-push-push](docs/rules/no-array-push-push.md) | Enforce combining multiple `Array#push()` into one call. | ✅ | 🔧 | 💡 |
 | [no-array-reduce](docs/rules/no-array-reduce.md) | Disallow `Array#reduce()` and `Array#reduceRight()`. | ✅ |  |  |

rules/no-array-for-each.js

@@ -6,20 +6,24 @@ const {
 	isSemicolonToken,
 	isClosingParenToken,
 	findVariable,
+	hasSideEffect,
 } = require('eslint-utils');
 const {methodCallSelector, referenceIdentifierSelector} = require('./selectors/index.js');
 const {extendFixRange} = require('./fix/index.js');
 const needsSemicolon = require('./utils/needs-semicolon.js');
 const shouldAddParenthesesToExpressionStatementExpression = require('./utils/should-add-parentheses-to-expression-statement-expression.js');
+const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object.js');
 const {getParentheses} = require('./utils/parentheses.js');
 const isFunctionSelfUsedInside = require('./utils/is-function-self-used-inside.js');
 const {isNodeMatches} = require('./utils/is-node-matches.js');
 const assertToken = require('./utils/assert-token.js');
 const {fixSpaceAroundKeyword} = require('./fix/index.js');
 
-const MESSAGE_ID = 'no-array-for-each';
+const MESSAGE_ID_ERROR = 'no-array-for-each/error';
+const MESSAGE_ID_SUGGESTION = 'no-array-for-each/suggestion';
 const messages = {
-	[MESSAGE_ID]: 'Use `for…of` instead of `Array#forEach(…)`.',
+	[MESSAGE_ID_ERROR]: 'Use `for…of` instead of `Array#forEach(…)`.',
+	[MESSAGE_ID_SUGGESTION]: 'Switch to `for…of`.',
 };
 
 const arrayForEachCallSelector = methodCallSelector({
@@ -36,6 +40,11 @@ const continueAbleNodeTypes = new Set([
 	'ForInStatement',
 ]);
 
+const stripChainExpression = node =>
+	(node.parent.type === 'ChainExpression' && node.parent.expression === node)
+		? node.parent
+		: node;
+
 function isReturnStatementInContinueAbleNodes(returnStatement, callbackFunction) {
 	for (let node = returnStatement; node && node !== callbackFunction; node = node.parent) {
 		if (continueAbleNodeTypes.has(node.type)) {
@@ -73,6 +82,9 @@ function getFixFunction(callExpression, functionInfo, context) {
 	const parameters = callback.params;
 	const array = callExpression.callee.object;
 	const {returnStatements} = functionInfo.get(callback);
+	const isOptionalArray = callExpression.callee.optional;
+	const expressionStatement = stripChainExpression(callExpression).parent;
+	const arrayText = sourceCode.getText(array);
 
 	const getForOfLoopHeadText = () => {
 		const [elementText, indexText] = parameters.map(parameter => sourceCode.getText(parameter));
@@ -84,12 +96,16 @@ function getFixFunction(callExpression, functionInfo, context) {
 		text += useEntries ? `[${indexText}, ${elementText}]` : elementText;
 		text += ' of ';
 
-		let arrayText = sourceCode.getText(array);
-		if (isParenthesized(array, sourceCode)) {
-			arrayText = `(${arrayText})`;
-		}
+		const shouldAddParenthesesToArray
+			= isParenthesized(array, sourceCode)
+			|| (
+				// `1?.forEach()` -> `(1).entries()`
+				isOptionalArray
+				&& useEntries
+				&& shouldAddParenthesesToMemberExpressionObject(array, sourceCode)
+			);
 
-		text += arrayText;
+		text += shouldAddParenthesesToArray ? `(${arrayText})` : arrayText;
 
 		if (useEntries) {
 			text += '.entries()';
@@ -228,7 +244,7 @@ function getFixFunction(callExpression, functionInfo, context) {
 			yield * replaceReturnStatement(returnStatement, fixer);
 		}
 
-		const expressionStatementLastToken = sourceCode.getLastToken(callExpression.parent);
+		const expressionStatementLastToken = sourceCode.getLastToken(expressionStatement);
 		// Remove semicolon if it's not needed anymore
 		// foo.forEach(bar => {});
 		//                       ^
@@ -238,6 +254,10 @@ function getFixFunction(callExpression, functionInfo, context) {
 
 		yield * fixSpaceAroundKeyword(fixer, callExpression.parent, sourceCode);
 
+		if (isOptionalArray) {
+			yield fixer.insertTextBefore(callExpression, `if (${arrayText}) `);
+		}
+
 		// Prevent possible variable conflicts
 		yield * extendFixRange(fixer, callExpression.parent.range);
 	};
@@ -302,32 +322,20 @@ function isFunctionParameterVariableReassigned(callbackFunction, context) {
 		});
 }
 
-const isExpressionStatement = node => {
-	if (node.type === 'ChainExpression') {
-		node = node.parent;
-	}
-
-	return node.type === 'ExpressionStatement';
-};
-
 function isFixable(callExpression, {scope, functionInfo, allIdentifiers, context}) {
 	const sourceCode = context.getSourceCode();
 	// Check `CallExpression`
 	if (
 		callExpression.optional
+		// TODO: Parenthesized should also be fixable.
 		|| isParenthesized(callExpression, sourceCode)
 		|| callExpression.arguments.length !== 1
 	) {
 		return false;
 	}
 
 	// Check ancestors, we only fix `ExpressionStatement`
-	if (!isExpressionStatement(callExpression.parent)) {
-		return false;
-	}
-
-	// Check `CallExpression.callee`
-	if (callExpression.callee.optional) {
+	if (stripChainExpression(callExpression).parent.type !== 'ExpressionStatement') {
 		return false;
 	}
 
@@ -379,6 +387,7 @@ const create = context => {
 	const callExpressions = [];
 	const allIdentifiers = [];
 	const functionInfo = new Map();
+	const sourceCode = context.getSourceCode();
 
 	return {
 		':function'(node) {
@@ -411,13 +420,31 @@ const create = context => {
 		},
 		* 'Program:exit'() {
 			for (const {node, scope} of callExpressions) {
+				// TODO: Rename this to iteratable
+				const array = node.callee;
+
 				const problem = {
-					node: node.callee.property,
-					messageId: MESSAGE_ID,
+					node: array.property,
+					messageId: MESSAGE_ID_ERROR,
 				};
 
-				if (isFixable(node, {scope, allIdentifiers, functionInfo, context})) {
-					problem.fix = getFixFunction(node, functionInfo, context);
+				if (!isFixable(node, {scope, allIdentifiers, functionInfo, context})) {
+					yield problem;
+					continue;
+				}
+
+				const shouldUseSuggestion = array.optional && hasSideEffect(array, sourceCode);
+				const fix = getFixFunction(node, functionInfo, context);
+
+				if (shouldUseSuggestion) {
+					problem.suggest = [
+						{
+							messageId: MESSAGE_ID_SUGGESTION,
+							fix,
+						},
+					];
+				} else {
+					problem.fix = fix;
 				}
 
 				yield problem;
@@ -435,6 +462,7 @@ module.exports = {
 			description: 'Prefer `for…of` over `Array#forEach(…)`.',
 		},
 		fixable: 'code',
+		hasSuggestions: true,
 		messages,
 	},
 };