From e47ec5186737fb58c6339fadf7924d6b6bc6f984 Mon Sep 17 00:00:00 2001 From: Alexander Afanasyev Date: Mon, 11 Jul 2016 18:43:59 -0400 Subject: [PATCH] feat(rules): add 'array-callback-return' rule --- README.md | 2 + docs/rules/array-callback-return.md | 57 +++++++++ index.js | 5 +- lib/is-element-array-finder.js | 33 ++++++ lib/rules/array-callback-return.js | 175 ++++++++++++++++++++++++++++ test/rules/array-callback-return.js | 113 ++++++++++++++++++ 6 files changed, 384 insertions(+), 1 deletion(-) create mode 100644 docs/rules/array-callback-return.md create mode 100644 lib/is-element-array-finder.js create mode 100644 lib/rules/array-callback-return.js create mode 100644 test/rules/array-callback-return.js diff --git a/README.md b/README.md index a1f16b3..3847521 100644 --- a/README.md +++ b/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 diff --git a/docs/rules/array-callback-return.md b/docs/rules/array-callback-return.md new file mode 100644 index 0000000..799748a --- /dev/null +++ b/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")); +``` diff --git a/index.js b/index.js index eb8db06..afd3f1b 100644 --- a/index.js +++ b/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: { diff --git a/lib/is-element-array-finder.js b/lib/is-element-array-finder.js new file mode 100644 index 0000000..39b75c4 --- /dev/null +++ b/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 +} diff --git a/lib/rules/array-callback-return.js b/lib/rules/array-callback-return.js new file mode 100644 index 0000000..47a9bb4 --- /dev/null +++ b/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 + } + } +} diff --git a/test/rules/array-callback-return.js b/test/rules/array-callback-return.js new file mode 100644 index 0000000..4b9f584 --- /dev/null +++ b/test/rules/array-callback-return.js @@ -0,0 +1,113 @@ +'use strict' + +var rule = require('../../lib/rules/array-callback-return') +var RuleTester = require('eslint').RuleTester + +var ruleTester = new RuleTester() + +ruleTester.run('array-callback-return', rule, { + valid: [ + '', + 'foo.filter();', + 'filter("test");', + 'obj.filter(function () { /* comment */ });', + '$$(".myclass").filter();', + 'foo.filter(function() { return true; })', + 'foo.map(function() { return true; })', + 'foo.reduce(function() { return true; })', + 'foo.filter(function() {})', + 'foo.map(function() {})', + 'foo.reduce(function() {})', + 'var elements = element.all(by.css(".myclass"));', + 'var elements = $$(".myclass");', + 'var elements = $$(".myclass").filter;', + 'element.all(by.css(".myclass")).filter();', + 'function test () { element.all(by.css(".myclass")).filter(); }', + 'element.all(by.css(".myclass")).filter(function() { return true; })', + 'element(by.id("myid")).all(by.css(".myclass")).map(function() { return true; })', + '$$(".myclass").reduce(function() { return true; })', + 'element(by.id("myid")).$$(".myclass").reduce(function() { return true; })', + '$$(".myclass").filter(function(){ return function() { return; }; })', + '$$(".myclass").reduce(function(){}())', + '$$(".myclass").reduce(function() { switch (a) { case 0: bar(); default: return true; } })', + '$$(".myclass").map(function() { try { bar(); return true; } catch (err) { return false; } })', + '$$(".myclass").filter(function(){ return function() { return; }; })', + '$$(".myclass").filter(function() { if (a) return true; else return false; })', + { + code: '$$(".myclass").filter(() => { return true; })', + parserOptions: { ecmaVersion: 6 } + }, + { + code: '$$(".myclass").filter(() => true)', + parserOptions: { ecmaVersion: 6 } + }, + 'element.all(by.css(".myclass")).filter(function(elm) { return elm.getText().then(function (text) { return text.indexOf("test") >= 0; }) });' + ], + invalid: [ + { + code: 'element.all(by.css(".myclass")).filter(function() {})', + errors: ['Expected to return a value in this function'] + }, + { + code: 'element.all(by.css(".myclass")).filter(function() { var a = 1; })', + errors: ['Expected to return a value in this function'] + }, + { + code: 'element.all(by.css(".myclass")).map(function() {})', + errors: ['Expected to return a value in this function'] + }, + { + code: 'element(by.id("myid")).all(by.css(".myclass")).reduce(function() {})', + errors: ['Expected to return a value in this function'] + }, + { + code: '$$(".myclass").filter(function cb() { if (a) return true; })', + errors: ['Expected to return a value at the end of this function'] + }, + { + code: 'element(by.id("myid")).$$(".myclass").filter(function() { switch (a) { case 0: break; default: return true; } })', + errors: ['Expected to return a value at the end of this function'] + }, + { + code: '$$(".myclass").filter(function() { try { bar(); } catch (err) { return true; } })', + errors: ['Expected to return a value at the end of this function'] + }, + { + code: '$$(".myclass").filter(function() { return; })', + errors: ['Expected a return value'] + }, + { + code: '$$(".myclass").filter(function() { if (a) return; })', + errors: ['Expected to return a value at the end of this function', 'Expected a return value'] + }, + { + code: '$$(".myclass").filter(function() { if (a) return; else return; })', + errors: ['Expected a return value', 'Expected a return value'] + }, + { + code: '$$(".myclass").filter(cb || function() {})', + errors: ['Expected to return a value in this function'] + }, + { + code: '$$(".myclass").filter(a ? function() {} : function() {})', + errors: ['Expected to return a value in this function', 'Expected to return a value in this function'] + }, + { + code: '$$(".myclass").filter(function(){ return function() {}; }())', + errors: ['Expected to return a value in this function'] + }, + { + code: '$$(".myclass").filter(() => {});', + parserOptions: { ecmaVersion: 6 }, + errors: ['Expected to return a value in this function'] + }, + { + code: '$$(".myclass").map(function() { try { bar(); } catch (err) { return true; } })', + errors: ['Expected to return a value at the end of this function'] + }, + { + code: 'element.all(by.css(".myclass")).filter(function(elm) { elm.getText().then(function (text) { return text.indexOf("test") >= 0; }) });', + errors: ['Expected to return a value in this function'] + } + ] +})