Skip to content

Commit

Permalink
Extend fixers for prefer-string-slice (#424)
Browse files Browse the repository at this point in the history
  • Loading branch information
@voxpelli @sindresorhus
voxpelli authored and sindresorhus committed Nov 29, 2019
1 parent fa8c80e commit 567c970
Show file tree
Hide file tree
Showing 2 changed files with 214 additions and 6 deletions.
94 changes: 88 additions & 6 deletions rules/prefer-string-slice.js
View file
Expand Up @@ -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();

Expand All @@ -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);
Expand All @@ -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, <value>), 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);
Expand Down
126 changes: 126 additions & 0 deletions test/prefer-string-slice.js
View file
@@ -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, {
Expand Down Expand Up @@ -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)',
Expand Down Expand Up @@ -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)',
Expand Down

0 comments on commit 567c970

Please sign in to comment.