From 267115ade4da87b0d5390b53793e593b15562b85 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Fri, 31 Dec 2021 07:17:27 +0800 Subject: [PATCH] `prefer-string-slice`: Improve fix (#1675) --- rules/fix/index.js | 1 + rules/fix/replace-argument.js | 8 + rules/prefer-string-slice.js | 208 +++++++++++--------- rules/utils/rule.js | 43 +++- test/prefer-string-slice.mjs | 26 +++ test/run-rules-on-codebase/lint.mjs | 2 + test/snapshots/prefer-string-slice.mjs.md | 170 ++++++++++++++++ test/snapshots/prefer-string-slice.mjs.snap | Bin 0 -> 776 bytes 8 files changed, 360 insertions(+), 98 deletions(-) create mode 100644 rules/fix/replace-argument.js create mode 100644 test/snapshots/prefer-string-slice.mjs.md create mode 100644 test/snapshots/prefer-string-slice.mjs.snap diff --git a/rules/fix/index.js b/rules/fix/index.js index 5ce2853202..ee9949ccdc 100644 --- a/rules/fix/index.js +++ b/rules/fix/index.js @@ -7,6 +7,7 @@ module.exports = { appendArgument: require('./append-argument.js'), removeArgument: require('./remove-argument.js'), + replaceArgument: require('./replace-argument.js'), switchNewExpressionToCallExpression: require('./switch-new-expression-to-call-expression.js'), switchCallExpressionToNewExpression: require('./switch-call-expression-to-new-expression.js'), removeMemberExpressionProperty: require('./remove-member-expression-property.js'), diff --git a/rules/fix/replace-argument.js b/rules/fix/replace-argument.js new file mode 100644 index 0000000000..c07a39f0f6 --- /dev/null +++ b/rules/fix/replace-argument.js @@ -0,0 +1,8 @@ +'use strict'; +const {getParenthesizedRange} = require('../utils/parentheses.js'); + +function replaceArgument(fixer, node, text, sourceCode) { + return fixer.replaceTextRange(getParenthesizedRange(node, sourceCode), text); +} + +module.exports = replaceArgument; diff --git a/rules/prefer-string-slice.js b/rules/prefer-string-slice.js index d52af7949d..1f4331d10b 100644 --- a/rules/prefer-string-slice.js +++ b/rules/prefer-string-slice.js @@ -1,7 +1,9 @@ 'use strict'; -const {getParenthesizedText} = require('./utils/parentheses.js'); +const {getStaticValue} = require('eslint-utils'); +const {getParenthesizedText, getParenthesizedRange} = require('./utils/parentheses.js'); const {methodCallSelector} = require('./selectors/index.js'); const isNumber = require('./utils/is-number.js'); +const {replaceArgument} = require('./fix/index.js'); const MESSAGE_ID_SUBSTR = 'substr'; const MESSAGE_ID_SUBSTRING = 'substring'; @@ -37,123 +39,139 @@ const isLengthProperty = node => ( && node.property.name === 'length' ); -function getFixArguments(node, context) { +function * fixSubstrArguments({node, fixer, context, abort}) { const argumentNodes = node.arguments; + const [firstArgument, secondArgument] = argumentNodes; - if (argumentNodes.length === 0) { - return []; + if (!secondArgument) { + return; } + const scope = context.getScope(); const sourceCode = context.getSourceCode(); - const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined; - const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined; - - const method = node.callee.property.name; - - if (method === 'substr') { - switch (argumentNodes.length) { - case 1: { - return [firstArgument]; - } - - case 2: { - if (firstArgument === '0') { - const sliceCallArguments = [firstArgument]; - if (isLiteralNumber(secondArgument) || isLengthProperty(argumentNodes[1])) { - sliceCallArguments.push(secondArgument); - } else if (typeof getNumericValue(argumentNodes[1]) === 'number') { - sliceCallArguments.push(Math.max(0, getNumericValue(argumentNodes[1]))); - } else { - sliceCallArguments.push(`Math.max(0, ${secondArgument})`); - } - - return sliceCallArguments; - } - - if (argumentNodes.every(node => isLiteralNumber(node))) { - return [ - firstArgument, - argumentNodes[0].value + argumentNodes[1].value, - ]; - } + const firstArgumentStaticResult = getStaticValue(firstArgument, scope); + const secondArgumentRange = getParenthesizedRange(secondArgument, sourceCode); + const replaceSecondArgument = text => replaceArgument(fixer, secondArgument, text, sourceCode); - if (argumentNodes.every(node => isNumber(node, context.getScope()))) { - return [firstArgument, firstArgument + ' + ' + secondArgument]; - } + if (firstArgumentStaticResult && firstArgumentStaticResult.value === 0) { + if (isLiteralNumber(secondArgument) || isLengthProperty(secondArgument)) { + return; + } - break; - } - // No default + if (typeof getNumericValue(secondArgument) === 'number') { + yield replaceSecondArgument(Math.max(0, getNumericValue(secondArgument))); + return; } - } else if (method === 'substring') { - const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined; - switch (argumentNodes.length) { - case 1: { - if (firstNumber !== undefined) { - return [Math.max(0, firstNumber)]; - } - if (isLengthProperty(argumentNodes[0])) { - return [firstArgument]; - } + yield fixer.insertTextBeforeRange(secondArgumentRange, 'Math.max(0, '); + yield fixer.insertTextAfterRange(secondArgumentRange, ')'); + return; + } - return [`Math.max(0, ${firstArgument})`]; - } + if (argumentNodes.every(node => isLiteralNumber(node))) { + yield replaceSecondArgument(firstArgument.value + secondArgument.value); + return; + } - case 2: { - const secondNumber = getNumericValue(argumentNodes[1]); + if (argumentNodes.every(node => isNumber(node, context.getScope()))) { + const firstArgumentText = getParenthesizedText(firstArgument, sourceCode); - if (firstNumber !== undefined && secondNumber !== undefined) { - return firstNumber > secondNumber - ? [Math.max(0, secondNumber), Math.max(0, firstNumber)] - : [Math.max(0, firstNumber), Math.max(0, secondNumber)]; - } + yield fixer.insertTextBeforeRange(secondArgumentRange, `${firstArgumentText} + `); + return; + } - if (firstNumber === 0 || secondNumber === 0) { - return [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`]; - } + return abort(); +} + +function * fixSubstringArguments({node, fixer, context, abort}) { + const sourceCode = context.getSourceCode(); + const [firstArgument, secondArgument] = node.arguments; - // 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 + const firstNumber = firstArgument ? getNumericValue(firstArgument) : undefined; + const firstArgumentText = getParenthesizedText(firstArgument, sourceCode); + const replaceFirstArgument = text => replaceArgument(fixer, firstArgument, text, sourceCode); - break; - } - // No default + if (!secondArgument) { + if (isLengthProperty(firstArgument)) { + return; } + + if (firstNumber !== undefined) { + yield replaceFirstArgument(Math.max(0, firstNumber)); + return; + } + + const firstArgumentRange = getParenthesizedRange(firstArgument, sourceCode); + yield fixer.insertTextBeforeRange(firstArgumentRange, 'Math.max(0, '); + yield fixer.insertTextAfterRange(firstArgumentRange, ')'); + return; } -} -/** @param {import('eslint').Rule.RuleContext} context */ -const create = context => { - const sourceCode = context.getSourceCode(); + const secondNumber = getNumericValue(secondArgument); + const secondArgumentText = getParenthesizedText(secondArgument, sourceCode); + const replaceSecondArgument = text => replaceArgument(fixer, secondArgument, text, sourceCode); + + if (firstNumber !== undefined && secondNumber !== undefined) { + const argumentsValue = [Math.max(0, firstNumber), Math.max(0, secondNumber)]; + if (firstNumber > secondNumber) { + argumentsValue.reverse(); + } + + if (argumentsValue[0] !== firstNumber) { + yield replaceFirstArgument(argumentsValue[0]); + } - return { - [selector](node) { - const problem = { - node, - messageId: node.callee.property.name, - }; + if (argumentsValue[1] !== secondNumber) { + yield replaceSecondArgument(argumentsValue[1]); + } - const sliceCallArguments = getFixArguments(node, context); - if (!sliceCallArguments) { - return problem; - } + return; + } - const objectNode = node.callee.object; - const objectText = getParenthesizedText(objectNode, sourceCode); - const optionalMemberSuffix = node.callee.optional ? '?' : ''; - const optionalCallSuffix = node.optional ? '?.' : ''; + if (firstNumber === 0 || secondNumber === 0) { + yield replaceFirstArgument(0); + yield replaceSecondArgument(`Math.max(0, ${firstNumber === 0 ? secondArgumentText : firstArgumentText})`); + return; + } - problem.fix = fixer => fixer.replaceText(node, `${objectText}${optionalMemberSuffix}.slice${optionalCallSuffix}(${sliceCallArguments.join(', ')})`); + // 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 - return problem; - }, - }; -}; + return abort(); +} + +/** @param {import('eslint').Rule.RuleContext} context */ +const create = context => ({ + [selector](node) { + const method = node.callee.property.name; + + return { + node, + messageId: method, + * fix(fixer, {abort}) { + yield fixer.replaceText(node.callee.property, 'slice'); + + if (node.arguments.length === 0) { + return; + } + + if ( + node.arguments.length > 2 + || node.arguments.some(node => node.type === 'SpreadElement') + ) { + return abort(); + } + + const fixArguments = method === 'substr' ? fixSubstrArguments : fixSubstringArguments; + yield * fixArguments({node, fixer, context, abort}); + }, + }; + }, +}); /** @type {import('eslint').Rule.RuleModule} */ module.exports = { diff --git a/rules/utils/rule.js b/rules/utils/rule.js index 146d4da92c..bdf47f938c 100644 --- a/rules/utils/rule.js +++ b/rules/utils/rule.js @@ -5,6 +5,34 @@ const getDocumentationUrl = require('./get-documentation-url.js'); const isIterable = object => typeof object[Symbol.iterator] === 'function'; +class FixAbortError extends Error {} +const fixOptions = { + abort() { + throw new FixAbortError('Fix aborted.'); + }, +}; + +function wrapFixFunction(fix) { + return fixer => { + const result = fix(fixer, fixOptions); + + if (result && isIterable(result)) { + try { + return [...result]; + } catch (error) { + if (error instanceof FixAbortError) { + return; + } + + /* istanbul ignore next: Safe */ + throw error; + } + } + + return result; + }; +} + function reportListenerProblems(listener, context) { // Listener arguments can be `codePath, node` or `node` return function (...listenerArguments) { @@ -18,11 +46,20 @@ function reportListenerProblems(listener, context) { problems = [problems]; } - // TODO: Allow `fix` function to abort for (const problem of problems) { - if (problem) { - context.report(problem); + if (problem.fix) { + problem.fix = wrapFixFunction(problem.fix); } + + if (Array.isArray(problem.suggest)) { + for (const suggest of problem.suggest) { + if (suggest.fix) { + suggest.fix = wrapFixFunction(suggest.fix); + } + } + } + + context.report(problem); } }; } diff --git a/test/prefer-string-slice.mjs b/test/prefer-string-slice.mjs index fa7c78bd71..4fd035da3b 100644 --- a/test/prefer-string-slice.mjs +++ b/test/prefer-string-slice.mjs @@ -321,3 +321,29 @@ test.typescript({ }, ], }); + +test.snapshot({ + valid: [], + invalid: [ + outdent` + /* 1 */ (( /* 2 */ 0 /* 3 */, /* 4 */ foo /* 5 */ )) /* 6 */ + . /* 7 */ substring /* 8 */ ( + /* 9 */ (( /* 10 */ bar /* 11 */ )) /* 12 */, + /* 13 */ (( /* 14 */ 0 /* 15 */ )) /* 16 */, + /* 17 */ + ) + /* 18 */ + `, + 'foo.substr(0, ...bar)', + 'foo.substr(...bar)', + 'foo.substr(0, (100, 1))', + 'foo.substr(0, 1, extraArgument)', + 'foo.substr((0, bar.length), (0, baz.length))', + // TODO: Fix this + // 'foo.substr(await 1, await 2)', + 'foo.substring((10, 1), 0)', + 'foo.substring(0, (10, 1))', + 'foo.substring(0, await 1)', + 'foo.substring((10, bar))', + ], +}); diff --git a/test/run-rules-on-codebase/lint.mjs b/test/run-rules-on-codebase/lint.mjs index 7b6044a92a..1d4031fb0f 100644 --- a/test/run-rules-on-codebase/lint.mjs +++ b/test/run-rules-on-codebase/lint.mjs @@ -32,6 +32,8 @@ const eslint = new ESLint({ ], // https://github.com/sindresorhus/eslint-plugin-unicorn/issues/1109#issuecomment-782689255 'unicorn/consistent-destructuring': 'off', + // Buggy + 'unicorn/custom-error-definition': 'off', 'unicorn/prefer-array-flat': [ 'error', { diff --git a/test/snapshots/prefer-string-slice.mjs.md b/test/snapshots/prefer-string-slice.mjs.md new file mode 100644 index 0000000000..e90a300376 --- /dev/null +++ b/test/snapshots/prefer-string-slice.mjs.md @@ -0,0 +1,170 @@ +# Snapshot report for `test/prefer-string-slice.mjs` + +The actual snapshot is saved in `prefer-string-slice.mjs.snap`. + +Generated by [AVA](https://avajs.dev). + +## Invalid #1 + 1 | /* 1 */ (( /* 2 */ 0 /* 3 */, /* 4 */ foo /* 5 */ )) /* 6 */ + 2 | . /* 7 */ substring /* 8 */ ( + 3 | /* 9 */ (( /* 10 */ bar /* 11 */ )) /* 12 */, + 4 | /* 13 */ (( /* 14 */ 0 /* 15 */ )) /* 16 */, + 5 | /* 17 */ + 6 | ) + 7 | /* 18 */ + +> Output + + `␊ + 1 | /* 1 */ (( /* 2 */ 0 /* 3 */, /* 4 */ foo /* 5 */ )) /* 6 */␊ + 2 | . /* 7 */ slice /* 8 */ (␊ + 3 | /* 9 */ 0 /* 12 */,␊ + 4 | /* 13 */ Math.max(0, (( /* 10 */ bar /* 11 */ ))) /* 16 */,␊ + 5 | /* 17 */␊ + 6 | )␊ + 7 | /* 18 */␊ + ` + +> Error 1/1 + + `␊ + > 1 | /* 1 */ (( /* 2 */ 0 /* 3 */, /* 4 */ foo /* 5 */ )) /* 6 */␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^␊ + > 2 | . /* 7 */ substring /* 8 */ (␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^␊ + > 3 | /* 9 */ (( /* 10 */ bar /* 11 */ )) /* 12 */,␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^␊ + > 4 | /* 13 */ (( /* 14 */ 0 /* 15 */ )) /* 16 */,␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^␊ + > 5 | /* 17 */␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^␊ + > 6 | )␊ + | ^^^ Prefer \`String#slice()\` over \`String#substring()\`.␊ + 7 | /* 18 */␊ + ` + +## Invalid #2 + 1 | foo.substr(0, ...bar) + +> Error 1/1 + + `␊ + > 1 | foo.substr(0, ...bar)␊ + | ^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substr()\`.␊ + ` + +## Invalid #3 + 1 | foo.substr(...bar) + +> Error 1/1 + + `␊ + > 1 | foo.substr(...bar)␊ + | ^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substr()\`.␊ + ` + +## Invalid #4 + 1 | foo.substr(0, (100, 1)) + +> Output + + `␊ + 1 | foo.slice(0, Math.max(0, (100, 1)))␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.substr(0, (100, 1))␊ + | ^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substr()\`.␊ + ` + +## Invalid #5 + 1 | foo.substr(0, 1, extraArgument) + +> Error 1/1 + + `␊ + > 1 | foo.substr(0, 1, extraArgument)␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substr()\`.␊ + ` + +## Invalid #6 + 1 | foo.substr((0, bar.length), (0, baz.length)) + +> Output + + `␊ + 1 | foo.slice((0, bar.length), (0, bar.length) + (0, baz.length))␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.substr((0, bar.length), (0, baz.length))␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substr()\`.␊ + ` + +## Invalid #7 + 1 | foo.substring((10, 1), 0) + +> Output + + `␊ + 1 | foo.slice(0, Math.max(0, (10, 1)))␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.substring((10, 1), 0)␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substring()\`.␊ + ` + +## Invalid #8 + 1 | foo.substring(0, (10, 1)) + +> Output + + `␊ + 1 | foo.slice(0, Math.max(0, (10, 1)))␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.substring(0, (10, 1))␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substring()\`.␊ + ` + +## Invalid #9 + 1 | foo.substring(0, await 1) + +> Output + + `␊ + 1 | foo.slice(0, Math.max(0, await 1))␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.substring(0, await 1)␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substring()\`.␊ + ` + +## Invalid #10 + 1 | foo.substring((10, bar)) + +> Output + + `␊ + 1 | foo.slice(Math.max(0, (10, bar)))␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.substring((10, bar))␊ + | ^^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#slice()\` over \`String#substring()\`.␊ + ` diff --git a/test/snapshots/prefer-string-slice.mjs.snap b/test/snapshots/prefer-string-slice.mjs.snap new file mode 100644 index 0000000000000000000000000000000000000000..fa4f35ba605025fc77db7e5d5c421ee831927f2e GIT binary patch literal 776 zcmV+j1NZzvRzV(1{fmf%E1wf<>PI zao~SLPtEoz=NRP@%F<&0O<@9yN;5<7Y!DU;EM-u9J-at${_EB$32aPY(HSfdyn~H_ zVO{YYp{P4|mPELonN{en+Fz(p;`# z2qb_GC(7ke#X1TGw0F5HhRZ?DO-d}%%Sp{kFUbJ90!=_c8!A!-6G8P9({u(5U*R$o z$p``-rO@w~9?!&dB+yBEdU`--;&lyW)@oveH%2l6IRHJO6yZ)NVC4jYkJ4nq^o{}Q zZhb8vrKPW+p#fqVftUs$mNAf}17etfgn-Tju}wj2O-&Hf49El;VFWaSQx7Cy4w3`A z1H`icn+a543{t=e6tRRGZ3wbO2PkI(lQRU_hMK>?&M`Cqi30-!%rS)64z}LV3~Gic z+zfMwi_Aa*pzt$?`VQn2RL=`CK|`NH&x2wJ21rThFaUG~dicR&3?&Q+XC$CWr~!>9 z{7|BRD5KHh32L?pB0`aZ1|Bg)nT!#8MCpP@F;q1