Skip to content

[babel-plugin] (edge case) Insert constant values verbatim, not as replacement patterns - #1872

Open
henryqdineen wants to merge 1 commit into
facebook:mainfrom
henryqdineen:hqd-fix-const-dollar-replacement
Open

henryqdineen wants to merge 1 commit into
facebook:mainfrom
henryqdineen:hqd-fix-const-dollar-replacement

Conversation

@henryqdineen

Copy link
Copy Markdown
Collaborator

What changed / motivation ?

This came out of an agentic code review of #1700. The review flagged that constant values are substituted with a string replacement; checking the claim against main confirmed it's a pre-existing bug, that it affects a second call site as well, and that in one case it hangs the build. It's independent of #1700, so it's split out here.

This is an edge case: it only affects a defineConsts value that contains a $. Nothing else changes. But within that narrow case the output is silently wrong, and one shape hangs the build outright — so it seems worth fixing even though most codebases will never hit it.

The cause: values are substituted with a string replacement, so String.prototype.replace/replaceAll expand the $ sequences inside them. A constant's value is data, never a replacement pattern. Per MDN — specifying a string as the replacement, $& inserts the matched substring, $$ inserts a literal $, and $`/$' insert the portion before/after the match. Two call sites are affected — the alias pre-collapse in resolveConstant, and the per-rule substitution — and both switch to function replacers, which are not subject to that expansion. Two lines, no other behavior change.

The plausible case: $$ in a content string. A price-range indicator, the familiar $ / $$ / $$$ scale:

// tokens.stylex.js
export const price = stylex.defineConsts({
  cheap: '"$"',
  moderate: '"$$"',
  expensive: '"$$$"',
});

// App.js
export const styles = stylex.create({
  cheap: { content: price.cheap },
  moderate: { content: price.moderate },
  expensive: { content: price.expensive },
});

Before, every tier past the first loses exactly one $, because $$ is the escape for a literal $:

.x1l6mih1{content:"$"}    /* cheap     — correct  */
.xgdktg8{content:"$"}     /* moderate  — lost a $ */
.xxyzbk8{content:"$$"}    /* expensive — lost a $ */

After:

.x1l6mih1{content:"$"}
.xgdktg8{content:"$$"}
.xxyzbk8{content:"$$$"}

So the UI renders $, $, $$ — "cheap" and "moderate" become visually identical and the whole scale shifts down one, with no error or warning. Any doubled dollar hits this: currency labels, content strings, tokens exported from a spreadsheet or design tool.

The contrived case: $& hangs the build. Replacing var(--b) with $& re-inserts var(--b) over itself, so the pre-collapse loop makes no progress and — having already cleared visited and reset lastIndex — rescans from the start forever. The circular reference detected guard can't catch it: it detects a cycle within one recursive descent, not a substitution that fails to advance.

These definitions alone hang the build, because the pre-collapse walks every entry in constsMap whether or not anything references it. No usage of prefix is required:

export const label = stylex.defineConsts({
  '--currency': '"$&"',
  prefix: 'var(--currency)',
});

After this change that compiles to .x11xwbpo{content:"$&"}. The alias chain needs a verbatim -- key (#1460) for one const to name another; a plain $& value doesn't hang, but is still silently left unsubstituted.

Linked PR/Issues

No tracking issue. Relates to #1460 (verbatim -- const keys, which make const→const aliasing possible) and #1700, where this was found — that PR rewrites the per-rule substitution loop and already uses function replacers there, so it independently fixes one of these two sites. Whichever lands second has a one-hunk conflict on the replaceAll line, resolved by taking the newer side. The resolveConstant fix doesn't conflict.

Additional Context

Verified against unfixed main (ee1d8a91): each of the three added tests fails, and the $& chain case hangs (killed by a 30s watchdog; the suite otherwise finishes in ~0.7s). The price-tier CSS above is real output from the transform on both sides.

Note the $& regression test hangs rather than fails if the fix regresses, since it's a synchronous loop Jest can't interrupt. The other two fail cleanly.

Tests: three cases in transform-process-test.js — a $& value reached through an alias chain (the hang), a $& value with no chain (silently unsubstituted by the per-rule site), and $$/$`/$' values preserved verbatim.

Pre-flight checklist

  • I have read the contributing guidelines
  • Performed a self-review of my code

🤖 Generated with Claude Code

@vercel

vercel Bot commented Sep 10, 2026

Copy link
Copy Markdown

@henryqdineen is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 10, 2026
…tterns

A `defineConsts` value was substituted with a string replacement, so
`String.prototype.replace`/`replaceAll` expanded the `$` sequences in it:
`$&` re-inserted the matched text, `$$` collapsed to a single `$`, and
`` $` ``/`$'` inserted the text surrounding the match. Values are data,
never replacement patterns.

`$&` was the worst case. In the alias pre-collapse, replacing `var(--b)`
with `$&` re-inserted `var(--b)` over itself, so the loop made no progress
and -- having already cleared `visited` and reset `lastIndex` -- rescanned
from the start forever. That hangs the build with no error; the circular
reference guard can't catch it, because it detects a cycle within one
recursive descent, not a substitution that fails to advance.

Two sites needed it: the alias pre-collapse in resolveConstant, and the
per-rule substitution. Switching both to function replacers, which are
immune to `$` expansion, fixes them without changing anything else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@henryqdineen
henryqdineen force-pushed the hqd-fix-const-dollar-replacement branch from 8c1eb0e to d2e3cd2 Compare September 10, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant