From 67bf517e00ed3c11d3cca8e7ddef4a2cdf8e10d5 Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Thu, 11 Feb 2021 22:02:16 +0000 Subject: [PATCH] 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 | 2 +- .../lint/linters/eslint-plugin-mozilla.rst | 1 + .../reject-chromeutils-import-params.rst | 26 ++++++ .../lib/configs/recommended.js | 2 +- .../eslint/eslint-plugin-mozilla/lib/index.js | 2 +- .../rules/reject-chromeutils-import-null.js | 44 ---------- .../rules/reject-chromeutils-import-params.js | 70 +++++++++++++++ .../tests/reject-chromeutils-import-null.js | 42 --------- .../tests/reject-chromeutils-import-params.js | 86 +++++++++++++++++++ 9 files changed, 186 insertions(+), 89 deletions(-) create mode 100644 docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-chromeutils-import-params.rst delete mode 100644 tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-null.js create mode 100644 tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-params.js delete mode 100644 tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-null.js create mode 100644 tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-params.js diff --git a/.eslintrc.js b/.eslintrc.js index b772e75ec301..c08a1f659e07 100644 --- a/.eslintrc.js +++ b/.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", }, }, ], diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst index bd844f752a82..131218f34241 100644 --- a/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst +++ b/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 diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-chromeutils-import-params.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/reject-chromeutils-import-params.rst new file mode 100644 index 000000000000..cccf24ecd6b3 --- /dev/null +++ b/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"); diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js index 72023fd0b91f..47890201cd99 100644 --- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js +++ b/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", diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js index c05cb9a08913..13c5852bf1ae 100644 --- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js +++ b/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"), diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-null.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-null.js deleted file mode 100644 index b7fd1593b81f..000000000000 --- a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-null.js +++ /dev/null @@ -1,44 +0,0 @@ -/** - * @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 && - 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." - ); - } - }, - }; -}; diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-params.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-chromeutils-import-params.js new file mode 100644 index 000000000000..f8ebfbafe6a5 --- /dev/null +++ b/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]})` + ); + }, + }, + ], + }); + } + } + }, + }; +}; diff --git a/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-null.js b/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-null.js deleted file mode 100644 index 6584f728ac5d..000000000000 --- a/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-null.js +++ /dev/null @@ -1,42 +0,0 @@ -/* 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-null"); -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-null", 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(), - }, - ], -}); diff --git a/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-params.js b/tools/lint/eslint/eslint-plugin-mozilla/tests/reject-chromeutils-import-params.js new file mode 100644 index 000000000000..2f173f985bd2 --- /dev/null +++ b/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");`, + }, + ], + }, + ], + }, + ], +});