Bug 1230373 - Add an ESLint rule to prefer using Services.jsm rather than getService. r=mossop

MozReview-Commit-ID: G9dp4PxcyT7

--HG--
extra : rebase_source : 957b5ead56c8c778b1ba812343c132b34030135f

toolkit/modules/Services.jsm

@@ -13,6 +13,11 @@ 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)

tools/lint/docs/linters/eslint-plugin-mozilla.rst

@@ -257,6 +257,11 @@ 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
 ---------------------

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

@@ -672,5 +672,9 @@ module.exports = {
 
   getSavedEnvironmentItems(environment) {
     return require("./environments/saved-globals.json").environments[environment];
+  },
+
+  getSavedRuleData(rule) {
+    return require("./rules/saved-rules-data.json").rulesData[rule];
   }
 };

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

@@ -60,6 +60,7 @@ module.exports = {
     "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: {
@@ -85,6 +86,7 @@ module.exports = {
     "reject-some-requires": "off",
     "use-default-preference-values": "off",
     "use-ownerGlobal": "off",
+    "use-services": "off",
     "var-only-at-top-level": "off"
   }
 };

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().`);
+    }
+  };
+};

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

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

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

@@ -1,6 +1,6 @@
 {
   "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",

tools/lint/eslint/eslint-plugin-mozilla/scripts/createExports.js

@@ -15,6 +15,8 @@ const eslintDir = path.join(helpers.rootDir,
 
 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");
 
@@ -26,7 +28,7 @@ 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 => {
@@ -36,4 +38,19 @@ return fs.writeFile(globalsFile, JSON.stringify({environments: environmentGlobal
   }
 
   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");
+  });
 });

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")
+  ]
+});