Bug 1521170 - Add a rule that prevents calling some Array and String accessor methods without using the return value. r=Standard8,Gijs
authorJared Wein <jwein@mozilla.com>
Wed, 23 Jan 2019 17:03:32 +0000
changeset 515136 f6094ca026ad
parent 515135 70d0497c3cb6
child 515137 46b19ac773fa
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8, Gijs
bugs1521170
milestone66.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 1521170 - Add a rule that prevents calling some Array and String accessor methods without using the return value. r=Standard8,Gijs Differential Revision: https://phabricator.services.mozilla.com/D17020
browser/actors/PageInfoChild.jsm
devtools/client/shared/widgets/VariablesViewController.jsm
devtools/client/webconsole/actions/messages.js
devtools/server/actors/inspector/walker.js
docshell/test/unit/test_nsDefaultURIFixup_info.js
services/fxaccounts/FxAccountsConfig.jsm
toolkit/components/ctypes/tests/unit/test_jsctypes.js
toolkit/mozapps/update/tests/data/shared.js
tools/lint/docs/linters/eslint-plugin-mozilla.rst
tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-returnValue.js
tools/lint/eslint/eslint-plugin-mozilla/package-lock.json
tools/lint/eslint/eslint-plugin-mozilla/package.json
tools/lint/eslint/eslint-plugin-mozilla/tests/use-returnValue.js
--- a/browser/actors/PageInfoChild.jsm
+++ b/browser/actors/PageInfoChild.jsm
@@ -103,17 +103,17 @@ class PageInfoChild extends ActorChild {
   }
 
   goThroughFrames(document, window) {
     let frameList = [document];
     if (window && window.frames.length > 0) {
       let num = window.frames.length;
       for (let i = 0; i < num; i++) {
         // Recurse through the frames.
-        frameList.concat(this.goThroughFrames(window.frames[i].document,
+        frameList = frameList.concat(this.goThroughFrames(window.frames[i].document,
                                               window.frames[i]));
       }
     }
     return frameList;
   }
 
   async processFrames(document, frameList, strings, mm) {
     let nodeCount = 0;
--- a/devtools/client/shared/widgets/VariablesViewController.jsm
+++ b/devtools/client/shared/widgets/VariablesViewController.jsm
@@ -214,16 +214,17 @@ VariablesViewController.prototype = {
    */
   _populateFromPropertyIterator: function(aTarget, aGrip) {
     if (aGrip.count >= MAX_PROPERTY_ITEMS) {
       // We already started to split, but there is still too many properties, split again.
       return this._populatePropertySlices(aTarget, aGrip);
     }
     // We started slicing properties, and the slice is now small enough to be displayed
     const deferred = defer();
+    // eslint-disable-next-line mozilla/use-returnValue
     aGrip.propertyIterator.slice(aGrip.start, aGrip.count,
       ({ ownProperties }) => {
         // Add all the variable properties.
         if (Object.keys(ownProperties).length > 0) {
           aTarget.addItems(ownProperties, {
             sorted: true,
             // Expansion handlers must be set after the properties are added.
             callback: this.addExpander,
--- a/devtools/client/webconsole/actions/messages.js
+++ b/devtools/client/webconsole/actions/messages.js
@@ -93,16 +93,17 @@ function messageTableDataGet(id, client,
     } else {
       fetchObjectActorData = (cb) => client.enumProperties({
         ignoreNonIndexedProperties: dataType === "Array",
       }, cb);
     }
 
     fetchObjectActorData(enumResponse => {
       const {iterator} = enumResponse;
+      // eslint-disable-next-line mozilla/use-returnValue
       iterator.slice(0, iterator.count, sliceResponse => {
         const {ownProperties} = sliceResponse;
         dispatch(messageTableDataReceive(id, ownProperties));
       });
     });
   };
 }
 
--- a/devtools/server/actors/inspector/walker.js
+++ b/devtools/server/actors/inspector/walker.js
@@ -1215,17 +1215,17 @@ var WalkerActor = protocol.ActorClassWit
       } else {
         sortB = "0" + sortB;
       }
 
       // String compare
       return sortA.localeCompare(sortB);
     });
 
-    result.slice(0, 25);
+    result = result.slice(0, 25);
 
     return {
       query: query,
       suggestions: result,
     };
   },
 
   /**
--- a/docshell/test/unit/test_nsDefaultURIFixup_info.js
+++ b/docshell/test/unit/test_nsDefaultURIFixup_info.js
@@ -31,29 +31,24 @@ registerCleanupFunction(function() {
   if (engine) {
     Services.search.removeEngine(engine);
   }
   Services.prefs.clearUserPref("keyword.enabled");
   Services.prefs.clearUserPref("browser.fixup.typo.scheme");
   Services.prefs.clearUserPref(kForceHostLookup);
 });
 
+// TODO(bug 1522134), this test should also use
+// combinations of the following flags.
 var flagInputs = [
   Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP,
   Services.uriFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI,
   Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS,
 ];
 
-flagInputs.concat([
-  flagInputs[0] | flagInputs[1],
-  flagInputs[1] | flagInputs[2],
-  flagInputs[0] | flagInputs[2],
-  flagInputs[0] | flagInputs[1] | flagInputs[2],
-]);
-
 /*
   The following properties are supported for these test cases:
   {
     input: "", // Input string, required
     fixedURI: "", // Expected fixedURI
     alternateURI: "", // Expected alternateURI
     keywordLookup: false, // Whether a keyword lookup is expected
     protocolChange: false, // Whether a protocol change is expected
--- a/services/fxaccounts/FxAccountsConfig.jsm
+++ b/services/fxaccounts/FxAccountsConfig.jsm
@@ -117,17 +117,17 @@ var FxAccountsConfig = {
   getAutoConfigURL() {
     let pref = Services.prefs.getCharPref("identity.fxaccounts.autoconfig.uri", "");
     if (!pref) {
       // no pref / empty pref means we don't bother here.
       return "";
     }
     let rootURL = Services.urlFormatter.formatURL(pref);
     if (rootURL.endsWith("/")) {
-      rootURL.slice(0, -1);
+      rootURL = rootURL.slice(0, -1);
     }
     return rootURL;
   },
 
   async ensureConfigured() {
     await this.tryPrefsMigration();
     let isSignedIn = !!(await this.getSignedInUser());
     if (!isSignedIn) {
--- a/toolkit/components/ctypes/tests/unit/test_jsctypes.js
+++ b/toolkit/components/ctypes/tests/unit/test_jsctypes.js
@@ -453,20 +453,22 @@ function run_Int64_tests() {
   do_check_throws(function() { ctypes.Int64.hi(0); }, TypeError);
 
   // Test ctypes.Int64.join.
   Assert.equal(ctypes.Int64.join(0, 0).toString(), "0");
   Assert.equal(ctypes.Int64.join(0x28590a1c, 0x921de000).toString(16), "28590a1c921de000");
   Assert.equal(ctypes.Int64.join(-0x28590a1d, 0x6de22000).toString(16), "-28590a1c921de000");
   Assert.equal(ctypes.Int64.join(0x7fffffff, 0xffffffff).toString(16), "7fffffffffffffff");
   Assert.equal(ctypes.Int64.join(-0x80000000, 0x00000000).toString(16), "-8000000000000000");
+  /* eslint-disable mozilla/use-returnValue */
   do_check_throws(function() { ctypes.Int64.join(-0x80000001, 0); }, TypeError);
   do_check_throws(function() { ctypes.Int64.join(0x80000000, 0); }, TypeError);
   do_check_throws(function() { ctypes.Int64.join(0, -0x1); }, TypeError);
   do_check_throws(function() { ctypes.Int64.join(0, 0x800000000); }, TypeError);
+  /* eslint-enable mozilla/use-returnValue */
 }
 
 function run_UInt64_tests() {
   do_check_throws(function() { ctypes.UInt64(); }, TypeError);
 
   Assert.ok(ctypes.UInt64.hasOwnProperty("prototype"));
   Assert.ok(ctypes.UInt64.prototype.hasOwnProperty("constructor"));
   Assert.ok(ctypes.UInt64.prototype.constructor === ctypes.UInt64);
@@ -600,20 +602,22 @@ function run_UInt64_tests() {
   do_check_throws(function() { ctypes.UInt64.hi(0); }, TypeError);
 
   // Test ctypes.UInt64.join.
   Assert.equal(ctypes.UInt64.join(0, 0).toString(), "0");
   Assert.equal(ctypes.UInt64.join(0x28590a1c, 0x921de000).toString(16), "28590a1c921de000");
   Assert.equal(ctypes.UInt64.join(0xa8590a1c, 0x921de000).toString(16), "a8590a1c921de000");
   Assert.equal(ctypes.UInt64.join(0xffffffff, 0xffffffff).toString(16), "ffffffffffffffff");
   Assert.equal(ctypes.UInt64.join(0, 0).toString(16), "0");
+  /* eslint-disable mozilla/use-returnValue */
   do_check_throws(function() { ctypes.UInt64.join(-0x1, 0); }, TypeError);
   do_check_throws(function() { ctypes.UInt64.join(0x100000000, 0); }, TypeError);
   do_check_throws(function() { ctypes.UInt64.join(0, -0x1); }, TypeError);
   do_check_throws(function() { ctypes.UInt64.join(0, 0x1000000000); }, TypeError);
+  /* eslint-enable mozilla/use-returnValue */
 }
 
 function run_basic_abi_tests(library, t, name, toprimitive,
                              get_test, set_tests, sum_tests, sum_many_tests) {
   // Test the function call ABI for calls involving the type.
   function declare_fn_cdecl(fn_t, prefix) {
     return library.declare(prefix + name + "_cdecl", fn_t);
   }
--- a/toolkit/mozapps/update/tests/data/shared.js
+++ b/toolkit/mozapps/update/tests/data/shared.js
@@ -422,17 +422,17 @@ function readFileBytes(aFile) {
   while (count > 0) {
     let bytes = bis.readByteArray(Math.min(65535, count));
     data.push(String.fromCharCode.apply(null, bytes));
     count -= bytes.length;
     if (bytes.length == 0) {
       throw "Nothing read from input stream!";
     }
   }
-  data.join("");
+  data = data.join("");
   fis.close();
   return data.toString();
 }
 
 /* Returns human readable status text from the updates.properties bundle */
 function getStatusText(aErrCode) {
   return getString("check_error-" + aErrCode);
 }
--- a/tools/lint/docs/linters/eslint-plugin-mozilla.rst
+++ b/tools/lint/docs/linters/eslint-plugin-mozilla.rst
@@ -271,16 +271,21 @@ use-ownerGlobal
 
 Require .ownerGlobal instead of .ownerDocument.defaultView.
 
 use-includes-instead-of-indexOf
 -------------------------------
 
 Use .includes instead of .indexOf to check if something is in an array or string.
 
+use-returnValue
+---------------
+
+Warn when idempotent methods are called and their return value is unused.
+
 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
 ---------------------
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
@@ -178,16 +178,17 @@ module.exports = {
     "mozilla/reject-importGlobalProperties": ["error", "allownonwebidl"],
     "mozilla/rejects-requires-await": "error",
     "mozilla/use-cc-etc": "error",
     "mozilla/use-chromeutils-generateqi": "error",
     "mozilla/use-chromeutils-import": "error",
     "mozilla/use-default-preference-values": "error",
     "mozilla/use-includes-instead-of-indexOf": "error",
     "mozilla/use-ownerGlobal": "error",
+    "mozilla/use-returnValue": "error",
     "mozilla/use-services": "error",
 
     // Always require parenthesis for new calls
     // "new-parens": "error",
 
     // Use [] instead of Array()
     "no-array-constructor": "error",
 
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ -61,12 +61,13 @@ module.exports = {
     "rejects-requires-await": require("../lib/rules/rejects-requires-await"),
     "use-cc-etc": require("../lib/rules/use-cc-etc"),
     "use-chromeutils-generateqi": require("../lib/rules/use-chromeutils-generateqi"),
     "use-chromeutils-import": require("../lib/rules/use-chromeutils-import"),
     "use-default-preference-values":
       require("../lib/rules/use-default-preference-values"),
     "use-ownerGlobal": require("../lib/rules/use-ownerGlobal"),
     "use-includes-instead-of-indexOf": require("../lib/rules/use-includes-instead-of-indexOf"),
+    "use-returnValue": require("../lib/rules/use-returnValue"),
     "use-services": require("../lib/rules/use-services"),
     "var-only-at-top-level": require("../lib/rules/var-only-at-top-level"),
   },
 };
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-returnValue.js
@@ -0,0 +1,39 @@
+/**
+ * @fileoverview Warn when idempotent methods are called and their return value is unused.
+ *
+ * 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
+// -----------------------------------------------------------------------------
+
+module.exports = function(context) {
+
+  // ---------------------------------------------------------------------------
+  // Public
+  //  --------------------------------------------------------------------------
+
+  return {
+    "ExpressionStatement": function(node) {
+      if (!node.expression ||
+          node.expression.type != "CallExpression" ||
+          !node.expression.callee ||
+          node.expression.callee.type != "MemberExpression" ||
+          !node.expression.callee.property ||
+          node.expression.callee.property.type != "Identifier" ||
+          (node.expression.callee.property.name != "concat" &&
+           node.expression.callee.property.name != "join" &&
+           node.expression.callee.property.name != "slice")) {
+        return;
+      }
+
+      context.report(node,
+                     `{Array/String}.${node.expression.callee.property.name} doesn't modify the instance in-place`);
+    },
+  };
+};
--- 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": "1.0.4",
+  "version": "1.0.5",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
     "@babel/code-frame": {
       "version": "7.0.0",
       "resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.0.0.tgz",
       "integrity": "sha512-OfC2uemaknXr87bdLUkWog7nYuliM9Ij5HUcajsVcMCpQrcLmtxRbVFTIqmcSkSeYRBFBRxs2FiUqFJDLdiebA==",
       "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": "1.0.4",
+  "version": "1.0.5",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],
new file mode 100644
--- /dev/null
+++ b/tools/lint/eslint/eslint-plugin-mozilla/tests/use-returnValue.js
@@ -0,0 +1,39 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// ------------------------------------------------------------------------------
+// Requirements
+// ------------------------------------------------------------------------------
+
+var rule = require("../lib/rules/use-returnValue");
+var RuleTester = require("eslint/lib/testers/rule-tester");
+
+const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
+
+// ------------------------------------------------------------------------------
+// Tests
+// ------------------------------------------------------------------------------
+
+function invalidCode(code, methodName) {
+  let message = `{Array/String}.${methodName} doesn't modify the instance in-place`;
+  return {code, errors: [{message, type: "ExpressionStatement"}]};
+}
+
+ruleTester.run("use-returnValue", rule, {
+  valid: [
+    "a = foo.concat(bar)",
+    "b = bar.concat([1,3,4])",
+    "c = baz.concat()",
+    "d = qux.join(' ')",
+    "e = quux.slice(1)",
+  ],
+  invalid: [
+    invalidCode("foo.concat(bar)", "concat"),
+    invalidCode("bar.concat([1,3,4])", "concat"),
+    invalidCode("baz.concat()", "concat"),
+    invalidCode("qux.join(' ')", "join"),
+    invalidCode("quux.slice(1)", "slice"),
+  ],
+});