feat(rules): add 'array-callback-return' rule

Author: alecxe

README.md

@@ -48,6 +48,7 @@ Rule                         | Default       | Options
 [no-shadowing][]             | 1             |
 [use-first-last][]           | 1             |
 [no-get-in-it][]             | 1             |
+[array-callback-return][]    | 1             |
 [by-css-shortcut][]          | 0             |
 
 For example, the `missing-perform` rule is enabled by default and will cause
@@ -77,6 +78,7 @@ See [configuring rules][] for more information.
 [no-shadowing]: docs/rules/no-shadowing.md
 [use-first-last]: docs/rules/use-first-last.md
 [no-get-in-it]: docs/rules/no-get-in-it.md
+[array-callback-return]: docs/rules/array-callback-return.md
 [by-css-shortcut]: docs/rules/by-css-shortcut.md
 [configuring rules]: http://eslint.org/docs/user-guide/configuring#configuring-rules
 

docs/rules/array-callback-return.md

@@ -0,0 +1,57 @@
+# Enforce `return` statements in callbacks of `ElementArrayFinder` methods
+
+`ElementArrayFinder` has multiple methods for filtering, mapping, reducing:
+
+ * [`map()`](http://www.protractortest.org/#/api?view=ElementArrayFinder.prototype.map)
+ * [`filter()`](http://www.protractortest.org/#/api?view=ElementArrayFinder.prototype.filter)
+ * [`reduce()`](http://www.protractortest.org/#/api?view=ElementArrayFinder.prototype.reduce)
+ 
+The callbacks for these methods have to return something for the method to work properly.
+
+This rule would issue a warning if there is no `return` statement detected in the callback.
+
+## Known Limitations
+
+This rule checks callback functions of methods with the given names only if called on `element.all()` or `$$()` explicitly.
+This means, that if there is, for example, a page object field which is later filtered:
+
+```js
+myPage.rows.filter(function (row) {
+    row.getText();
+});
+```
+
+it would not be detected as a violation. Look into having [`array-callback-return`](http://eslint.org/docs/rules/array-callback-return) built-in ESLint rule enabled to catch these cases.
+
+## Rule details
+
+Any use of the following patterns are considered warnings:
+
+```js
+element.all(by.css(".myclass")).filter(function() {
+  elm.getText().then(function (text) { 
+    return text.indexOf("test") >= 0;
+  })
+});
+
+$$(".myclass").filter(function cb() { if (a) return true; });
+element(by.id("myid")).$$(".myclass").filter(function() { switch (a) { case 0: break; default: return true; } });
+$$(".myclass").filter(function() { return; });
+$$(".myclass").filter(function() { if (a) return; else return; });
+$$(".myclass").filter(a ? function() {} : function() {});
+$$(".myclass").filter(function(){ return function() {}; }())
+```
+
+The following patterns are not warnings:
+
+```js
+element.all(by.css(".myclass")).filter(function(elm) { 
+  return elm.getText().then(function (text) { 
+    return text.indexOf("test") >= 0;
+  })
+});
+
+$$(".myclass").reduce(function() { return true; });
+$$(".myclass").reduce(function() { switch (a) { case 0: bar(); default: return true; } })
+var elements = element.all(by.css(".myclass"));
+```

index.js

@@ -13,6 +13,7 @@ var useSimpleRepeaters = require('./lib/rules/use-simple-repeaters')
 var noShadowing = require('./lib/rules/no-shadowing')
 var useFirstLast = require('./lib/rules/use-first-last')
 var noGetInIt = require('./lib/rules/no-get-in-it')
+var arrayCallbackReturn = require('./lib/rules/array-callback-return')
 
 module.exports = {
   rules: {
@@ -28,7 +29,8 @@ module.exports = {
     'use-simple-repeaters': useSimpleRepeaters,
     'no-shadowing': noShadowing,
     'use-first-last': useFirstLast,
-    'no-get-in-it': noGetInIt
+    'no-get-in-it': noGetInIt,
+    'array-callback-return': arrayCallbackReturn
   },
   configs: {
     recommended: {
@@ -45,6 +47,7 @@ module.exports = {
         'protractor/no-shadowing': 1,
         'protractor/use-first-last': 1,
         'protractor/no-get-in-it': 1,
+        'protractor/array-callback-return': 1,
         'protractor/by-css-shortcut': 0
       },
       globals: {

lib/is-element-array-finder.js

@@ -0,0 +1,33 @@
+'use strict'
+
+/**
+ * Checks a given MemberExpression node is an ElementArrayFinder instance.
+ *
+ * @fileoverview Utility function to determine if a node is an ElementArrayFinder
+ * @author Alexander Afanasyev
+ */
+module.exports = function (node) {
+  // handling $$ shortcut
+  var object = node.object
+  if (object && object.callee && object.callee.name === '$$') {
+    return true
+  }
+
+  if (object) {
+    var callee = object.callee
+    if (callee && callee.object && callee.property) {
+      // handling chained $$
+      if (callee.property.name === '$$') {
+        return true
+      }
+
+      // handling element.all() and chained element() and all()
+      if (callee.property.name === 'all' &&
+        (callee.object.name === 'element' || (callee.object.callee && callee.object.callee.name === 'element'))) {
+        return true
+      }
+    }
+  }
+
+  return false
+}

lib/rules/array-callback-return.js

@@ -0,0 +1,175 @@
+'use strict'
+
+/**
+ * @fileoverview Rule to enforce return statements in callbacks of ElementArrayFinder's methods
+ * @author Alexander Afanasyev (based on Toru Nagashima's work)
+ */
+
+var astUtils = require('eslint/lib/ast-utils')
+var isElementArrayFinder = require('../is-element-array-finder')
+
+var TARGET_NODE_TYPE = /^(?:Arrow)?FunctionExpression$/
+var TARGET_METHODS = /^(?:filter|map|reduce)$/
+
+/**
+ * Checks a given code path segment is reachable.
+ *
+ * @param {CodePathSegment} segment - A segment to check.
+ * @returns {boolean} `true` if the segment is reachable.
+ */
+function isReachable (segment) {
+  return segment.reachable
+}
+
+/**
+ * Gets a readable location.
+ *
+ * - FunctionExpression -> the function name or `function` keyword.
+ * - ArrowFunctionExpression -> `=>` token.
+ *
+ * @param {ASTNode} node - A function node to get.
+ * @param {SourceCode} sourceCode - A source code to get tokens.
+ * @returns {ASTNode|Token} The node or the token of a location.
+ */
+function getLocation (node, sourceCode) {
+  if (node.type === 'ArrowFunctionExpression') {
+    return sourceCode.getTokenBefore(node.body)
+  }
+  return node.id || node
+}
+
+/**
+ * Checks a given node is a MemberExpression node which has the specified name's
+ * property.
+ *
+ * @param {ASTNode} node - A node to check.
+ * @returns {boolean} `true` if the node is a MemberExpression node which has
+ *      the specified name's property
+ */
+function isTargetMethod (node) {
+  return (isElementArrayFinder(node) &&
+          node.type === 'MemberExpression' &&
+          node.property &&
+          TARGET_METHODS.test(node.property.name)
+  )
+}
+
+/**
+ * Checks whether or not a given node is a function expression which is the
+ * callback of an array method.
+ *
+ * @param {ASTNode} node - A node to check. This is one of
+ *      FunctionExpression or ArrowFunctionExpression.
+ * @returns {boolean} `true` if the node is the callback of an array method.
+ */
+function isCallbackOfArrayMethod (node) {
+  while (node) {
+    var parent = node.parent
+
+    switch (parent.type) {
+      case 'LogicalExpression':
+      case 'ConditionalExpression':
+        node = parent
+        break
+
+      case 'ReturnStatement':
+        var func = astUtils.getUpperFunction(parent)
+
+        if (func === null || !astUtils.isCallee(func)) {
+          return false
+        }
+        node = func.parent
+        break
+
+      case 'CallExpression':
+        if (isTargetMethod(parent.callee)) {
+          return (parent.arguments.length >= 1 && parent.arguments[0] === node)
+        }
+        return false
+
+      // Otherwise this node is not target.
+      /* istanbul ignore next: unreachable */
+      default:
+        return false
+    }
+  }
+
+  /* istanbul ignore next: unreachable */
+  return false
+}
+
+module.exports = {
+  meta: {
+    schema: []
+  },
+
+  create: function (context) {
+    var funcInfo = {
+      upper: null,
+      codePath: null,
+      hasReturn: false,
+      shouldCheck: false
+    }
+
+    /**
+     * Checks whether or not the last code path segment is reachable.
+     * Then reports this function if the segment is reachable.
+     *
+     * If the last code path segment is reachable, there are paths which are not
+     * returned or thrown.
+     *
+     * @param {ASTNode} node - A node to check.
+     * @returns {void}
+     */
+    function checkLastSegment (node) {
+      if (funcInfo.shouldCheck &&
+        funcInfo.codePath.currentSegments.some(isReachable)
+      ) {
+        context.report({
+          node: node,
+          loc: getLocation(node, context.getSourceCode()).loc.start,
+          message: funcInfo.hasReturn
+            ? 'Expected to return a value at the end of this function'
+            : 'Expected to return a value in this function'
+        })
+      }
+    }
+
+    return {
+      // Stacks this function's information.
+      'onCodePathStart': function (codePath, node) {
+        funcInfo = {
+          upper: funcInfo,
+          codePath: codePath,
+          hasReturn: false,
+          shouldCheck: TARGET_NODE_TYPE.test(node.type) &&
+                       node.body.type === 'BlockStatement' &&
+                       isCallbackOfArrayMethod(node)
+        }
+      },
+
+      // Pops this function's information.
+      'onCodePathEnd': function () {
+        funcInfo = funcInfo.upper
+      },
+
+      // Checks the return statement is valid.
+      'ReturnStatement': function (node) {
+        if (funcInfo.shouldCheck) {
+          funcInfo.hasReturn = true
+
+          if (!node.argument) {
+            context.report({
+              node: node,
+              message: 'Expected a return value'
+            })
+          }
+        }
+      },
+
+      // Reports a given function if the last path is reachable.
+      'FunctionExpression:exit': checkLastSegment,
+      'ArrowFunctionExpression:exit': checkLastSegment
+    }
+  }
+}