Accept targeted literal .replace() escapes in require-escaped-regexp-interpolation - #49674
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
.replace() escapes in require-escaped-regexp-interpolation
There was a problem hiding this comment.
Pull request overview
Broadens the ESLint rule to recognize targeted literal regex escaping.
Changes:
- Adds string and regex-literal escape detection.
- Adds valid and invalid regression tests.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts |
Implements targeted escape recognition. |
eslint-factory/src/rules/require-escaped-regexp-interpolation.test.ts |
Tests accepted and rejected replacements. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts:57
- The regex flags are ignored, so a sticky search such as
value.replace(/\./y, "\\.")is accepted. Forvalue === "a.b", that search fails at index 0 and returns the unescaped value, while the rule suppresses the diagnostic. Rejecty(or whitelist only flags that preserve the fixed-literal search behavior) and cover the sticky case with an invalid test.
if (node.type !== AST_NODE_TYPES.Literal || !("regex" in node) || !node.regex) return null;
return decodeFixedLiteralRegexPattern(node.regex.pattern);
eslint-factory/src/rules/require-escaped-regexp-interpolation.ts:82
- Comparing the literal replacement value does not account for
String.prototype.replacereplacement tokens. A search consisting of$followed by a backtick, paired with the corresponding backslash-prefixed replacement, passes these checks because$is a regex metacharacter; however, the$-backtick token expands to the text before the match rather than emitting the searched literal. The$'token analogously expands the suffix. Reject targeted searches that create these special replacement tokens, and add invalid regression cases.
function isLiteralRegexEscapeReplacement(search: string, replacement: string): boolean {
if (search.length === 0 || replacement !== `\\${search}`) return false;
if (!REGEXP_META_CHARS.has(search[0])) return false;
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| return replacement.type === AST_NODE_TYPES.Literal && typeof replacement.value === "string" && replacement.value === "\\$&"; | ||
| const search = getFixedLiteralSearchText(args[0]); | ||
| const replacement = getStringLiteralValue(args[1]); | ||
| if (replacement === "\\$&") return true; |
There was a problem hiding this comment.
Fixed in 82e1c94. The unconditional branch is replaced by isCanonicalMetacharEscapeRegex(args[0]), which requires the search to be exactly /[.*+?^${}()|[\]\\]/g with flags === "g". This rejects narrower patterns like /./ and sticky-flag variants. Three new invalid regression tests cover: non-canonical search, sticky flag, and $' replacement token.
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30727625239
|
…ent-token safety Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ent; guard against replacement tokens Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commit
Also corrected a pre-existing bug in the valid test cases where the canonical regex had too many backslashes and didn't match the actual AST pattern. |
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30731801766
|
Both reviewer concerns are fully resolved on the current HEAD (
The one remaining open review thread ( Local validation status: |
🔍 Triage Summary
Adjusts require-escaped-regexp-interpolation rule to accept literal .replace() escapes. CI green. Automated triage — see the triage report for full context.
|
require-escaped-regexp-interpolationonly recognized the canonical global metacharacter escape form (replace(/[...]/g, "\\$&")). That caused false positives for narrower but correct inline escapes such asreplace(".", "\\."), including the liveinterpolate_prompt.cjscall site.Rule behavior
isRegexEscapeReplaceCall()to accept two safe shapes:replace(/[.*+?^${}()|[\]\\]/g, "\\$&")replace("<literal>", "\\<literal>")and equivalent regex-literal search forms\+ the same literal.replace()calls that do not actually neutralize regex meaningCoverage
.replace(".", "\\.").replace(/\./g, "\\.").replace(".", ".")is still flaggedExample