diff --git a/docs/rules/prefer-string-replace-all.md b/docs/rules/prefer-string-replace-all.md index aeecf93af4..63f6580ffb 100644 --- a/docs/rules/prefer-string-replace-all.md +++ b/docs/rules/prefer-string-replace-all.md @@ -7,12 +7,16 @@ -The [`String#replaceAll()`](https://github.com/tc39/proposal-string-replaceall) method is both faster and safer as you don't have to escape the regex if the string is not a literal. +The [`String#replaceAll()`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll) method is both faster and safer as you don't have to use a regex and remember to escape it if the string is not a literal. And when used with a regex, it makes the intent clearer. ## Fail ```js -string.replace(/This has no special regex symbols/g, ''); +string.replace(/RegExp with global flag/igu, ''); +``` + +```js +string.replace(/RegExp without special symbols/g, ''); ``` ```js @@ -26,17 +30,17 @@ string.replace(/Works for u flag too/gu, ''); ## Pass ```js -string.replace(/Non-literal characters .*/g, ''); +string.replace(/Non-global regexp/iu, ''); ``` ```js -string.replace(/Extra flags/gi, ''); +string.replace('Not a regex expression', '') ``` ```js -string.replace('Not a regex expression', '') +string.replaceAll('string', ''); ``` ```js -string.replaceAll('Literal characters only', ''); +string.replaceAll(/\s/, ''); ``` diff --git a/rules/ast/index.js b/rules/ast/index.js index 0f82fd5252..d1184080dd 100644 --- a/rules/ast/index.js +++ b/rules/ast/index.js @@ -21,4 +21,5 @@ module.exports = { isEmptyNode: require('./is-empty-node.js'), isStaticRequire: require('./is-static-require.js'), isUndefined: require('./is-undefined.js'), + isNewExpression: require('./is-new-expression.js'), }; diff --git a/rules/ast/is-new-expression.js b/rules/ast/is-new-expression.js new file mode 100644 index 0000000000..965a83cb2c --- /dev/null +++ b/rules/ast/is-new-expression.js @@ -0,0 +1,22 @@ +'use strict'; + +function isNewExpression(node, options) { + if (node?.type !== 'NewExpression') { + return false; + } + + const { + name, + } = { + ...options, + }; + + if (name) { + return node.callee.type === 'Identifier' && node.callee.name === name; + } + + /* c8 ignore next */ + return true; +} + +module.exports = isNewExpression; diff --git a/rules/no-unsafe-regex.js b/rules/no-unsafe-regex.js index 143fd982c9..6d69d8154b 100644 --- a/rules/no-unsafe-regex.js +++ b/rules/no-unsafe-regex.js @@ -1,6 +1,7 @@ 'use strict'; const safeRegex = require('safe-regex'); const {newExpressionSelector} = require('./selectors/index.js'); +const {isNewExpression} = require('./ast/index.js'); const MESSAGE_ID = 'no-unsafe-regex'; const messages = { @@ -16,10 +17,7 @@ const newRegExpSelector = [ const create = () => ({ 'Literal[regex]'(node) { // Handle regex literal inside RegExp constructor in the other handler - if ( - node.parent.type === 'NewExpression' - && node.parent.callee.name === 'RegExp' - ) { + if (isNewExpression(node.parent, {name: 'RegExp'})) { return; } diff --git a/rules/prefer-regexp-test.js b/rules/prefer-regexp-test.js index 5a2e1ebd00..ca5983e7aa 100644 --- a/rules/prefer-regexp-test.js +++ b/rules/prefer-regexp-test.js @@ -2,7 +2,7 @@ const {isParenthesized, getStaticValue} = require('eslint-utils'); const {checkVueTemplate} = require('./utils/rule.js'); const {methodCallSelector} = require('./selectors/index.js'); -const {isRegexLiteral} = require('./ast/index.js'); +const {isRegexLiteral, isNewExpression} = require('./ast/index.js'); const {isBooleanNode} = require('./utils/boolean.js'); const shouldAddParenthesesToMemberExpressionObject = require('./utils/should-add-parentheses-to-member-expression-object.js'); @@ -69,15 +69,9 @@ const cases = [ }, ]; -const isRegExpNode = node => - isRegexLiteral(node) - || ( - node.type === 'NewExpression' - && node.callee.type === 'Identifier' - && node.callee.name === 'RegExp' - ); +const isRegExpNode = node => isRegexLiteral(node) || isNewExpression(node, {name: 'RegExp'}); -const isRegExpWithoutGFlag = (node, scope) => { +const isRegExpWithoutGlobalFlag = (node, scope) => { const staticResult = getStaticValue(node, scope); // Don't know if there is `g` flag @@ -88,7 +82,7 @@ const isRegExpWithoutGFlag = (node, scope) => { const {value} = staticResult; return ( Object.prototype.toString.call(value) === '[object RegExp]' - && !value.flags.includes('g') + && !value.global ); }; @@ -118,7 +112,7 @@ const create = context => Object.fromEntries( if ( isRegExpNode(regexpNode) - || isRegExpWithoutGFlag(regexpNode, context.getScope()) + || isRegExpWithoutGlobalFlag(regexpNode, context.getScope()) ) { problem.fix = fixFunction; } else { diff --git a/rules/prefer-set-size.js b/rules/prefer-set-size.js index c7b7eaf6ec..53d6ab1fcd 100644 --- a/rules/prefer-set-size.js +++ b/rules/prefer-set-size.js @@ -2,6 +2,7 @@ const {findVariable} = require('eslint-utils'); const {memberExpressionSelector} = require('./selectors/index.js'); const {fixSpaceAroundKeyword} = require('./fix/index.js'); +const {isNewExpression} = require('./ast/index.js'); const MESSAGE_ID = 'prefer-set-size'; const messages = { @@ -15,10 +16,7 @@ const lengthAccessSelector = [ '[object.elements.0.type="SpreadElement"]', ].join(''); -const isNewSet = node => - node?.type === 'NewExpression' - && node.callee.type === 'Identifier' - && node.callee.name === 'Set'; +const isNewSet = node => isNewExpression(node, {name: 'Set'}); function isSet(node, scope) { if (isNewSet(node)) { diff --git a/rules/prefer-string-replace-all.js b/rules/prefer-string-replace-all.js index 5f0e8e032d..04e18a5054 100644 --- a/rules/prefer-string-replace-all.js +++ b/rules/prefer-string-replace-all.js @@ -1,7 +1,8 @@ 'use strict'; +const {getStaticValue} = require('eslint-utils'); const quoteString = require('./utils/quote-string.js'); const {methodCallSelector} = require('./selectors/index.js'); -const {isRegexLiteral} = require('./ast/index.js'); +const {isRegexLiteral, isNewExpression} = require('./ast/index.js'); const MESSAGE_ID = 'prefer-string-replace-all'; const messages = { @@ -13,14 +14,38 @@ const selector = methodCallSelector({ argumentsLength: 2, }); -const isRegexWithGlobalFlag = node => +const canReplacePatternWithString = node => isRegexLiteral(node) - && node.regex.flags.replace('u', '') === 'g'; + && node.regex.flags.replace('u', '') === 'g' + && !/[$()*+.?[\\\]^{|}]/.test(node.regex.pattern.replace(/\\[$()*+.?[\\\]^{|}]/g, '')); -function isLiteralCharactersOnly(node) { - const searchPattern = node.regex.pattern; - return !/[$()*+.?[\\\]^{|}]/.test(searchPattern.replace(/\\[$()*+.?[\\\]^{|}]/g, '')); -} +const isRegExpWithGlobalFlag = (node, scope) => { + if (isRegexLiteral(node)) { + return node.regex.flags.includes('g'); + } + + if ( + isNewExpression(node, {name: 'RegExp'}) + && node.arguments[0]?.type !== 'SpreadElement' + && node.arguments[1]?.type === 'Literal' + && typeof node.arguments[1].value === 'string' + ) { + return node.arguments[1].value.includes('g'); + } + + const staticResult = getStaticValue(node, scope); + + // Don't know if there is `g` flag + if (!staticResult) { + return false; + } + + const {value} = staticResult; + return ( + Object.prototype.toString.call(value) === '[object RegExp]' + && value.global + ); +}; function removeEscapeCharacters(regexString) { let fixedString = regexString; @@ -38,22 +63,32 @@ function removeEscapeCharacters(regexString) { } /** @param {import('eslint').Rule.RuleContext} context */ -const create = () => ({ +const create = context => ({ [selector](node) { - const {arguments: arguments_, callee} = node; - const [search] = arguments_; + const { + arguments: [pattern], + callee: {property}, + } = node; - if (!isRegexWithGlobalFlag(search) || !isLiteralCharactersOnly(search)) { + if (!isRegExpWithGlobalFlag(pattern, context.getScope())) { return; } return { - node, + node: property, messageId: MESSAGE_ID, - fix: fixer => [ - fixer.insertTextAfter(callee, 'All'), - fixer.replaceText(search, quoteString(removeEscapeCharacters(search.regex.pattern))), - ], + /** @param {import('eslint').Rule.RuleFixer} fixer */ + * fix(fixer) { + yield fixer.insertTextAfter(property, 'All'); + + if (!canReplacePatternWithString(pattern)) { + return; + } + + const string = removeEscapeCharacters(pattern.regex.pattern); + + yield fixer.replaceText(pattern, quoteString(string)); + }, }; }, }); diff --git a/test/prefer-string-replace-all.mjs b/test/prefer-string-replace-all.mjs index 2bfc78e19d..b0a0ffa423 100644 --- a/test/prefer-string-replace-all.mjs +++ b/test/prefer-string-replace-all.mjs @@ -1,29 +1,14 @@ +import outdent from 'outdent'; import {getTester} from './utils/test.mjs'; const {test} = getTester(import.meta); -const error = { - message: 'Prefer `String#replaceAll()` over `String#replace()`.', -}; - -test({ +test.snapshot({ valid: [ // No global flag 'foo.replace(/a/, bar)', - // Special characters - 'foo.replace(/[a]/g, bar)', - 'foo.replace(/a?/g, bar)', - 'foo.replace(/.*/g, bar)', - 'foo.replace(/a|b/g, bar)', - 'foo.replace(/\\W/g, bar)', - 'foo.replace(/\\u{61}/g, bar)', - 'foo.replace(/\\u{61}/gu, bar)', - // Extra flag - 'foo.replace(/a/gi, bar)', - 'foo.replace(/a/gui, bar)', - 'foo.replace(/a/uig, bar)', // Not regex literal - 'foo.replace(\'string\', bar)', + 'foo.replace("string", bar)', // Not 2 arguments 'foo.replace(/a/g)', 'foo.replace(/\\\\./g)', @@ -41,64 +26,54 @@ test({ 'foo.replace(/a/g, bar, extra);', 'foo.replace();', 'foo.replace(...argumentsArray, ...argumentsArray2)', + // Unknown/non-regexp/non-global value + 'foo.replace(unknown, bar)', + 'const pattern = new RegExp("foo", unknown); foo.replace(pattern, bar)', + 'const pattern = new RegExp("foo"); foo.replace(pattern, bar)', + 'const pattern = new RegExp(); foo.replace(pattern, bar)', + 'const pattern = "string"; foo.replace(pattern, bar)', + 'const pattern = new RegExp("foo", "g"); foo.replace(...[pattern], bar)', + 'const pattern = "not-a-regexp"; foo.replace(pattern, bar)', + 'const pattern = new RegExp("foo", "i"); foo.replace(pattern, bar)', + 'foo.replace(new NotRegExp("foo", "g"), bar)', + // We are not checking this + 'foo.replaceAll(/a/g, bar)', ], invalid: [ - { - code: 'foo.replace(/a/g, bar)', - output: 'foo.replaceAll(\'a\', bar)', - errors: [error], - }, + 'foo.replace(/a/g, bar)', // Comments - { - code: ` - foo/* comment 1 */ - .replace/* comment 2 */( - /* comment 3 */ - /a/g // comment 4 - , - bar - ) - `, - output: ` - foo/* comment 1 */ - .replaceAll/* comment 2 */( - /* comment 3 */ - 'a' // comment 4 - , - bar - ) - `, - errors: [error], - }, + outdent` + foo/* comment 1 */ + .replace/* comment 2 */( + /* comment 3 */ + /a/g // comment 4 + , + bar + ) + `, // Quotes - { - code: 'foo.replace(/"\'/g, \'\\\'\')', - output: 'foo.replaceAll(\'"\\\'\', \'\\\'\')', - errors: [error], - }, + 'foo.replace(/"\'/g, \'\\\'\')', // Escaped symbols - { - code: 'foo.replace(/\\./g, bar)', - output: 'foo.replaceAll(\'.\', bar)', - errors: [error], - }, - { - code: 'foo.replace(/\\\\\\./g, bar)', - output: 'foo.replaceAll(\'\\\\.\', bar)', - errors: [error], - }, - { - code: 'foo.replace(/\\|/g, bar)', - output: 'foo.replaceAll(\'|\', bar)', - errors: [error], - }, - ], -}); - -test.snapshot({ - valid: [], - invalid: [ + 'foo.replace(/\\./g, bar)', + 'foo.replace(/\\\\\\./g, bar)', + 'foo.replace(/\\|/g, bar)', + // `u` flag 'foo.replace(/a/gu, bar)', 'foo.replace(/a/ug, bar)', + // Special characters + 'foo.replace(/[a]/g, bar)', + 'foo.replace(/a?/g, bar)', + 'foo.replace(/.*/g, bar)', + 'foo.replace(/a|b/g, bar)', + 'foo.replace(/\\W/g, bar)', + 'foo.replace(/\\u{61}/g, bar)', + 'foo.replace(/\\u{61}/gu, bar)', + // Extra flag + 'foo.replace(/a/gi, bar)', + 'foo.replace(/a/gui, bar)', + 'foo.replace(/a/uig, bar)', + // Variables + 'const pattern = new RegExp("foo", "g"); foo.replace(pattern, bar)', + 'foo.replace(new RegExp("foo", "g"), bar)', ], }); diff --git a/test/snapshots/prefer-string-replace-all.mjs.md b/test/snapshots/prefer-string-replace-all.mjs.md index 0f3ae82acc..32a94728f2 100644 --- a/test/snapshots/prefer-string-replace-all.mjs.md +++ b/test/snapshots/prefer-string-replace-all.mjs.md @@ -5,6 +5,120 @@ The actual snapshot is saved in `prefer-string-replace-all.mjs.snap`. Generated by [AVA](https://avajs.dev). ## Invalid #1 + 1 | foo.replace(/a/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll('a', bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/a/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #2 + 1 | foo/* comment 1 */ + 2 | .replace/* comment 2 */( + 3 | /* comment 3 */ + 4 | /a/g // comment 4 + 5 | , + 6 | bar + 7 | ) + +> Output + + `␊ + 1 | foo/* comment 1 */␊ + 2 | .replaceAll/* comment 2 */(␊ + 3 | /* comment 3 */␊ + 4 | 'a' // comment 4␊ + 5 | ,␊ + 6 | bar␊ + 7 | )␊ + ` + +> Error 1/1 + + `␊ + 1 | foo/* comment 1 */␊ + > 2 | .replace/* comment 2 */(␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + 3 | /* comment 3 */␊ + 4 | /a/g // comment 4␊ + 5 | ,␊ + 6 | bar␊ + 7 | )␊ + ` + +## Invalid #3 + 1 | foo.replace(/"'/g, '\'') + +> Output + + `␊ + 1 | foo.replaceAll('"\\'', '\\'')␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/"'/g, '\\'')␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #4 + 1 | foo.replace(/\./g, bar) + +> Output + + `␊ + 1 | foo.replaceAll('.', bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/\\./g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #5 + 1 | foo.replace(/\\\./g, bar) + +> Output + + `␊ + 1 | foo.replaceAll('\\\\.', bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/\\\\\\./g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #6 + 1 | foo.replace(/\|/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll('|', bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/\\|/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #7 1 | foo.replace(/a/gu, bar) > Output @@ -17,10 +131,10 @@ Generated by [AVA](https://avajs.dev). `␊ > 1 | foo.replace(/a/gu, bar)␊ - | ^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ ` -## Invalid #2 +## Invalid #8 1 | foo.replace(/a/ug, bar) > Output @@ -33,5 +147,197 @@ Generated by [AVA](https://avajs.dev). `␊ > 1 | foo.replace(/a/ug, bar)␊ - | ^^^^^^^^^^^^^^^^^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #9 + 1 | foo.replace(/[a]/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/[a]/g, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/[a]/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #10 + 1 | foo.replace(/a?/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/a?/g, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/a?/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #11 + 1 | foo.replace(/.*/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/.*/g, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/.*/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #12 + 1 | foo.replace(/a|b/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/a|b/g, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/a|b/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #13 + 1 | foo.replace(/\W/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/\\W/g, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/\\W/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #14 + 1 | foo.replace(/\u{61}/g, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/\\u{61}/g, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/\\u{61}/g, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #15 + 1 | foo.replace(/\u{61}/gu, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/\\u{61}/gu, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/\\u{61}/gu, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #16 + 1 | foo.replace(/a/gi, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/a/gi, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/a/gi, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #17 + 1 | foo.replace(/a/gui, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/a/gui, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/a/gui, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #18 + 1 | foo.replace(/a/uig, bar) + +> Output + + `␊ + 1 | foo.replaceAll(/a/uig, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(/a/uig, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #19 + 1 | const pattern = new RegExp("foo", "g"); foo.replace(pattern, bar) + +> Output + + `␊ + 1 | const pattern = new RegExp("foo", "g"); foo.replaceAll(pattern, bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | const pattern = new RegExp("foo", "g"); foo.replace(pattern, bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ + ` + +## Invalid #20 + 1 | foo.replace(new RegExp("foo", "g"), bar) + +> Output + + `␊ + 1 | foo.replaceAll(new RegExp("foo", "g"), bar)␊ + ` + +> Error 1/1 + + `␊ + > 1 | foo.replace(new RegExp("foo", "g"), bar)␊ + | ^^^^^^^ Prefer \`String#replaceAll()\` over \`String#replace()\`.␊ ` diff --git a/test/snapshots/prefer-string-replace-all.mjs.snap b/test/snapshots/prefer-string-replace-all.mjs.snap index f215646b80..903934852e 100644 Binary files a/test/snapshots/prefer-string-replace-all.mjs.snap and b/test/snapshots/prefer-string-replace-all.mjs.snap differ