From 567c9700e1557dce326001b7f2fe2ad1c7b0adc7 Mon Sep 17 00:00:00 2001 From: Pelle Wessman Date: Fri, 29 Nov 2019 10:52:04 +0100 Subject: [PATCH] Extend fixers for `prefer-string-slice` (#424) --- rules/prefer-string-slice.js | 94 ++++++++++++++++++++++++-- test/prefer-string-slice.js | 126 +++++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 6 deletions(-) diff --git a/rules/prefer-string-slice.js b/rules/prefer-string-slice.js index 1157a6e75d..b8ad529318 100644 --- a/rules/prefer-string-slice.js +++ b/rules/prefer-string-slice.js @@ -10,6 +10,29 @@ const argumentsVariable = templates.spreadVariable(); const substrCallTemplate = templates.template`${objectVariable}.substr(${argumentsVariable})`; const substringCallTemplate = templates.template`${objectVariable}.substring(${argumentsVariable})`; +const isLiteralNumber = node => node && node.type === 'Literal' && typeof node.value === 'number'; + +const getNumericValue = node => { + if (isLiteralNumber(node)) { + return node.value; + } + + if (node.type === 'UnaryExpression' && node.operator === '-') { + return -getNumericValue(node.argument); + } +}; + +// This handles cases where the argument is very likely to be a number, such as `.substring('foo'.length)`. +const isLengthProperty = node => ( + node && + node.type === 'MemberExpression' && + node.computed === false && + node.property.type === 'Identifier' && + node.property.name === 'length' +); + +const isLikelyNumeric = node => isLiteralNumber(node) || isLengthProperty(node); + const create = context => { const sourceCode = context.getSourceCode(); @@ -23,10 +46,31 @@ const create = context => { message: 'Prefer `String#slice()` over `String#substr()`.' }; - const canFix = argumentNodes.length === 0; + const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined; + const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined; + + let slice; - if (canFix) { - problem.fix = fixer => fixer.replaceText(node, sourceCode.getText(objectNode) + '.slice()'); + if (argumentNodes.length === 0) { + slice = []; + } else if (argumentNodes.length === 1) { + slice = [firstArgument]; + } else if (argumentNodes.length === 2) { + if (firstArgument === '0') { + slice = [firstArgument, secondArgument]; + } else if (isLiteralNumber(argumentNodes[0]) && isLiteralNumber(argumentNodes[1])) { + slice = [firstArgument, argumentNodes[0].value + argumentNodes[1].value]; + } else if (isLikelyNumeric(argumentNodes[0]) && isLikelyNumeric(argumentNodes[1])) { + slice = [firstArgument, firstArgument + ' + ' + secondArgument]; + } + } + + if (slice) { + const objectText = objectNode.type === 'LogicalExpression' ? + `(${sourceCode.getText(objectNode)})` : + sourceCode.getText(objectNode); + + problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`); } context.report(problem); @@ -41,10 +85,48 @@ const create = context => { message: 'Prefer `String#slice()` over `String#substring()`.' }; - const canFix = argumentNodes.length === 0; + const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined; + const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined; + + const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined; + + let slice; + + if (argumentNodes.length === 0) { + slice = []; + } else if (argumentNodes.length === 1) { + if (firstNumber !== undefined) { + slice = [Math.max(0, firstNumber)]; + } else if (isLengthProperty(argumentNodes[0])) { + slice = [firstArgument]; + } else { + slice = [`Math.max(0, ${firstArgument})`]; + } + } else if (argumentNodes.length === 2) { + const secondNumber = argumentNodes[1] ? getNumericValue(argumentNodes[1]) : undefined; + + if (firstNumber !== undefined && secondNumber !== undefined) { + slice = 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})`]; + } 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 + // .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, ), but the resulting code would not be nice + } + } + + if (slice) { + const objectText = objectNode.type === 'LogicalExpression' ? + `(${sourceCode.getText(objectNode)})` : + sourceCode.getText(objectNode); - if (canFix) { - problem.fix = fixer => fixer.replaceText(node, sourceCode.getText(objectNode) + '.slice()'); + problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`); } context.report(problem); diff --git a/test/prefer-string-slice.js b/test/prefer-string-slice.js index a60112c22e..1aee27161f 100644 --- a/test/prefer-string-slice.js +++ b/test/prefer-string-slice.js @@ -1,5 +1,6 @@ import test from 'ava'; import avaRuleTester from 'eslint-ava-rule-tester'; +import {outdent} from 'outdent'; import rule from '../rules/prefer-string-slice'; const ruleTester = avaRuleTester(test, { @@ -34,6 +35,86 @@ ruleTester.run('prefer-string-slice', rule, { output: '"foo".slice()', errors }, + { + code: '"foo".substr(1)', + output: '"foo".slice(1)', + errors + }, + { + code: '"foo".substr(1, 2)', + output: '"foo".slice(1, 3)', + errors + }, + { + code: '"foo".substr(1, length)', + output: '"foo".substr(1, length)', + errors + }, + { + code: '"foo".substr(1, "abc".length)', + output: '"foo".slice(1, 1 + "abc".length)', + errors + }, + { + code: '"foo".substr("1", 2)', + output: '"foo".substr("1", 2)', + errors + }, + { + code: outdent` + const length = 123; + "foo".substr(1, length) + `, + output: outdent` + const length = 123; + "foo".substr(1, length) + `, + errors + }, + { + code: outdent` + const length = 123; + "foo".substr(0, length) + `, + output: outdent` + const length = 123; + "foo".slice(0, length) + `, + errors + }, + { + code: outdent` + const length = 123; + "foo".substr('0', length) + `, + output: outdent` + const length = 123; + "foo".substr('0', length) + `, + errors + }, + { + code: outdent` + const length = 123; + "foo".substr(1, length - 4) + `, + output: outdent` + const length = 123; + "foo".substr(1, length - 4) + `, + errors + }, + { + code: outdent` + const uri = 'foo'; + (uri || '').substr(1) + `, + output: outdent` + const uri = 'foo'; + (uri || '').slice(1) + `, + errors + }, { code: 'foo.substr(start)', @@ -62,6 +143,51 @@ ruleTester.run('prefer-string-slice', rule, { output: '"foo".slice()', errors }, + { + code: '"foo".substring(1)', + output: '"foo".slice(1)', + errors + }, + { + code: '"foo".substring(1, 2)', + output: '"foo".slice(1, 2)', + errors + }, + { + code: '"foo".substring(2, 1)', + output: '"foo".slice(1, 2)', + errors + }, + { + code: '"foo".substring(-1, -5)', + output: '"foo".slice(0, 0)', + errors + }, + { + code: '"foo".substring(-1, 2)', + output: '"foo".slice(0, 2)', + errors + }, + { + code: '"foo".substring(length)', + output: '"foo".slice(Math.max(0, length))', + errors + }, + { + code: '"foo".substring("fo".length)', + output: '"foo".slice("fo".length)', + errors + }, + { + code: '"foo".substring(0, length)', + output: '"foo".slice(0, Math.max(0, length))', + errors + }, + { + code: '"foo".substring(length, 0)', + output: '"foo".slice(0, Math.max(0, length))', + errors + }, { code: 'foo.substring(start)',