Skip to content

Commit 567c970

Browse files
voxpellisindresorhus
voxpelli
authored and
sindresorhus
committed
Extend fixers for prefer-string-slice (#424)
1 parent fa8c80e commit 567c970

File tree

2 files changed

+214
-6
lines changed

2 files changed

+214
-6
lines changed

‎rules/prefer-string-slice.js‎

Lines changed: 88 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,29 @@ const argumentsVariable = templates.spreadVariable();
1010
const substrCallTemplate = templates.template`${objectVariable}.substr(${argumentsVariable})`;
1111
const substringCallTemplate = templates.template`${objectVariable}.substring(${argumentsVariable})`;
1212

13+
const isLiteralNumber = node => node && node.type === 'Literal' && typeof node.value === 'number';
14+
15+
const getNumericValue = node => {
16+
if (isLiteralNumber(node)) {
17+
return node.value;
18+
}
19+
20+
if (node.type === 'UnaryExpression' && node.operator === '-') {
21+
return -getNumericValue(node.argument);
22+
}
23+
};
24+
25+
// This handles cases where the argument is very likely to be a number, such as `.substring('foo'.length)`.
26+
const isLengthProperty = node => (
27+
node &&
28+
node.type === 'MemberExpression' &&
29+
node.computed === false &&
30+
node.property.type === 'Identifier' &&
31+
node.property.name === 'length'
32+
);
33+
34+
const isLikelyNumeric = node => isLiteralNumber(node) || isLengthProperty(node);
35+
1336
const create = context => {
1437
const sourceCode = context.getSourceCode();
1538

@@ -23,10 +46,31 @@ const create = context => {
2346
message: 'Prefer `String#slice()` over `String#substr()`.'
2447
};
2548

26-
const canFix = argumentNodes.length === 0;
49+
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
50+
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
51+
52+
let slice;
2753

28-
if (canFix) {
29-
problem.fix = fixer => fixer.replaceText(node, sourceCode.getText(objectNode) + '.slice()');
54+
if (argumentNodes.length === 0) {
55+
slice = [];
56+
} else if (argumentNodes.length === 1) {
57+
slice = [firstArgument];
58+
} else if (argumentNodes.length === 2) {
59+
if (firstArgument === '0') {
60+
slice = [firstArgument, secondArgument];
61+
} else if (isLiteralNumber(argumentNodes[0]) && isLiteralNumber(argumentNodes[1])) {
62+
slice = [firstArgument, argumentNodes[0].value + argumentNodes[1].value];
63+
} else if (isLikelyNumeric(argumentNodes[0]) && isLikelyNumeric(argumentNodes[1])) {
64+
slice = [firstArgument, firstArgument + ' + ' + secondArgument];
65+
}
66+
}
67+
68+
if (slice) {
69+
const objectText = objectNode.type === 'LogicalExpression' ?
70+
`(${sourceCode.getText(objectNode)})` :
71+
sourceCode.getText(objectNode);
72+
73+
problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`);
3074
}
3175

3276
context.report(problem);
@@ -41,10 +85,48 @@ const create = context => {
4185
message: 'Prefer `String#slice()` over `String#substring()`.'
4286
};
4387

44-
const canFix = argumentNodes.length === 0;
88+
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
89+
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
90+
91+
const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;
92+
93+
let slice;
94+
95+
if (argumentNodes.length === 0) {
96+
slice = [];
97+
} else if (argumentNodes.length === 1) {
98+
if (firstNumber !== undefined) {
99+
slice = [Math.max(0, firstNumber)];
100+
} else if (isLengthProperty(argumentNodes[0])) {
101+
slice = [firstArgument];
102+
} else {
103+
slice = [`Math.max(0, ${firstArgument})`];
104+
}
105+
} else if (argumentNodes.length === 2) {
106+
const secondNumber = argumentNodes[1] ? getNumericValue(argumentNodes[1]) : undefined;
107+
108+
if (firstNumber !== undefined && secondNumber !== undefined) {
109+
slice = firstNumber > secondNumber ?
110+
[Math.max(0, secondNumber), Math.max(0, firstNumber)] :
111+
[Math.max(0, firstNumber), Math.max(0, secondNumber)];
112+
} else if (firstNumber === 0 || secondNumber === 0) {
113+
slice = [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
114+
} else {
115+
// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
116+
// .substring(0, 2) and .substring(2, 0) returns the same result
117+
// .slice(0, 2) and .slice(2, 0) doesn't return the same result
118+
// There's also an issue with us now knowing whether the value will be negative or not, due to:
119+
// .substring() treats a negative number the same as it treats a zero.
120+
// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
121+
}
122+
}
123+
124+
if (slice) {
125+
const objectText = objectNode.type === 'LogicalExpression' ?
126+
`(${sourceCode.getText(objectNode)})` :
127+
sourceCode.getText(objectNode);
45128

46-
if (canFix) {
47-
problem.fix = fixer => fixer.replaceText(node, sourceCode.getText(objectNode) + '.slice()');
129+
problem.fix = fixer => fixer.replaceText(node, `${objectText}.slice(${slice.join(', ')})`);
48130
}
49131

50132
context.report(problem);

‎test/prefer-string-slice.js‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import test from 'ava';
22
import avaRuleTester from 'eslint-ava-rule-tester';
3+
import {outdent} from 'outdent';
34
import rule from '../rules/prefer-string-slice';
45

56
const ruleTester = avaRuleTester(test, {
@@ -34,6 +35,86 @@ ruleTester.run('prefer-string-slice', rule, {
3435
output: '"foo".slice()',
3536
errors
3637
},
38+
{
39+
code: '"foo".substr(1)',
40+
output: '"foo".slice(1)',
41+
errors
42+
},
43+
{
44+
code: '"foo".substr(1, 2)',
45+
output: '"foo".slice(1, 3)',
46+
errors
47+
},
48+
{
49+
code: '"foo".substr(1, length)',
50+
output: '"foo".substr(1, length)',
51+
errors
52+
},
53+
{
54+
code: '"foo".substr(1, "abc".length)',
55+
output: '"foo".slice(1, 1 + "abc".length)',
56+
errors
57+
},
58+
{
59+
code: '"foo".substr("1", 2)',
60+
output: '"foo".substr("1", 2)',
61+
errors
62+
},
63+
{
64+
code: outdent`
65+
const length = 123;
66+
"foo".substr(1, length)
67+
`,
68+
output: outdent`
69+
const length = 123;
70+
"foo".substr(1, length)
71+
`,
72+
errors
73+
},
74+
{
75+
code: outdent`
76+
const length = 123;
77+
"foo".substr(0, length)
78+
`,
79+
output: outdent`
80+
const length = 123;
81+
"foo".slice(0, length)
82+
`,
83+
errors
84+
},
85+
{
86+
code: outdent`
87+
const length = 123;
88+
"foo".substr('0', length)
89+
`,
90+
output: outdent`
91+
const length = 123;
92+
"foo".substr('0', length)
93+
`,
94+
errors
95+
},
96+
{
97+
code: outdent`
98+
const length = 123;
99+
"foo".substr(1, length - 4)
100+
`,
101+
output: outdent`
102+
const length = 123;
103+
"foo".substr(1, length - 4)
104+
`,
105+
errors
106+
},
107+
{
108+
code: outdent`
109+
const uri = 'foo';
110+
(uri || '').substr(1)
111+
`,
112+
output: outdent`
113+
const uri = 'foo';
114+
(uri || '').slice(1)
115+
`,
116+
errors
117+
},
37118

38119
{
39120
code: 'foo.substr(start)',
@@ -62,6 +143,51 @@ ruleTester.run('prefer-string-slice', rule, {
62143
output: '"foo".slice()',
63144
errors
64145
},
146+
{
147+
code: '"foo".substring(1)',
148+
output: '"foo".slice(1)',
149+
errors
150+
},
151+
{
152+
code: '"foo".substring(1, 2)',
153+
output: '"foo".slice(1, 2)',
154+
errors
155+
},
156+
{
157+
code: '"foo".substring(2, 1)',
158+
output: '"foo".slice(1, 2)',
159+
errors
160+
},
161+
{
162+
code: '"foo".substring(-1, -5)',
163+
output: '"foo".slice(0, 0)',
164+
errors
165+
},
166+
{
167+
code: '"foo".substring(-1, 2)',
168+
output: '"foo".slice(0, 2)',
169+
errors
170+
},
171+
{
172+
code: '"foo".substring(length)',
173+
output: '"foo".slice(Math.max(0, length))',
174+
errors
175+
},
176+
{
177+
code: '"foo".substring("fo".length)',
178+
output: '"foo".slice("fo".length)',
179+
errors
180+
},
181+
{
182+
code: '"foo".substring(0, length)',
183+
output: '"foo".slice(0, Math.max(0, length))',
184+
errors
185+
},
186+
{
187+
code: '"foo".substring(length, 0)',
188+
output: '"foo".slice(0, Math.max(0, length))',
189+
errors
190+
},
65191

66192
{
67193
code: 'foo.substring(start)',

0 commit comments

Comments
 (0)