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

--HG--
extra : moz-landing-system : lando

browser/actors/PageInfoChild.jsm

@@ -108,7 +108,7 @@ class PageInfoChild extends ActorChild {
       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]));
       }
     }

devtools/client/shared/widgets/VariablesViewController.jsm

@@ -219,6 +219,7 @@ VariablesViewController.prototype = {
     }
     // 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.

devtools/client/webconsole/actions/messages.js

@@ -98,6 +98,7 @@ function messageTableDataGet(id, client, dataType) {
 
     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));

devtools/server/actors/inspector/walker.js

@@ -1220,7 +1220,7 @@ var WalkerActor = protocol.ActorClassWithSpec(walkerSpec, {
       return sortA.localeCompare(sortB);
     });
 
-    result.slice(0, 25);
+    result = result.slice(0, 25);
 
     return {
       query: query,

docshell/test/unit/test_nsDefaultURIFixup_info.js

@@ -36,19 +36,14 @@ registerCleanupFunction(function() {
   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:
   {

services/fxaccounts/FxAccountsConfig.jsm

@@ -122,7 +122,7 @@ var FxAccountsConfig = {
     }
     let rootURL = Services.urlFormatter.formatURL(pref);
     if (rootURL.endsWith("/")) {
-      rootURL.slice(0, -1);
+      rootURL = rootURL.slice(0, -1);
     }
     return rootURL;
   },

toolkit/components/ctypes/tests/unit/test_jsctypes.js

@@ -458,10 +458,12 @@ function run_Int64_tests() {
   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() {
@@ -605,10 +607,12 @@ function run_UInt64_tests() {
   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,

toolkit/mozapps/update/tests/data/shared.js

@@ -427,7 +427,7 @@ function readFileBytes(aFile) {
       throw "Nothing read from input stream!";
     }
   }
-  data.join("");
+  data = data.join("");
   fis.close();
   return data.toString();
 }

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

@@ -276,6 +276,11 @@ 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
 ------------
 

tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js

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

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

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

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

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

@@ -1,6 +1,6 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "1.0.4",
+  "version": "1.0.5",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {

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

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

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