prefer-string-slice: Improve fix (#1675)

sindresorhus · Dec 30, 2021 · 267115a · 267115a
1 parent 152f153
commit 267115a
Show file tree

Hide file tree

Showing 8 changed files with 360 additions and 98 deletions.
diff --git a/rules/fix/index.js b/rules/fix/index.js
@@ -7,6 +7,7 @@ module.exports = {
 
 	appendArgument: require('./append-argument.js'),
 	removeArgument: require('./remove-argument.js'),
+	replaceArgument: require('./replace-argument.js'),
 	switchNewExpressionToCallExpression: require('./switch-new-expression-to-call-expression.js'),
 	switchCallExpressionToNewExpression: require('./switch-call-expression-to-new-expression.js'),
 	removeMemberExpressionProperty: require('./remove-member-expression-property.js'),

diff --git a/rules/fix/replace-argument.js b/rules/fix/replace-argument.js
@@ -0,0 +1,8 @@
+'use strict';
+const {getParenthesizedRange} = require('../utils/parentheses.js');
+
+function replaceArgument(fixer, node, text, sourceCode) {
+	return fixer.replaceTextRange(getParenthesizedRange(node, sourceCode), text);
+}
+
+module.exports = replaceArgument;
diff --git a/rules/prefer-string-slice.js b/rules/prefer-string-slice.js
@@ -1,7 +1,9 @@
 'use strict';
-const {getParenthesizedText} = require('./utils/parentheses.js');
+const {getStaticValue} = require('eslint-utils');
+const {getParenthesizedText, getParenthesizedRange} = require('./utils/parentheses.js');
 const {methodCallSelector} = require('./selectors/index.js');
 const isNumber = require('./utils/is-number.js');
+const {replaceArgument} = require('./fix/index.js');
 
 const MESSAGE_ID_SUBSTR = 'substr';
 const MESSAGE_ID_SUBSTRING = 'substring';
@@ -37,123 +39,139 @@ const isLengthProperty = node => (
 	&& node.property.name === 'length'
 );
 
-function getFixArguments(node, context) {
+function * fixSubstrArguments({node, fixer, context, abort}) {
 	const argumentNodes = node.arguments;
+	const [firstArgument, secondArgument] = argumentNodes;
 
-	if (argumentNodes.length === 0) {
-		return [];
+	if (!secondArgument) {
+		return;
 	}
 
+	const scope = context.getScope();
 	const sourceCode = context.getSourceCode();
-	const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
-	const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
-
-	const method = node.callee.property.name;
-
-	if (method === 'substr') {
-		switch (argumentNodes.length) {
-			case 1: {
-				return [firstArgument];
-			}
-
-			case 2: {
-				if (firstArgument === '0') {
-					const sliceCallArguments = [firstArgument];
-					if (isLiteralNumber(secondArgument) || isLengthProperty(argumentNodes[1])) {
-						sliceCallArguments.push(secondArgument);
-					} else if (typeof getNumericValue(argumentNodes[1]) === 'number') {
-						sliceCallArguments.push(Math.max(0, getNumericValue(argumentNodes[1])));
-					} else {
-						sliceCallArguments.push(`Math.max(0, ${secondArgument})`);
-					}
-
-					return sliceCallArguments;
-				}
-
-				if (argumentNodes.every(node => isLiteralNumber(node))) {
-					return [
-						firstArgument,
-						argumentNodes[0].value + argumentNodes[1].value,
-					];
-				}
+	const firstArgumentStaticResult = getStaticValue(firstArgument, scope);
+	const secondArgumentRange = getParenthesizedRange(secondArgument, sourceCode);
+	const replaceSecondArgument = text => replaceArgument(fixer, secondArgument, text, sourceCode);
 
-				if (argumentNodes.every(node => isNumber(node, context.getScope()))) {
-					return [firstArgument, firstArgument + ' + ' + secondArgument];
-				}
+	if (firstArgumentStaticResult && firstArgumentStaticResult.value === 0) {
+		if (isLiteralNumber(secondArgument) || isLengthProperty(secondArgument)) {
+			return;
+		}
 
-				break;
-			}
-			// No default
+		if (typeof getNumericValue(secondArgument) === 'number') {
+			yield replaceSecondArgument(Math.max(0, getNumericValue(secondArgument)));
+			return;
 		}
-	} else if (method === 'substring') {
-		const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;
-		switch (argumentNodes.length) {
-			case 1: {
-				if (firstNumber !== undefined) {
-					return [Math.max(0, firstNumber)];
-				}
 
-				if (isLengthProperty(argumentNodes[0])) {
-					return [firstArgument];
-				}
+		yield fixer.insertTextBeforeRange(secondArgumentRange, 'Math.max(0, ');
+		yield fixer.insertTextAfterRange(secondArgumentRange, ')');
+		return;
+	}
 
-				return [`Math.max(0, ${firstArgument})`];
-			}
+	if (argumentNodes.every(node => isLiteralNumber(node))) {
+		yield replaceSecondArgument(firstArgument.value + secondArgument.value);
+		return;
+	}
 
-			case 2: {
-				const secondNumber = getNumericValue(argumentNodes[1]);
+	if (argumentNodes.every(node => isNumber(node, context.getScope()))) {
+		const firstArgumentText = getParenthesizedText(firstArgument, sourceCode);
 
-				if (firstNumber !== undefined && secondNumber !== undefined) {
-					return firstNumber > secondNumber
-						? [Math.max(0, secondNumber), Math.max(0, firstNumber)]
-						: [Math.max(0, firstNumber), Math.max(0, secondNumber)];
-				}
+		yield fixer.insertTextBeforeRange(secondArgumentRange, `${firstArgumentText} + `);
+		return;
+	}
 
-				if (firstNumber === 0 || secondNumber === 0) {
-					return [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
-				}
+	return abort();
+}
+
+function * fixSubstringArguments({node, fixer, context, abort}) {
+	const sourceCode = context.getSourceCode();
+	const [firstArgument, secondArgument] = node.arguments;
 
-				// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
-				//   .substring(0, 2) and .substring(2, 0) returns the same result
-				//   .slice(0, 2) and .slice(2, 0) doesn't return the same result
-				// There's also an issue with us now knowing whether the value will be negative or not, due to:
-				//   .substring() treats a negative number the same as it treats a zero.
-				// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
+	const firstNumber = firstArgument ? getNumericValue(firstArgument) : undefined;
+	const firstArgumentText = getParenthesizedText(firstArgument, sourceCode);
+	const replaceFirstArgument = text => replaceArgument(fixer, firstArgument, text, sourceCode);
 
-				break;
-			}
-			// No default
+	if (!secondArgument) {
+		if (isLengthProperty(firstArgument)) {
+			return;
 		}
+
+		if (firstNumber !== undefined) {
+			yield replaceFirstArgument(Math.max(0, firstNumber));
+			return;
+		}
+
+		const firstArgumentRange = getParenthesizedRange(firstArgument, sourceCode);
+		yield fixer.insertTextBeforeRange(firstArgumentRange, 'Math.max(0, ');
+		yield fixer.insertTextAfterRange(firstArgumentRange, ')');
+		return;
 	}
-}
 
-/** @param {import('eslint').Rule.RuleContext} context */
-const create = context => {
-	const sourceCode = context.getSourceCode();
+	const secondNumber = getNumericValue(secondArgument);
+	const secondArgumentText = getParenthesizedText(secondArgument, sourceCode);
+	const replaceSecondArgument = text => replaceArgument(fixer, secondArgument, text, sourceCode);
+
+	if (firstNumber !== undefined && secondNumber !== undefined) {
+		const argumentsValue = [Math.max(0, firstNumber), Math.max(0, secondNumber)];
+		if (firstNumber > secondNumber) {
+			argumentsValue.reverse();
+		}
+
+		if (argumentsValue[0] !== firstNumber) {
+			yield replaceFirstArgument(argumentsValue[0]);
+		}
 
-	return {
-		[selector](node) {
-			const problem = {
-				node,
-				messageId: node.callee.property.name,
-			};
+		if (argumentsValue[1] !== secondNumber) {
+			yield replaceSecondArgument(argumentsValue[1]);
+		}
 
-			const sliceCallArguments = getFixArguments(node, context);
-			if (!sliceCallArguments) {
-				return problem;
-			}
+		return;
+	}
 
-			const objectNode = node.callee.object;
-			const objectText = getParenthesizedText(objectNode, sourceCode);
-			const optionalMemberSuffix = node.callee.optional ? '?' : '';
-			const optionalCallSuffix = node.optional ? '?.' : '';
+	if (firstNumber === 0 || secondNumber === 0) {
+		yield replaceFirstArgument(0);
+		yield replaceSecondArgument(`Math.max(0, ${firstNumber === 0 ? secondArgumentText : firstArgumentText})`);
+		return;
+	}
 
-			problem.fix = fixer => fixer.replaceText(node, `${objectText}${optionalMemberSuffix}.slice${optionalCallSuffix}(${sliceCallArguments.join(', ')})`);
+	// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
+	//   .substring(0, 2) and .substring(2, 0) returns the same result
+	//   .slice(0, 2) and .slice(2, 0) doesn't return the same result
+	// There's also an issue with us now knowing whether the value will be negative or not, due to:
+	//   .substring() treats a negative number the same as it treats a zero.
+	// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
 
-			return problem;
-		},
-	};
-};
+	return abort();
+}
+
+/** @param {import('eslint').Rule.RuleContext} context */
+const create = context => ({
+	[selector](node) {
+		const method = node.callee.property.name;
+
+		return {
+			node,
+			messageId: method,
+			* fix(fixer, {abort}) {
+				yield fixer.replaceText(node.callee.property, 'slice');
+
+				if (node.arguments.length === 0) {
+					return;
+				}
+
+				if (
+					node.arguments.length > 2
+					|| node.arguments.some(node => node.type === 'SpreadElement')
+				) {
+					return abort();
+				}
+
+				const fixArguments = method === 'substr' ? fixSubstrArguments : fixSubstringArguments;
+				yield * fixArguments({node, fixer, context, abort});
+			},
+		};
+	},
+});
 
 /** @type {import('eslint').Rule.RuleModule} */
 module.exports = {

diff --git a/rules/utils/rule.js b/rules/utils/rule.js
@@ -5,6 +5,34 @@ const getDocumentationUrl = require('./get-documentation-url.js');
 
 const isIterable = object => typeof object[Symbol.iterator] === 'function';
 
+class FixAbortError extends Error {}
+const fixOptions = {
+	abort() {
+		throw new FixAbortError('Fix aborted.');
+	},
+};
+
+function wrapFixFunction(fix) {
+	return fixer => {
+		const result = fix(fixer, fixOptions);
+
+		if (result && isIterable(result)) {
+			try {
+				return [...result];
+			} catch (error) {
+				if (error instanceof FixAbortError) {
+					return;
+				}
+
+				/* istanbul ignore next: Safe */
+				throw error;
+			}
+		}
+
+		return result;
+	};
+}
+
 function reportListenerProblems(listener, context) {
 	// Listener arguments can be `codePath, node` or `node`
 	return function (...listenerArguments) {
@@ -18,11 +46,20 @@ function reportListenerProblems(listener, context) {
 			problems = [problems];
 		}
 
-		// TODO: Allow `fix` function to abort
 		for (const problem of problems) {
-			if (problem) {
-				context.report(problem);
+			if (problem.fix) {
+				problem.fix = wrapFixFunction(problem.fix);
 			}
+
+			if (Array.isArray(problem.suggest)) {
+				for (const suggest of problem.suggest) {
+					if (suggest.fix) {
+						suggest.fix = wrapFixFunction(suggest.fix);
+					}
+				}
+			}
+
+			context.report(problem);
 		}
 	};
 }

diff --git a/test/prefer-string-slice.mjs b/test/prefer-string-slice.mjs
@@ -321,3 +321,29 @@ test.typescript({
 		},
 	],
 });
+
+test.snapshot({
+	valid: [],
+	invalid: [
+		outdent`
+			/* 1 */ (( /* 2 */ 0 /* 3 */, /* 4 */ foo /* 5 */ )) /* 6 */
+				. /* 7 */ substring /* 8 */ (
+					/* 9 */ (( /* 10 */ bar /* 11 */ )) /* 12 */,
+					/* 13 */ (( /* 14 */ 0 /* 15 */ )) /* 16 */,
+					/* 17 */
+				)
+			/* 18 */
+		`,
+		'foo.substr(0, ...bar)',
+		'foo.substr(...bar)',
+		'foo.substr(0, (100, 1))',
+		'foo.substr(0, 1, extraArgument)',
+		'foo.substr((0, bar.length), (0, baz.length))',
+		// TODO: Fix this
+		// 'foo.substr(await 1, await 2)',
+		'foo.substring((10, 1), 0)',
+		'foo.substring(0, (10, 1))',
+		'foo.substring(0, await 1)',
+		'foo.substring((10, bar))',
+	],
+});
diff --git a/test/run-rules-on-codebase/lint.mjs b/test/run-rules-on-codebase/lint.mjs
@@ -32,6 +32,8 @@ const eslint = new ESLint({
 			],
 			// https://github.com/sindresorhus/eslint-plugin-unicorn/issues/1109#issuecomment-782689255
 			'unicorn/consistent-destructuring': 'off',
+			// Buggy
+			'unicorn/custom-error-definition': 'off',
 			'unicorn/prefer-array-flat': [
 				'error',
 				{