Skip to content

Commit

Permalink
prefer-string-replace-all: Report all String#replace() when the p…
Browse files Browse the repository at this point in the history 
…attern has `g` flag (#1965)

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
  • Loading branch information
@fisker @sindresorhus
fisker and sindresorhus committed Nov 17, 2022
1 parent 17556d7 commit 6316f05
Show file tree
Hide file tree
Showing 10 changed files with 447 additions and 114 deletions.
16 changes: 10 additions & 6 deletions docs/rules/prefer-string-replace-all.md
View file
Expand Up @@ -7,12 +7,16 @@
<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

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
Expand All @@ -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/, '');
```
1 change: 1 addition & 0 deletions rules/ast/index.js
View file
Expand Up @@ -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'),
};
22 changes: 22 additions & 0 deletions rules/ast/is-new-expression.js
View file
@@ -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;
6 changes: 2 additions & 4 deletions rules/no-unsafe-regex.js
View file
@@ -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 = {
Expand All @@ -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;
}

Expand Down
16 changes: 5 additions & 11 deletions rules/prefer-regexp-test.js
View file
Expand Up @@ -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');

Expand Down Expand Up @@ -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
Expand All @@ -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
);
};

Expand Down Expand Up @@ -118,7 +112,7 @@ const create = context => Object.fromEntries(

if (
isRegExpNode(regexpNode)
|| isRegExpWithoutGFlag(regexpNode, context.getScope())
|| isRegExpWithoutGlobalFlag(regexpNode, context.getScope())
) {
problem.fix = fixFunction;
} else {
Expand Down
6 changes: 2 additions & 4 deletions rules/prefer-set-size.js
View file
Expand Up @@ -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 = {
Expand All @@ -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)) {
Expand Down
67 changes: 51 additions & 16 deletions rules/prefer-string-replace-all.js
View file
@@ -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 = {
Expand All @@ -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;
Expand All @@ -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));
},
};
},
});
Expand Down
115 changes: 45 additions & 70 deletions test/prefer-string-replace-all.mjs
View file
@@ -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)',
Expand All @@ -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)',
],
});

0 comments on commit 6316f05

Please sign in to comment.