Add no-single-promise-in-promise-methods rule (#2258)

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

configs/recommended.js

@@ -40,6 +40,7 @@ module.exports = {
 	'unicorn/no-null': 'error',
 	'unicorn/no-object-as-default-parameter': 'error',
 	'unicorn/no-process-exit': 'error',
+	'unicorn/no-single-promise-in-promise-methods': 'error',
 	'unicorn/no-static-only-class': 'error',
 	'unicorn/no-thenable': 'error',
 	'unicorn/no-this-assignment': 'error',

docs/rules/no-single-promise-in-promise-methods.md

@@ -0,0 +1,50 @@
+# Disallow passing single-element arrays to `Promise` methods
+
+💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs).
+
+🔧💡 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).
+
+<!-- end auto-generated rule header -->
+<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->
+
+Passing a single-element array to `Promise.all()`, `Promise.any()`, or `Promise.race()` is likely a mistake.
+
+## Fail
+
+```js
+const foo = await Promise.all([promise]);
+```
+
+```js
+const foo = await Promise.any([promise]);
+```
+
+```js
+const foo = await Promise.race([promise]);
+```
+
+```js
+const promise = Promise.all([nonPromise]);
+```
+
+## Pass
+
+```js
+const foo = await promise;
+```
+
+```js
+const promise = Promise.resolve(nonPromise);
+```
+
+```js
+const foo = await Promise.all(promises);
+```
+
+```js
+const foo = await Promise.any([promise, anotherPromise]);
+```
+
+```js
+const [{value: foo, reason: error}] = await Promise.allSettled([promise]);
+```

readme.md

@@ -148,6 +148,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c
 | [no-null](docs/rules/no-null.md)                                                                 | Disallow the use of the `null` literal.                                                                                                                                                                           | ✅  | 🔧 | 💡 |
 | [no-object-as-default-parameter](docs/rules/no-object-as-default-parameter.md)                   | Disallow the use of objects as default parameters.                                                                                                                                                                | ✅  |    |    |
 | [no-process-exit](docs/rules/no-process-exit.md)                                                 | Disallow `process.exit()`.                                                                                                                                                                                        | ✅  |    |    |
+| [no-single-promise-in-promise-methods](docs/rules/no-single-promise-in-promise-methods.md)       | Disallow passing single-element arrays to `Promise` methods.                                                                                                                                                      | ✅  | 🔧 | 💡 |
 | [no-static-only-class](docs/rules/no-static-only-class.md)                                       | Disallow classes that only have static members.                                                                                                                                                                   | ✅  | 🔧 |    |
 | [no-thenable](docs/rules/no-thenable.md)                                                         | Disallow `then` property.                                                                                                                                                                                         | ✅  |    |    |
 | [no-this-assignment](docs/rules/no-this-assignment.md)                                           | Disallow assigning `this` to a variable.                                                                                                                                                                          | ✅  |    |    |

rules/no-single-promise-in-promise-methods.js

@@ -0,0 +1,167 @@
+'use strict';
+const {
+	isCommaToken,
+} = require('@eslint-community/eslint-utils');
+const {isMethodCall} = require('./ast/index.js');
+const {
+	getParenthesizedText,
+	isParenthesized,
+	needsSemicolon,
+	shouldAddParenthesesToAwaitExpressionArgument,
+} = require('./utils/index.js');
+
+const MESSAGE_ID_ERROR = 'no-single-promise-in-promise-methods/error';
+const MESSAGE_ID_SUGGESTION_UNWRAP = 'no-single-promise-in-promise-methods/unwrap';
+const MESSAGE_ID_SUGGESTION_SWITCH_TO_PROMISE_RESOLVE = 'no-single-promise-in-promise-methods/use-promise-resolve';
+const messages = {
+	[MESSAGE_ID_ERROR]: 'Wrapping single-element array with `Promise.{{method}}()` is unnecessary.',
+	[MESSAGE_ID_SUGGESTION_UNWRAP]: 'Use the value directly.',
+	[MESSAGE_ID_SUGGESTION_SWITCH_TO_PROMISE_RESOLVE]: 'Switch to `Promise.resolve(…)`.',
+};
+const METHODS = ['all', 'any', 'race'];
+
+const isPromiseMethodCallWithSingleElementArray = node =>
+	isMethodCall(node, {
+		object: 'Promise',
+		methods: METHODS,
+		optionalMember: false,
+		optionalCall: false,
+		argumentsLength: 1,
+	})
+	&& node.arguments[0].type === 'ArrayExpression'
+	&& node.arguments[0].elements.length === 1
+	&& node.arguments[0].elements[0]
+	&& node.arguments[0].elements[0].type !== 'SpreadElement';
+
+const unwrapAwaitedCallExpression = (callExpression, sourceCode) => fixer => {
+	const [promiseNode] = callExpression.arguments[0].elements;
+	let text = getParenthesizedText(promiseNode, sourceCode);
+
+	if (
+		!isParenthesized(promiseNode, sourceCode)
+		&& shouldAddParenthesesToAwaitExpressionArgument(promiseNode)
+	) {
+		text = `(${text})`;
+	}
+
+	// The next node is already behind a `CallExpression`, there should be no ASI problem
+
+	return fixer.replaceText(callExpression, text);
+};
+
+const unwrapNonAwaitedCallExpression = (callExpression, sourceCode) => fixer => {
+	const [promiseNode] = callExpression.arguments[0].elements;
+	let text = getParenthesizedText(promiseNode, sourceCode);
+
+	if (
+		!isParenthesized(promiseNode, sourceCode)
+		// Since the original call expression can be anywhere, it's hard to tell if the promise
+		// need to be parenthesized, but it's safe to add parentheses
+		&& !(
+			// Known cases that not need parentheses
+			promiseNode.type === 'Identifier'
+			|| promiseNode.type === 'MemberExpression'
+		)
+	) {
+		text = `(${text})`;
+	}
+
+	const previousToken = sourceCode.getTokenBefore(callExpression);
+	if (needsSemicolon(previousToken, sourceCode, text)) {
+		text = `;${text}`;
+	}
+
+	return fixer.replaceText(callExpression, text);
+};
+
+const switchToPromiseResolve = (callExpression, sourceCode) => function * (fixer) {
+	/*
+	```
+	Promise.all([promise,])
+	//      ^^^ methodNameNode
+	```
+	*/
+	const methodNameNode = callExpression.callee.property;
+	yield fixer.replaceText(methodNameNode, 'resolve');
+
+	const [arrayExpression] = callExpression.arguments;
+	/*
+	```
+	Promise.all([promise,])
+	//          ^ openingBracketToken
+	```
+	*/
+	const openingBracketToken = sourceCode.getFirstToken(arrayExpression);
+	/*
+	```
+	Promise.all([promise,])
+	//                  ^ penultimateToken
+	//                   ^ closingBracketToken
+	```
+	*/
+	const [
+		penultimateToken,
+		closingBracketToken,
+	] = sourceCode.getLastTokens(arrayExpression, 2);
+
+	yield fixer.remove(openingBracketToken);
+	yield fixer.remove(closingBracketToken);
+
+	if (isCommaToken(penultimateToken)) {
+		yield fixer.remove(penultimateToken);
+	}
+};
+
+/** @param {import('eslint').Rule.RuleContext} context */
+const create = context => ({
+	CallExpression(callExpression) {
+		if (!isPromiseMethodCallWithSingleElementArray(callExpression)) {
+			return;
+		}
+
+		const problem = {
+			node: callExpression.arguments[0],
+			messageId: MESSAGE_ID_ERROR,
+			data: {
+				method: callExpression.callee.property.name,
+			},
+		};
+
+		const {sourceCode} = context;
+
+		if (
+			callExpression.parent.type === 'AwaitExpression'
+			&& callExpression.parent.argument === callExpression
+		) {
+			problem.fix = unwrapAwaitedCallExpression(callExpression, sourceCode);
+			return problem;
+		}
+
+		problem.suggest = [
+			{
+				messageId: MESSAGE_ID_SUGGESTION_UNWRAP,
+				fix: unwrapNonAwaitedCallExpression(callExpression, sourceCode),
+			},
+			{
+				messageId: MESSAGE_ID_SUGGESTION_SWITCH_TO_PROMISE_RESOLVE,
+				fix: switchToPromiseResolve(callExpression, sourceCode),
+			},
+		];
+
+		return problem;
+	},
+});
+
+/** @type {import('eslint').Rule.RuleModule} */
+module.exports = {
+	create,
+	meta: {
+		type: 'suggestion',
+		docs: {
+			description: 'Disallow passing single-element arrays to `Promise` methods.',
+		},
+		fixable: 'code',
+		hasSuggestions: true,
+		messages,
+	},
+};

rules/utils/index.js

@@ -45,6 +45,7 @@ module.exports = {
 	isValueNotUsable: require('./is-value-not-usable.js'),
 	needsSemicolon: require('./needs-semicolon.js'),
 	shouldAddParenthesesToMemberExpressionObject: require('./should-add-parentheses-to-member-expression-object.js'),
+	shouldAddParenthesesToAwaitExpressionArgument: require('./should-add-parentheses-to-await-expression-argument.js'),
 	singular: require('./singular.js'),
 	toLocation: require('./to-location.js'),
 	getAncestor: require('./get-ancestor.js'),

rules/utils/should-add-parentheses-to-await-expression-argument.js

@@ -0,0 +1,21 @@
+'use strict';
+
+/**
+Check if parentheses should be added to a `node` when it's used as `argument` of `AwaitExpression`.
+
+@param {Node} node - The AST node to check.
+@returns {boolean}
+*/
+function shouldAddParenthesesToAwaitExpressionArgument(node) {
+	return (
+		node.type === 'SequenceExpression'
+		|| node.type === 'YieldExpression'
+		|| node.type === 'ArrowFunctionExpression'
+		|| node.type === 'ConditionalExpression'
+		|| node.type === 'AssignmentExpression'
+		|| node.type === 'LogicalExpression'
+		|| node.type === 'BinaryExpression'
+	);
+}
+
+module.exports = shouldAddParenthesesToAwaitExpressionArgument;

test/no-single-promise-in-promise-methods.mjs

@@ -0,0 +1,104 @@
+import outdent from 'outdent';
+import {getTester} from './utils/test.mjs';
+
+const {test} = getTester(import.meta);
+
+// `await`ed
+test.snapshot({
+	valid: [
+	],
+	invalid: [
+		'await Promise.all([(0, promise)])',
+		'async function * foo() {await Promise.all([yield promise])}',
+		'async function * foo() {await Promise.all([yield* promise])}',
+		'await Promise.all([() => promise,],)',
+		'await Promise.all([a ? b : c,],)',
+		'await Promise.all([x ??= y,],)',
+		'await Promise.all([x ||= y,],)',
+		'await Promise.all([x &&= y,],)',
+		'await Promise.all([x |= y,],)',
+		'await Promise.all([x ^= y,],)',
+		'await Promise.all([x ??= y,],)',
+		'await Promise.all([x ||= y,],)',
+		'await Promise.all([x &&= y,],)',
+		'await Promise.all([x | y,],)',
+		'await Promise.all([x ^ y,],)',
+		'await Promise.all([x & y,],)',
+		'await Promise.all([x !== y,],)',
+		'await Promise.all([x == y,],)',
+		'await Promise.all([x in y,],)',
+		'await Promise.all([x >>> y,],)',
+		'await Promise.all([x + y,],)',
+		'await Promise.all([x / y,],)',
+		'await Promise.all([x ** y,],)',
+		'await Promise.all([promise,],)',
+		'await Promise.all([getPromise(),],)',
+		'await Promise.all([promises[0],],)',
+		'await Promise.all([await promise])',
+		'await Promise.any([promise])',
+		'await Promise.race([promise])',
+		'await Promise.all([new Promise(() => {})])',
+		'+await Promise.all([+1])',
+
+		// ASI, `Promise.all()` is not really `await`ed
+		outdent`
+			await Promise.all([(x,y)])
+			[0].toString()
+		`,
+	],
+});
+
+// Not `await`ed
+test.snapshot({
+	valid: [
+		'Promise.all([promise, anotherPromise])',
+		'Promise.all(notArrayLiteral)',
+		'Promise.all([...promises])',
+		'Promise.any([promise, anotherPromise])',
+		'Promise.race([promise, anotherPromise])',
+		'Promise.notListedMethod([promise])',
+		'Promise[all]([promise])',
+		'Promise.all([,])',
+		'NotPromise.all([promise])',
+		'Promise?.all([promise])',
+		'Promise.all?.([promise])',
+		'Promise.all(...[promise])',
+		'Promise.all([promise], extraArguments)',
+		'Promise.all()',
+		'new Promise.all([promise])',
+
+		// We are not checking these cases
+		'globalThis.Promise.all([promise])',
+		'Promise["all"]([promise])',
+
+		// This can't be checked
+		'Promise.allSettled([promise])',
+	],
+	invalid: [
+		'Promise.all([promise,],)',
+		outdent`
+			foo
+			Promise.all([(0, promise),],)
+		`,
+		outdent`
+			foo
+			Promise.all([[array][0],],)
+		`,
+		'Promise.all([promise]).then()',
+		'Promise.all([1]).then()',
+		'Promise.all([1.]).then()',
+		'Promise.all([.1]).then()',
+		'Promise.all([(0, promise)]).then()',
+		'const _ = () => Promise.all([ a ?? b ,],)',
+		'Promise.all([ {a} = 1 ,],)',
+		'Promise.all([ function () {} ,],)',
+		'Promise.all([ class {} ,],)',
+		'Promise.all([ new Foo ,],).then()',
+		'Promise.all([ new Foo ,],).toString',
+		'foo(Promise.all([promise]))',
+		'Promise.all([promise]).foo = 1',
+		'Promise.all([promise])[0] ||= 1',
+		'Promise.all([undefined]).then()',
+		'Promise.all([null]).then()',
+	],
+});