Bug 1230373 - Add an ESLint rule to prefer using Services.jsm rather than getService. r?Mossop draft
authorMark Banner <standard8@mozilla.com>
Fri, 06 Oct 2017 17:03:38 +0100
changeset 680871 cf161b4ff7165e2c5045a9f2c092a58aff7142e6
parent 680870 f80aae22b52bcef3a54d88a759898fc1ea496b11
child 680872 ec78c6f44b6691e0c153930ec9c90089fdd7cf3c
push id84660
push userbmo:standard8@mozilla.com
push dateMon, 16 Oct 2017 14:18:24 +0000
reviewersMossop
bugs1230373
milestone58.0a1
Bug 1230373 - Add an ESLint rule to prefer using Services.jsm rather than getService. r?Mossop MozReview-Commit-ID: G9dp4PxcyT7
toolkit/modules/Services.jsm
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-services.js
tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
tools/lint/eslint/eslint-plugin-mozilla/package.json
tools/lint/eslint/eslint-plugin-mozilla/scripts/createExports.js
tools/lint/eslint/eslint-plugin-mozilla/tests/use-services.js
--- a/toolkit/modules/Services.jsm
+++ b/toolkit/modules/Services.jsm
@@ -8,16 +8,21 @@ const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cr = Components.results;
 
 Components.utils.import("resource://gre/modules/AppConstants.jsm");
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
 
 this.Services = {};
 
+/**
+ * WARNING: If you add a getter that isn't in the initTable, please update the
+ * eslint rule in /tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-services.js
+ */
+
 XPCOMUtils.defineLazyGetter(Services, "prefs", function() {
   return Cc["@mozilla.org/preferences-service;1"]
            .getService(Ci.nsIPrefService)
            .QueryInterface(Ci.nsIPrefBranch);
 });
 
 XPCOMUtils.defineLazyGetter(Services, "appinfo", function() {
   let appinfo = Cc["@mozilla.org/xre/app-info;1"]
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -252,16 +252,21 @@ use-default-preference-values
 Require providing a second parameter to get*Pref methods instead of
 using a try/catch block.
 
 use-ownerGlobal
 ---------------
 
 Require .ownerGlobal instead of .ownerDocument.defaultView.
 
+use-services
+------------
+
+Requires the use of Services.jsm rather than Cc[].getService() where a service
+is already defined in Services.jsm.
 
 var-only-at-top-level
 ---------------------
 
 Marks all var declarations that are not at the top level invalid.
 
 
 Example
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
@@ -667,10 +667,14 @@ module.exports = {
   },
 
   isMozillaCentralBased() {
     return fs.existsSync(this.globalScriptsPath);
   },
 
   getSavedEnvironmentItems(environment) {
     return require("./environments/saved-globals.json").environments[environment];
+  },
+
+  getSavedRuleData(rule) {
+    return require("./rules/saved-rules-data.json").rulesData[rule];
   }
 };
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -55,16 +55,17 @@ module.exports = {
     "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-default-preference-values":
       require("../lib/rules/use-default-preference-values"),
     "use-ownerGlobal": require("../lib/rules/use-ownerGlobal"),
+    "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",
     "avoid-nsISupportsString-preferences": "off",
     "balanced-listeners": "off",
     "import-browser-window-globals": "off",
@@ -80,11 +81,12 @@ module.exports = {
     "no-task": "off",
     "no-useless-parameters": "off",
     "no-useless-run-test": "off",
     "no-useless-removeEventListener": "off",
     "reject-importGlobalProperties": "off",
     "reject-some-requires": "off",
     "use-default-preference-values": "off",
     "use-ownerGlobal": "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/use-services.js
@@ -0,0 +1,157 @@
+/**
+ * @fileoverview Require use of Services.* rather than getService.
+ *
+ * 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";
+const helpers = require("../helpers");
+const fs = require("fs");
+const path = require("path");
+
+/**
+ * An object that is used to help parse the AST of Services.jsm to extract
+ * the services that it caches.
+ *
+ * It is specifically built around the current structure of the Services.jsm
+ * file.
+ */
+var servicesASTParser = {
+  identifiers: {},
+  // These interfaces are difficult/not possible to get via processing.
+  result: {
+    "nsIPrefBranch": "prefs",
+    "nsIPrefService": "prefs",
+    "nsIXULRuntime": "appInfo",
+    "nsIXULAppInfo": "appInfo",
+    "nsIDirectoryService": "dirsvc",
+    "nsIProperties": "dirsvc",
+    "nsIFrameScriptLoader": "mm",
+    "nsIProcessScriptLoader": "ppmm",
+    "nsIIOService": "io",
+    "nsIIOService2": "io",
+    "nsISpeculativeConnect": "io",
+    // Bug 1407720 - Services lists nsICookieManager2, but that inherits directly
+    // from nsICookieManager, so we have to list it separately.
+    "nsICookieManager": "cookies"
+  },
+
+  /**
+   * This looks for any global variable declarations that are being initialised
+   * with objects, and records the assumed interface definitions.
+   */
+  VariableDeclaration(node, parents) {
+    if (node.declarations.length === 1 &&
+        node.declarations[0].id &&
+        helpers.getIsGlobalScope(parents) &&
+        node.declarations[0].init.type === "ObjectExpression") {
+      let name = node.declarations[0].id.name;
+      let interfaces = {};
+
+      for (let property of node.declarations[0].init.properties) {
+        interfaces[property.key.name] = property.value.elements[1].value;
+      }
+
+      this.identifiers[name] = interfaces;
+    }
+  },
+
+  /**
+   * This looks for any additions to the global variable declarations, and adds
+   * them to the identifier tables created by the VariableDeclaration calls.
+   */
+  AssignmentExpression(node, parents) {
+    if (node.left.type === "MemberExpression" &&
+        node.right.type === "ArrayExpression" &&
+        helpers.getIsGlobalScope(parents)) {
+      let variableName = node.left.object.name;
+      if (variableName in this.identifiers) {
+        let servicesPropName = node.left.property.name;
+        this.identifiers[variableName][servicesPropName] = node.right.elements[1].value;
+      }
+    }
+  },
+
+  /**
+   * This looks for any XPCOMUtils.defineLazyServiceGetters calls, and looks
+   * into the arguments they are called with to work out the interfaces that
+   * Services.jsm is caching.
+   */
+  CallExpression(node) {
+    if (node.callee.object &&
+        node.callee.object.name === "XPCOMUtils" &&
+        node.callee.property &&
+        node.callee.property.name === "defineLazyServiceGetters" &&
+        node.arguments.length >= 2) {
+      // The second argument has the getters name.
+      let gettersVarName = node.arguments[1].name;
+      if (!(gettersVarName in this.identifiers)) {
+        throw new Error(`Could not find definition for ${gettersVarName}`);
+      }
+
+      for (let name of Object.keys(this.identifiers[gettersVarName])) {
+        this.result[this.identifiers[gettersVarName][name]] = name;
+      }
+    }
+  }
+};
+
+function getInterfacesFromServicesFile() {
+  let filePath = path.join(helpers.rootDir, "toolkit", "modules", "Services.jsm");
+
+  let content = fs.readFileSync(filePath, "utf8");
+
+  // Parse the content into an AST
+  let ast = helpers.getAST(content);
+
+  helpers.walkAST(ast, (type, node, parents) => {
+    if (type in servicesASTParser) {
+      servicesASTParser[type](node, parents);
+    }
+  });
+
+  return servicesASTParser.result;
+}
+
+let getServicesInterfaceMap = helpers.isMozillaCentralBased() ?
+  getInterfacesFromServicesFile() :
+  helpers.getSavedRuleData("use-services.js");
+
+// -----------------------------------------------------------------------------
+// Rule Definition
+// -----------------------------------------------------------------------------
+module.exports = function(context) {
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+  return {
+    // ESLint assumes that items returned here are going to be a listener
+    // for part of the rule. We want to export the servicesInterfaceArray for
+    // the purposes of createExports, so we return it via a function which
+    // makes everything happy.
+    getServicesInterfaceMap() {
+      return getServicesInterfaceMap;
+    },
+
+    CallExpression(node) {
+      if (!node.callee ||
+          !node.callee.property ||
+          node.callee.property.type != "Identifier" ||
+          node.callee.property.name != "getService" ||
+          node.arguments.length != 1 ||
+          !node.arguments[0].property ||
+          node.arguments[0].property.type != "Identifier" ||
+          !node.arguments[0].property.name ||
+          !(node.arguments[0].property.name in getServicesInterfaceMap)) {
+        return;
+      }
+
+      let serviceName = getServicesInterfaceMap[node.arguments[0].property.name];
+
+      context.report(node,
+                     `Use Services.${serviceName} rather than getService().`);
+    }
+  };
+};
--- 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.4.4",
+  "version": "0.4.5",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
     "acorn": {
       "version": "5.1.2",
       "resolved": "https://registry.npmjs.org/acorn/-/acorn-5.1.2.tgz",
       "integrity": "sha512-o96FZLJBPY1lvTuJylGA9Bk3t/GKPPJG8H0ydQQl01crzwJgspa4AEIq/pVTXigmK0PHVQhiAtn8WMBLL9D2WA==",
       "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.4.4",
+  "version": "0.4.5",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],
--- a/tools/lint/eslint/eslint-plugin-mozilla/scripts/createExports.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/scripts/createExports.js
@@ -10,30 +10,47 @@ var fs = require("fs");
 var path = require("path");
 var helpers = require("../lib/helpers");
 
 const eslintDir = path.join(helpers.rootDir,
                             "tools", "lint", "eslint");
 
 const globalsFile = path.join(eslintDir, "eslint-plugin-mozilla",
                               "lib", "environments", "saved-globals.json");
+const rulesFile = path.join(eslintDir, "eslint-plugin-mozilla",
+                            "lib", "rules", "saved-rules-data.json");
 
 console.log("Copying modules.json");
 
 const modulesFile = path.join(eslintDir, "modules.json");
 const shipModulesFile = path.join(eslintDir, "eslint-plugin-mozilla", "lib",
                                   "modules.json");
 
 fs.writeFileSync(shipModulesFile, fs.readFileSync(modulesFile));
 
 console.log("Generating globals file");
 
-// We only export the configs section.
+// Export the environments.
 let environmentGlobals = require("../lib/index.js").environments;
 
 return fs.writeFile(globalsFile, JSON.stringify({environments: environmentGlobals}), err => {
   if (err) {
     console.error(err);
     process.exit(1);
   }
 
   console.log("Globals file generation complete");
+
+  console.log("Creating rules data file");
+  // Also export data for the use-services.js rule
+  let rulesData = {
+    "use-services.js": require("../lib/rules/use-services.js")().getServicesInterfaceMap()
+  };
+
+  return fs.writeFile(rulesFile, JSON.stringify({rulesData}), err1 => {
+    if (err1) {
+      console.error(err1);
+      process.exit(1);
+    }
+
+    console.log("Globals file generation complete");
+  });
 });
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/use-services.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-services");
+var RuleTester = require("eslint/lib/testers/rule-tester");
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidCode(code, name) {
+  let message = `Use Services.${name} rather than getService().`;
+  return {code, errors: [{message, type: "CallExpression"}]};
+}
+
+ruleTester.run("use-services", rule, {
+  valid: [
+    'Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator)',
+    'Components.classes["@mozilla.org/uuid-generator;1"].getService(Components.interfaces.nsIUUIDGenerator)',
+    "Services.wm.addListener()"
+  ],
+  invalid: [
+    invalidCode('Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);',
+      "wm"),
+    invalidCode(
+      'Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup);',
+      "startup")
+  ]
+});