From 9e1414ee16b8caf582920f8fdf3b6ee1eb0b7cd5 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 19 Jun 2020 17:50:00 +0200 Subject: [PATCH] New: Add no-promise-executor-return rule (fixes #12640) (#12648) * New: Add no-promise-executor-return rule (fixes #12640) * Fix eslint comments in docs * Change error message --- docs/rules/no-promise-executor-return.md | 96 ++++++ lib/rules/index.js | 1 + lib/rules/no-promise-executor-return.js | 121 +++++++ tests/lib/rules/no-promise-executor-return.js | 303 ++++++++++++++++++ tools/rule-types.json | 1 + 5 files changed, 522 insertions(+) create mode 100644 docs/rules/no-promise-executor-return.md create mode 100644 lib/rules/no-promise-executor-return.js create mode 100644 tests/lib/rules/no-promise-executor-return.js diff --git a/docs/rules/no-promise-executor-return.md b/docs/rules/no-promise-executor-return.md new file mode 100644 index 00000000000..1adfe37b1e4 --- /dev/null +++ b/docs/rules/no-promise-executor-return.md @@ -0,0 +1,96 @@ +# Disallow returning values from Promise executor functions (no-promise-executor-return) + +The `new Promise` constructor accepts a single argument, called an *executor*. + +```js +const myPromise = new Promise(function executor(resolve, reject) { + readFile('foo.txt', function(err, result) { + if (err) { + reject(err); + } else { + resolve(result); + } + }); +}); +``` + +The executor function usually initiates some asynchronous operation. Once it is finished, the executor should call `resolve` with the result, or `reject` if an error occurred. + +The return value of the executor is ignored. Returning a value from an executor function is a possible error because the returned value cannot be used and it doesn't affect the promise in any way. + +## Rule Details + +This rule disallows returning values from Promise executor functions. + +Only `return` without a value is allowed, as it's a control flow statement. + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-promise-executor-return: "error"*/ + +new Promise((resolve, reject) => { + if (someCondition) { + return defaultResult; + } + getSomething((err, result) => { + if (err) { + reject(err); + } else { + resolve(result); + } + }); +}); + +new Promise((resolve, reject) => getSomething((err, data) => { + if (err) { + reject(err); + } else { + resolve(data); + } +})); + +new Promise(() => { + return 1; +}); +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-promise-executor-return: "error"*/ + +new Promise((resolve, reject) => { + if (someCondition) { + resolve(defaultResult); + return; + } + getSomething((err, result) => { + if (err) { + reject(err); + } else { + resolve(result); + } + }); +}); + +new Promise((resolve, reject) => { + getSomething((err, data) => { + if (err) { + reject(err); + } else { + resolve(data); + } + }); +}); + +Promise.resolve(1); +``` + +## Further Reading + +* [MDN Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) + +## Related Rules + +* [no-async-promise-executor](no-async-promise-executor.md) diff --git a/lib/rules/index.js b/lib/rules/index.js index 6a211e598c9..567cd4a0eca 100644 --- a/lib/rules/index.js +++ b/lib/rules/index.js @@ -176,6 +176,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({ "no-plusplus": () => require("./no-plusplus"), "no-process-env": () => require("./no-process-env"), "no-process-exit": () => require("./no-process-exit"), + "no-promise-executor-return": () => require("./no-promise-executor-return"), "no-proto": () => require("./no-proto"), "no-prototype-builtins": () => require("./no-prototype-builtins"), "no-redeclare": () => require("./no-redeclare"), diff --git a/lib/rules/no-promise-executor-return.js b/lib/rules/no-promise-executor-return.js new file mode 100644 index 00000000000..32ee6e15124 --- /dev/null +++ b/lib/rules/no-promise-executor-return.js @@ -0,0 +1,121 @@ +/** + * @fileoverview Rule to disallow returning values from Promise executor functions + * @author Milos Djermanovic + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const { findVariable } = require("eslint-utils"); + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const functionTypesToCheck = new Set(["ArrowFunctionExpression", "FunctionExpression"]); + +/** + * Determines whether the given identifier node is a reference to a global variable. + * @param {ASTNode} node `Identifier` node to check. + * @param {Scope} scope Scope to which the node belongs. + * @returns {boolean} True if the identifier is a reference to a global variable. + */ +function isGlobalReference(node, scope) { + const variable = findVariable(scope, node); + + return variable !== null && variable.scope.type === "global" && variable.defs.length === 0; +} + +/** + * Finds function's outer scope. + * @param {Scope} scope Function's own scope. + * @returns {Scope} Function's outer scope. + */ +function getOuterScope(scope) { + const upper = scope.upper; + + if (upper.type === "function-expression-name") { + return upper.upper; + } + return upper; +} + +/** + * Determines whether the given function node is used as a Promise executor. + * @param {ASTNode} node The node to check. + * @param {Scope} scope Function's own scope. + * @returns {boolean} `true` if the node is a Promise executor. + */ +function isPromiseExecutor(node, scope) { + const parent = node.parent; + + return parent.type === "NewExpression" && + parent.arguments[0] === node && + parent.callee.type === "Identifier" && + parent.callee.name === "Promise" && + isGlobalReference(parent.callee, getOuterScope(scope)); +} + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + type: "problem", + + docs: { + description: "disallow returning values from Promise executor functions", + category: "Possible Errors", + recommended: false, + url: "https://eslint.org/docs/rules/no-promise-executor-return" + }, + + schema: [], + + messages: { + returnsValue: "Return values from promise executor functions cannot be read." + } + }, + + create(context) { + + let funcInfo = null; + + /** + * Reports the given node. + * @param {ASTNode} node Node to report. + * @returns {void} + */ + function report(node) { + context.report({ node, messageId: "returnsValue" }); + } + + return { + + onCodePathStart(_, node) { + funcInfo = { + upper: funcInfo, + shouldCheck: functionTypesToCheck.has(node.type) && isPromiseExecutor(node, context.getScope()) + }; + + if (funcInfo.shouldCheck && node.type === "ArrowFunctionExpression" && node.expression) { + report(node.body); + } + }, + + onCodePathEnd() { + funcInfo = funcInfo.upper; + }, + + ReturnStatement(node) { + if (funcInfo.shouldCheck && node.argument) { + report(node); + } + } + }; + } +}; diff --git a/tests/lib/rules/no-promise-executor-return.js b/tests/lib/rules/no-promise-executor-return.js new file mode 100644 index 00000000000..a24629b6a44 --- /dev/null +++ b/tests/lib/rules/no-promise-executor-return.js @@ -0,0 +1,303 @@ +/** + * @fileoverview Tests for the no-promise-executor-return rule + * @author Milos Djermanovic + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/no-promise-executor-return"); +const { RuleTester } = require("../../../lib/rule-tester"); + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +/** + * Creates an error object. + * @param {number} [column] Reported column. + * @param {string} [type="ReturnStatement"] Reported node type. + * @returns {Object} The error object. + */ +function error(column, type = "ReturnStatement") { + const errorObject = { + messageId: "returnsValue", + type + }; + + if (column) { + errorObject.column = column; + } + + return errorObject; +} + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2015 }, env: { es6: true } }); + +ruleTester.run("no-promise-executor-return", rule, { + valid: [ + + //------------------------------------------------------------------------------ + // General + //------------------------------------------------------------------------------ + + // not a promise executor + "function foo(resolve, reject) { return 1; }", + "function Promise(resolve, reject) { return 1; }", + "(function (resolve, reject) { return 1; })", + "(function foo(resolve, reject) { return 1; })", + "(function Promise(resolve, reject) { return 1; })", + "var foo = function (resolve, reject) { return 1; }", + "var foo = function Promise(resolve, reject) { return 1; }", + "var Promise = function (resolve, reject) { return 1; }", + "(resolve, reject) => { return 1; }", + "(resolve, reject) => 1", + "var foo = (resolve, reject) => { return 1; }", + "var Promise = (resolve, reject) => { return 1; }", + "var foo = (resolve, reject) => 1", + "var Promise = (resolve, reject) => 1", + "var foo = { bar(resolve, reject) { return 1; } }", + "var foo = { Promise(resolve, reject) { return 1; } }", + "new foo(function (resolve, reject) { return 1; });", + "new foo(function bar(resolve, reject) { return 1; });", + "new foo(function Promise(resolve, reject) { return 1; });", + "new foo((resolve, reject) => { return 1; });", + "new foo((resolve, reject) => 1);", + "new promise(function foo(resolve, reject) { return 1; });", + "new Promise.foo(function foo(resolve, reject) { return 1; });", + "new foo.Promise(function foo(resolve, reject) { return 1; });", + "new Promise.Promise(function foo(resolve, reject) { return 1; });", + "new Promise()(function foo(resolve, reject) { return 1; });", + + // not a promise executor - Promise() without new + "Promise(function (resolve, reject) { return 1; });", + "Promise((resolve, reject) => { return 1; });", + "Promise((resolve, reject) => 1);", + + // not a promise executor - not the first argument + "new Promise(foo, function (resolve, reject) { return 1; });", + "new Promise(foo, (resolve, reject) => { return 1; });", + "new Promise(foo, (resolve, reject) => 1);", + + // global Promise doesn't exist + "/* globals Promise:off */ new Promise(function (resolve, reject) { return 1; });", + { + code: "new Promise((resolve, reject) => { return 1; });", + globals: { Promise: "off" } + }, + { + code: "new Promise((resolve, reject) => 1);", + env: { es6: false } + }, + + // global Promise is shadowed + "let Promise; new Promise(function (resolve, reject) { return 1; });", + "function f() { new Promise((resolve, reject) => { return 1; }); var Promise; }", + "function f(Promise) { new Promise((resolve, reject) => 1); }", + "if (x) { const Promise = foo(); new Promise(function (resolve, reject) { return 1; }); }", + "x = function Promise() { new Promise((resolve, reject) => { return 1; }); }", + + // return without a value is allowed + "new Promise(function (resolve, reject) { return; });", + "new Promise(function (resolve, reject) { reject(new Error()); return; });", + "new Promise(function (resolve, reject) { if (foo) { return; } });", + "new Promise((resolve, reject) => { return; });", + "new Promise((resolve, reject) => { if (foo) { resolve(1); return; } reject(new Error()); });", + + // throw is allowed + "new Promise(function (resolve, reject) { throw new Error(); });", + "new Promise((resolve, reject) => { throw new Error(); });", + + // not returning from the promise executor + "new Promise(function (resolve, reject) { function foo() { return 1; } });", + "new Promise((resolve, reject) => { (function foo() { return 1; })(); });", + "new Promise(function (resolve, reject) { () => { return 1; } });", + "new Promise((resolve, reject) => { () => 1 });", + "function foo() { return new Promise(function (resolve, reject) { resolve(bar); }) };", + "foo => new Promise((resolve, reject) => { bar(foo, (err, data) => { if (err) { reject(err); return; } resolve(data); })});", + + // promise executors do not have effect on other functions (tests function info tracking) + "new Promise(function (resolve, reject) {}); function foo() { return 1; }", + "new Promise((resolve, reject) => {}); (function () { return 1; });", + "new Promise(function (resolve, reject) {}); () => { return 1; };", + "new Promise((resolve, reject) => {}); () => 1;", + + // does not report global return + { + code: "return 1;", + env: { node: true } + }, + { + code: "return 1;", + parserOptions: { ecmaFeatures: { globalReturn: true } } + }, + { + code: "return 1; function foo(){ return 1; } return 1;", + env: { node: true } + }, + { + code: "function foo(){} return 1; var bar = function*(){ return 1; }; return 1; var baz = () => {}; return 1;", + env: { node: true } + }, + { + code: "new Promise(function (resolve, reject) {}); return 1;", + env: { node: true } + } + ], + + invalid: [ + + // full error tests + { + code: "new Promise(function (resolve, reject) { return 1; })", + errors: [{ message: "Return values from promise executor functions cannot be read.", type: "ReturnStatement", column: 42, endColumn: 51 }] + }, + { + code: "new Promise((resolve, reject) => resolve(1))", + errors: [{ message: "Return values from promise executor functions cannot be read.", type: "CallExpression", column: 34, endColumn: 44 }] + }, + + // other basic tests + { + code: "new Promise(function foo(resolve, reject) { return 1; })", + errors: [error()] + }, + { + code: "new Promise((resolve, reject) => { return 1; })", + errors: [error()] + }, + + // any returned value + { + code: "new Promise(function (resolve, reject) { return undefined; })", + errors: [error()] + }, + { + code: "new Promise((resolve, reject) => { return null; })", + errors: [error()] + }, + { + code: "new Promise(function (resolve, reject) { return false; })", + errors: [error()] + }, + { + code: "new Promise((resolve, reject) => resolve)", + errors: [error(34, "Identifier")] + }, + { + code: "new Promise((resolve, reject) => null)", + errors: [error(34, "Literal")] + }, + { + code: "new Promise(function (resolve, reject) { return resolve(foo); })", + errors: [error()] + }, + { + code: "new Promise((resolve, reject) => { return reject(foo); })", + errors: [error()] + }, + { + code: "new Promise((resolve, reject) => x + y)", + errors: [error(34, "BinaryExpression")] + }, + { + code: "new Promise((resolve, reject) => { return Promise.resolve(42); })", + errors: [error()] + }, + + // any return statement location + { + code: "new Promise(function (resolve, reject) { if (foo) { return 1; } })", + errors: [error()] + }, + { + code: "new Promise((resolve, reject) => { try { return 1; } catch(e) {} })", + errors: [error()] + }, + { + code: "new Promise(function (resolve, reject) { while (foo){ if (bar) break; else return 1; } })", + errors: [error()] + }, + + // absence of arguments has no effect + { + code: "new Promise(function () { return 1; })", + errors: [error()] + }, + { + code: "new Promise(() => { return 1; })", + errors: [error()] + }, + { + code: "new Promise(() => 1)", + errors: [error(19, "Literal")] + }, + + // various scope tracking tests + { + code: "function foo() {} new Promise(function () { return 1; });", + errors: [error(45)] + }, + { + code: "function foo() { return; } new Promise(() => { return 1; });", + errors: [error(48)] + }, + { + code: "function foo() { return 1; } new Promise(() => { return 2; });", + errors: [error(50)] + }, + { + code: "function foo () { return new Promise(function () { return 1; }); }", + errors: [error(52)] + }, + { + code: "function foo() { return new Promise(() => { bar(() => { return 1; }); return false; }); }", + errors: [error(71)] + }, + { + code: "() => new Promise(() => { if (foo) { return 0; } else bar(() => { return 1; }); })", + errors: [error(38)] + }, + { + code: "function foo () { return 1; return new Promise(function () { return 2; }); return 3;}", + errors: [error(62)] + }, + { + code: "() => 1; new Promise(() => { return 1; })", + errors: [error(30)] + }, + { + code: "new Promise(function () { return 1; }); function foo() { return 1; } ", + errors: [error(27)] + }, + { + code: "() => new Promise(() => { return 1; });", + errors: [error(27)] + }, + { + code: "() => new Promise(() => 1);", + errors: [error(25, "Literal")] + }, + { + code: "() => new Promise(() => () => 1);", + errors: [error(25, "ArrowFunctionExpression")] + }, + + // edge cases for global Promise reference + { + code: "new Promise((Promise) => { return 1; })", + errors: [error()] + }, + { + code: "new Promise(function Promise(resolve, reject) { return 1; })", + errors: [error()] + } + ] +}); diff --git a/tools/rule-types.json b/tools/rule-types.json index 683d0fef8d5..6f2e6a834b3 100644 --- a/tools/rule-types.json +++ b/tools/rule-types.json @@ -163,6 +163,7 @@ "no-plusplus": "suggestion", "no-process-env": "suggestion", "no-process-exit": "suggestion", + "no-promise-executor-return": "problem", "no-proto": "suggestion", "no-prototype-builtins": "problem", "no-redeclare": "suggestion",