Skip to content

[rust-compiler] Skip empty optional ScopeInfo maps on serialize#36522

Open
poteto wants to merge 1 commit into
lauren/swc-13-flow-typecast-in-patternfrom
lauren/swc-15-scope-info-empty-map-skip
Open

[rust-compiler] Skip empty optional ScopeInfo maps on serialize#36522
poteto wants to merge 1 commit into
lauren/swc-13-flow-typecast-in-patternfrom
lauren/swc-15-scope-info-empty-map-skip

Conversation

@poteto
Copy link
Copy Markdown
Collaborator

@poteto poteto commented May 21, 2026

bash compiler/scripts/test-babel-ast.sh runs three sub-tests in
order: round_trip_all_fixtures, scope_info_round_trip, and
scope_resolution_rename. The first and third are green after the
earlier slices in this stack, but the middle one fails 1780/1780
with diffs of the shape:

--- compiler/<fixture>.json ---
Round-trip mismatch:
+   "nodeIdToScope": {},

This was previously masked because the round-trip failure earlier in
the script tripped set -e and the scope-info test never ran. With
round-trip green, it now surfaces a pre-existing serialization
mismatch in ScopeInfo.

Root cause: the Babel-side fixture producer in
compiler/scripts/babel-ast-to-json.mjs::collectScopeInfo emits only
five keys per .scope.json file — scopes, bindings,
nodeToScope, referenceToBinding, and programScope. It never
writes nodeIdToScope, refNodeIdToBinding, or nodeToScopeEnd.

The Rust ScopeInfo struct declares those three fields with
#[serde(default)], so deserialization happily produces empty
HashMap / IndexMap values when the keys are missing. Serialization
was then emitting them as "nodeIdToScope": {} (etc.), which the
round-trip diff treats as a mismatch.

Add skip_serializing_if = "<Map>::is_empty" to all three fields so
a fixture that came in without the key goes back out without the
key. Required fields (no #[serde(default)]: scopes, bindings,
node_to_scope, reference_to_binding, program_scope) are
deliberately untouched — they are present in every fixture by
design.

Alternatives considered:

  • Remove #[serde(default)] from the three fields. Rejected: would
    break deserialization of legacy fixtures that legitimately omit
    these supplemental maps.
  • Wrap the maps in Option<...>. Rejected: changes field types and
    forces every reader to unwrap, with no behavior win over the
    existing default + skip-on-empty combination.
  • Custom skip function. Rejected: HashMap::is_empty and
    IndexMap::is_empty express intent directly and are already in
    scope via the existing imports.
  • Add a Default impl for ScopeInfo. Rejected: the issue is on
    the serialize side, not deserialize; serde(default) already
    supplies empty maps on deserialize correctly.

Test plan:

  • bash compiler/scripts/test-babel-ast.sh:
    Before: round_trip 1779/1779, scope_info 0/1780 (FAIL), rename 1779/1779; exit 101
    After: round_trip 1779/1779, scope_info 1780/1780, rename 1779/1779; exit 0
  • cargo build: clean across the workspace.

`bash compiler/scripts/test-babel-ast.sh` runs three sub-tests in
order: `round_trip_all_fixtures`, `scope_info_round_trip`, and
`scope_resolution_rename`. The first and third are green after the
earlier slices in this stack, but the middle one fails 1780/1780
with diffs of the shape:

    --- compiler/<fixture>.json ---
    Round-trip mismatch:
    +   "nodeIdToScope": {},

This was previously masked because the round-trip failure earlier in
the script tripped `set -e` and the scope-info test never ran. With
round-trip green, it now surfaces a pre-existing serialization
mismatch in `ScopeInfo`.

Root cause: the Babel-side fixture producer in
`compiler/scripts/babel-ast-to-json.mjs::collectScopeInfo` emits only
five keys per `.scope.json` file — `scopes`, `bindings`,
`nodeToScope`, `referenceToBinding`, and `programScope`. It never
writes `nodeIdToScope`, `refNodeIdToBinding`, or `nodeToScopeEnd`.

The Rust `ScopeInfo` struct declares those three fields with
`#[serde(default)]`, so deserialization happily produces empty
`HashMap` / `IndexMap` values when the keys are missing. Serialization
was then emitting them as `"nodeIdToScope": {}` (etc.), which the
round-trip diff treats as a mismatch.

Add `skip_serializing_if = "<Map>::is_empty"` to all three fields so
a fixture that came in without the key goes back out without the
key. Required fields (no `#[serde(default)]`: `scopes`, `bindings`,
`node_to_scope`, `reference_to_binding`, `program_scope`) are
deliberately untouched — they are present in every fixture by
design.

Alternatives considered:

- Remove `#[serde(default)]` from the three fields. Rejected: would
  break deserialization of legacy fixtures that legitimately omit
  these supplemental maps.
- Wrap the maps in `Option<...>`. Rejected: changes field types and
  forces every reader to unwrap, with no behavior win over the
  existing default + skip-on-empty combination.
- Custom skip function. Rejected: `HashMap::is_empty` and
  `IndexMap::is_empty` express intent directly and are already in
  scope via the existing imports.
- Add a `Default` impl for `ScopeInfo`. Rejected: the issue is on
  the serialize side, not deserialize; `serde(default)` already
  supplies empty maps on deserialize correctly.

Test plan:
- bash compiler/scripts/test-babel-ast.sh:
    Before: round_trip 1779/1779, scope_info 0/1780 (FAIL), rename 1779/1779; exit 101
    After:  round_trip 1779/1779, scope_info 1780/1780,        rename 1779/1779; exit 0
- cargo build: clean across the workspace.
@meta-cla meta-cla Bot added the CLA Signed label May 21, 2026
@github-actions github-actions Bot added the React Core Team Opened by a member of the React Core Team label May 21, 2026
@poteto poteto requested a review from mvitousek May 21, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant