Bug 1344711 - Add an eslint rule to report an error when a get*Pref call is the only instruction in a try block, r=jaws.
authorFlorian Quèze <florian@queze.net>
Tue, 07 Mar 2017 15:29:47 +0100
changeset 394478 4a36e117f41369e0045582d15401b765ad064a6b
parent 394477 7ebcd45634eef3711dccf68e4e1390134d48b63b
child 394479 bff53385b4941566f66332ddb51d339341069f32
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1344711
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1344711 - Add an eslint rule to report an error when a get*Pref call is the only instruction in a try block, r=jaws.
.eslintrc.js
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-default-preference-values.js
tools/lint/eslint/eslint-plugin-mozilla/package.json
tools/lint/eslint/eslint-plugin-mozilla/tests/no-useless-parameters.js
tools/lint/eslint/eslint-plugin-mozilla/tests/use-default-preference-values.js
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -6,16 +6,17 @@ module.exports = {
     "mozilla"
   ],
   "rules": {
     "mozilla/avoid-removeChild": "error",
     "mozilla/import-globals": "warn",
     "mozilla/no-import-into-var-and-global": "error",
     "mozilla/no-useless-parameters": "error",
     "mozilla/no-useless-removeEventListener": "error",
+    "mozilla/use-default-preference-values": "error",
     "mozilla/use-ownerGlobal": "error",
 
     // No (!foo in bar) or (!object instanceof Class)
     "no-unsafe-negation": "error",
   },
   "env": {
     "es6": true
   },
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -192,16 +192,22 @@ this-top-level-scope
 Treats top-level assignments like ``this.mumble = value`` as declaring a global.
 
 Note: These are string matches so we will miss situations where the parent
 object is assigned to another variable e.g.::
 
    var b = gBrowser;
    b.content // Would not be detected as a CPOW.
 
+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.
 
 
 var-only-at-top-level
 ---------------------
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -35,16 +35,18 @@ module.exports = {
     "no-import-into-var-and-global":
       require("../lib/rules/no-import-into-var-and-global.js"),
     "no-useless-parameters": require("../lib/rules/no-useless-parameters"),
     "no-useless-removeEventListener":
       require("../lib/rules/no-useless-removeEventListener"),
     "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"),
     "var-only-at-top-level": require("../lib/rules/var-only-at-top-level")
   },
   rulesConfig: {
     "avoid-removeChild": 0,
     "balanced-listeners": 0,
     "import-globals": 0,
     "import-headjs-globals": 0,
@@ -52,12 +54,13 @@ module.exports = {
     "no-aArgs": 0,
     "no-cpows-in-tests": 0,
     "no-single-arg-cu-import": 0,
     "no-import-into-var-and-global": 0,
     "no-useless-parameters": 0,
     "no-useless-removeEventListener": 0,
     "reject-importGlobalProperties": 0,
     "reject-some-requires": 0,
+    "use-default-preference-values": 0,
     "use-ownerGlobal": 0,
     "var-only-at-top-level": 0
   }
 };
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js
@@ -34,18 +34,17 @@ module.exports = function(context) {
           node.arguments.length === 3) {
         let arg = node.arguments[2];
         if (arg.type === "Literal" && arg.value === false) {
           context.report(node, callee.property.name +
                          "'s third parameter can be omitted when it's false.");
         }
       }
 
-      if ((["getCharPref", "getBoolPref", "getIntPref", "clearUserPref"]
-           .indexOf(callee.property.name) != -1) &&
+      if (callee.property.name == "clearUserPref" &&
           node.arguments.length > 1) {
         context.report(node, callee.property.name + " takes only 1 parameter.");
       }
 
       if (callee.property.name === "removeObserver" &&
           node.arguments.length === 3) {
         let arg = node.arguments[2];
         if (arg.type === "Literal" && (arg.value === false ||
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-default-preference-values.js
@@ -0,0 +1,47 @@
+/**
+ * @fileoverview Require providing a second parameter to get*Pref
+ * methods instead of using a try/catch block.
+ *
+ * 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
+// -----------------------------------------------------------------------------
+
+var helpers = require("../helpers");
+
+module.exports = function(context) {
+
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    "TryStatement": function(node) {
+      let types = ["Bool", "Char", "Float", "Int"];
+      let methods = types.map(type => "get" + type + "Pref");
+      if (node.block.type != "BlockStatement" ||
+          node.block.body.length != 1) {
+        return;
+      }
+
+      let firstStm = node.block.body[0];
+      if (firstStm.type != "ExpressionStatement" ||
+          firstStm.expression.type != "AssignmentExpression" ||
+          firstStm.expression.right.type != "CallExpression" ||
+          firstStm.expression.right.callee.type != "MemberExpression" ||
+          firstStm.expression.right.callee.property.type != "Identifier" ||
+          !methods.includes(firstStm.expression.right.callee.property.name)) {
+        return;
+      }
+
+      let msg = "provide a default value instead of using a try/catch block";
+      context.report(node, msg);
+    }
+  };
+};
--- 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.2.25",
+  "version": "0.2.26",
   "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/tests/no-useless-parameters.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/no-useless-parameters.js
@@ -16,19 +16,16 @@ var rule = require("../lib/rules/no-usel
 function callError(message) {
   return [{message: message, type: "CallExpression"}];
 }
 
 exports.runTest = function(ruleTester) {
   ruleTester.run("no-useless-parameters", rule, {
     valid: [
       "Services.prefs.clearUserPref('browser.search.custom');",
-      "Services.prefs.getBoolPref('browser.search.custom');",
-      "Services.prefs.getCharPref('browser.search.custom');",
-      "Services.prefs.getIntPref('browser.search.custom');",
       "Services.removeObserver('notification name', {});",
       "Services.io.newURI('http://example.com');",
       "Services.io.newURI('http://example.com', 'utf8');",
       "elt.addEventListener('click', handler);",
       "elt.addEventListener('click', handler, true);",
       "elt.addEventListener('click', handler, {once: true});",
       "elt.removeEventListener('click', handler);",
       "elt.removeEventListener('click', handler, true);",
@@ -36,28 +33,16 @@ exports.runTest = function(ruleTester) {
       "window.getComputedStyle(elt, ':before');"
     ],
     invalid: [
       {
         code: "Services.prefs.clearUserPref('browser.search.custom', false);",
         errors: callError("clearUserPref takes only 1 parameter.")
       },
       {
-        code: "Services.prefs.getBoolPref('browser.search.custom', true);",
-        errors: callError("getBoolPref takes only 1 parameter.")
-      },
-      {
-        code: "Services.prefs.getCharPref('browser.search.custom', 'a');",
-        errors: callError("getCharPref takes only 1 parameter.")
-      },
-      {
-        code: "Services.prefs.getIntPref('browser.search.custom', 42);",
-        errors: callError("getIntPref takes only 1 parameter.")
-      },
-      {
         code: "Services.removeObserver('notification name', {}, false);",
         errors: callError("removeObserver only takes 2 parameters.")
       },
       {
         code: "Services.removeObserver('notification name', {}, true);",
         errors: callError("removeObserver only takes 2 parameters.")
       },
       {
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/use-default-preference-values.js
@@ -0,0 +1,38 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/use-default-preference-values");
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+function invalidCode(code) {
+  let message = "provide a default value instead of using a try/catch block";
+  return {code: code, errors: [{message: message, type: "TryStatement"}]};
+}
+
+let types = ["Bool", "Char", "Float", "Int"];
+let methods = types.map(type => "get" + type + "Pref");
+
+exports.runTest = function(ruleTester) {
+  ruleTester.run("use-ownerGlobal", rule, {
+    valid: [].concat(
+      methods.map(m => "blah = branch." + m + "('blah', true);"),
+      methods.map(m => "blah = branch." + m + "('blah');"),
+      methods.map(m => "try { canThrow();" +
+                            " blah = branch." + m + "('blah'); } catch(e) {}")
+    ),
+
+    invalid: [].concat(
+      methods.map(m =>
+        invalidCode("try { blah = branch." + m + "('blah'); } catch(e) {}"))
+    )
+  });
+};