fix: skip symbols that are imported from other module when deconflicting module symbols#7510
Merged
IWANABETHATGUY merged 1 commit intomainfrom Dec 15, 2025
Merged
fix: skip symbols that are imported from other module when deconflicting module symbols#7510IWANABETHATGUY merged 1 commit intomainfrom
IWANABETHATGUY merged 1 commit intomainfrom
Conversation
Member
Author
How to use the Graphite Merge QueueAdd the label graphite: merge to this PR to add it to 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. |
IWANABETHATGUY
changed the title
IWANABETHATGUY
changed the title
✅ Deploy Preview for rolldown-rs canceled.
|
✅ Deploy Preview for rolldown-rs canceled.
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the deconflicting logic where symbols imported from other modules were incorrectly being processed during module symbol deconflicting. The fix ensures that only symbols owned by the current module are deconflicted, while imported symbols (which already have their canonical names determined in their original modules) are skipped.
Key changes:
- Added a check to skip imported symbols during deconflicting by verifying
canonical_ref.owner != module.idx - Updated test snapshots to reflect the corrected symbol naming behavior
- Added a new test case (
issue_7507) to verify the fix works correctly withkeepNamesoption
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs |
Core fix: added check to skip symbols imported from other modules during deconflicting; includes debug statements that should be removed |
crates/rolldown/tests/rolldown/topics/keep_names/issue_7507/src/* |
New test case demonstrating the bug fix with imported functions that have the same name |
Multiple artifacts.snap files |
Updated test snapshots showing corrected symbol renaming behavior after the fix |
integration_rolldown__filename_with_hash.snap |
Updated hash values in snapshot due to changed code generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
IWANABETHATGUY
force-pushed
the
12-15-fix_7507
branch
from
d1ef270 to
64d18ac
Compare
IWANABETHATGUY
force-pushed
the
12-15-fix_7507
branch
from
64d18ac to
8ea0a88
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Benchmarks Rust |
sapphi-red
approved these changes
Dec 15, 2025
Contributor
Merge activity
|
graphite-app bot
pushed a commit
that referenced
this pull request
Dec 15, 2025
…ing module symbols (#7510) closed #7507 **Before:** We were deconflicting symbols even when the final declaration was not in the current module. Some symbols declared via `ImportStmt` actually point to another module. This caused the deconfliction order to differ from our intended logic. **Expected behavior:** When multiple modules use the same top-level name, modules with higher execution order (i.e., executed later) should be more likely to retain the original name. - Previous logic: https://github.com/rolldown/rolldown/blob/64d18ac5a18050dedf22f8c9a9ee48a232af6ba1/crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs#L127-L132 **After:** The output now aligns with our deconfliction logic. - Updated logic: https://github.com/rolldown/rolldown/blob/8ea0a88bf0ae95a71028c8c9c5b170f225a37dd1/crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs#L110-L115 here is an example: https://github.com/rolldown/rolldown/pull/7510/changes#diff-18487ef445297a090c03a52fc7ac8ab76667ed7a3ea19e182c6ac9f9c1e45260 You could refer any other cases that snapshot has changed.
graphite-app
bot
force-pushed
the
12-15-fix_7507
branch
from
8ea0a88 to
059081b
Compare
…ing module symbols (#7510) closed #7507 **Before:** We were deconflicting symbols even when the final declaration was not in the current module. Some symbols declared via `ImportStmt` actually point to another module. This caused the deconfliction order to differ from our intended logic. **Expected behavior:** When multiple modules use the same top-level name, modules with higher execution order (i.e., executed later) should be more likely to retain the original name. - Previous logic: https://github.com/rolldown/rolldown/blob/64d18ac5a18050dedf22f8c9a9ee48a232af6ba1/crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs#L127-L132 **After:** The output now aligns with our deconfliction logic. - Updated logic: https://github.com/rolldown/rolldown/blob/8ea0a88bf0ae95a71028c8c9c5b170f225a37dd1/crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs#L110-L115 here is an example: https://github.com/rolldown/rolldown/pull/7510/changes#diff-18487ef445297a090c03a52fc7ac8ab76667ed7a3ea19e182c6ac9f9c1e45260 You could refer any other cases that snapshot has changed.
graphite-app
bot
force-pushed
the
12-15-fix_7507
branch
from
059081b to
1748472
Compare
This was referenced Dec 17, 2025
shulaoda
added a commit
that referenced
this pull request
Dec 17, 2025
## [1.0.0-beta.55] - 2025-12-17 ### 🚀 Features - add validation errors for incompatible `inlineDynamicImports` options (#7539) by @Copilot - rolldown_plugin_vite_reporter: truncate long importer lists in ineffective dynamic import warnings (#7528) by @Copilot - export `memfs` from `rolldown/experimental` for browser builds (#7490) by @Copilot - implement `postBanner` and `postFooter` (#7487) by @sevenc-nanashi - port getLogFilter helper from Rollup for advanced log filtering (#6890) by @taearls - dev: initialize `rolldown_plugin_lazy_compilation` (#7488) by @hyf0 - enable `experimental.transform_hires_sourcemap: 'boundary'` by default (#7478) by @sapphi-red - apply merge CJS namespace optimizations more generally (#7475) by @sapphi-red ### 🐛 Bug Fixes - avoid panic on invalid values for `output.exports`, `output.format`, `output.hash_characters` (#7542) by @sapphi-red - handle `__proto__` export for module namespace correctly (#7534) by @sapphi-red - handle `__proto__` export correctly (#7533) by @sapphi-red - rolldown_plugin_vite_resolve: acquire resolver lock before package json resolution (#7524) by @sapphi-red - disable syntax transform optimization for `minify: 'dce-only'` (#7520) by @sapphi-red - arrow function expression keep names (#7519) by @IWANABETHATGUY - skip symbols that are imported from other module when deconflicting module symbols (#7510) by @IWANABETHATGUY - preserve class names in assignment expressions with keepNames option (#7491) by @IWANABETHATGUY - accessor with decorators should be kept (#7499) by @Copilot - rolldown_plugin_vite_reporter: support `Infinity` for chunk_limit (#7497) by @shulaoda - change filename template validation errors from UNHANDLEABLE_ERROR to INVALID_OPTION (#7472) by @Copilot - `no entry found for key` error when merging CJS namespace exports (#7474) by @sapphi-red - generate output with syntax error when wrapped esm module using tla syntax (#7468) by @IWANABETHATGUY ### 🚜 Refactor - dev: rename `experimental.hmr` to `experimental.devMode` (#7527) by @hyf0 - pass addon option to rust as it is (#7526) by @hyf0 - remove unused stmtinfo meta flag (#7518) by @IWANABETHATGUY ### 📚 Documentation - clarify `closeBundle` behavior (#7525) by @sapphi-red ### 🧪 Testing - add tests for static string postBanner and postFooter (#7516) by @Copilot - disable pluginTimings in Rust integration tests to avoid snapshot noise (#7485) by @shulaoda - port Rollup JSX tests to `crates/rolldown/tests/rollup` (#7480) by @sapphi-red - re-triage some esbuild JSX preserve tests (#7479) by @sapphi-red - disable pluginTimings by default to avoid snapshot noise (#7471) by @shulaoda ### ⚙️ Miscellaneous Tasks - mark feature that will not be supported as ignored in rollup test status (#7535) by @sapphi-red - support `banner`/`footer`/`intro`/`outro` for config variants (#7532) by @sapphi-red - `dce/dce_of_decorators` esbuild test is now passing (#7531) by @sapphi-red - deps: update github-actions (major) (#7495) by @renovate[bot] - deps: update npm packages (#7493) by @renovate[bot] - add a check to verify failedReasons are updated (#7521) by @sapphi-red - deps: update test262 submodule for tests (#7523) by @sapphi-red - deps: update taiki-e/install-action action to v2.63.2 (#7496) by @renovate[bot] - deps: update oxc apps (#7511) by @renovate[bot] - deps: update oxc to v0.103.0 (#7513) by @camc314 - fix format in `.github/instructions/bug-investigation.instructions.md` (#7506) by @sapphi-red - add instructions for REPL decoding (#7502) by @sapphi-red - deps: update github-actions (#7492) by @renovate[bot] - deps: cargo-shear v1.9.0 (#7483) by @Boshen - deps: update dependency oxlint-tsgolint to v0.9.0 (#7484) by @renovate[bot] - deps: update dependency oxlint-tsgolint to v0.8.6 (#7470) by @renovate[bot] - add syntax validation for test output chunks that are not executed (#7466) by @IWANABETHATGUY - deps: update dependency rust to v1.92.0 (#7467) by @renovate[bot] - deps: update test262 submodule for tests (#7457) by @sapphi-red ### ❤️ New Contributors * @sevenc-nanashi made their first contribution in [#7487](#7487) * @taearls made their first contribution in [#6890](#6890) Co-authored-by: shulaoda <[email protected]>
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
2 participants
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.
closed #7507
Before:
We were deconflicting symbols even when the final declaration was not in the current module. Some symbols declared via
ImportStmtactually point to another module. This caused the deconfliction order to differ from our intended logic.Expected behavior: When multiple modules use the same top-level name, modules with higher execution order (i.e., executed later) should be more likely to retain the original name.
rolldown/crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs
Lines 127 to 132 in 64d18ac
After:
The output now aligns with our deconfliction logic.
rolldown/crates/rolldown/src/utils/chunk/deconflict_chunk_symbols.rs
Lines 110 to 115 in 8ea0a88
here is an example:
https://github.com/rolldown/rolldown/pull/7510/changes#diff-18487ef445297a090c03a52fc7ac8ab76667ed7a3ea19e182c6ac9f9c1e45260
You could refer any other cases that snapshot has changed.