New: prefer-promise-reject-errors rule (fixes #7685) (#7689)

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",

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)

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;
+        }
     }
 };

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") {

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));
+                }
+            }
+        };
+    }
+};

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"

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]);
+            });
+        });
+    });
 });

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: [
         {