diff --git a/docs/src/rules/no-promise-executor-return.md b/docs/src/rules/no-promise-executor-return.md index 28891624822..d82e4473477 100644 --- a/docs/src/rules/no-promise-executor-return.md +++ b/docs/src/rules/no-promise-executor-return.md @@ -63,6 +63,8 @@ new Promise((resolve, reject) => getSomething((err, data) => { new Promise(() => { return 1; }); + +new Promise(r => r(1)); ``` ::: @@ -74,6 +76,7 @@ Examples of **correct** code for this rule: ```js /*eslint no-promise-executor-return: "error"*/ +// Turn return inline into two lines new Promise((resolve, reject) => { if (someCondition) { resolve(defaultResult); @@ -88,6 +91,7 @@ new Promise((resolve, reject) => { }); }); +// Add curly braces new Promise((resolve, reject) => { getSomething((err, data) => { if (err) { @@ -98,7 +102,50 @@ new Promise((resolve, reject) => { }); }); +new Promise(r => { r(1) }); +// or just use Promise.resolve Promise.resolve(1); ``` ::: + +## Options + +This rule takes one option, an object, with the following properties: + +* `allowVoid`: If set to `true` (`false` by default), this rule will allow returning void values. + +### allowVoid + +Examples of **correct** code for this rule with the `{ "allowVoid": true }` option: + +::: correct + +```js +/*eslint no-promise-executor-return: ["error", { allowVoid: true }]*/ + +new Promise((resolve, reject) => { + if (someCondition) { + return void resolve(defaultResult); + } + getSomething((err, result) => { + if (err) { + reject(err); + } else { + resolve(result); + } + }); +}); + +new Promise((resolve, reject) => void getSomething((err, data) => { + if (err) { + reject(err); + } else { + resolve(data); + } +})); + +new Promise(r => void r(1)); +``` + +::: diff --git a/lib/rules/no-promise-executor-return.js b/lib/rules/no-promise-executor-return.js index d46a730e474..e6ed7a22efc 100644 --- a/lib/rules/no-promise-executor-return.js +++ b/lib/rules/no-promise-executor-return.js @@ -10,6 +10,7 @@ //------------------------------------------------------------------------------ const { findVariable } = require("@eslint-community/eslint-utils"); +const astUtils = require("./utils/ast-utils"); //------------------------------------------------------------------------------ // Helpers @@ -59,6 +60,78 @@ function isPromiseExecutor(node, scope) { isGlobalReference(parent.callee, getOuterScope(scope)); } +/** + * Checks if the given node is a void expression. + * @param {ASTNode} node The node to check. + * @returns {boolean} - `true` if the node is a void expression + */ +function expressionIsVoid(node) { + return node.type === "UnaryExpression" && node.operator === "void"; +} + +/** + * Fixes the linting error by prepending "void " to the given node + * @param {Object} sourceCode context given by context.sourceCode + * @param {ASTNode} node The node to fix. + * @param {Object} fixer The fixer object provided by ESLint. + * @returns {Array} - An array of fix objects to apply to the node. + */ +function voidPrependFixer(sourceCode, node, fixer) { + + const requiresParens = + + // prepending `void ` will fail if the node has a lower precedence than void + astUtils.getPrecedence(node) < astUtils.getPrecedence({ type: "UnaryExpression", operator: "void" }) && + + // check if there are parentheses around the node to avoid redundant parentheses + !astUtils.isParenthesised(sourceCode, node); + + // avoid parentheses issues + const returnOrArrowToken = sourceCode.getTokenBefore( + node, + node.parent.type === "ArrowFunctionExpression" + ? astUtils.isArrowToken + + // isReturnToken + : token => token.type === "Keyword" && token.value === "return" + ); + + const firstToken = sourceCode.getTokenAfter(returnOrArrowToken); + + const prependSpace = + + // is return token, as => allows void to be adjacent + returnOrArrowToken.value === "return" && + + // If two tokens (return and "(") are adjacent + returnOrArrowToken.range[1] === firstToken.range[0]; + + return [ + fixer.insertTextBefore(firstToken, `${prependSpace ? " " : ""}void ${requiresParens ? "(" : ""}`), + fixer.insertTextAfter(node, requiresParens ? ")" : "") + ]; +} + +/** + * Fixes the linting error by `wrapping {}` around the given node's body. + * @param {Object} sourceCode context given by context.sourceCode + * @param {ASTNode} node The node to fix. + * @param {Object} fixer The fixer object provided by ESLint. + * @returns {Array} - An array of fix objects to apply to the node. + */ +function curlyWrapFixer(sourceCode, node, fixer) { + + // https://github.com/eslint/eslint/pull/17282#issuecomment-1592795923 + const arrowToken = sourceCode.getTokenBefore(node.body, astUtils.isArrowToken); + const firstToken = sourceCode.getTokenAfter(arrowToken); + const lastToken = sourceCode.getLastToken(node); + + return [ + fixer.insertTextBefore(firstToken, "{"), + fixer.insertTextAfter(lastToken, "}") + ]; +} + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -74,10 +147,27 @@ module.exports = { url: "https://eslint.org/docs/latest/rules/no-promise-executor-return" }, - schema: [], + hasSuggestions: true, + + schema: [{ + type: "object", + properties: { + allowVoid: { + type: "boolean", + default: false + } + }, + additionalProperties: false + }], messages: { - returnsValue: "Return values from promise executor functions cannot be read." + returnsValue: "Return values from promise executor functions cannot be read.", + + // arrow and function suggestions + prependVoid: "Prepend `void` to the expression.", + + // only arrow suggestions + wrapBraces: "Wrap the expression in `{}`." } }, @@ -85,26 +175,52 @@ module.exports = { let funcInfo = null; const sourceCode = context.sourceCode; - - /** - * Reports the given node. - * @param {ASTNode} node Node to report. - * @returns {void} - */ - function report(node) { - context.report({ node, messageId: "returnsValue" }); - } + const { + allowVoid = false + } = context.options[0] || {}; return { onCodePathStart(_, node) { funcInfo = { upper: funcInfo, - shouldCheck: functionTypesToCheck.has(node.type) && isPromiseExecutor(node, sourceCode.getScope(node)) + shouldCheck: + functionTypesToCheck.has(node.type) && + isPromiseExecutor(node, sourceCode.getScope(node)) }; - if (funcInfo.shouldCheck && node.type === "ArrowFunctionExpression" && node.expression) { - report(node.body); + if (// Is a Promise executor + funcInfo.shouldCheck && + node.type === "ArrowFunctionExpression" && + node.expression && + + // Except void + !(allowVoid && expressionIsVoid(node.body)) + ) { + const suggest = []; + + // prevent useless refactors + if (allowVoid) { + suggest.push({ + messageId: "prependVoid", + fix(fixer) { + return voidPrependFixer(sourceCode, node.body, fixer); + } + }); + } + + suggest.push({ + messageId: "wrapBraces", + fix(fixer) { + return curlyWrapFixer(sourceCode, node, fixer); + } + }); + + context.report({ + node: node.body, + messageId: "returnsValue", + suggest + }); } }, @@ -113,9 +229,31 @@ module.exports = { }, ReturnStatement(node) { - if (funcInfo.shouldCheck && node.argument) { - report(node); + if (!(funcInfo.shouldCheck && node.argument)) { + return; } + + // node is `return ` + if (!allowVoid) { + context.report({ node, messageId: "returnsValue" }); + return; + } + + if (expressionIsVoid(node.argument)) { + return; + } + + // allowVoid && !expressionIsVoid + context.report({ + node, + messageId: "returnsValue", + suggest: [{ + messageId: "prependVoid", + fix(fixer) { + return voidPrependFixer(sourceCode, node.argument, fixer); + } + }] + }); } }; } diff --git a/tests/lib/rules/no-promise-executor-return.js b/tests/lib/rules/no-promise-executor-return.js index a24629b6a44..4054edc7244 100644 --- a/tests/lib/rules/no-promise-executor-return.js +++ b/tests/lib/rules/no-promise-executor-return.js @@ -22,7 +22,7 @@ const { RuleTester } = require("../../../lib/rule-tester"); * @param {string} [type="ReturnStatement"] Reported node type. * @returns {Object} The error object. */ -function error(column, type = "ReturnStatement") { +function eReturnsValue(column, type = "ReturnStatement") { const errorObject = { messageId: "returnsValue", type @@ -35,6 +35,26 @@ function error(column, type = "ReturnStatement") { return errorObject; } + +/** + * Creates invalid object + * @param {Object} [properties] suggestion properties + * @param {string} [properties.code] code + * @param {number} [properties.options] rule options + * @param {string[]} [fixes] Code suggestions + * @returns {Object} The invalid object. + */ +function suggestion(properties, fixes = []) { + return { + ...properties, + errors: [{ + suggestions: fixes.map(fix => ({ + output: fix + })) + }] + }; +} + //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ @@ -149,7 +169,37 @@ ruleTester.run("no-promise-executor-return", rule, { { code: "new Promise(function (resolve, reject) {}); return 1;", env: { node: true } - } + }, + + /* + * allowVoid: true + * `=> void` and `return void` are allowed + */ + { + code: "new Promise((r) => void cbf(r));", + options: [{ + allowVoid: true + }] + }, + { + code: "new Promise(r => void 0)", + options: [{ + allowVoid: true + }] + }, + { + code: "new Promise(r => { return void 0 })", + options: [{ + allowVoid: true + }] + }, + { + code: "new Promise(r => { if (foo) { return void 0 } return void 0 })", + options: [{ + allowVoid: true + }] + }, + "new Promise(r => {0})" ], invalid: [ @@ -161,143 +211,376 @@ ruleTester.run("no-promise-executor-return", rule, { }, { code: "new Promise((resolve, reject) => resolve(1))", - errors: [{ message: "Return values from promise executor functions cannot be read.", type: "CallExpression", column: 34, endColumn: 44 }] + options: [{ + allowVoid: true + }], + errors: [{ + message: "Return values from promise executor functions cannot be read.", + type: "CallExpression", + column: 34, + endColumn: 44, + suggestions: [ + { output: "new Promise((resolve, reject) => void resolve(1))" }, + { output: "new Promise((resolve, reject) => {resolve(1)})" } + ] + }] + }, + { + code: "new Promise((resolve, reject) => { return 1 })", + options: [{ + allowVoid: true + }], + errors: [{ + message: "Return values from promise executor functions cannot be read.", + type: "ReturnStatement", + column: 36, + endColumn: 44, + suggestions: [ + { output: "new Promise((resolve, reject) => { return void 1 })" } + ] + }] }, + // suggestions arrow function expression + suggestion({ + code: "new Promise(r => 1)", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void 1)", + "new Promise(r => {1})" + ]), + suggestion({ + code: "new Promise(r => 1 ? 2 : 3)", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void (1 ? 2 : 3))", + "new Promise(r => {1 ? 2 : 3})" + ]), + suggestion({ + code: "new Promise(r => (1 ? 2 : 3))", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void (1 ? 2 : 3))", + "new Promise(r => {(1 ? 2 : 3)})" + ]), + suggestion({ + code: + "new Promise(r => (1))", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void (1))", + "new Promise(r => {(1)})" + ]), + suggestion({ + code: + "new Promise(r => () => {})", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void (() => {}))", + "new Promise(r => {() => {}})" + ]), + + // primitives + suggestion({ + code: + "new Promise(r => null)", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void null)", + "new Promise(r => {null})" + ]), + suggestion({ + code: + "new Promise(r => null)", + options: [{ + allowVoid: false + }] + }, [ + "new Promise(r => {null})" + ]), + + // inline comments + suggestion({ + code: + "new Promise(r => /*hi*/ ~0)", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => /*hi*/ void ~0)", + "new Promise(r => /*hi*/ {~0})" + ]), + suggestion({ + code: + "new Promise(r => /*hi*/ ~0)", + options: [{ + allowVoid: false + }] + }, [ + "new Promise(r => /*hi*/ {~0})" + ]), + + // suggestions function + suggestion({ + code: + "new Promise(r => { return 0 })", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => { return void 0 })" + ]), + suggestion({ + code: + "new Promise(r => { return 0 })", + options: [{ + allowVoid: false + }] + }), + + // multiple returns + suggestion({ + code: + "new Promise(r => { if (foo) { return void 0 } return 0 })", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => { if (foo) { return void 0 } return void 0 })" + ]), + + // return assignment + suggestion({ + code: "new Promise(resolve => { return (foo = resolve(1)); })", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(resolve => { return void (foo = resolve(1)); })" + ]), + suggestion({ + code: "new Promise(resolve => r = resolve)", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(resolve => void (r = resolve))", + "new Promise(resolve => {r = resolve})" + ]), + + // return (range check) + suggestion({ + code: + "new Promise(r => { return(1) })", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => { return void (1) })" + ]), + suggestion({ + code: + "new Promise(r =>1)", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r =>void 1)", + "new Promise(r =>{1})" + ]), + + // snapshot + suggestion({ + code: + "new Promise(r => ((1)))", + options: [{ + allowVoid: true + }] + }, [ + "new Promise(r => void ((1)))", + "new Promise(r => {((1))})" + ]), + // other basic tests { code: "new Promise(function foo(resolve, reject) { return 1; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise((resolve, reject) => { return 1; })", - errors: [error()] + errors: [eReturnsValue()] }, // any returned value { code: "new Promise(function (resolve, reject) { return undefined; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise((resolve, reject) => { return null; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise(function (resolve, reject) { return false; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise((resolve, reject) => resolve)", - errors: [error(34, "Identifier")] + errors: [eReturnsValue(34, "Identifier")] }, { code: "new Promise((resolve, reject) => null)", - errors: [error(34, "Literal")] + errors: [eReturnsValue(34, "Literal")] }, { code: "new Promise(function (resolve, reject) { return resolve(foo); })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise((resolve, reject) => { return reject(foo); })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise((resolve, reject) => x + y)", - errors: [error(34, "BinaryExpression")] + errors: [eReturnsValue(34, "BinaryExpression")] }, { code: "new Promise((resolve, reject) => { return Promise.resolve(42); })", - errors: [error()] + errors: [eReturnsValue()] }, // any return statement location { code: "new Promise(function (resolve, reject) { if (foo) { return 1; } })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise((resolve, reject) => { try { return 1; } catch(e) {} })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise(function (resolve, reject) { while (foo){ if (bar) break; else return 1; } })", - errors: [error()] + errors: [eReturnsValue()] + }, + + // `return void` is not allowed without `allowVoid: true` + { + code: "new Promise(() => { return void 1; })", + errors: [eReturnsValue()] + }, + + { + code: "new Promise(() => (1))", + errors: [eReturnsValue(20, "Literal")] + }, + { + code: "() => new Promise(() => ({}));", + errors: [eReturnsValue(26, "ObjectExpression")] }, // absence of arguments has no effect { code: "new Promise(function () { return 1; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise(() => { return 1; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise(() => 1)", - errors: [error(19, "Literal")] + errors: [eReturnsValue(19, "Literal")] }, // various scope tracking tests { code: "function foo() {} new Promise(function () { return 1; });", - errors: [error(45)] + errors: [eReturnsValue(45)] }, { code: "function foo() { return; } new Promise(() => { return 1; });", - errors: [error(48)] + errors: [eReturnsValue(48)] }, { code: "function foo() { return 1; } new Promise(() => { return 2; });", - errors: [error(50)] + errors: [eReturnsValue(50)] }, { code: "function foo () { return new Promise(function () { return 1; }); }", - errors: [error(52)] + errors: [eReturnsValue(52)] }, { code: "function foo() { return new Promise(() => { bar(() => { return 1; }); return false; }); }", - errors: [error(71)] + errors: [eReturnsValue(71)] }, { code: "() => new Promise(() => { if (foo) { return 0; } else bar(() => { return 1; }); })", - errors: [error(38)] + errors: [eReturnsValue(38)] }, { code: "function foo () { return 1; return new Promise(function () { return 2; }); return 3;}", - errors: [error(62)] + errors: [eReturnsValue(62)] }, { code: "() => 1; new Promise(() => { return 1; })", - errors: [error(30)] + errors: [eReturnsValue(30)] }, { code: "new Promise(function () { return 1; }); function foo() { return 1; } ", - errors: [error(27)] + errors: [eReturnsValue(27)] }, { code: "() => new Promise(() => { return 1; });", - errors: [error(27)] + errors: [eReturnsValue(27)] }, { code: "() => new Promise(() => 1);", - errors: [error(25, "Literal")] + errors: [eReturnsValue(25, "Literal")] }, { code: "() => new Promise(() => () => 1);", - errors: [error(25, "ArrowFunctionExpression")] + errors: [eReturnsValue(25, "ArrowFunctionExpression")] + }, + { + code: "() => new Promise(() => async () => 1);", + parserOptions: { ecmaVersion: 2017 }, + + // for async + errors: [eReturnsValue(25, "ArrowFunctionExpression")] + }, + { + code: "() => new Promise(() => function () {});", + errors: [eReturnsValue(25, "FunctionExpression")] + }, + { + code: "() => new Promise(() => function foo() {});", + errors: [eReturnsValue(25, "FunctionExpression")] + }, + { + code: "() => new Promise(() => []);", + errors: [eReturnsValue(25, "ArrayExpression")] }, // edge cases for global Promise reference { code: "new Promise((Promise) => { return 1; })", - errors: [error()] + errors: [eReturnsValue()] }, { code: "new Promise(function Promise(resolve, reject) { return 1; })", - errors: [error()] + errors: [eReturnsValue()] } ] });