prefer-string-slice: Handle negative length in .substr() (#709)

sindresorhus · May 4, 2020 · 7439465 · 7439465
1 parent da978e3
commit 7439465
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 19 deletions.
diff --git a/rules/prefer-string-slice.js b/rules/prefer-string-slice.js
@@ -63,35 +63,42 @@ const create = context => {
 			const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
 			const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
 
-			let slice;
+			let sliceArguments;
 
 			if (argumentNodes.length === 0) {
-				slice = [];
+				sliceArguments = [];
 			} else if (argumentNodes.length === 1) {
-				slice = [firstArgument];
+				sliceArguments = [firstArgument];
 			} else if (argumentNodes.length === 2) {
 				if (firstArgument === '0') {
-					slice = [firstArgument, secondArgument];
+					sliceArguments = [firstArgument];
+					if (isLiteralNumber(secondArgument) || isLengthProperty(argumentNodes[1])) {
+						sliceArguments.push(secondArgument);
+					} else if (typeof getNumericValue(argumentNodes[1]) === 'number') {
+						sliceArguments.push(Math.max(0, getNumericValue(argumentNodes[1])));
+					} else {
+						sliceArguments.push(`Math.max(0, ${secondArgument})`);
+					}
 				} else if (
 					isLiteralNumber(argumentNodes[0]) &&
 					isLiteralNumber(argumentNodes[1])
 				) {
-					slice = [
+					sliceArguments = [
 						firstArgument,
 						argumentNodes[0].value + argumentNodes[1].value
 					];
 				} else if (
 					isLikelyNumeric(argumentNodes[0]) &&
 					isLikelyNumeric(argumentNodes[1])
 				) {
-					slice = [firstArgument, firstArgument + ' + ' + secondArgument];
+					sliceArguments = [firstArgument, firstArgument + ' + ' + secondArgument];
 				}
 			}
 
-			if (slice) {
+			if (sliceArguments) {
 				const objectText = getNodeText(objectNode);
 
-				problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`);
+				problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${sliceArguments.join(', ')})`);
 			}
 
 			context.report(problem);
@@ -111,27 +118,27 @@ const create = context => {
 
 			const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;
 
-			let slice;
+			let sliceArguments;
 
 			if (argumentNodes.length === 0) {
-				slice = [];
+				sliceArguments = [];
 			} else if (argumentNodes.length === 1) {
 				if (firstNumber !== undefined) {
-					slice = [Math.max(0, firstNumber)];
+					sliceArguments = [Math.max(0, firstNumber)];
 				} else if (isLengthProperty(argumentNodes[0])) {
-					slice = [firstArgument];
+					sliceArguments = [firstArgument];
 				} else {
-					slice = [`Math.max(0, ${firstArgument})`];
+					sliceArguments = [`Math.max(0, ${firstArgument})`];
 				}
 			} else if (argumentNodes.length === 2) {
-				const secondNumber = argumentNodes[1] ? getNumericValue(argumentNodes[1]) : undefined;
+				const secondNumber = getNumericValue(argumentNodes[1]);
 
 				if (firstNumber !== undefined && secondNumber !== undefined) {
-					slice = firstNumber > secondNumber ?
+					sliceArguments = firstNumber > secondNumber ?
 						[Math.max(0, secondNumber), Math.max(0, firstNumber)] :
 						[Math.max(0, firstNumber), Math.max(0, secondNumber)];
 				} else if (firstNumber === 0 || secondNumber === 0) {
-					slice = [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
+					sliceArguments = [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
 				} else {
 					// 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
@@ -142,9 +149,9 @@ const create = context => {
 				}
 			}
 
-			if (slice) {
+			if (sliceArguments) {
 				const objectText = getNodeText(objectNode);
-				problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`);
+				problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${sliceArguments.join(', ')})`);
 			}
 
 			context.report(problem);

diff --git a/test/prefer-string-slice.js b/test/prefer-string-slice.js
@@ -84,7 +84,7 @@ ruleTester.run('prefer-string-slice', rule, {
 			`,
 			output: outdent`
 				const length = 123;
-				"foo".slice(0, length)
+				"foo".slice(0, Math.max(0, length))
 			`,
 			errors
 		},
@@ -99,6 +99,16 @@ ruleTester.run('prefer-string-slice', rule, {
 			`,
 			errors
 		},
+		{
+			code: '"foo".substr(0, -1)',
+			output: '"foo".slice(0, 0)',
+			errors
+		},
+		{
+			code: '"foo".substr(0, "foo".length)',
+			output: '"foo".slice(0, "foo".length)',
+			errors
+		},
 		{
 			code: outdent`
 				const length = 123;
@@ -138,6 +148,18 @@ ruleTester.run('prefer-string-slice', rule, {
 			code: '"foo".substr(1, 2)',
 			errors
 		},
+		// Extra arguments
+		{
+			code: 'foo.substr(1, 2, 3)',
+			output: 'foo.substr(1, 2, 3)',
+			errors
+		},
+		// #700
+		{
+			code: '"Sample".substr(0, "Sample".lastIndexOf("/"))',
+			output: '"Sample".slice(0, Math.max(0, "Sample".lastIndexOf("/")))',
+			errors
+		},
 
 		{
 			code: 'foo.substring()',
@@ -210,6 +232,12 @@ ruleTester.run('prefer-string-slice', rule, {
 		{
 			code: '"foo".substring(1, 3)',
 			errors
+		},
+		// Extra arguments
+		{
+			code: 'foo.substring(1, 2, 3)',
+			output: 'foo.substring(1, 2, 3)',
+			errors
 		}
 	]
 });