Bug 1230369 - Add ESLint rules to disallow defining Cc/Ci etc and to prefer the use of Cc/Ci rather than the Components.* equivalents. r?florian draft
authorMark Banner <standard8@mozilla.com>
Tue, 06 Feb 2018 22:40:12 +0000
changeset 752105 d874fc9a5acf10e05ab5f864365d3ecc2ab02d23
parent 752104 10cdad612312bb5bf21e8c31ce40995c6c6976f7
push id98159
push userbmo:standard8@mozilla.com
push dateWed, 07 Feb 2018 14:39:32 +0000
reviewersflorian
bugs1230369
milestone60.0a1
Bug 1230369 - Add ESLint rules to disallow defining Cc/Ci etc and to prefer the use of Cc/Ci rather than the Components.* equivalents. r?florian MozReview-Commit-ID: 9eAgUO3iIJW
.eslintrc.js
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/configs/mochitest-test.js
tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-define-cc-etc.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-cc-etc.js
tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
tools/lint/eslint/eslint-plugin-mozilla/package.json
tools/lint/eslint/eslint-plugin-mozilla/tests/no-define-cc-etc.js
tools/lint/eslint/eslint-plugin-mozilla/tests/use-cc-etc.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -49,10 +49,39 @@ module.exports = {
     "rules": {
       "mozilla/mark-exported-symbols-as-used": "error",
       "no-unused-vars": ["error", {
         "args": "none",
         "vars": "local",
         "varsIgnorePattern": "^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS"
       }]
     }
+  }, {
+    // XXX Bug 1433175. These directories are still being fixed, so turn off
+    // mozilla/use-cc-etc for now.
+    "files": [
+      "accessible/**",
+      "browser/**",
+      "devtools/**",
+      "dom/**",
+      "extensions/pref/**",
+      "mobile/android/**",
+      "security/manager/**",
+      "services/**",
+      "storage/test/**",
+      "testing/**",
+      "toolkit/**",
+      "xpcom/**",
+    ],
+    "rules": {
+      "mozilla/use-cc-etc": "off",
+    }
+  }, {
+    // XXX Bug 1436303. These directories are still being fixed, so turn off
+    // mozilla/no-cc-etc for now.
+    "files": [
+      "devtools/**"
+    ],
+    "rules": {
+      "mozilla/no-define-cc-etc": "off",
+    }
   }]
 };
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -181,16 +181,29 @@ possible CPOW usage by checking for the 
 Note: These are string matches so we will miss situations where the parent
 object is assigned to another variable e.g.:
 
 .. code-block:: js
 
     var b = gBrowser;
     b.content // Would not be detected as a CPOW.
 
+no-define-cc-etc
+----------------
+
+This disallows statements such as:
+
+.. code-block:: js
+
+   var Cc = Components.classes;
+   var Ci = Components.interfaces;
+   var {Ci: interfaces, Cc: classes, Cu: utils} = Components;
+
+These used to be necessary but have now been defined globally for all chrome
+contexts.
 
 no-single-arg-cu-import
 -----------------------
 
 Rejects calls to "Cu.import" that do not supply a second argument (meaning they
 add the exported properties into global scope).
 
 
@@ -251,16 +264,23 @@ Treats top-level assignments like ``this
 Note: These are string matches so we will miss situations where the parent
 object is assigned to another variable e.g.:
 
 .. code-block:: js
 
    var b = gBrowser;
    b.content // Would not be detected as a CPOW.
 
+use-cc-etc
+----------
+
+This requires using ``Cc`` rather than ``Components.classes``, and the same for
+``Components.interfaces``, ``Components.results`` and ``Components.utils``. This has
+a slight performance advantage by avoiding the use of the dot.
+
 use-chromeutils-import
 ----------------------
 
 Require use of ``ChromeUtils.import`` and ``ChromeUtils.defineModuleGetter``
 rather than ``Components.utils.import`` and
 ``XPCOMUtils.defineLazyModuleGetter``.
 
 use-default-preference-values
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/mochitest-test.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/mochitest-test.js
@@ -42,11 +42,14 @@ module.exports = {
   "plugins": [
     "mozilla"
   ],
 
   "rules": {
     "mozilla/import-content-task-globals": "error",
     "mozilla/import-headjs-globals": "error",
     "mozilla/mark-test-function-used": "error",
+    // Turn off no-define-cc-etc for mochitests as these don't have Cc etc defined in the
+    // global scope.
+    "mozilla/no-define-cc-etc": "off",
     "no-shadow": "error"
   }
 };
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
@@ -165,19 +165,21 @@ module.exports = {
     "max-depth": "off",
 
     // Maximum depth callbacks can be nested.
     "max-nested-callbacks": ["error", 10],
 
     "mozilla/avoid-removeChild": "error",
     "mozilla/import-browser-window-globals": "error",
     "mozilla/import-globals": "error",
+    "mozilla/no-define-cc-etc": "error",
     "mozilla/no-import-into-var-and-global": "error",
     "mozilla/no-useless-parameters": "error",
     "mozilla/no-useless-removeEventListener": "error",
+    "mozilla/use-cc-etc": "error",
     "mozilla/use-chromeutils-import": "error",
     "mozilla/use-default-preference-values": "error",
     "mozilla/use-includes-instead-of-indexOf": "error",
     "mozilla/use-ownerGlobal": "error",
     "mozilla/use-services": "error",
 
     // Always require parenthesis for new calls
     // "new-parens": "error",
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -40,56 +40,31 @@ module.exports = {
       require("../lib/rules/import-content-task-globals"),
     "import-globals": require("../lib/rules/import-globals"),
     "import-headjs-globals": require("../lib/rules/import-headjs-globals"),
     "mark-exported-symbols-as-used": require("../lib/rules/mark-exported-symbols-as-used"),
     "mark-test-function-used": require("../lib/rules/mark-test-function-used"),
     "no-aArgs": require("../lib/rules/no-aArgs"),
     "no-arbitrary-setTimeout": require("../lib/rules/no-arbitrary-setTimeout"),
     "no-cpows-in-tests": require("../lib/rules/no-cpows-in-tests"),
+    "no-define-cc-etc": require("../lib/rules/no-define-cc-etc"),
     "no-single-arg-cu-import": require("../lib/rules/no-single-arg-cu-import"),
     "no-import-into-var-and-global":
       require("../lib/rules/no-import-into-var-and-global.js"),
     "no-task": require("../lib/rules/no-task"),
     "no-useless-parameters": require("../lib/rules/no-useless-parameters"),
     "no-useless-removeEventListener":
       require("../lib/rules/no-useless-removeEventListener"),
     "no-useless-run-test":
       require("../lib/rules/no-useless-run-test"),
     "reject-importGlobalProperties":
       require("../lib/rules/reject-importGlobalProperties"),
     "reject-some-requires": require("../lib/rules/reject-some-requires"),
+    "use-cc-etc": require("../lib/rules/use-cc-etc"),
     "use-chromeutils-import": require("../lib/rules/use-chromeutils-import"),
     "use-default-preference-values":
       require("../lib/rules/use-default-preference-values"),
     "use-ownerGlobal": require("../lib/rules/use-ownerGlobal"),
     "use-includes-instead-of-indexOf": require("../lib/rules/use-includes-instead-of-indexOf"),
     "use-services": require("../lib/rules/use-services"),
     "var-only-at-top-level": require("../lib/rules/var-only-at-top-level")
-  },
-  rulesConfig: {
-    "avoid-Date-timing": "off",
-    "avoid-removeChild": "off",
-    "balanced-listeners": "off",
-    "import-browser-window-globals": "off",
-    "import-content-task-globals": "off",
-    "import-globals": "off",
-    "import-headjs-globals": "off",
-    "mark-test-function-used": "off",
-    "no-aArgs": "off",
-    "no-arbitrary-setTimeout": "off",
-    "no-cpows-in-tests": "off",
-    "no-single-arg-cu-import": "off",
-    "no-import-into-var-and-global": "off",
-    "no-task": "off",
-    "no-useless-parameters": "off",
-    "no-useless-run-test": "off",
-    "no-useless-removeEventListener": "off",
-    "reject-importGlobalProperties": "off",
-    "reject-some-requires": "off",
-    "use-chromeutils-import": "off",
-    "use-default-preference-values": "off",
-    "use-ownerGlobal": "off",
-    "use-includes-instead-of-indexOf": "off",
-    "use-services": "off",
-    "var-only-at-top-level": "off"
   }
 };
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-define-cc-etc.js
@@ -0,0 +1,40 @@
+/**
+ * @fileoverview Reject defining Cc/Ci/Cr/Cu.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+"use strict";
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+
+const componentsBlacklist = ["Cc", "Ci", "Cr", "Cu"];
+
+module.exports = function(context) {
+
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    "VariableDeclarator": function(node) {
+      if (node.id.type == "Identifier" && componentsBlacklist.includes(node.id.name)) {
+        context.report(node,
+          `${node.id.name} is now defined in global scope, a separate definition is no longer necessary.`);
+      }
+
+      if (node.id.type == "ObjectPattern") {
+        for (let property of node.id.properties) {
+          if (componentsBlacklist.includes(property.key.name)) {
+            context.report(node,
+              `${property.key.name} is now defined in global scope, a separate definition is no longer necessary.`);
+          }
+        }
+      }
+    }
+  };
+};
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-cc-etc.js
@@ -0,0 +1,39 @@
+/**
+ * @fileoverview Reject use of Components.classes etc, prefer the shorthand instead.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+"use strict";
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+
+const componentsMap = {
+  classes: "Cc",
+  interfaces: "Ci",
+  results: "Cr",
+  utils: "Cu"
+};
+
+module.exports = function(context) {
+
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    "MemberExpression": function(node) {
+      if (node.object.type === "Identifier" &&
+          node.object.name === "Components" &&
+          node.property.type === "Identifier" &&
+          Object.getOwnPropertyNames(componentsMap).includes(node.property.name)) {
+        context.report(node,
+          `Use ${componentsMap[node.property.name]} rather than Components.${node.property.name}`);
+      }
+    }
+  };
+};
--- a/tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
+++ b/tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
@@ -1,11 +1,11 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "0.7.0",
+  "version": "0.8.0",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
     "acorn": {
       "version": "5.2.1",
       "resolved": "https://registry.npmjs.org/acorn/-/acorn-5.2.1.tgz",
       "integrity": "sha512-jG0u7c4Ly+3QkkW18V+NRDN+4bWHdln30NL1ZL2AvFZZmQe/BfopYCtghCKKVBUSetZ4QKcyA0pY6/4Gw8Pv8w==",
       "dev": true
--- a/tools/lint/eslint/eslint-plugin-mozilla/package.json
+++ b/tools/lint/eslint/eslint-plugin-mozilla/package.json
@@ -1,11 +1,11 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "0.7.0",
+  "version": "0.8.0",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/no-define-cc-etc.js
@@ -0,0 +1,54 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/no-define-cc-etc");
+var RuleTester = require("eslint/lib/testers/rule-tester");
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidCode(code, varName) {
+  return {code, errors: [{
+    message: `${varName} is now defined in global scope, a separate definition is no longer necessary.`,
+    type: "VariableDeclarator"
+  }]};
+}
+
+ruleTester.run("no-define-cc-etc", rule, {
+  valid: [
+    "var Cm = Components.manager;",
+    "const CC = Components.Constructor;",
+    "var {CC: Constructor, Cm: manager} = Components;",
+    "const {CC: Constructor, Cm: manager} = Components;",
+    "foo.Cc.test();"
+  ],
+  invalid: [
+    invalidCode("var Cc;", "Cc"),
+    invalidCode("let Cc;", "Cc"),
+    invalidCode("let Ci;", "Ci"),
+    invalidCode("let Cr;", "Cr"),
+    invalidCode("let Cu;", "Cu"),
+    invalidCode("var Cc = Components.classes;", "Cc"),
+    invalidCode("const {Cc} = Components;", "Cc"),
+    invalidCode("let {Cc, Cm} = Components", "Cc"),
+    invalidCode("const Cu = Components.utils;", "Cu"), {
+      code: "var {Ci: interfaces, Cc: classes} = Components;",
+      errors: [{
+        message: `Ci is now defined in global scope, a separate definition is no longer necessary.`,
+        type: "VariableDeclarator"
+      }, {
+        message: `Cc is now defined in global scope, a separate definition is no longer necessary.`,
+        type: "VariableDeclarator"
+      }]
+    }
+  ]
+});
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/use-cc-etc.js
@@ -0,0 +1,37 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/use-cc-etc");
+var RuleTester = require("eslint/lib/testers/rule-tester");
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidCode(code, originalName, newName) {
+  return {code, errors: [{
+    message: `Use ${newName} rather than ${originalName}`,
+    type: "MemberExpression"
+  }]};
+}
+
+ruleTester.run("use-cc-etc", rule, {
+  valid: [
+    "Components.Constructor();",
+    "let x = Components.foo;"
+  ],
+  invalid: [
+    invalidCode("let foo = Components.classes['bar'];", "Components.classes", "Cc"),
+    invalidCode("let bar = Components.interfaces.bar;", "Components.interfaces", "Ci"),
+    invalidCode("Components.results.NS_ERROR_ILLEGAL_INPUT;", "Components.results", "Cr"),
+    invalidCode("Components.utils.reportError('fake');", "Components.utils", "Cu")
+  ]
+});