[rust-compiler] Skip empty optional ScopeInfo maps on serialize#36522
Open
poteto wants to merge 1 commit into
Open
[rust-compiler] Skip empty optional ScopeInfo maps on serialize#36522poteto wants to merge 1 commit into
poteto wants to merge 1 commit into
Conversation
`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.
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
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.
bash compiler/scripts/test-babel-ast.shruns three sub-tests inorder:
round_trip_all_fixtures,scope_info_round_trip, andscope_resolution_rename. The first and third are green after theearlier slices in this stack, but the middle one fails 1780/1780
with diffs of the shape:
This was previously masked because the round-trip failure earlier in
the script tripped
set -eand the scope-info test never ran. Withround-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::collectScopeInfoemits onlyfive keys per
.scope.jsonfile —scopes,bindings,nodeToScope,referenceToBinding, andprogramScope. It neverwrites
nodeIdToScope,refNodeIdToBinding, ornodeToScopeEnd.The Rust
ScopeInfostruct declares those three fields with#[serde(default)], so deserialization happily produces emptyHashMap/IndexMapvalues when the keys are missing. Serializationwas then emitting them as
"nodeIdToScope": {}(etc.), which theround-trip diff treats as a mismatch.
Add
skip_serializing_if = "<Map>::is_empty"to all three fields soa 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) aredeliberately untouched — they are present in every fixture by
design.
Alternatives considered:
#[serde(default)]from the three fields. Rejected: wouldbreak deserialization of legacy fixtures that legitimately omit
these supplemental maps.
Option<...>. Rejected: changes field types andforces every reader to unwrap, with no behavior win over the
existing default + skip-on-empty combination.
HashMap::is_emptyandIndexMap::is_emptyexpress intent directly and are already inscope via the existing imports.
Defaultimpl forScopeInfo. Rejected: the issue is onthe serialize side, not deserialize;
serde(default)alreadysupplies empty maps on deserialize correctly.
Test plan:
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