Skip to content

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

Closed
poteto wants to merge 1 commit into
lauren/swc-14-roundtrip-skip-lone-surrogatefrom
lauren/swc-15-scope-info-empty-map-skip
Closed

[rust-compiler] Skip empty optional ScopeInfo maps on serialize#36520
poteto wants to merge 1 commit into
lauren/swc-14-roundtrip-skip-lone-surrogatefrom
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 deleted the branch lauren/swc-14-roundtrip-skip-lone-surrogate May 21, 2026 18:01
@poteto poteto closed this May 21, 2026
@poteto
Copy link
Copy Markdown
Collaborator Author

poteto commented May 21, 2026

Reopened as #36522 after rebasing onto lauren/swc-13-flow-typecast-in-pattern (dropping the now-withdrawn slice-14 from the stack). This PR is closed because its original base branch was deleted.

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