Bug 1536556 - Extend no-throw-cr-literal ESLint rule to forbid and fix throw new Error(Cr.ERROR);. r=Standard8

Code should be using `throw Components.Exception("", Cr.ERROR);` instead,
since `new Error()` just converts the int value of the Cr.ERROR into a string,
whereas `Exception` constructs an Exception object with the result property set
to the Cr.ERROR value, so other code can identify it.

Differential Revision: https://phabricator.services.mozilla.com/D28074

docs/code-quality/lint/linters/eslint-plugin-mozilla.rst

@@ -193,12 +193,15 @@ It disallows statements such as:
 
    throw Cr.NS_ERROR_UNEXPECTED;
    throw Components.results.NS_ERROR_ABORT;
+   throw new Error(Cr.NS_ERROR_NO_INTERFACE);
 
 Throwing bare literals is inferior to throwing Exception objects, which provide
 stack information.  Cr.ERRORs should be be passed as the second argument to
 ``Components.Exception()`` to create an Exception object with stack info, and
 the correct result property corresponding to the NS_ERROR that other code
 expects.
+Using a regular ``new Error()`` to wrap just turns it into a string and doesn't
+set the result property, so the errors can't be recognised.
 
 This option can be autofixed (``--fix``).
 

tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-throw-cr-literal.js

@@ -12,6 +12,34 @@
 // Rule Definition
 // -----------------------------------------------------------------------------
 
+function isCr(object) {
+  return object.type === "Identifier" && object.name === "Cr";
+}
+
+function isComponentsResults(object) {
+  return (
+    object.type === "MemberExpression" &&
+    object.object.type === "Identifier" &&
+    object.object.name === "Components" &&
+    object.property.type === "Identifier" &&
+    object.property.name === "results"
+  );
+}
+
+function isNewError(argument) {
+  return (
+    argument.type === "NewExpression" &&
+    argument.callee.type === "Identifier" &&
+    argument.callee.name === "Error" &&
+    argument.arguments.length === 1
+  );
+}
+
+function fixT(context, node, argument, fixer) {
+  const sourceText = context.getSourceCode().getText(argument);
+  return fixer.replaceText(node, `Components.Exception("", ${sourceText})`);
+}
+
 module.exports = {
   meta: {
     fixable: "code",
@@ -19,6 +47,10 @@ module.exports = {
       bareCR: "Do not throw bare Cr.ERRORs, use Components.Exception instead",
       bareComponentsResults:
         "Do not throw bare Components.results.ERRORs, use Components.Exception instead",
+      newErrorCR:
+        "Do not pass Cr.ERRORs to new Error(), use Components.Exception instead",
+      newErrorComponentsResults:
+        "Do not pass Components.results.ERRORs to new Error(), use Components.Exception instead",
     },
   },
 
@@ -26,35 +58,41 @@ module.exports = {
     return {
       ThrowStatement(node) {
         if (node.argument.type === "MemberExpression") {
-          function fix(fixer) {
-            const sourceCode = context.getSourceCode();
-            const source = sourceCode.getText(node.argument);
-            return fixer.replaceText(
-              node.argument,
-              `Components.Exception("", ${source})`
-            );
-          }
+          const fix = fixT.bind(null, context, node.argument, node.argument);
 
-          const obj = node.argument.object;
-          if (obj.type === "Identifier" && obj.name === "Cr") {
+          if (isCr(node.argument.object)) {
             context.report({
               node,
               messageId: "bareCR",
               fix,
             });
-          } else if (
-            obj.type === "MemberExpression" &&
-            obj.object.type === "Identifier" &&
-            obj.object.name === "Components" &&
-            obj.property.type === "Identifier" &&
-            obj.property.name === "results"
-          ) {
+          } else if (isComponentsResults(node.argument.object)) {
             context.report({
               node,
               messageId: "bareComponentsResults",
               fix,
             });
           }
+        } else if (isNewError(node.argument)) {
+          const argument = node.argument.arguments[0];
+
+          if (argument.type === "MemberExpression") {
+            const fix = fixT.bind(null, context, node.argument, argument);
+
+            if (isCr(argument.object)) {
+              context.report({
+                node,
+                messageId: "newErrorCR",
+                fix,
+              });
+            } else if (isComponentsResults(argument.object)) {
+              context.report({
+                node,
+                messageId: "newErrorComponentsResults",
+                fix,
+              });
+            }
+          }
         }
       },
     };

tools/lint/eslint/eslint-plugin-mozilla/package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "2.5.0",
+  "version": "2.6.0",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {

tools/lint/eslint/eslint-plugin-mozilla/package.json

@@ -1,6 +1,6 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "2.5.0",
+  "version": "2.6.0",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",

tools/lint/eslint/eslint-plugin-mozilla/tests/no-throw-cr-literal.js

@@ -48,5 +48,15 @@ ruleTester.run("no-throw-cr-literal", rule, {
       'function t() { throw Components.Exception("", Cr.NS_ERROR_NULL_POINTER); }',
       "bareCR"
     ),
+    invalidCode(
+      "throw new Error(Cr.NS_ERROR_ABORT);",
+      'throw Components.Exception("", Cr.NS_ERROR_ABORT);',
+      "newErrorCR"
+    ),
+    invalidCode(
+      "throw new Error(Components.results.NS_ERROR_ABORT);",
+      'throw Components.Exception("", Components.results.NS_ERROR_ABORT);',
+      "newErrorComponentsResults"
+    ),
   ],
 });