Skip to content

fix(string): do not double-escape genString single-quoted output - #152

Open
MFA-G wants to merge 1 commit into
unjs:mainfrom
MFA-G:fix/gen-string-single-quotes
Open

MFA-G wants to merge 1 commit into
unjs:mainfrom
MFA-G:fix/gen-string-single-quotes

Conversation

@MFA-G

@MFA-G MFA-G commented Aug 31, 2026

Copy link
Copy Markdown

Problem

genString(input, { singleQuotes: true }) escapes its input twice:

const str = JSON.stringify(input);          // escapes once
return `'${escapeString(str).slice(1, -1)}'`; // escapes the escapes

The generated literal no longer evaluates back to the input it was generated from:

input singleQuotes: true output evaluates to
a\nb (newline) 'a\\nb' a\nb — literal \ + n
a\\b (backslash) 'a\\\\b' a\\b — two backslashes
a\tb (tab) 'a\\tb' a\tb — literal \ + t
a b (space) 'a\ b' abthe space is gone

The last row is the worst case. escapeString escapes characters that carry no meaning behind a backslash, and in a JS string literal \<char> for an unknown <char> is just <char> — so \ collapses to a space in some positions and the character disappears in others. A codegen helper silently emitting a different string than it was asked to is a hard bug to trace back to its source.

Why the two passes cannot be composed

After JSON.stringify, a backslash in the buffer is no longer a literal backslash — it is the opening character of an escape sequence. escapeString cannot tell the two apart, so it re-escapes payloads that were already correct.

Instead of escaping twice, the fix converts the JSON body into a single-quoted body. Only two things differ between the two literal forms:

  • \" no longer needs escaping → emitted as "
  • ' now needs escaping → emitted as \'

Everything else (\\, \n, \uXXXX, …) is already a valid escape sequence in a single-quoted literal and is copied verbatim. The scan steps over escape sequences so their payload is never re-examined.

escapeString itself is unchanged and remains exported.

Also fixed: U+2028 / U+2029

JSON.stringify leaves U+2028 and U+2029 raw. They are legal in ES2019+ string literals but not in JSON, so the double-quoted output was not safe to embed in a JSON document. The old single-quoted path escaped them via escapeString; both paths now do.

Tests

The existing fixtures in test/string.test.ts encoded the doubled output ([`foo\nbar`, `"foo\\nbar"`, `'foo\\\\nbar'`]) and are corrected to the values that actually round-trip. Added fixtures for a lone backslash, a tab, a backslash followed by a quote, and both line separators.

Added a third suite asserting the property the bug violated — that every generated literal evaluates back to the string it was generated from — across both quote styles.

I also fuzzed the change locally (not committed) over 400k random strings drawn from a ' " \ \n \r \t \u2028 \u2029 \0 \b \u0001 é 😀, evaluating each literal and comparing to the input:

  • before: 186,654 / 400,000 failed
  • after: 0 / 400,000 failed

Validation

pnpm test   # eslint + prettier + vitest --coverage
  • 78 tests passing across 4 files (was 72)
  • lint clean, prettier clean

Summary by CodeRabbit

  • Bug Fixes

    • Improved generated string literals containing newlines, tabs, quotes, backslashes, and special Unicode line separators.
    • Fixed single-quoted output so embedded double quotes remain readable and strings can be safely embedded in source code.
    • Improved round-trip accuracy when generated strings are evaluated.
  • Tests

    • Added coverage for special characters and verified generated strings preserve their original values.

`genString(input, { singleQuotes: true })` ran `escapeString` over the
result of `JSON.stringify`, so every escape the JSON pass had already
produced was escaped a second time. The generated literal no longer
evaluates back to the input:

    genString("a\nb", { singleQuotes: true })
    // 'a\\nb'  -> evaluates to  "a\nb" (literal backslash + n)

The same applies to `\\`, `\t`, `\u0000` and any other escape. It is
worse than a wrong string: `escapeString` also escapes characters that
carry no meaning behind a backslash, so a plain space becomes `\ `,
which evaluates to nothing at all and silently drops the character.

The two passes cannot be composed, because after `JSON.stringify` a
backslash is no longer a literal backslash — it is the start of an
escape sequence. Instead of escaping twice, convert the JSON body to a
single-quoted body: `\"` no longer needs escaping, `'` now does, and
every other escape sequence is already valid and is copied verbatim.

While here, escape U+2028/U+2029 on both paths. `JSON.stringify` leaves
them raw, so the double-quoted output was not safe to embed in JSON;
the single-quoted path escaped them already.

Existing expectations in `test/string.test.ts` encoded the doubled
output (`'foo\\\\nbar'`) and are corrected. Added cases for backslash,
tab, a backslash followed by a quote, and the line separators, plus a
round-trip test asserting each generated literal evaluates back to the
input it was generated from — the property that was broken.
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ac595094-b27f-4f38-bd26-e529be8fc735

📥 Commits

Reviewing files that changed from the base of the PR and between a95f4d4 and 92e01fe.

📒 Files selected for processing (2)
  • src/string.ts
  • test/string.test.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

genString now escapes U+2028 and U+2029 characters and converts JSON string bodies to single-quoted literals without double-escaping. Tests cover additional escape sequences and evaluate generated literals against their original inputs.

Changes

String literal escaping

Layer / File(s) Summary
Update string escaping and validation
src/string.ts, test/string.test.ts
genString escapes line separators and uses singleQuoteBody for single-quoted output. Tests cover backslashes, tabs, quotes, line separators, and evaluation round trips.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 92e01

This localized change corrects generated string escaping and adds round-trip coverage; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing double-escaping in single-quoted genString output.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant