Bug 1521170 - Add a rule that prevents calling some Array and String accessor methods without using the return value. r=Standard8
☠☠ backed out by 9d2e8060ccde ☠ ☠
authorJared Wein <jwein@mozilla.com>
Tue, 22 Jan 2019 23:02:24 +0000
changeset 514924 c9f087ff5a524647ed3278749a28f3777e6e2257
parent 514923 24310d684cca9d98aa8c46a6a3d0cf8734dff155
child 514925 413e705328be4731589d78ec41a12ea9f5a2dfec
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
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 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
@@ -1203,17 +1203,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
@@ -37,17 +37,17 @@ registerCleanupFunction(function() {
 });
 
 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 = 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:
--- 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
@@ -420,17 +420,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,40 @@
+/* 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: "MemberExpression"}]};
+}
+
+ruleTester.run("use-ownerGlobal", rule, {
+  valid: [
+    "a = foo.concat(bar)",
+    "b = bar.concat([1,3,4])",
+    "c = baz.concat()",
+    "d = qux.join(' ')",
+    "e = quux.slice(1)",
+    "Int64.join(-0x80000001, 0)",
+  ],
+  invalid: [
+    invalidCode("foo.concat(bar)"),
+    invalidCode("bar.concat([1,3,4])"),
+    invalidCode("baz.concat()"),
+    invalidCode("qux.join(' ')"),
+    invalidCode("quux.slice(1)"),
+  ],
+});