Rewrite no-fn-reference-in-iterator (#666)

Author: fisker

docs/rules/no-fn-reference-in-iterator.md

@@ -30,8 +30,6 @@ const unicorn = require('unicorn');
 //=> [2, 3, 5]
 ```
 
-This rule is fixable.
-
 
 ## Fail
 

index.js

@@ -31,7 +31,7 @@ module.exports = {
 				'unicorn/no-abusive-eslint-disable': 'error',
 				'unicorn/no-array-instanceof': 'error',
 				'unicorn/no-console-spaces': 'error',
-				'unicorn/no-fn-reference-in-iterator': 'off',
+				'unicorn/no-fn-reference-in-iterator': 'error',
 				'unicorn/no-for-loop': 'error',
 				'unicorn/no-hex-escape': 'error',
 				'unicorn/no-keyword-prefix': 'off',

readme.md

@@ -47,7 +47,7 @@ Configure it in `package.json`.
 			"unicorn/no-abusive-eslint-disable": "error",
 			"unicorn/no-array-instanceof": "error",
 			"unicorn/no-console-spaces": "error",
-			"unicorn/no-fn-reference-in-iterator": "off",
+			"unicorn/no-fn-reference-in-iterator": "error",
 			"unicorn/no-for-loop": "error",
 			"unicorn/no-hex-escape": "error",
 			"unicorn/no-keyword-prefix": "off",
@@ -105,7 +105,7 @@ Configure it in `package.json`.
 - [no-abusive-eslint-disable](docs/rules/no-abusive-eslint-disable.md) - Enforce specifying rules to disable in `eslint-disable` comments.
 - [no-array-instanceof](docs/rules/no-array-instanceof.md) - Require `Array.isArray()` instead of `instanceof Array`. *(fixable)*
 - [no-console-spaces](docs/rules/no-console-spaces.md) - Do not use leading/trailing space between `console.log` parameters. *(fixable)*
-- [no-fn-reference-in-iterator](docs/rules/no-fn-reference-in-iterator.md) - Prevent passing a function reference directly to iterator methods. *(fixable)*
+- [no-fn-reference-in-iterator](docs/rules/no-fn-reference-in-iterator.md) - Prevent passing a function reference directly to iterator methods.
 - [no-for-loop](docs/rules/no-for-loop.md) - Do not use a `for` loop that can be replaced with a `for-of` loop. *(partly fixable)*
 - [no-hex-escape](docs/rules/no-hex-escape.md) - Enforce the use of Unicode escapes instead of hexadecimal escapes. *(fixable)*
 - [no-keyword-prefix](docs/rules/no-keyword-prefix.md) - Disallow identifiers starting with `new` or `class`.

rules/custom-error-definition.js

@@ -108,7 +108,7 @@ const customErrorDefinition = (context, node) => {
 
 	const constructorBody = constructorBodyNode.body;
 
-	const superExpression = constructorBody.find(isSuperExpression);
+	const superExpression = constructorBody.find(body => isSuperExpression(body));
 	const messageExpressionIndex = constructorBody.findIndex(x => isAssignmentExpression(x, 'message'));
 
 	if (!superExpression) {

rules/expiring-todo-comments.js

@@ -237,7 +237,7 @@ const create = context => {
 		)
 		// Flatten
 		.reduce((accumulator, array) => accumulator.concat(array), [])
-		.filter(processComment);
+		.filter(comment => processComment(comment));
 
 	// This is highly dependable on ESLint's `no-warning-comments` implementation.
 	// What we do is patch the parts we know the rule will use, `getAllComments`.

rules/no-fn-reference-in-iterator.js

@@ -1,69 +1,148 @@
 'use strict';
+const {isParenthesized} = require('eslint-utils');
 const getDocumentationUrl = require('./utils/get-documentation-url');
+const methodSelector = require('./utils/method-selector');
 
-const iteratorMethods = new Map([
-	['map', 1],
-	['forEach', 1],
-	['every', 1],
-	['filter', 1],
-	['find', 1],
-	['findIndex', 1],
-	['some', 1],
-	['reduce', 2],
-	['reduceRight', 2]
-]);
-
-const functionWhitelist = new Set([
-	'Boolean'
-]);
-
-const calleeBlacklist = [
+const ERROR_WITH_NAME_MESSAGE_ID = 'error-with-name';
+const ERROR_WITHOUT_NAME_MESSAGE_ID = 'error-without-name';
+const REPLACE_WITH_NAME_MESSAGE_ID = 'replace-with-name';
+const REPLACE_WITHOUT_NAME_MESSAGE_ID = 'replace-without-name';
+
+const iteratorMethods = [
+	['every'],
+	['filter'],
+	['find'],
+	['findIndex'],
+	['flatMap'],
+	['forEach'],
+	['map'],
+	[
+		'reduce', {
+			parameters: [
+				'accumulator',
+				'element',
+				'index',
+				'array'
+			],
+			minParameters: 2,
+			ignore: []
+		}
+	],
+	[
+		'reduceRight', {
+			parameters: [
+				'accumulator',
+				'element',
+				'index',
+				'array'
+			],
+			minParameters: 2,
+			ignore: []
+		}
+	],
+	['some']
+].map(([method, options]) => {
+	options = {
+		parameters: ['element', 'index', 'array'],
+		ignore: ['Boolean'],
+		minParameters: 1,
+		...options
+	};
+	return [method, options];
+});
+
+const ignoredCallee = [
 	'Promise',
 	'React.children',
+	'lodash',
+	'underscore',
 	'_',
 	'Async',
 	'async'
 ];
 
-const isIteratorMethod = node => node.callee.property && iteratorMethods.has(node.callee.property.name);
-const hasFunctionArgument = node => node.arguments.length > 0 && (node.arguments[0].type === 'Identifier' || node.arguments[0].type === 'CallExpression') && !functionWhitelist.has(node.arguments[0].name);
-
-const getNumberOfArguments = node => node.callee.property && iteratorMethods.get(node.callee.property.name);
-const parseArgument = (context, argument) => argument.type === 'Identifier' ? argument.name : context.getSourceCode().getText(argument);
-
-const fix = (context, node) => {
-	const numberOfArguments = getNumberOfArguments(node);
-	const argument = node.arguments[0];
-	const argumentString = numberOfArguments === 1 ? 'x' : 'a, b';
-
-	return fixer => fixer.replaceText(argument, `${numberOfArguments === 1 ? argumentString : `(${argumentString})`} => ${parseArgument(context, argument)}(${argumentString})`);
-};
-
 const toSelector = name => {
 	const splitted = name.split('.');
 	return `[callee.${'object.'.repeat(splitted.length)}name!="${splitted.shift()}"]`;
 };
 
 // Select all the call expressions except the ones present in the blacklist
-const selector = `CallExpression${calleeBlacklist.map(toSelector).join('')}`;
-
-const create = context => ({
-	[selector]: node => {
-		if (
-			isIteratorMethod(node) &&
-			hasFunctionArgument(node) &&
-			node.arguments.length <= getNumberOfArguments(node)
-		) {
-			const [argument] = node.arguments;
-
-			context.report({
-				node: argument,
-				message: 'Do not pass a function reference directly to an iterator method.',
-				fix: fix(context, node)
-			});
-		}
+const ignoredCalleeSelector = `${ignoredCallee.map(name => toSelector(name)).join('')}`;
+
+function check(context, node, method, options) {
+	const {type} = node;
+
+	if (type === 'FunctionExpression' || type === 'ArrowFunctionExpression') {
+		return;
 	}
-});
+
+	const name = type === 'Identifier' ? node.name : '';
+
+	if (type === 'Identifier' && options.ignore.includes(name)) {
+		return;
+	}
+
+	const problem = {
+		node,
+		messageId: name ? ERROR_WITH_NAME_MESSAGE_ID : ERROR_WITHOUT_NAME_MESSAGE_ID,
+		data: {
+			name,
+			method
+		},
+		suggest: []
+	};
+
+	const {parameters, minParameters} = options;
+	for (let parameterLength = minParameters; parameterLength <= parameters.length; parameterLength++) {
+		const suggestionParameters = parameters.slice(0, parameterLength).join(', ');
+
+		const suggest = {
+			messageId: name ? REPLACE_WITH_NAME_MESSAGE_ID : REPLACE_WITHOUT_NAME_MESSAGE_ID,
+			data: {
+				name,
+				parameters: suggestionParameters
+			},
+			fix: fixer => {
+				const sourceCode = context.getSourceCode();
+				let nodeText = sourceCode.getText(node);
+				if (isParenthesized(node, sourceCode) || type === 'ConditionalExpression') {
+					nodeText = `(${nodeText})`;
+				}
+
+				return fixer.replaceText(
+					node,
+					`(${suggestionParameters}) => ${nodeText}(${suggestionParameters})`
+				);
+			}
+		};
+
+		problem.suggest.push(suggest);
+	}
+
+	context.report(problem);
+}
+
+const create = context => {
+	const sourceCode = context.getSourceCode();
+	const rules = {};
+
+	for (const [method, options] of iteratorMethods) {
+		const selector = [
+			methodSelector({
+				name: method,
+				min: 1,
+				max: 2
+			}),
+			ignoredCalleeSelector
+		].join('');
+		rules[selector] = node => {
+			const [iterator] = node.arguments;
+			check(context, iterator, method, options, sourceCode);
+		};
+	}
+
+	return rules;
+};
 
 module.exports = {
 	create,
@@ -72,6 +151,11 @@ module.exports = {
 		docs: {
 			url: getDocumentationUrl(__filename)
 		},
-		fixable: 'code'
+		messages: {
+			[ERROR_WITH_NAME_MESSAGE_ID]: 'Do not pass function `{{name}}` directly to `.{{method}}(…)`.',
+			[ERROR_WITHOUT_NAME_MESSAGE_ID]: 'Do not pass function directly to `.{{method}}(…)`.',
+			[REPLACE_WITH_NAME_MESSAGE_ID]: 'Replace function `{{name}}` with `… => {{name}}({{parameters}})`.',
+			[REPLACE_WITHOUT_NAME_MESSAGE_ID]: 'Replace function with `… => …({{parameters}})`.'
+		}
 	}
 };

rules/no-unreadable-array-destructuring.js

@@ -8,7 +8,7 @@ const isCommaFollowedWithComma = (element, index, array) =>
 const create = context => {
 	return {
 		'ArrayPattern[elements.length>=3]': node => {
-			if (node.elements.some(isCommaFollowedWithComma)) {
+			if (node.elements.some((element, index, array) => isCommaFollowedWithComma(element, index, array))) {
 				context.report({
 					node,
 					message

rules/no-unused-properties.js

@@ -210,11 +210,11 @@ const create = context => {
 	};
 
 	const checkVariables = scope => {
-		scope.variables.forEach(checkVariable);
+		scope.variables.forEach(variable => checkVariable(variable));
 	};
 
 	const checkChildScopes = scope => {
-		scope.childScopes.forEach(checkScope);
+		scope.childScopes.forEach(scope => checkScope(scope));
 	};
 
 	const checkScope = scope => {

rules/prefer-spread.js

@@ -24,7 +24,7 @@ const create = context => {
 				node,
 				message: 'Prefer the spread operator over `Array.from()`.',
 				fix: fixer => {
-					const [arrayLikeArgument, mapFn, thisArgument] = node.arguments.map(getSource);
+					const [arrayLikeArgument, mapFn, thisArgument] = node.arguments.map(node => getSource(node));
 					let replacement = `${
 						needsSemicolon(sourceCode.getTokenBefore(node), sourceCode) ? ';' : ''
 					}[...${arrayLikeArgument}]`;

rules/prevent-abbreviations.js

@@ -278,9 +278,10 @@ const getWordReplacements = (word, {replacements, whitelist}) => {
 
 	let wordReplacement = [];
 	if (replacement) {
+		const transform = isUpperFirst(word) ? upperFirst : lowerFirst;
 		wordReplacement = [...replacement.keys()]
 			.filter(name => replacement.get(name))
-			.map(isUpperFirst(word) ? upperFirst : lowerFirst);
+			.map(name => transform(name));
 	}
 
 	return wordReplacement.length > 0 ? wordReplacement.sort() : [];
@@ -391,7 +392,7 @@ const isExportedIdentifier = identifier => {
 };
 
 const shouldFix = variable => {
-	return !getVariableIdentifiers(variable).some(isExportedIdentifier);
+	return !getVariableIdentifiers(variable).some(identifier => isExportedIdentifier(identifier));
 };
 
 const isDefaultOrNamespaceImportName = identifier => {
@@ -634,11 +635,11 @@ const create = context => {
 	};
 
 	const checkVariables = scope => {
-		scope.variables.forEach(checkPossiblyWeirdClassVariable);
+		scope.variables.forEach(variable => checkPossiblyWeirdClassVariable(variable));
 	};
 
 	const checkChildScopes = scope => {
-		scope.childScopes.forEach(checkScope);
+		scope.childScopes.forEach(scope => checkScope(scope));
 	};
 
 	const checkScope = scope => {