Bug 1382647 - Improve eslint-plugin-mozilla's performance when searching for globals by avoiding rebuilding source when we don't need to. r?Mossop draft
authorMark Banner <standard8@mozilla.com>
Tue, 18 Jul 2017 22:23:58 +0100
changeset 612180 64f6154bab661c043f880fa91242218850acec98
parent 612171 68046a58f82913eb7804e4796ec981f6f8ea490e
child 638341 ff7f9b5444085b7f1780a6c485c04e56bf33d7f1
push id69418
push userbmo:standard8@mozilla.com
push dateThu, 20 Jul 2017 13:17:49 +0000
reviewersMossop
bugs1382647
milestone56.0a1
Bug 1382647 - Improve eslint-plugin-mozilla's performance when searching for globals by avoiding rebuilding source when we don't need to. r?Mossop MozReview-Commit-ID: 84uHuepWhZR
tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js
tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-test-function-used.js
tools/lint/eslint/eslint-plugin-mozilla/package.json
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js
@@ -117,21 +117,25 @@ GlobalsForNode.prototype = {
       globals = globals.concat(module.exports.getGlobalsForFile(filePath));
     }
 
     return globals;
   },
 
   ExpressionStatement(node, parents, globalScope) {
     let isGlobal = helpers.getIsGlobalScope(parents);
-    let globals = helpers.convertExpressionToGlobals(node, isGlobal);
-    // Map these globals now, as getGlobalsForFile is pre-mapped.
-    globals = globals.map(name => {
-      return { name, writable: true };
-    });
+    let globals = [];
+
+    // Note: We check the expression types here and only call the necessary
+    // functions to aid performance.
+    if (node.expression.type === "AssignmentExpression") {
+      globals = helpers.convertThisAssignmentExpressionToGlobals(node, isGlobal);
+    } else if (node.expression.type === "CallExpression") {
+      globals = helpers.convertCallExpressionToGlobals(node, isGlobal);
+    }
 
     // Here we assume that if importScripts is set in the global scope, then
     // this is a worker. It would be nice if eslint gave us a way of getting
     // the environment directly.
     if (globalScope && globalScope.set.get("importScripts")) {
       let workerDetails = helpers.convertWorkerExpressionToGlobals(node,
         isGlobal, this.dirname);
       globals = globals.concat(workerDetails);
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
@@ -1,49 +1,48 @@
 /**
  * @fileoverview A collection of helper functions.
  * 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";
 
-var espree = require("espree");
-var estraverse = require("estraverse");
-var path = require("path");
-var fs = require("fs");
-var ini = require("ini-parser");
+const espree = require("espree");
+const estraverse = require("estraverse");
+const path = require("path");
+const fs = require("fs");
+const ini = require("ini-parser");
 
 var gModules = null;
 var gRootDir = null;
 var directoryManifests = new Map();
 
-var definitions = [
+const callExpressionDefinitions = [
   /^loader\.lazyGetter\(this, "(\w+)"/,
   /^loader\.lazyImporter\(this, "(\w+)"/,
   /^loader\.lazyServiceGetter\(this, "(\w+)"/,
   /^loader\.lazyRequireGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyModuleGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyPreferenceGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyServiceGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineConstant\(this, "(\w+)"/,
   /^DevToolsUtils\.defineLazyModuleGetter\(this, "(\w+)"/,
   /^DevToolsUtils\.defineLazyGetter\(this, "(\w+)"/,
   /^Object\.defineProperty\(this, "(\w+)"/,
   /^Reflect\.defineProperty\(this, "(\w+)"/,
-  /^this\.__defineGetter__\("(\w+)"/,
-  /^this\.(\w+) =/
+  /^this\.__defineGetter__\("(\w+)"/
 ];
 
-var imports = [
+const imports = [
   /^(?:Cu|Components\.utils)\.import\(".*\/((.*?)\.jsm?)"(?:, this)?\)/
 ];
 
-var workerImportFilenameMatch = /(.*\/)*(.*?\.jsm?)/;
+const workerImportFilenameMatch = /(.*\/)*(.*?\.jsm?)/;
 
 module.exports = {
   get modulesGlobalData() {
     if (!gModules) {
       if (this.isMozillaCentralBased()) {
         gModules = require(path.join(this.rootDir, "tools", "lint", "eslint", "modules.json"));
       } else {
         gModules = require("./modules.json");
@@ -191,50 +190,96 @@ module.exports = {
           }
         }
       }
     }
 
     return results;
   },
 
-  convertExpressionToGlobals(node, isGlobal) {
+  /**
+   * Attempts to convert an AssignmentExpression into a global variable
+   * definition if it applies to `this` in the global scope.
+   *
+   * @param  {Object} node
+   *         The AST node to convert.
+   * @param  {boolean} isGlobal
+   *         True if the current node is in the global scope.
+   *
+   * @return {Array}
+   *         An array of objects that contain details about the globals:
+   *         - {String} name
+   *                    The name of the global.
+   *         - {Boolean} writable
+   *                     If the global is writeable or not.
+   */
+  convertThisAssignmentExpressionToGlobals(node, isGlobal) {
+    if (isGlobal &&
+        node.expression.left &&
+        node.expression.left.object &&
+        node.expression.left.object.type === "ThisExpression" &&
+        node.expression.left.property &&
+        node.expression.left.property.type === "Identifier") {
+      return [{ name: node.expression.left.property.name, writable: true }];
+    }
+    return [];
+  },
+
+  /**
+   * Attempts to convert an CallExpressions that look like module imports
+   * into global variable definitions, using modules.json data if appropriate.
+   *
+   * @param  {Object} node
+   *         The AST node to convert.
+   * @param  {boolean} isGlobal
+   *         True if the current node is in the global scope.
+   *
+   * @return {Array}
+   *         An array of objects that contain details about the globals:
+   *         - {String} name
+   *                    The name of the global.
+   *         - {Boolean} writable
+   *                     If the global is writeable or not.
+   */
+  convertCallExpressionToGlobals(node, isGlobal) {
+    let source;
     try {
-      var source = this.getASTSource(node);
+      source = this.getASTSource(node);
     } catch (e) {
       return [];
     }
 
-    for (var reg of definitions) {
-      let match = source.match(reg);
-      if (match) {
-        // Must be in the global scope
-        if (!isGlobal) {
-          return [];
-        }
-
-        return [match[1]];
-      }
-    }
-
-    let globalModules = this.modulesGlobalData;
-
-    for (reg of imports) {
+    for (let reg of imports) {
       let match = source.match(reg);
       if (match) {
         // The two argument form is only acceptable in the global scope
         if (node.expression.arguments.length > 1 && !isGlobal) {
           return [];
         }
 
+        let globalModules = this.modulesGlobalData;
+
         if (match[1] in globalModules) {
-          return globalModules[match[1]];
+          return globalModules[match[1]].map(name => ({ name, writable: true }));
         }
 
-        return [match[2]];
+        return [{ name: match[2], writable: true }];
+      }
+    }
+
+    // The definition matches below must be in the global scope for us to define
+    // a global, so bail out early if we're not a global.
+    if (!isGlobal) {
+      return [];
+    }
+
+    for (let reg of callExpressionDefinitions) {
+      let match = source.match(reg);
+      if (match) {
+        return [{ name: match[1], writable: true }];
       }
     }
 
     return [];
   },
 
   /**
    * Add a variable to the current scope.
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-test-function-used.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-test-function-used.js
@@ -18,20 +18,20 @@ var helpers = require("../helpers");
 
 module.exports = function(context) {
   // ---------------------------------------------------------------------------
   // Public
   // ---------------------------------------------------------------------------
 
   return {
     Program() {
-      if (helpers.getTestType(context) == "browser") {
+      let testType = helpers.getTestType(context);
+      if (testType == "browser") {
         context.markVariableAsUsed("test");
         return;
       }
 
-      if (helpers.getTestType(context) == "xpcshell") {
+      if (testType == "xpcshell") {
         context.markVariableAsUsed("run_test");
-
       }
     }
   };
 };
--- 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.1",
+  "version": "0.4.2",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],