Bug 1608272 - Extend an ESLint rule to disallow 'this' as the second …

…argument to ChromeUtils.import. r=Gijs

Depends on D104684

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

.eslintrc.js

@@ -658,7 +658,7 @@ module.exports = {
         "toolkit/mozapps/installer/precompile_cache.js",
       ],
       rules: {
-        "mozilla/reject-chromeutils-import-null": "off",
+        "mozilla/reject-chromeutils-import-params": "off",
       },
     },
   ],

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

@@ -27,6 +27,7 @@ The plugin implements the following rules:
    eslint-plugin-mozilla/no-define-cc-etc
    eslint-plugin-mozilla/no-throw-cr-literal
    eslint-plugin-mozilla/no-useless-parameters
+   eslint-plugin-mozilla/reject-chromeutils-import-params
    eslint-plugin-mozilla/use-chromeutils-import
 
 avoid-removeChild

docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-chromeutils-import-params.rst

@@ -0,0 +1,26 @@
+================================
+reject-chromeutils-import-params
+================================
+
+``ChromeUtils.import`` can be called with two arguments, however these are now
+largely deprecated.
+
+The use of object destructuring is preferred over the second parameter being
+``this``.
+
+Using explicit exports is preferred over the second parameter being ``null``.
+
+Examples of incorrect code for this rule:
+-----------------------------------------
+
+.. code-block:: js
+
+    ChromeUtils.import("resource://gre/modules/Services.jsm", this);
+    ChromeUtils.import("resource://gre/modules/Services.jsm", null);
+
+Examples of correct code for this rule:
+---------------------------------------
+
+.. code-block:: js
+
+    const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");

tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js

@@ -129,7 +129,7 @@ module.exports = {
     "mozilla/no-useless-removeEventListener": "error",
     "mozilla/prefer-boolean-length-check": "error",
     "mozilla/prefer-formatValues": "error",
-    "mozilla/reject-chromeutils-import-null": "error",
+    "mozilla/reject-chromeutils-import-params": "error",
     "mozilla/reject-importGlobalProperties": ["error", "allownonwebidl"],
     "mozilla/rejects-requires-await": "error",
     "mozilla/use-cc-etc": "error",

tools/lint/eslint/eslint-plugin-mozilla/lib/index.js

@@ -54,7 +54,7 @@ module.exports = {
     "no-useless-run-test": require("../lib/rules/no-useless-run-test"),
     "prefer-boolean-length-check": require("../lib/rules/prefer-boolean-length-check"),
     "prefer-formatValues": require("../lib/rules/prefer-formatValues"),
-    "reject-chromeutils-import-null": require("../lib/rules/reject-chromeutils-import-null"),
+    "reject-chromeutils-import-params": require("../lib/rules/reject-chromeutils-import-params"),
     "reject-importGlobalProperties": require("../lib/rules/reject-importGlobalProperties"),
     "reject-relative-requires": require("../lib/rules/reject-relative-requires"),
     "reject-some-requires": require("../lib/rules/reject-some-requires"),

tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-params.js

@@ -0,0 +1,70 @@
+/**
+ * @fileoverview Reject calls to ChromeUtils.import(..., null). This allows to
+ * retrieve the global object for the JSM, instead we should rely on explicitly
+ * exported symbols.
+ *
+ * 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
+// -----------------------------------------------------------------------------
+
+function isIdentifier(node, id) {
+  return node && node.type === "Identifier" && node.name === id;
+}
+
+module.exports = function(context) {
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    CallExpression(node) {
+      let { callee } = node;
+      if (
+        isIdentifier(callee.object, "ChromeUtils") &&
+        isIdentifier(callee.property, "import") &&
+        node.arguments.length >= 2
+      ) {
+        if (
+          node.arguments[1].type == "Literal" &&
+          node.arguments[1].raw == "null"
+        ) {
+          context.report(
+            node,
+            "ChromeUtils.import should not be called with (..., null) to " +
+              "retrieve the JSM global object. Rely on explicit exports instead."
+          );
+        } else if (node.arguments[1].type == "ThisExpression") {
+          context.report({
+            node,
+            message:
+              "ChromeUtils.import should not be called with (..., this) to " +
+              "retrieve the JSM global object. Use destructuring instead.",
+            suggest: [
+              {
+                desc: "Use destructuring for imports.",
+                fix: fixer => {
+                  let source = context.getSourceCode().getText(node);
+                  let match = source.match(
+                    /ChromeUtils.import\(\s*(".*\/(.*).jsm?")/m
+                  );
+
+                  return fixer.replaceText(
+                    node,
+                    `const { ${match[2]} } = ChromeUtils.import(${match[1]})`
+                  );
+                },
+              },
+            ],
+          });
+        }
+      }
+    },
+  };
+};

tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-params.js

@@ -0,0 +1,86 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/reject-chromeutils-import-params");
+var RuleTester = require("eslint").RuleTester;
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 8 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidError() {
+  let message =
+    "ChromeUtils.import should not be called with (..., null) to " +
+    "retrieve the JSM global object. Rely on explicit exports instead.";
+  return [{ message, type: "CallExpression" }];
+}
+
+ruleTester.run("reject-chromeutils-import-params", rule, {
+  valid: ['ChromeUtils.import("resource://some/path/to/My.jsm")'],
+  invalid: [
+    {
+      code: 'ChromeUtils.import("resource://some/path/to/My.jsm", null)',
+      errors: invalidError(),
+    },
+    {
+      code: `
+ChromeUtils.import(
+  "resource://some/path/to/My.jsm",
+  null
+);`,
+      errors: invalidError(),
+    },
+    {
+      code: 'ChromeUtils.import("resource://some/path/to/My.jsm", this)',
+      errors: [
+        {
+          suggestions: [
+            {
+              desc: "Use destructuring for imports.",
+              output: `const { My } = ChromeUtils.import("resource://some/path/to/My.jsm")`,
+            },
+          ],
+        },
+      ],
+    },
+    {
+      code: 'ChromeUtils.import("resource://some/path/to/My.js", this)',
+      errors: [
+        {
+          suggestions: [
+            {
+              desc: "Use destructuring for imports.",
+              output: `const { My } = ChromeUtils.import("resource://some/path/to/My.js")`,
+            },
+          ],
+        },
+      ],
+    },
+    {
+      code: `
+ChromeUtils.import(
+  "resource://some/path/to/My.jsm",
+  this
+);`,
+      errors: [
+        {
+          suggestions: [
+            {
+              desc: "Use destructuring for imports.",
+              output: `
+const { My } = ChromeUtils.import("resource://some/path/to/My.jsm");`,
+            },
+          ],
+        },
+      ],
+    },
+  ],
+});