Skip to content

Commit 267115a

Browse files
fiskerfisker
authored
prefer-string-slice: Improve fix (#1675)
1 parent 152f153 commit 267115a

File tree

8 files changed

+360
-98
lines changed

8 files changed

+360
-98
lines changed

‎rules/fix/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ module.exports = {
77

88
appendArgument: require('./append-argument.js'),
99
removeArgument: require('./remove-argument.js'),
10+
replaceArgument: require('./replace-argument.js'),
1011
switchNewExpressionToCallExpression: require('./switch-new-expression-to-call-expression.js'),
1112
switchCallExpressionToNewExpression: require('./switch-call-expression-to-new-expression.js'),
1213
removeMemberExpressionProperty: require('./remove-member-expression-property.js'),

‎rules/fix/replace-argument.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
'use strict';
2+
const {getParenthesizedRange} = require('../utils/parentheses.js');
3+
4+
function replaceArgument(fixer, node, text, sourceCode) {
5+
return fixer.replaceTextRange(getParenthesizedRange(node, sourceCode), text);
6+
}
7+
8+
module.exports = replaceArgument;

‎rules/prefer-string-slice.js‎

Lines changed: 113 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
'use strict';
2-
const {getParenthesizedText} = require('./utils/parentheses.js');
2+
const {getStaticValue} = require('eslint-utils');
3+
const {getParenthesizedText, getParenthesizedRange} = require('./utils/parentheses.js');
34
const {methodCallSelector} = require('./selectors/index.js');
45
const isNumber = require('./utils/is-number.js');
6+
const {replaceArgument} = require('./fix/index.js');
57

68
const MESSAGE_ID_SUBSTR = 'substr';
79
const MESSAGE_ID_SUBSTRING = 'substring';
@@ -37,123 +39,139 @@ const isLengthProperty = node => (
3739
&& node.property.name === 'length'
3840
);
3941

40-
function getFixArguments(node, context) {
42+
function * fixSubstrArguments({node, fixer, context, abort}) {
4143
const argumentNodes = node.arguments;
44+
const [firstArgument, secondArgument] = argumentNodes;
4245

43-
if (argumentNodes.length === 0) {
44-
return [];
46+
if (!secondArgument) {
47+
return;
4548
}
4649

50+
const scope = context.getScope();
4751
const sourceCode = context.getSourceCode();
48-
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
49-
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
50-
51-
const method = node.callee.property.name;
52-
53-
if (method === 'substr') {
54-
switch (argumentNodes.length) {
55-
case 1: {
56-
return [firstArgument];
57-
}
58-
59-
case 2: {
60-
if (firstArgument === '0') {
61-
const sliceCallArguments = [firstArgument];
62-
if (isLiteralNumber(secondArgument) || isLengthProperty(argumentNodes[1])) {
63-
sliceCallArguments.push(secondArgument);
64-
} else if (typeof getNumericValue(argumentNodes[1]) === 'number') {
65-
sliceCallArguments.push(Math.max(0, getNumericValue(argumentNodes[1])));
66-
} else {
67-
sliceCallArguments.push(`Math.max(0, ${secondArgument})`);
68-
}
69-
70-
return sliceCallArguments;
71-
}
72-
73-
if (argumentNodes.every(node => isLiteralNumber(node))) {
74-
return [
75-
firstArgument,
76-
argumentNodes[0].value + argumentNodes[1].value,
77-
];
78-
}
52+
const firstArgumentStaticResult = getStaticValue(firstArgument, scope);
53+
const secondArgumentRange = getParenthesizedRange(secondArgument, sourceCode);
54+
const replaceSecondArgument = text => replaceArgument(fixer, secondArgument, text, sourceCode);
7955

80-
if (argumentNodes.every(node => isNumber(node, context.getScope()))) {
81-
return [firstArgument, firstArgument + ' + ' + secondArgument];
82-
}
56+
if (firstArgumentStaticResult && firstArgumentStaticResult.value === 0) {
57+
if (isLiteralNumber(secondArgument) || isLengthProperty(secondArgument)) {
58+
return;
59+
}
8360

84-
break;
85-
}
86-
// No default
61+
if (typeof getNumericValue(secondArgument) === 'number') {
62+
yield replaceSecondArgument(Math.max(0, getNumericValue(secondArgument)));
63+
return;
8764
}
88-
} else if (method === 'substring') {
89-
const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;
90-
switch (argumentNodes.length) {
91-
case 1: {
92-
if (firstNumber !== undefined) {
93-
return [Math.max(0, firstNumber)];
94-
}
9565

96-
if (isLengthProperty(argumentNodes[0])) {
97-
return [firstArgument];
98-
}
66+
yield fixer.insertTextBeforeRange(secondArgumentRange, 'Math.max(0, ');
67+
yield fixer.insertTextAfterRange(secondArgumentRange, ')');
68+
return;
69+
}
9970

100-
return [`Math.max(0, ${firstArgument})`];
101-
}
71+
if (argumentNodes.every(node => isLiteralNumber(node))) {
72+
yield replaceSecondArgument(firstArgument.value + secondArgument.value);
73+
return;
74+
}
10275

103-
case 2: {
104-
const secondNumber = getNumericValue(argumentNodes[1]);
76+
if (argumentNodes.every(node => isNumber(node, context.getScope()))) {
77+
const firstArgumentText = getParenthesizedText(firstArgument, sourceCode);
10578

106-
if (firstNumber !== undefined && secondNumber !== undefined) {
107-
return firstNumber > secondNumber
108-
? [Math.max(0, secondNumber), Math.max(0, firstNumber)]
109-
: [Math.max(0, firstNumber), Math.max(0, secondNumber)];
110-
}
79+
yield fixer.insertTextBeforeRange(secondArgumentRange, `${firstArgumentText} + `);
80+
return;
81+
}
11182

112-
if (firstNumber === 0 || secondNumber === 0) {
113-
return [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
114-
}
83+
return abort();
84+
}
85+
86+
function * fixSubstringArguments({node, fixer, context, abort}) {
87+
const sourceCode = context.getSourceCode();
88+
const [firstArgument, secondArgument] = node.arguments;
11589

116-
// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
117-
// .substring(0, 2) and .substring(2, 0) returns the same result
118-
// .slice(0, 2) and .slice(2, 0) doesn't return the same result
119-
// There's also an issue with us now knowing whether the value will be negative or not, due to:
120-
// .substring() treats a negative number the same as it treats a zero.
121-
// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
90+
const firstNumber = firstArgument ? getNumericValue(firstArgument) : undefined;
91+
const firstArgumentText = getParenthesizedText(firstArgument, sourceCode);
92+
const replaceFirstArgument = text => replaceArgument(fixer, firstArgument, text, sourceCode);
12293

123-
break;
124-
}
125-
// No default
94+
if (!secondArgument) {
95+
if (isLengthProperty(firstArgument)) {
96+
return;
12697
}
98+
99+
if (firstNumber !== undefined) {
100+
yield replaceFirstArgument(Math.max(0, firstNumber));
101+
return;
102+
}
103+
104+
const firstArgumentRange = getParenthesizedRange(firstArgument, sourceCode);
105+
yield fixer.insertTextBeforeRange(firstArgumentRange, 'Math.max(0, ');
106+
yield fixer.insertTextAfterRange(firstArgumentRange, ')');
107+
return;
127108
}
128-
}
129109

130-
/** @param {import('eslint').Rule.RuleContext} context */
131-
const create = context => {
132-
const sourceCode = context.getSourceCode();
110+
const secondNumber = getNumericValue(secondArgument);
111+
const secondArgumentText = getParenthesizedText(secondArgument, sourceCode);
112+
const replaceSecondArgument = text => replaceArgument(fixer, secondArgument, text, sourceCode);
113+
114+
if (firstNumber !== undefined && secondNumber !== undefined) {
115+
const argumentsValue = [Math.max(0, firstNumber), Math.max(0, secondNumber)];
116+
if (firstNumber > secondNumber) {
117+
argumentsValue.reverse();
118+
}
119+
120+
if (argumentsValue[0] !== firstNumber) {
121+
yield replaceFirstArgument(argumentsValue[0]);
122+
}
133123

134-
return {
135-
[selector](node) {
136-
const problem = {
137-
node,
138-
messageId: node.callee.property.name,
139-
};
124+
if (argumentsValue[1] !== secondNumber) {
125+
yield replaceSecondArgument(argumentsValue[1]);
126+
}
140127

141-
const sliceCallArguments = getFixArguments(node, context);
142-
if (!sliceCallArguments) {
143-
return problem;
144-
}
128+
return;
129+
}
145130

146-
const objectNode = node.callee.object;
147-
const objectText = getParenthesizedText(objectNode, sourceCode);
148-
const optionalMemberSuffix = node.callee.optional ? '?' : '';
149-
const optionalCallSuffix = node.optional ? '?.' : '';
131+
if (firstNumber === 0 || secondNumber === 0) {
132+
yield replaceFirstArgument(0);
133+
yield replaceSecondArgument(`Math.max(0, ${firstNumber === 0 ? secondArgumentText : firstArgumentText})`);
134+
return;
135+
}
150136

151-
problem.fix = fixer => fixer.replaceText(node, `${objectText}${optionalMemberSuffix}.slice${optionalCallSuffix}(${sliceCallArguments.join(', ')})`);
137+
// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
138+
// .substring(0, 2) and .substring(2, 0) returns the same result
139+
// .slice(0, 2) and .slice(2, 0) doesn't return the same result
140+
// There's also an issue with us now knowing whether the value will be negative or not, due to:
141+
// .substring() treats a negative number the same as it treats a zero.
142+
// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
152143

153-
return problem;
154-
},
155-
};
156-
};
144+
return abort();
145+
}
146+
147+
/** @param {import('eslint').Rule.RuleContext} context */
148+
const create = context => ({
149+
[selector](node) {
150+
const method = node.callee.property.name;
151+
152+
return {
153+
node,
154+
messageId: method,
155+
* fix(fixer, {abort}) {
156+
yield fixer.replaceText(node.callee.property, 'slice');
157+
158+
if (node.arguments.length === 0) {
159+
return;
160+
}
161+
162+
if (
163+
node.arguments.length > 2
164+
|| node.arguments.some(node => node.type === 'SpreadElement')
165+
) {
166+
return abort();
167+
}
168+
169+
const fixArguments = method === 'substr' ? fixSubstrArguments : fixSubstringArguments;
170+
yield * fixArguments({node, fixer, context, abort});
171+
},
172+
};
173+
},
174+
});
157175

158176
/** @type {import('eslint').Rule.RuleModule} */
159177
module.exports = {

‎rules/utils/rule.js‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,34 @@ const getDocumentationUrl = require('./get-documentation-url.js');
55

66
const isIterable = object => typeof object[Symbol.iterator] === 'function';
77

8+
class FixAbortError extends Error {}
9+
const fixOptions = {
10+
abort() {
11+
throw new FixAbortError('Fix aborted.');
12+
},
13+
};
14+
15+
function wrapFixFunction(fix) {
16+
return fixer => {
17+
const result = fix(fixer, fixOptions);
18+
19+
if (result && isIterable(result)) {
20+
try {
21+
return [...result];
22+
} catch (error) {
23+
if (error instanceof FixAbortError) {
24+
return;
25+
}
26+
27+
/* istanbul ignore next: Safe */
28+
throw error;
29+
}
30+
}
31+
32+
return result;
33+
};
34+
}
35+
836
function reportListenerProblems(listener, context) {
937
// Listener arguments can be `codePath, node` or `node`
1038
return function (...listenerArguments) {
@@ -18,11 +46,20 @@ function reportListenerProblems(listener, context) {
1846
problems = [problems];
1947
}
2048

21-
// TODO: Allow `fix` function to abort
2249
for (const problem of problems) {
23-
if (problem) {
24-
context.report(problem);
50+
if (problem.fix) {
51+
problem.fix = wrapFixFunction(problem.fix);
2552
}
53+
54+
if (Array.isArray(problem.suggest)) {
55+
for (const suggest of problem.suggest) {
56+
if (suggest.fix) {
57+
suggest.fix = wrapFixFunction(suggest.fix);
58+
}
59+
}
60+
}
61+
62+
context.report(problem);
2663
}
2764
};
2865
}

‎test/prefer-string-slice.mjs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,3 +321,29 @@ test.typescript({
321321
},
322322
],
323323
});
324+
325+
test.snapshot({
326+
valid: [],
327+
invalid: [
328+
outdent`
329+
/* 1 */ (( /* 2 */ 0 /* 3 */, /* 4 */ foo /* 5 */ )) /* 6 */
330+
. /* 7 */ substring /* 8 */ (
331+
/* 9 */ (( /* 10 */ bar /* 11 */ )) /* 12 */,
332+
/* 13 */ (( /* 14 */ 0 /* 15 */ )) /* 16 */,
333+
/* 17 */
334+
)
335+
/* 18 */
336+
`,
337+
'foo.substr(0, ...bar)',
338+
'foo.substr(...bar)',
339+
'foo.substr(0, (100, 1))',
340+
'foo.substr(0, 1, extraArgument)',
341+
'foo.substr((0, bar.length), (0, baz.length))',
342+
// TODO: Fix this
343+
// 'foo.substr(await 1, await 2)',
344+
'foo.substring((10, 1), 0)',
345+
'foo.substring(0, (10, 1))',
346+
'foo.substring(0, await 1)',
347+
'foo.substring((10, bar))',
348+
],
349+
});

‎test/run-rules-on-codebase/lint.mjs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ const eslint = new ESLint({
3232
],
3333
// https://github.com/sindresorhus/eslint-plugin-unicorn/issues/1109#issuecomment-782689255
3434
'unicorn/consistent-destructuring': 'off',
35+
// Buggy
36+
'unicorn/custom-error-definition': 'off',
3537
'unicorn/prefer-array-flat': [
3638
'error',
3739
{

0 commit comments

Comments
 (0)