[rust-compiler] Bump round_trip test thread stack to 32 MiB in CI#36512
Merged
Conversation
github-actions
Bot
added
the
React Core Team
…ntend
After the cluster-1 BytePos shift, `ConvertCtx::position()` emitted
`loc.column` and `loc.index` as 0-based UTF-8 byte offsets. Babel emits
them as 0-based UTF-16 code unit offsets (matching JS string indexing).
For files containing any character above U+FFFF (e.g. an emoji like
🔴 U+1F534), the two diverge by +2 per such character because the
char is 4 bytes in UTF-8 but 2 code units in UTF-16.
Precompute a `utf16_offsets: Vec<u32>` table in `ConvertCtx::new`
that maps each source byte index to its 0-based UTF-16 code unit
offset. `position()` then looks up `index` directly and computes
`column` as `index - utf16_index_of_line_start`. O(1) per call; the
table costs ~4× the source length in memory, which is bounded for
fixture/file inputs.
Considered an alternative that walks the source line on each
`position()` call to count UTF-16 code units. More memory-frugal but
O(line length) per call. The precomputed table wins on O(1) lookup
and the per-call cost matters because `position()` is invoked on
every node, comment, and reference in the converter.
Clamp the byte index in `position()` to the sentinel at
`utf16_offsets.len() - 1`. Synthetic spans (e.g. compiler-generated
imports given `BytePos(1)`) can point past EOF in degenerate cases;
clamping avoids a panic.
Line numbers stay 1-based and the binary-search remains keyed on
byte offsets, since the underlying `line_offsets` table is byte-based.
Fixes 4 e2e parity fixtures (3 targeted + 1 latent):
- effect-derived-computations/invalid-derived-computation-in-effect.js
- error.invalid-derived-computation-in-effect.js
- fbt/error.todo-multiple-fbt-plural.tsx
- (one additional latent fixture passes for free)
Test plan:
- bash compiler/scripts/test-e2e.sh --variant swc:
Before: Total 1770/1795
After: Total 1774/1795 (4 fixed)
- bash compiler/scripts/test-e2e.sh --variant babel: 1788/1795 (unchanged)
- bash compiler/scripts/test-e2e.sh --variant oxc: 1702/1795 (unchanged)
- cargo test --workspace: 56 passed, 0 failed
…e info
`convert_scope.rs::visit_fn_decl` allocated a fresh `BindingId` for
every `function x()` declaration. Babel and OXC collapse same-named
function declarations in the same hoist scope into one binding, with
the second declaration registered as a `constantViolations` reference
(reassignment) rather than a new binding.
For `function-declaration-redeclare.js` the SWC variant emitted
`const x_0 = t0; return x_0;` because the compiler saw two distinct
bindings and renamed one. Babel's output is `let x; ... x = t0;
return x;` because there is one binding that gets reassigned.
In `visit_fn_decl`, check whether the hoist scope already has a
binding for the name. If yes, record the redeclaration's ident
position in a new `redeclaration_refs` map and skip adding a fresh
binding. `build_scope_info` overlays this map onto
`reference_to_binding` so the second function's ident resolves to the
first binding's `BindingId`.
Fixes 1 e2e parity fixture:
- function-declaration-redeclare.js
`valid-setState-in-effect-from-ref-function-call.js` and its sibling
`valid-setState-in-useEffect-controlled-by-ref-value.js` still fail.
Those have a distinct root cause: the SWC frontend discards the
compiler's `renames` output (`lib.rs:91-98`) instead of applying it
to the emitted SWC AST the way the babel adapter does via
`applyRenames`. That fix is its own commit.
Test plan:
- bash compiler/scripts/test-e2e.sh --variant swc:
Before: Total 1774/1795
After: Total 1775/1795 (1 fixed)
- bash compiler/scripts/test-e2e.sh --variant babel: 1788/1795 (unchanged)
- bash compiler/scripts/test-e2e.sh --variant oxc: 1702/1795 (unchanged)
- cargo test --workspace: 56 passed, 0 failed
The React Compiler's `RenameVariables` pass records identifier renames
in `CompileResult::Success.renames` as `Vec<BindingRenameInfo>` with
`{original, renamed, declaration_start}`. It does not rewrite the AST
itself; the frontend has to apply them. The Babel adapter does this
via `applyRenames` + `scope.rename()` in `BabelPlugin.ts:206-234`.
The SWC frontend was discarding the field via `..` in the
`CompileResult::Success` destructure, so inner-shadowing renames like
`ref → ref_0` never made it into the emitted output.
Add `apply_renames.rs` with a single pass that mirrors the Babel
semantics:
1. Re-run `build_scope_info` on the post-compile SWC module to get a
binding table and `reference_to_binding` keyed by source position.
2. Match `BindingRenameInfo` entries to bindings by `declaration_start`
and `name`, producing the set of `BindingId`s to rename.
3. Walk `reference_to_binding` to collect every position (declaration
plus references) belonging to a renamed binding.
4. A `VisitMut` rewrites `Ident.sym` at matching `span.lo.0`, expands
`Prop::Shorthand` and `ObjectPatProp::Assign` (with or without a
default) into key-value form so `{ref}` becomes `{ref: ref_0}`
instead of `{ref_0}`, and skips `MemberProp::Ident` so `x.ref`
stays `x.ref`.
Wire into `lib.rs::transform`: destructure `renames` from the
`CompileResult::Success` arm, and if `program_json` is `None` but
`renames` is non-empty (the `@outputMode:"lint"` path) clone the
original input module so we still have something to rewrite.
Fixes 2 e2e parity fixtures:
- valid-setState-in-effect-from-ref-function-call.js (`ref → ref_0`)
- valid-setState-in-useEffect-controlled-by-ref-value.js (`data → data_0`)
Test plan:
- bash compiler/scripts/test-e2e.sh --variant swc:
Before: Total 1775/1795
After: Total 1777/1795 (2 fixed)
- bash compiler/scripts/test-e2e.sh --variant babel: 1788/1795 (unchanged)
- bash compiler/scripts/test-e2e.sh --variant oxc: 1702/1795 (unchanged)
- cargo test --workspace: 56 passed, 0 failed
The e2e CLI (`react_compiler_e2e_cli`) accepted `--filename <path>`
and used it only for syntax detection. The value was never copied
into `PluginOptions.filename`, so the compiler's instrumentation
pass (`enableEmitInstrumentForget`) received `None` and emitted
`useRenderCounter("Bar", "")` with an empty path string where the TS
baseline emits the absolute file path.
Set `options.filename = Some(filename.to_string())` at the top of
both `compile_swc` and `compile_oxc`. One line each.
Fixes 5 e2e parity fixtures:
- codegen-instrument-forget-test.js (swc)
- conflict-codegen-instrument-forget.js (swc)
- gating/codegen-instrument-forget-gating-test.js (swc)
- (and 2 corresponding OXC variants of the same fixtures, fixed for free)
Test plan:
- bash compiler/scripts/test-e2e.sh --variant swc:
Before: Total 1777/1795
After: Total 1780/1795 (3 fixed)
- bash compiler/scripts/test-e2e.sh --variant babel: 1788/1795 (unchanged)
- bash compiler/scripts/test-e2e.sh --variant oxc:
Before: Total 1702/1795
After: Total 1704/1795 (2 fixed)
- cargo test --workspace: 56 passed, 0 failed
Snapshot of remaining e2e failures across all three frontends: - SWC: 1780/1795 (15 failures), grouped by fix path — fixture maintenance for TS-bug-Rust-correct cases (6), external PR dependency (1), and real SWC frontend bugs (8). Each line names the fixture, the failure shape, and where to look. - Babel: 1788/1795 (7 failures) — placeholder; needs triage. - OXC: 1704/1795 (91 failures) — placeholder; needs triage. Companion artifact to the current 9-commit SWC-correctness stack.
`scripts/test-babel-ast.sh` runs `cargo test -p react_compiler_ast
--test round_trip`, which serde-walks the parsed Babel AST of every
fixture in `__tests__/fixtures` and round-trips it through JSON.
After the SWC location and TS-annotation fixes earlier in this stack,
the script now parses 1780 / 1795 fixtures (up from 1720), and at
least one of the newly-passing fixtures has deep enough JSX or
expression nesting to overflow the default 8 MiB Rust thread stack on
the Linux CI runner:
thread 'round_trip_all_fixtures' (7109) has overflowed its stack
fatal runtime error: stack overflow, aborting
Set `RUST_MIN_STACK=33554432` (32 MiB) before invoking cargo test.
`RUST_MIN_STACK` applies to threads spawned via `std::thread`, which
is how the libtest harness runs each test, so this is sufficient
without changing the test sources or rewriting the recursive serde
visitor.
The proper fix is to convert the recursive AST walk into an
iterative one; this commit is the cheap unblock so the rest of the
stack can land. Tracked in compiler/crates/TODO.md as future work
(to be added once this lands).
poteto
force-pushed
the
lauren/swc-10-todo-md
branch
from
c75cfc4 to
ba729ad
Compare
poteto
force-pushed
the
lauren/swc-11-ci-round-trip-stack-size
branch
from
721f949 to
5d235e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scripts/test-babel-ast.shrunscargo test -p react_compiler_ast --test round_trip, which serde-walks the parsed Babel AST of everyfixture in
__tests__/fixturesand round-trips it through JSON.After the SWC location and TS-annotation fixes earlier in this stack,
the script now parses 1780 / 1795 fixtures (up from 1720), and at
least one of the newly-passing fixtures has deep enough JSX or
expression nesting to overflow the default 8 MiB Rust thread stack on
the Linux CI runner:
Set
RUST_MIN_STACK=33554432(32 MiB) before invoking cargo test.RUST_MIN_STACKapplies to threads spawned viastd::thread, whichis how the libtest harness runs each test, so this is sufficient
without changing the test sources or rewriting the recursive serde
visitor.
The proper fix is to convert the recursive AST walk into an
iterative one; this commit is the cheap unblock so the rest of the
stack can land. Tracked in compiler/crates/TODO.md as future work
(to be added once this lands).