Detect unnecessary Promise.resolve/reject in promise callback functions (#1666)

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

Author: cherryblossom000

docs/rules/no-useless-promise-resolve-reject.md

@@ -1,10 +1,10 @@
-# Disallow returning/yielding `Promise.resolve/reject()` in async functions
+# Disallow returning/yielding `Promise.resolve/reject()` in async functions or promise callbacks
 
 ✅ *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).*
 
-Wrapping a return value in `Promise.resolve` in an async function is unnecessary as all return values of an async function are already wrapped in a `Promise`. Similarly, returning an error wrapped in `Promise.reject` is equivalent to simply `throw`ing the error. This is the same for `yield`ing in async generators as well.
+Wrapping a return value in `Promise.resolve` in an async function or a `Promise#then`/`catch`/`finally` callback is unnecessary as all return values in async functions and promise callback functions are already wrapped in a `Promise`. Similarly, returning an error wrapped in `Promise.reject` is equivalent to simply `throw`ing the error. This is the same for `yield`ing in async generators as well.
 
 ## Fail
 
@@ -21,6 +21,18 @@ async function * generator() {
 	yield Promise.resolve(result);
 	yield Promise.reject(error);
 }
+
+promise
+	.then(x => {
+		if (x % 2 == 0) {
+			return Promise.resolve(x / 2);
+		}
+
+		return Promise.reject(new Error('odd number'));
+	});
+	.catch(error => Promise.reject(new FancyError(error)));
+
+promise.finally(() => Promise.reject(new Error('oh no')));
 ```
 
 ## Pass
@@ -38,4 +50,20 @@ async function * generator() {
 	yield result;
 	throw error;
 }
+
+promise
+	.then(x => {
+		if (x % 2 == 0) {
+			return x / 2;
+		}
+
+		throw new Error('odd number');
+	});
+	.catch(error => {
+		throw new FancyError(error);
+	});
+
+promise.finally(() => {
+	throw new Error('oh no');
+});
 ```

readme.md

@@ -196,7 +196,7 @@ Each rule has emojis denoting:
 | [no-unused-properties](docs/rules/no-unused-properties.md) | Disallow unused object properties. |  |  |  |
 | [no-useless-fallback-in-spread](docs/rules/no-useless-fallback-in-spread.md) | Forbid useless fallback when spreading in object literals. | ✅ | 🔧 |  |
 | [no-useless-length-check](docs/rules/no-useless-length-check.md) | Disallow useless array length check. | ✅ | 🔧 |  |
-| [no-useless-promise-resolve-reject](docs/rules/no-useless-promise-resolve-reject.md) | Disallow returning/yielding `Promise.resolve/reject()` in async functions | ✅ | 🔧 |  |
+| [no-useless-promise-resolve-reject](docs/rules/no-useless-promise-resolve-reject.md) | Disallow returning/yielding `Promise.resolve/reject()` in async functions or promise callbacks | ✅ | 🔧 |  |
 | [no-useless-spread](docs/rules/no-useless-spread.md) | Disallow unnecessary spread. | ✅ | 🔧 |  |
 | [no-useless-undefined](docs/rules/no-useless-undefined.md) | Disallow useless `undefined`. | ✅ | 🔧 |  |
 | [no-zero-fractions](docs/rules/no-zero-fractions.md) | Disallow number literals with zero fractions or dangling dots. | ✅ | 🔧 |  |

rules/no-useless-promise-resolve-reject.js

@@ -15,9 +15,9 @@ const selector = [
 		methods: ['resolve', 'reject'],
 	}),
 	matches([
-		'ArrowFunctionExpression[async=true] > .body',
+		'ArrowFunctionExpression > .body',
 		'ReturnStatement > .argument',
-		'YieldExpression[delegate=false] > .argument',
+		'YieldExpression[delegate!=true] > .argument',
 	]),
 ].join('');
 
@@ -46,6 +46,42 @@ function getFunctionNode(node) {
 	};
 }
 
+function isPromiseCallback(node) {
+	if (
+		node.parent.type === 'CallExpression'
+		&& node.parent.callee.type === 'MemberExpression'
+		&& !node.parent.callee.computed
+		&& node.parent.callee.property.type === 'Identifier'
+	) {
+		const {callee: {property}, arguments: arguments_} = node.parent;
+
+		if (
+			arguments_.length === 1
+			&& (
+				property.name === 'then'
+				|| property.name === 'catch'
+				|| property.name === 'finally'
+			)
+			&& arguments_[0] === node
+		) {
+			return true;
+		}
+
+		if (
+			arguments_.length === 2
+			&& property.name === 'then'
+			&& (
+				arguments_[0] === node
+				|| (arguments_[0].type !== 'SpreadElement' && arguments_[1] === node)
+			)
+		) {
+			return true;
+		}
+	}
+
+	return false;
+}
+
 function createProblem(callExpression, fix) {
 	const {callee, parent} = callExpression;
 	const method = callee.property.name;
@@ -142,7 +178,7 @@ const create = context => {
 	return {
 		[selector](callExpression) {
 			const {functionNode, isInTryStatement} = getFunctionNode(callExpression);
-			if (!functionNode || !functionNode.async) {
+			if (!functionNode || !(functionNode.async || isPromiseCallback(functionNode))) {
 				return;
 			}
 
@@ -162,7 +198,7 @@ module.exports = {
 	meta: {
 		type: 'suggestion',
 		docs: {
-			description: 'Disallow returning/yielding `Promise.resolve/reject()` in async functions',
+			description: 'Disallow returning/yielding `Promise.resolve/reject()` in async functions or promise callbacks',
 		},
 		fixable: 'code',
 		schema,

test/no-useless-promise-resolve-reject.mjs

@@ -77,6 +77,13 @@ test({
 				yield* Promise.${fn}(bar);
 			}
 		`),
+		// Promise#then/catch/finally
+		'promise.then(() => foo).catch(() => bar).finally(() => baz)',
+		'promise.then(() => foo, () => bar).finally(() => baz)',
+		'promise.then(x, y, () => Promise.resolve(foo))',
+		'promise.catch(x, () => Promise.resolve(foo))',
+		'promise.finally(x, () => Promise.resolve(foo))',
+		'promise[then](() => Promise.resolve(foo))',
 	],
 	invalid: [
 		{
@@ -469,5 +476,53 @@ test({
 			`,
 			errors: [yieldRejectError],
 		})),
+		// Promise#then/catch/finally callbacks returning Promise.resolve/reject
+		...['then', 'catch', 'finally'].flatMap(fn => [
+			{
+				code: `promise.${fn}(() => Promise.resolve(bar))`,
+				errors: [returnResolveError],
+				output: `promise.${fn}(() => bar)`,
+			},
+			{
+				code: `promise.${fn}(() => { return Promise.resolve(bar); })`,
+				errors: [returnResolveError],
+				output: `promise.${fn}(() => { return bar; })`,
+			},
+			{
+				code: `promise.${fn}(async () => Promise.reject(bar))`,
+				errors: [returnRejectError],
+				output: `promise.${fn}(async () => { throw bar; })`,
+			},
+			{
+				code: `promise.${fn}(async () => { return Promise.reject(bar); })`,
+				errors: [returnRejectError],
+				output: `promise.${fn}(async () => { throw bar; })`,
+			},
+		]),
+		{
+			code: 'promise.then(() => {}, () => Promise.resolve(bar))',
+			errors: [returnResolveError],
+			output: 'promise.then(() => {}, () => bar)',
+		},
+		{
+			code: 'promise.then(() => Promise.resolve(bar), () => Promise.resolve(baz))',
+			errors: [returnResolveError, returnResolveError],
+			output: 'promise.then(() => bar, () => baz)',
+		},
+		{
+			code: 'promise.then(() => {}, () => { return Promise.resolve(bar); })',
+			errors: [returnResolveError],
+			output: 'promise.then(() => {}, () => { return bar; })',
+		},
+		{
+			code: 'promise.then(() => {}, async () => Promise.reject(bar))',
+			errors: [returnRejectError],
+			output: 'promise.then(() => {}, async () => { throw bar; })',
+		},
+		{
+			code: 'promise.then(() => {}, async () => { return Promise.reject(bar); })',
+			errors: [returnRejectError],
+			output: 'promise.then(() => {}, async () => { throw bar; })',
+		},
 	],
 });