diff --git a/conf/eslint.json b/conf/eslint.json index 81f5bb8aa5e..4276b51543e 100755 --- a/conf/eslint.json +++ b/conf/eslint.json @@ -206,6 +206,7 @@ "prefer-const": "off", "prefer-destructuring": "off", "prefer-numeric-literals": "off", + "prefer-promise-reject-errors": "off", "prefer-reflect": "off", "prefer-rest-params": "off", "prefer-spread": "off", diff --git a/docs/rules/prefer-promise-reject-errors.md b/docs/rules/prefer-promise-reject-errors.md new file mode 100644 index 00000000000..b81e05d57fd --- /dev/null +++ b/docs/rules/prefer-promise-reject-errors.md @@ -0,0 +1,79 @@ +# require using Error objects as Promise rejection reasons (prefer-promise-reject-errors) + +It is considered good practice to only instances of the built-in `Error` object for user-defined errors in Promises. `Error` objects automatically store a stack trace, which can be used to debug an error by determining where it came from. If a Promise is rejected with a non-`Error` value, it can be difficult to determine where the rejection occurred. + + +## Rule Details + +This rule aims to ensure that Promises are only rejected with `Error` objects. + +## Options + +This rule takes one optional object argument: + +* `allowEmptyReject: true` (`false` by default) allows calls to `Promise.reject()` with no arguments. + +Examples of **incorrect** code for this rule: + +```js +/*eslint prefer-promise-reject-errors: "error"*/ + +Promise.reject("something bad happened"); + +Promise.reject(5); + +Promise.reject(); + +new Promise(function(resolve, reject) { + reject("something bad happened"); +}); + +new Promise(function(resolve, reject) { + reject(); +}); + +``` + +Examples of **correct** code for this rule: + +```js +/*eslint prefer-promise-reject-errors: "error"*/ + +Promise.reject(new Error("something bad happened")); + +Promise.reject(new TypeError("something bad happened")); + +new Promise(function(resolve, reject) { + reject(new Error("something bad happened")); +}); + +var foo = getUnknownValue(); +Promise.reject(foo); +``` + +Examples of **correct** code for this rule with the `allowEmptyReject: true` option: + +```js +/*eslint prefer-promise-reject-errors: ["error", {"allowEmptyReject": true}]*/ + +Promise.reject(); + +new Promise(function(resolve, reject) { + reject(); +}); +``` + +## Known Limitations + +Due to the limits of static analysis, this rule cannot guarantee that you will only reject Promises with `Error` objects. While the rule will report cases where it can guarantee that the rejection reason is clearly not an `Error`, it will not report cases where there is uncertainty about whether a given reason is an `Error`. For more information on this caveat, see the [similar limitations](http://eslint.org/docs/rules/no-throw-literal#known-limitations) in the `no-throw-literal` rule. + +To avoid conflicts between rules, this rule does not report non-error values used in `throw` statements in async functions, even though these lead to Promise rejections. To lint for these cases, use the [`no-throw-literal`](http://eslint.org/docs/rules/no-throw-literal) rule. + +## When Not To Use It + +If you're using custom non-error values as Promise rejection reasons, you can turn off this rule. + +## Further Reading + +* [`no-throw-literal`](http://eslint.org/docs/rules/no-throw-literal) +* [Warning: a promise was rejected with a non-error](http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-rejected-with-a-non-error) diff --git a/lib/ast-utils.js b/lib/ast-utils.js index 4f23e5ff688..189bc56921e 100644 --- a/lib/ast-utils.js +++ b/lib/ast-utils.js @@ -1112,5 +1112,41 @@ module.exports = { } return sourceCode.getText().slice(leftToken.range[0], rightToken.range[1]); + }, + + /* + * Determine if a node has a possiblity to be an Error object + * @param {ASTNode} node ASTNode to check + * @returns {boolean} True if there is a chance it contains an Error obj + */ + couldBeError(node) { + switch (node.type) { + case "Identifier": + case "CallExpression": + case "NewExpression": + case "MemberExpression": + case "TaggedTemplateExpression": + case "YieldExpression": + case "AwaitExpression": + return true; // possibly an error object. + + case "AssignmentExpression": + return module.exports.couldBeError(node.right); + + case "SequenceExpression": { + const exprs = node.expressions; + + return exprs.length !== 0 && module.exports.couldBeError(exprs[exprs.length - 1]); + } + + case "LogicalExpression": + return module.exports.couldBeError(node.left) || module.exports.couldBeError(node.right); + + case "ConditionalExpression": + return module.exports.couldBeError(node.consequent) || module.exports.couldBeError(node.alternate); + + default: + return false; + } } }; diff --git a/lib/rules/no-throw-literal.js b/lib/rules/no-throw-literal.js index 0d1f42985f8..5e9054399a2 100644 --- a/lib/rules/no-throw-literal.js +++ b/lib/rules/no-throw-literal.js @@ -5,44 +5,7 @@ "use strict"; -//------------------------------------------------------------------------------ -// Helpers -//------------------------------------------------------------------------------ - -/** - * Determine if a node has a possiblity to be an Error object - * @param {ASTNode} node ASTNode to check - * @returns {boolean} True if there is a chance it contains an Error obj - */ -function couldBeError(node) { - switch (node.type) { - case "Identifier": - case "CallExpression": - case "NewExpression": - case "MemberExpression": - case "TaggedTemplateExpression": - case "YieldExpression": - return true; // possibly an error object. - - case "AssignmentExpression": - return couldBeError(node.right); - - case "SequenceExpression": { - const exprs = node.expressions; - - return exprs.length !== 0 && couldBeError(exprs[exprs.length - 1]); - } - - case "LogicalExpression": - return couldBeError(node.left) || couldBeError(node.right); - - case "ConditionalExpression": - return couldBeError(node.consequent) || couldBeError(node.alternate); - - default: - return false; - } -} +const astUtils = require("../ast-utils"); //------------------------------------------------------------------------------ // Rule Definition @@ -64,7 +27,7 @@ module.exports = { return { ThrowStatement(node) { - if (!couldBeError(node.argument)) { + if (!astUtils.couldBeError(node.argument)) { context.report({ node, message: "Expected an object to be thrown." }); } else if (node.argument.type === "Identifier") { if (node.argument.name === "undefined") { diff --git a/lib/rules/prefer-promise-reject-errors.js b/lib/rules/prefer-promise-reject-errors.js new file mode 100644 index 00000000000..97223a65a82 --- /dev/null +++ b/lib/rules/prefer-promise-reject-errors.js @@ -0,0 +1,124 @@ +/** + * @fileoverview restrict values that can be used as Promise rejection reasons + * @author Teddy Katz + */ +"use strict"; + +const astUtils = require("../ast-utils"); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: "require using Error objects as Promise rejection reasons", + category: "Best Practices", + recommended: false + }, + fixable: null, + schema: [ + { + type: "object", + properties: { + allowEmptyReject: { type: "boolean" } + }, + additionalProperties: false + } + ] + }, + + create(context) { + + const ALLOW_EMPTY_REJECT = context.options.length && context.options[0].allowEmptyReject; + + //---------------------------------------------------------------------- + // Helpers + //---------------------------------------------------------------------- + + /** + * Checks the argument of a reject() or Promise.reject() CallExpression, and reports it if it can't be an Error + * @param {ASTNode} callExpression A CallExpression node which is used to reject a Promise + * @returns {void} + */ + function checkRejectCall(callExpression) { + if (!callExpression.arguments.length && ALLOW_EMPTY_REJECT) { + return; + } + if ( + !callExpression.arguments.length || + !astUtils.couldBeError(callExpression.arguments[0]) || + callExpression.arguments[0].type === "Identifier" && callExpression.arguments[0].name === "undefined" + ) { + context.report({ + node: callExpression, + message: "Expected the Promise rejection reason to be an Error." + }); + } + } + + /** + * Determines whether a function call is a Promise.reject() call + * @param {ASTNode} node A CallExpression node + * @returns {boolean} `true` if the call is a Promise.reject() call + */ + function isPromiseRejectCall(node) { + return node.callee.type === "MemberExpression" && + node.callee.object.type === "Identifier" && node.callee.object.name === "Promise" && + node.callee.property.type === "Identifier" && node.callee.property.name === "reject"; + } + + //---------------------------------------------------------------------- + // Public + //---------------------------------------------------------------------- + + return { + + // Check `Promise.reject(value)` calls. + CallExpression(node) { + if (isPromiseRejectCall(node)) { + checkRejectCall(node); + } + }, + + /* + * Check for `new Promise((resolve, reject) => {})`, and check for reject() calls. + * This function is run on "NewExpression:exit" instead of "NewExpression" to ensure that + * the nodes in the expression already have the `parent` property. + */ + "NewExpression:exit"(node) { + if ( + node.callee.type === "Identifier" && node.callee.name === "Promise" && + node.arguments.length && astUtils.isFunction(node.arguments[0]) && + node.arguments[0].params.length > 1 && node.arguments[0].params[1].type === "Identifier" + ) { + context.getDeclaredVariables(node.arguments[0]) + + /* + * Find the first variable that matches the second parameter's name. + * If the first parameter has the same name as the second parameter, then the variable will actually + * be "declared" when the first parameter is evaluated, but then it will be immediately overwritten + * by the second parameter. It's not possible for an expression with the variable to be evaluated before + * the variable is overwritten, because functions with duplicate parameters cannot have destructuring or + * default assignments in their parameter lists. Therefore, it's not necessary to explicitly account for + * this case. + */ + .find(variable => variable.name === node.arguments[0].params[1].name) + + // Get the references to that variable. + .references + + // Only check the references that read the parameter's value. + .filter(ref => ref.isRead()) + + // Only check the references that are used as the callee in a function call, e.g. `reject(foo)`. + .filter(ref => ref.identifier.parent.type === "CallExpression" && ref.identifier === ref.identifier.parent.callee) + + // Check the argument of the function call to determine whether it's an Error. + .forEach(ref => checkRejectCall(ref.identifier.parent)); + } + } + }; + } +}; diff --git a/packages/eslint-config-eslint/default.yml b/packages/eslint-config-eslint/default.yml index 2201f09b55f..1c06482dddc 100644 --- a/packages/eslint-config-eslint/default.yml +++ b/packages/eslint-config-eslint/default.yml @@ -70,6 +70,7 @@ rules: no-process-exit: "error" no-proto: "error" no-redeclare: "error" + prefer-promise-reject-errors: "error" no-return-assign: "error" no-script-url: "error" no-self-assign: "error" diff --git a/tests/lib/ast-utils.js b/tests/lib/ast-utils.js index bb1be654d29..df6605cbfaf 100644 --- a/tests/lib/ast-utils.js +++ b/tests/lib/ast-utils.js @@ -970,4 +970,40 @@ describe("ast-utils", () => { }); }); }); + + describe("couldBeError", () => { + const EXPECTED_RESULTS = { + 5: false, + null: false, + true: false, + "'foo'": false, + "`foo`": false, + foo: true, + "new Foo": true, + "Foo()": true, + "foo`bar`": true, + "foo.bar": true, + "(foo = bar)": true, + "(foo = 1)": false, + "(1, 2, 3)": false, + "(foo, 2, 3)": false, + "(1, 2, foo)": true, + "1 && 2": false, + "1 && foo": true, + "foo && 2": true, + "foo ? 1 : 2": false, + "foo ? bar : 2": true, + "foo ? 1 : bar": true, + "[1, 2, 3]": false, + "({ foo: 1 })": false + }; + + Object.keys(EXPECTED_RESULTS).forEach(key => { + it(`returns ${EXPECTED_RESULTS[key]} for ${key}`, () => { + const ast = espree.parse(key, { ecmaVersion: 6 }); + + assert.strictEqual(astUtils.couldBeError(ast.body[0].expression), EXPECTED_RESULTS[key]); + }); + }); + }); }); diff --git a/tests/lib/rules/no-throw-literal.js b/tests/lib/rules/no-throw-literal.js index 054e12ffcc5..45e1ec658ee 100644 --- a/tests/lib/rules/no-throw-literal.js +++ b/tests/lib/rules/no-throw-literal.js @@ -37,7 +37,8 @@ ruleTester.run("no-throw-literal", rule, { "throw foo ? new Error() : 'literal';", // ConditionalExpression (consequent) "throw foo ? 'literal' : new Error();", // ConditionalExpression (alternate) { code: "throw tag `${foo}`;", parserOptions: { ecmaVersion: 6 } }, // TaggedTemplateExpression - { code: "function* foo() { var index = 0; throw yield index++; }", parserOptions: { ecmaVersion: 6 } } // YieldExpression + { code: "function* foo() { var index = 0; throw yield index++; }", parserOptions: { ecmaVersion: 6 } }, // YieldExpression + { code: "async function foo() { throw await bar; }", parserOptions: { ecmaVersion: 8 } } // AwaitExpression ], invalid: [ { diff --git a/tests/lib/rules/prefer-promise-reject-errors.js b/tests/lib/rules/prefer-promise-reject-errors.js new file mode 100644 index 00000000000..6a06af4f9d6 --- /dev/null +++ b/tests/lib/rules/prefer-promise-reject-errors.js @@ -0,0 +1,96 @@ +/** + * @fileoverview restrict values that can be used as Promise rejection reasons + * @author Teddy Katz + */ +"use strict"; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require("../../../lib/rules/prefer-promise-reject-errors"); +const RuleTester = require("../../../lib/testers/rule-tester"); + + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 8 } }); + +ruleTester.run("prefer-promise-reject-errors", rule, { + + valid: [ + "Promise.resolve(5)", + "Foo.reject(5)", + "Promise.reject(foo)", + "Promise.reject(foo.bar)", + "Promise.reject(foo.bar())", + "Promise.reject(new Error())", + "Promise.reject(new TypeError)", + "Promise.reject(new Error('foo'))", + "new Foo((resolve, reject) => reject(5))", + "new Promise(function(resolve, reject) { return function(reject) { reject(5) } })", + "new Promise(function(resolve, reject) { if (foo) { const reject = somethingElse; reject(5) } })", + "new Promise(function(resolve, {apply}) { apply(5) })", + "new Promise(function(resolve, reject) { resolve(5, reject) })", + "async function foo() { Promise.reject(await foo); }", + { + code: "Promise.reject()", + options: [{ allowEmptyReject: true }] + }, + { + code: "new Promise(function(resolve, reject) { reject() })", + options: [{ allowEmptyReject: true }] + } + ], + + invalid: [ + "Promise.reject(5)", + "Promise.reject('foo')", + "Promise.reject(`foo`)", + "Promise.reject(!foo)", + "Promise.reject(void foo)", + "Promise.reject()", + "Promise.reject(undefined)", + "Promise.reject({ foo: 1 })", + "Promise.reject([1, 2, 3])", + { + code: "Promise.reject()", + options: [{ allowEmptyReject: false }] + }, + { + code: "new Promise(function(resolve, reject) { reject() })", + options: [{ allowEmptyReject: false }] + }, + { + code: "Promise.reject(undefined)", + options: [{ allowEmptyReject: true }] + }, + "Promise.reject('foo', somethingElse)", + "new Promise(function(resolve, reject) { reject(5) })", + "new Promise((resolve, reject) => { reject(5) })", + "new Promise((resolve, reject) => reject(5))", + "new Promise((resolve, reject) => reject())", + "new Promise(function(yes, no) { no(5) })", + ` + new Promise((resolve, reject) => { + fs.readFile('foo.txt', (err, file) => { + if (err) reject('File not found') + else resolve(file) + }) + }) + `, + "new Promise(({foo, bar, baz}, reject) => reject(5))", + "new Promise(function(reject, reject) { reject(5) })", + "new Promise(function(foo, arguments) { arguments(5) })", + "new Promise((foo, arguments) => arguments(5))", + "new Promise(function({}, reject) { reject(5) })", + "new Promise(({}, reject) => reject(5))", + "new Promise((resolve, reject, somethingElse = reject(5)) => {})" + ].map(invalidCase => { + const errors = { errors: [{ message: "Expected the Promise rejection reason to be an Error.", type: "CallExpression" }] }; + + return Object.assign({}, errors, typeof invalidCase === "string" ? { code: invalidCase } : invalidCase); + }) +});