Add no-array-push-push rule (#1015)

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>

docs/rules/no-array-push-push.md

@@ -0,0 +1,29 @@
+# Enforce combining multiple `Array#push()` into one call
+
+[`Array#push()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push) accepts multiple arguments. Multiple calls should be combined into one.
+
+This rule is partly fixable.
+
+## Fail
+
+```js
+foo.push(1);
+foo.push(2, 3);
+```
+
+## Pass
+
+```js
+foo.push(1, 2, 3);
+```
+
+```js
+const length = foo.push(1);
+foo.push(2);
+```
+
+```js
+foo.push(1);
+bar();
+foo.push(2);
+```

index.js

@@ -55,6 +55,7 @@ module.exports = {
 				'unicorn/new-for-builtins': 'error',
 				'unicorn/no-abusive-eslint-disable': 'error',
 				'unicorn/no-array-callback-reference': 'error',
+				'unicorn/no-array-push-push': 'error',
 				'unicorn/no-array-reduce': 'error',
 				'unicorn/no-console-spaces': 'error',
 				'unicorn/no-for-loop': 'error',

readme.md

@@ -49,6 +49,7 @@ Configure it in `package.json`.
 			"unicorn/new-for-builtins": "error",
 			"unicorn/no-abusive-eslint-disable": "error",
 			"unicorn/no-array-callback-reference": "error",
+			"unicorn/no-array-push-push": "error",
 			"unicorn/no-array-reduce": "error",
 			"unicorn/no-console-spaces": "error",
 			"unicorn/no-for-loop": "error",
@@ -125,6 +126,7 @@ Configure it in `package.json`.
 - [new-for-builtins](docs/rules/new-for-builtins.md) - Enforce the use of `new` for all builtins, except `String`, `Number`, `Boolean`, `Symbol` and `BigInt`. *(partly fixable)*
 - [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-push-push](docs/rules/no-array-push-push.md) - Enforce combining multiple `Array#push()` into one call. *(partly fixable)*
 - [no-array-reduce](docs/rules/no-array-reduce.md) - Disallow `Array#reduce()` and `Array#reduceRight()`.
 - [no-console-spaces](docs/rules/no-console-spaces.md) - Do not use leading/trailing space between `console.log` parameters. *(fixable)*
 - [no-for-loop](docs/rules/no-for-loop.md) - Do not use a `for` loop that can be replaced with a `for-of` loop. *(partly fixable)*

rules/no-array-push-push.js

@@ -0,0 +1,124 @@
+'use strict';
+const {hasSideEffect, isCommaToken, isOpeningParenToken, isSemicolonToken} = require('eslint-utils');
+const getDocumentationUrl = require('./utils/get-documentation-url');
+const methodSelector = require('./utils/method-selector');
+
+const ERROR = 'error';
+const SUGGESTION = 'suggestion';
+const messages = {
+	[ERROR]: 'Do not call `Array#push()` multiple times.',
+	[SUGGESTION]: 'Merge with previous one.'
+};
+
+const arrayPushExpressionStatement = [
+	'ExpressionStatement',
+	methodSelector({
+		name: 'push',
+		property: 'expression'
+	})
+].join('');
+
+const selector = `${arrayPushExpressionStatement} + ${arrayPushExpressionStatement}`;
+
+const getCallExpressionArgumentsText = (node, sourceCode) => {
+	const openingParenthesisToken = sourceCode.getTokenAfter(node.callee, isOpeningParenToken);
+	const closingParenthesisToken = sourceCode.getLastToken(node);
+
+	return sourceCode.text.slice(
+		openingParenthesisToken.range[1],
+		closingParenthesisToken.range[0]
+	);
+};
+
+function getFirstExpression(node, sourceCode) {
+	const {parent} = node;
+	const visitorKeys = sourceCode.visitorKeys[parent.type] || Object.keys(parent);
+
+	for (const property of visitorKeys) {
+		const value = parent[property];
+		if (Array.isArray(value)) {
+			const index = value.indexOf(node);
+
+			if (index !== -1) {
+				return value[index - 1];
+			}
+		}
+	}
+
+	/* istanbul ignore next */
+	throw new Error('Cannot find the first `Array#push()` call.\nPlease open an issue at https://github.com/sindresorhus/eslint-plugin-unicorn/issues/new?title=%60no-array-push-push%60%3A%20Cannot%20find%20first%20%60push()%60');
+}
+
+function create(context) {
+	const sourceCode = context.getSourceCode();
+
+	return {
+		[selector](secondExpression) {
+			const firstExpression = getFirstExpression(secondExpression, sourceCode);
+			const firstCall = firstExpression.expression;
+			const secondCall = secondExpression.expression;
+
+			const firstCallArray = firstCall.callee.object;
+			const secondCallArray = secondCall.callee.object;
+
+			// Not same array
+			if (sourceCode.getText(firstCallArray) !== sourceCode.getText(secondCallArray)) {
+				return;
+			}
+
+			const secondCallArguments = secondCall.arguments;
+			const problem = {
+				node: secondCall.callee.property,
+				messageId: ERROR
+			};
+
+			const fix = function * (fixer) {
+				if (secondCallArguments.length > 0) {
+					const text = getCallExpressionArgumentsText(secondCall, sourceCode);
+
+					const [penultimateToken, lastToken] = sourceCode.getLastTokens(firstCall, 2);
+					yield (isCommaToken(penultimateToken) ? fixer.insertTextAfter(penultimateToken, ` ${text}`) : fixer.insertTextBefore(
+						lastToken,
+						firstCall.arguments.length > 0 ? `, ${text}` : text
+					));
+				}
+
+				const shouldKeepSemicolon = !isSemicolonToken(sourceCode.getLastToken(firstExpression)) &&
+					isSemicolonToken(sourceCode.getLastToken(secondExpression));
+
+				yield fixer.replaceTextRange(
+					[firstExpression.range[1], secondExpression.range[1]],
+					shouldKeepSemicolon ? ';' : ''
+				);
+			};
+
+			if (
+				hasSideEffect(firstCallArray, sourceCode) ||
+				secondCallArguments.some(element => hasSideEffect(element, sourceCode))
+			) {
+				problem.suggest = [
+					{
+						messageId: SUGGESTION,
+						fix
+					}
+				];
+			} else {
+				problem.fix = fix;
+			}
+
+			context.report(problem);
+		}
+	};
+}
+
+module.exports = {
+	create,
+	meta: {
+		type: 'suggestion',
+		docs: {
+			url: getDocumentationUrl(__filename)
+		},
+		fixable: 'code',
+		messages
+	}
+};

rules/prevent-abbreviations.js

@@ -389,8 +389,10 @@ const formatMessage = (discouragedName, replacements, nameTypeText) => {
 			replacementsText += `, ... (${omittedReplacementsCount > 99 ? '99+' : omittedReplacementsCount} more omitted)`;
 		}
 
-		message.push(`Please rename the ${nameTypeText} \`${discouragedName}\`.`);
-		message.push(`Suggested names are: ${replacementsText}.`);
+		message.push(
+			`Please rename the ${nameTypeText} \`${discouragedName}\`.`,
+			`Suggested names are: ${replacementsText}.`
+		);
 	}
 
 	message.push(anotherNameMessage);

rules/utils/method-selector.js

@@ -37,8 +37,10 @@ module.exports = options => {
 	}
 
 	if (object) {
-		selector.push(`[${prefix}callee.object.type="Identifier"]`);
-		selector.push(`[${prefix}callee.object.name="${object}"]`);
+		selector.push(
+			`[${prefix}callee.object.type="Identifier"]`,
+			`[${prefix}callee.object.name="${object}"]`
+		);
 	}
 
 	if (typeof length === 'number') {

scripts/template/test.js.jst

@@ -1,5 +1,5 @@
 import {outdent} from 'outdent';
-import {test} from './utils/test';
+import {test} from './utils/test.js';
 
 const MESSAGE_ID = '<%= id %>';
 const errors = [

test/no-array-push-push.js

@@ -0,0 +1,147 @@
+import {outdent} from 'outdent';
+import {test} from './utils/test.js';
+
+test.visualize({
+	valid: [
+		outdent`
+			foo.forEach(fn);
+			foo.forEach(fn);
+		`,
+		'foo.push(1);',
+		outdent`
+			foo.push(1);
+			foo.unshift(2);
+		`,
+		outdent`
+			foo.push(1);; // <- there is a "EmptyStatement" between
+			foo.push(2);
+		`,
+		// Not same array
+		outdent`
+			foo.push(1);
+			bar.push(2);
+		`,
+		// `.push` selector
+		'foo.push(1);push(2)',
+		'push(1);foo.push(2)',
+		'new foo.push(1);foo.push(2)',
+		'foo.push(1);new foo.push(2)',
+		'foo[push](1);foo.push(2)',
+		'foo.push(1);foo[push](2)',
+		'foo.push(foo.push(1));',
+		// Not `ExpressionStatement`
+		outdent`
+			const length = foo.push(1);
+			foo.push(2);
+		`,
+		outdent`
+			foo.push(1);
+			const length = foo.push(2);
+		`,
+		// We are comparing array with source code
+		outdent`
+			foo.bar.push(1);
+			(foo).bar.push(2);
+		`
+	],
+	invalid: [
+		outdent`
+			foo.push(1);
+			foo.push(2);
+		`,
+		outdent`
+			(foo.push)(1);
+			(foo.push)(2);
+		`,
+		outdent`
+			foo.bar.push(1);
+			foo.bar.push(2);
+		`,
+		outdent`
+			foo.push(1);
+			(foo).push(2);
+		`,
+		outdent`
+			foo.push();
+			foo.push();
+		`,
+		outdent`
+			foo.push(1);
+			foo.push();
+		`,
+		outdent`
+			foo.push();
+			foo.push(2);
+		`,
+		outdent`
+			foo.push(1, 2);
+			foo.push((3), (4));
+		`,
+		outdent`
+			foo.push(1, 2,);
+			foo.push(3, 4);
+		`,
+		outdent`
+			foo.push(1, 2);
+			foo.push(3, 4,);
+		`,
+		outdent`
+			foo.push(1, 2,);
+			foo.push(3, 4,);
+		`,
+		outdent`
+			foo.push(1, 2, ...a,);
+			foo.push(...b,);
+		`,
+		outdent`
+			foo.push(bar());
+			foo.push(1);
+		`,
+		// `arguments` in second push has side effect
+		outdent`
+			foo.push(1);
+			foo.push(bar());
+		`,
+		// Array has side effect
+		outdent`
+			foo().push(1);
+			foo().push(2);
+		`,
+		outdent`
+			foo().bar.push(1);
+			foo().bar.push(2);
+		`,
+		// Multiple
+		outdent`
+			foo.push(1,);
+			foo.push(2,);
+			foo.push(3,);
+		`,
+		// Should be able to find the previous expression
+		outdent`
+			if (a) {
+				foo.push(1);
+				foo.push(2);
+			}
+		`,
+		outdent`
+			switch (a) {
+				default:
+					foo.push(1);
+					foo.push(2);
+			}
+		`,
+		outdent`
+			function a() {
+				foo.push(1);
+				foo.push(2);
+			}
+		`,
+		// ASI
+		outdent`
+			foo.push(1)
+			foo.push(2)
+			;[foo].forEach(bar)
+		`
+	]
+});