refactor(rust): remove FacadeScoping, use Scoping::create_symbol for facade symbols#8540
Merged
refactor(rust): remove FacadeScoping, use Scoping::create_symbol for facade symbols#8540
Conversation
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors rolldown’s Rust symbol infrastructure to remove the old “facade symbol” ID partitioning scheme and instead allocate synthetic (facade) symbols directly inside oxc’s Scoping symbol table, identifying them via a new SymbolRefFlags::IsFacade flag. This simplifies symbol bookkeeping (notably classic_data access) and fixes incremental-build cache syncing for symbols created during linking.
Changes:
- Remove
FacadeScoping/boundary-based facade symbol handling; create facade symbols withScoping::create_symbol()and mark them withSymbolRefFlags::IsFacade. - Simplify symbol classic-data lookup to direct
IndexVec<SymbolId, ...>indexing; add cache sync viaSymbolRefDbForModule::merge_from_build(). - Replace the deleted
SymbolIdExtnamespace detection withModule::namespace_object_ref()and update call sites.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown_common/src/types/symbol_ref_db.rs | Add IsFacade flag, simplify classic data access, and add incremental cache merge (merge_from_build). |
| crates/rolldown_common/src/types/ast_scopes.rs | Remove FacadeScoping; allocate facade symbols in Scoping via create_symbol. |
| crates/rolldown_common/src/module/mod.rs | Add Module::namespace_object_ref() to replace hardcoded namespace symbol IDs. |
| crates/rolldown_common/src/lib.rs | Stop re-exporting removed SymbolIdExt. |
| crates/rolldown_common/src/ecmascript/symbol_id_ext.rs | Delete the SymbolIdExt trait (magic namespace SymbolId value). |
| crates/rolldown_common/src/ecmascript/mod.rs | Remove symbol_id_ext module export. |
| crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs | Stop iterating the removed facade-classic-data map; rely on unified classic_data. |
| crates/rolldown/src/stages/link_stage/tree_shaking/include_statements.rs | Replace namespace checks using SymbolIdExt with Module::namespace_object_ref(). |
| crates/rolldown/src/stages/link_stage/generate_lazy_export.rs | Recreate facade symbol refs when JSON modules rebuild Scoping (old IDs become invalid). |
| crates/rolldown/src/stages/generate_stage/compute_cross_chunk_links.rs | Replace hardcoded namespace symbol construction with module-provided namespace refs. |
| crates/rolldown/src/module_finalizers/mod.rs | Replace SymbolIdExt namespace refs with Module::namespace_object_ref() usage. |
| crates/rolldown/src/bundle/bundle.rs | Use merge_from_build when updating the incremental-build cache snapshot. |
| crates/rolldown/src/ast_scanner/mod.rs | Update facade-symbol detection to use the DB’s IsFacade flag. |
df3186a to
103f6ab
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/rolldown_common/src/types/symbol_ref_db.rs:136
merge_from_buildonly extendsclassic_datawhenbuild_dbhas more symbols. If a rebuild ever produces fewer symbols (e.g. module source shrinks / different wrapping decisions),classic_datawill remain longer thanast_scopes.total_symbol_count(), leaving stale per-symbol data (and potentially staleflagsentries) addressable by oldSymbolIds. Consider resizingclassic_datato exactlynew_len(truncate when shrinking) and pruning anyflagskeyed by removedSymbolIds to keep the DB invariant in sync withScoping.
// Extend classic_data for any symbols added during linking (e.g., facade symbols)
let new_len = build_db.ast_scopes.total_symbol_count();
let current_len = self.classic_data.len();
for _ in current_len..new_len {
self.classic_data.push(SymbolRefDataClassic::default());
}
Contributor
Benchmarks Rust |
1462722 to
103f6ab
Compare
Collaborator
Author
|
This PR aims to standardize the handling of facade symbols, making it easier to understand. Before:The old After:This PR switches to oxc's 
Additionally, we achieved a 0.5% performance improvement. Also, this change allows us to use |
…facade symbols Replace the two-layer FacadeScoping system (reverse-counting IDs from u32::MAX) with oxc's Scoping::create_symbol() API, giving facade symbols contiguous IDs in the same symbol table as real AST symbols. Key changes: - Remove FacadeScoping struct and minimum_symbol_id boundary from AstScopes - Add SymbolRefFlags::IsFacade to identify facade symbols via flag instead of ID range - Simplify get_classic_data from boundary-check + HashMap fallback to direct indexing - Delete SymbolIdExt trait (hardcoded u32::MAX-2 for namespace detection) - Add Module::namespace_object_ref() API to replace SymbolIdExt usage - Add SymbolRefDbForModule::merge_from_build() for incremental build cache correctness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
103f6ab to
8d9f1c9
Compare
IWANABETHATGUY
approved these changes
Mar 4, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/rolldown_common/src/types/symbol_ref_db.rs:147
merge_from_buildrestoresScopingand addsIsFacadeflags for newly-created facade symbols, but it doesn't mergecreate_reason(debug-only) for those symbols. In debug builds this can makeSymbolRefDb::get_create_reasonreturn "No create reason found" for a facade symbol, which contradicts the method’s doc comment and makes incremental-build debugging harder. Consider mergingbuild_db.create_reasonentries for symbols that are new to the cache DB whencfg(debug_assertions)is enabled.
/// Merge immutable fields (Scoping) from a build's DB into this cache DB.
/// Also extends `classic_data` and preserves `IsFacade` flags for facade symbols
/// that were created during the linking phase.
pub fn merge_from_build(&mut self, build_db: SymbolRefDbForModule) {
// Extend classic_data for any symbols added during linking (e.g., facade symbols)
let new_len = build_db.ast_scopes.total_symbol_count();
let current_len = self.classic_data.len();
for _ in current_len..new_len {
self.classic_data.push(SymbolRefDataClassic::default());
}
// Preserve IsFacade flags for facade symbols added during linking
for (symbol_id, flags) in &build_db.flags {
if flags.contains(SymbolRefFlags::IsFacade) {
self.flags.entry(*symbol_id).or_default().insert(SymbolRefFlags::IsFacade);
}
}
let scoping = build_db.ast_scopes.into_scoping();
self.ast_scopes.set_scoping(scoping);
}
hyf0
approved these changes
Mar 4, 2026
Contributor
4 tasks
Merged
shulaoda
added a commit
that referenced
this pull request
Mar 5, 2026
## [1.0.0-rc.7] - 2026-03-05 ⚡ Smarter Code Generation Defaults - DCE-only minification and smart constant inlining are now enabled by default - Produces cleaner, smaller output bundles without requiring explicit configuration 💡 LLM-Friendly Bundle Analyzer Reports - New markdown output format for the bundle analyzer plugin with bundle summaries, module graphs, dependency chains, and optimization suggestions - Optimization suggestions now also recommend using the entriesAware option when common chunks contain modules only reachable from specific entries ### 💥 BREAKING CHANGES - enable minify: 'dce-only' by default (#8465) by @IWANABETHATGUY - settings `inlineConst: { mode: 'smart', pass: 1}` by default (#8444) by @IWANABETHATGUY ### 🚀 Features - binding: add original getter to BindingMagicString (#8533) by @IWANABETHATGUY - native-magic-string: add `offset` property support (#8531) by @IWANABETHATGUY - add `output.strict` option to control `"use strict"` directive emission (#8489) by @Copilot - watch: expose `watcher.compareContentsForPolling` (#8526) by @hyf0 - watch: use new watcher to support watch mode (#8475) by @hyf0 - rust/watch: handle bulk-change (#8466) by @hyf0 - add LLM-friendly markdown output format to bundle analyzer plugin (#8242) by @IWANABETHATGUY ### 🐛 Bug Fixes - expose `plugins` on `NormalizedInputOptions` for `buildStart` hook (#8521) by @Copilot - only uppercase facade symbols in JSX preserve mode (#8519) by @IWANABETHATGUY - binding: export BindingResult in generated dts header (#8537) by @minsoo-web - pre-resolve paths option to avoid `invoke_sync` deadlock (#8518) by @IWANABETHATGUY - remove debug-only jsx_preset and UntranspiledSyntaxError (#8511) by @IWANABETHATGUY - apply `topLevelVar` to exported `const`/`let` declarations (#8507) by @IWANABETHATGUY - rolldown_plugin_vite_web_worker_post: avoid replacing `new.target` (#8488) by @sapphi-red - update copyright year to 2026 (#8486) by @maciekzygmunt ### 🚜 Refactor - rust: use Oxc's SymbolFlags::ConstVariable instead of custom IsConst flag (#8543) by @Dunqing - rust: remove FacadeScoping, use Scoping::create_symbol for facade symbols (#8540) by @Dunqing - rust/watch: remove hacky `reset_closed_for_watch_mode` (#8530) by @hyf0 - binding: return &str instead of String in filename() getter (#8534) by @IWANABETHATGUY - rust: remove old watch mode implementation (#8525) by @hyf0 - rust/watch: simply watch logic in the binding layer (#8516) by @hyf0 - rust/watch: tweak struct/function names (#8464) by @hyf0 ### 📚 Documentation - explain how external modules work in rolldown (#8457) by @sapphi-red - add some diagrams using graphviz (#8499) by @sapphi-red - use `vitepress-plugin-graphviz` (#8498) by @sapphi-red - list s390x/ppc64le prebuilt binaries (#8495) by @crusty-voidzero - fix error type for `RolldownBuild.generate` and others (#8490) by @sapphi-red ### ⚡ Performance - string_wizard: reduce allocations and add ASCII fast paths (#8541) by @IWANABETHATGUY - use IndexBitSet to replace IndexVec<XXXIdx, bool> for module/stmt inclusion tracking (#8503) by @IWANABETHATGUY - plugin: use IndexBitSet to optimize skipped plugins checking (#8497) by @ShroXd - rust/tla: skip compute_tla if there is no module use TLA (#8487) by @ShroXd ### 🧪 Testing - node/watch: make watch tests run in concurrent and retry-able (#8512) by @hyf0 - add test case for static flag tree-shaking (#8476) by @IWANABETHATGUY - migrate post-banner sourcemap-with-shebang to Rust (#8477) by @Copilot ### ⚙️ Miscellaneous Tasks - vscode: `formatOnSave` for markdown files using oxc formatter (#8536) by @minsoo-web - deps: update test262 submodule for tests (#8528) by @sapphi-red - remove `retry` workaround from output paths test fixtures (#8520) by @Copilot - docs: add Shuyuan Wang (h-a-n-a) and remove from acknowledgements (#8509) by @Copilot - consolidate top_level_var test cases using configVariants (#8508) by @IWANABETHATGUY - add s390x and ppc64le linux gnu targets (#8493) by @Brooooooklyn ###◀️ Revert - fix(rolldown): increase tokio blocking threads size for watch mode (#8517) by @hyf0 ### ❤️ New Contributors * @minsoo-web made their first contribution in [#8536](#8536) * @crusty-voidzero made their first contribution in [#8495](#8495) * @maciekzygmunt made their first contribution in [#8486](#8486) Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
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.
Summary
FacadeScopingstruct andminimum_symbol_idboundary that separated "real" AST symbols from synthetic "facade" symbols using reverse-counting IDs (u32::MAX - 1downward)Scoping::create_symbol()API instead, giving facade symbols contiguous forward-counting IDs in the same symbol tableSymbolRefFlags::IsFacadeflag to identify facade symbols, replacing the old boundary-based checkget_classic_datafrom boundary-check + HashMap fallback to directIndexVecindexingSymbolIdExttrait (hardcodedu32::MAX - 2for namespace detection), addModule::namespace_object_ref()APISymbolRefDbForModule::merge_from_build()for correct incremental build cache handlingTest plan
cargo clippy -p rolldown -p rolldown_common— no errorsutil_deprecate)no_accept_outside_circular— passes🤖 Generated with Claude Code