Bug 1344711 - Add an eslint rule to report an error when a get*Pref c…

…all is the only instruction in a try block, r=jaws.

--HG--
extra : rebase_source : b98fc9c75089c3eeb2f1317623b08ee9cd4d1541

.eslintrc.js

@@ -11,6 +11,7 @@ module.exports = {
     "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)

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

@@ -197,6 +197,12 @@ 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
 ---------------
 

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

@@ -40,6 +40,8 @@ module.exports = {
     "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")
   },
@@ -57,6 +59,7 @@ module.exports = {
     "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
   }

tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js

@@ -39,8 +39,7 @@ module.exports = function(context) {
         }
       }
 
-      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.");
       }

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

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

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

tools/lint/eslint/eslint-plugin-mozilla/tests/no-useless-parameters.js

@@ -21,9 +21,6 @@ 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');",
@@ -40,18 +37,6 @@ exports.runTest = function(ruleTester) {
         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.")

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