Skip to content

refactor(semantic): add move_binding_by_symbol_id#22408

Merged
graphite-app[bot] merged 1 commit into
mainfrom
refactor/semantic-move-binding-by-symbol-id
May 14, 2026
Merged

refactor(semantic): add move_binding_by_symbol_id#22408
graphite-app[bot] merged 1 commit into
mainfrom
refactor/semantic-move-binding-by-symbol-id

Conversation

@Dunqing

@Dunqing Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member

Summary

Adds Scoping::move_binding_by_symbol_id(from, to, symbol_id) that bundles move_binding + set_symbol_scope_id, looking up the symbol's name internally.

Every existing call to move_binding is paired with set_symbol_scope_id to keep symbol_scope_ids consistent with the binding map. Folding them into one call also lets callers move a binding without holding an arena-lifetime Ident across the &mut Scoping access — useful when the only name source is Scoping's own binding map (the next step in the ERM scope-binding fix in #22320).

The 6 existing pair sites are migrated:

  • oxc_semantic/src/binder.rs (var-hoist Annex B)
  • oxc_transformer/src/common/arrow_function_converter.rs (adjust_binding_scope)
  • oxc_transformer/src/es2017/async_to_generator.rs (BindingMover)
  • oxc_transformer/src/es2018/object_rest_spread.rs (for-init bindings)
  • oxc_transformer/src/es2026/explicit_resource_management.rs x3 (for-of init, static block, try statement)

Notes

  • The helper updates symbol_scope_ids unconditionally — matching the pre-existing pair behavior, which always set the scope id even when move_binding was a silent no-op.
  • The internal remove_entry(&name) lookup uses Ident's precomputed hash via IdentBuildHasher::write_u64, skipping the byte-by-byte hash path that name.as_str() would take.
  • No new tests: each migrated call site is exercised by transformer Babel/conformance and semantic_* coverage; snapshots are unchanged.

@github-actions github-actions Bot added A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler labels May 14, 2026

Dunqing commented May 14, 2026

Copy link
Copy Markdown
Member Author

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codspeed-hq

codspeed-hq Bot commented May 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing refactor/semantic-move-binding-by-symbol-id (16a055b) with main (2be5526)

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Dunqing Dunqing force-pushed the refactor/semantic-move-binding-by-symbol-id branch from f837839 to 16a055b Compare May 14, 2026 07:47
@Dunqing Dunqing marked this pull request as ready for review May 14, 2026 09:07
@Dunqing Dunqing requested a review from overlookmotel as a code owner May 14, 2026 09:07
@graphite-app graphite-app Bot added the 0-merge Merge with Graphite Merge Queue label May 14, 2026
@graphite-app

graphite-app Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Merge activity

## Summary

Adds `Scoping::move_binding_by_symbol_id(from, to, symbol_id)` that bundles `move_binding` + `set_symbol_scope_id`, looking up the symbol's name internally.

Every existing call to `move_binding` is paired with `set_symbol_scope_id` to keep `symbol_scope_ids` consistent with the binding map. Folding them into one call also lets callers move a binding without holding an arena-lifetime `Ident` across the `&mut Scoping` access — useful when the only name source is `Scoping`'s own binding map (the next step in the ERM scope-binding fix in #22320).

The 6 existing pair sites are migrated:

- `oxc_semantic/src/binder.rs` (var-hoist Annex B)
- `oxc_transformer/src/common/arrow_function_converter.rs` (`adjust_binding_scope`)
- `oxc_transformer/src/es2017/async_to_generator.rs` (`BindingMover`)
- `oxc_transformer/src/es2018/object_rest_spread.rs` (for-init bindings)
- `oxc_transformer/src/es2026/explicit_resource_management.rs` x3 (for-of init, static block, try statement)

## Notes

- The helper updates `symbol_scope_ids` unconditionally — matching the pre-existing pair behavior, which always set the scope id even when `move_binding` was a silent no-op.
- The internal `remove_entry(&name)` lookup uses `Ident`'s precomputed hash via `IdentBuildHasher::write_u64`, skipping the byte-by-byte hash path that `name.as_str()` would take.
- No new tests: each migrated call site is exercised by transformer Babel/conformance and `semantic_*` coverage; snapshots are unchanged.
@graphite-app graphite-app Bot force-pushed the refactor/semantic-move-binding-by-symbol-id branch from 16a055b to fb4d98b Compare May 14, 2026 09:08
@graphite-app graphite-app Bot merged commit fb4d98b into main May 14, 2026
28 checks passed
@graphite-app graphite-app Bot deleted the refactor/semantic-move-binding-by-symbol-id branch May 14, 2026 09:13
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants