fix: preserve sourcemap mappings for indented lines when a CJS module shares the chunk#10074
Conversation
✅ Deploy Preview for rolldown-rs canceled.
|
839b007 to
e9fe506
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a sourcemap correctness bug in rolldown’s Rust core when a CJS module shares a chunk with concatenated ESM modules, causing mappings for indented ESM lines to be dropped. The fix increases the sourcemap “hires” granularity used for MagicString mutation maps so collapse_sourcemaps can reliably find same-line tokens during composition, and adds a regression test covering issue #10070.
Changes:
- Generate MagicString mutation sourcemaps with
string_wizard::Hires::BoundaryinNormalModule::renderto preserve mappings for indented lines during sourcemap collapsing. - Add a new
sourcemap/issue_10070integration test fixture + snapshot with sourcemap visualization enabled. - Update the
cjs_compat/mix-cjs-esmsnapshot to reflect improved (more accurate) column mappings.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown_common/src/module/normal_module.rs | Uses Hires::Boundary when generating mutation sourcemaps so collapse_sourcemaps doesn’t drop indented-line mappings. |
| crates/rolldown/tests/rolldown/sourcemap/issue_10070/_config.json | Enables sourcemap visualization for the new regression test. |
| crates/rolldown/tests/rolldown/sourcemap/issue_10070/main.js | New regression test entry that imports CJS + ESM and exposes values on globalThis. |
| crates/rolldown/tests/rolldown/sourcemap/issue_10070/dep.cjs | New CJS dependency to reproduce the mixed-chunk condition. |
| crates/rolldown/tests/rolldown/sourcemap/issue_10070/app.js | New ESM module with indented lines that previously lost mappings. |
| crates/rolldown/tests/rolldown/sourcemap/issue_10070/artifacts.snap | Snapshot asserting the generated output and visualized sourcemap mappings for the repro. |
| crates/rolldown/tests/rolldown/cjs_compat/mix-cjs-esm/artifacts.snap | Snapshot update reflecting the improved column-accurate mappings. |
Merging this PR will not alter performance
Comparing Footnotes
|
Merge activity
|
… shares the chunk (#10074) Workaround fix for #10070 **Problem.** When a CJS module shares a chunk with concatenated ESM modules, the ESM modules lose sourcemap mappings for their indented lines (`globalThis.side = 1` / `return 42` in the repro) — only when the CJS file is present. **Cause.** A CJS module's require-init is prepended into a concatenated ESM module as a MagicString mutation, and `NormalModule::render` built that mutation map with the default `Hires::False` (one token per line, at column 0). `collapse_sourcemaps` composes by walking the last map's tokens and looking each up in the codegen map per line; the lookup returns `None` (dropping the token) when the line has no token at column ≤ the query. For indented lines the codegen token sits after the indent, so the column-0 query misses. **Fix.** Build the mutation map with `Hires::Boundary` (a token at every identifier/punctuation boundary — a superset of codegen's columns), so the collapse always finds a match. **Not the root fix.** `Hires` only controls maps rolldown generates itself; the same drop still happens when `collapse_sourcemaps` walks a coarse map from a plugin's `renderChunk`/`transform` hook. The real fix belongs in `oxc_sourcemap`: approximate to the line's nearest token on a same-line miss instead of returning `None`, as Rollup's `traceSegment` already does. **Testing.** Added `sourcemap/issue_10070`; app.js body lines now map correctly. Only other snapshot change is `cjs_compat/mix-cjs-esm` — coarse per-line positions become accurate columns (strict improvement).
e9fe506 to
047a136
Compare
…es::Boundary workaround Switch `collapse_sourcemaps` to `lookup_source_view_token_approx` (oxc-project/oxc-sourcemap#392): when a token's position falls before the first token on its line in the previous map (an indented line), clamp to that first token instead of dropping the mapping — the same approximation Rollup's `collapseSourcemaps` does. This fixes the dropped-mapping class behind #10070 across all collapse paths, so the module-level `Hires::Boundary` workaround from #10074 is removed. Snapshot changes: - 5 fixtures recover previously-dropped mappings (topics/deconflict/*, module_types/copy/*, misc/wrapped_esm). - The two mutation-path fixtures (issue_10070, mix-cjs-esm) move from column-level to line-level source precision, since the mutation map is no longer hi-res. Depends on oxc-project/oxc-sourcemap#392, pulled via [patch.crates-io] git dependency; not for merge until that lands in a published oxc_sourcemap.
## [1.1.5] - 2026-07-08 ### 🚀 Features - detect top-level import-binding reads as execution-order sensitive (#10180) by @hyf0 - sourcemap_filenames: add a sourcemapFileNames option (#9271) by @V1OL3TF0X - binding: record plugin hook result kind in tracing spans (#10154) by @IWANABETHATGUY - linking: skip side-effect-free modules in per-entry reachability (#10111) by @IWANABETHATGUY - improve error message for unresolved virtual imports (#10156) by @sapphi-red - add descriptive metadata to plugin API (#10106) by @sapphi-red - add `--configLoader=native` option (#10118) by @sapphi-red ### 🐛 Bug Fixes - improve invalid annotation warnings (#10185) by @hyf0 - keep deduplicated asset filenames stable once they can be observed (#10191) by @shulaoda - sourcemap_filenames: use public option name in pattern errors (#10188) by @IWANABETHATGUY - sourcemap_filenames: hash prepared sourcemap content (#10178) by @hyf0 - tree-shake unused circular declarators exported via export list (#10166) by @IWANABETHATGUY - dev: don't panic when an HMR rebuild hits an unresolved import (#10162) by @shulaoda - propagate errors from output.globals function (#9880) by @shulaoda - dev: revert cache mutations when a partial scan fails (#10110) by @shulaoda - dev: update importer relationships of cached modules in incremental build (#10107) by @shulaoda - hmr: fall back to full reload when a changed module is not registered as executed (#10132) by @shulaoda - chunk-optimizer: follow entry facade edges in runtime placement cycle check (#10101) by @hyf0 - dev: ignore watcher events after close (#10113) by @hyf0 - emit async wrapper for TLA modules under onDemandWrapping (#10086) by @IWANABETHATGUY - gate sideEffects:false modules' side effects on body demand (#10080) by @IWANABETHATGUY - rolldown_plugin_vite_resolve: return empty object for `browser: false` mapped modules (#10082) by @sapphi-red - reset the word-boundary state on newline in Hires::Boundary sourcemaps (#10025) by @shulaoda - trim an emptied chunk's outro/intro instead of skipping past it (#10029) by @shulaoda - test each edited chunk's own start against indent exclude ranges (#10026) by @shulaoda - preserve sourcemap mappings for indented lines when a CJS module shares the chunk (#10074) by @hyf0 ### 🚜 Refactor - separate tree-shaking side effects from execution order sensitivity (#10168) by @hyf0 - type construct_vite_preload_call to take an ObjectPattern (#10135) by @shulaoda - treeshake: single-source the own-export classification shared with the lazy-barrel loader (#10098) by @IWANABETHATGUY - dev: reuse Vite's bundledDev server (#10081) by @h-a-n-a - clippy: ban std HashMap/HashSet in favour of FxHashMap/FxHashSet (#10108) by @Boshen - treeshake: make body demand a second module bit instead of a stmt multimap (#10097) by @IWANABETHATGUY - seal used_symbol_refs by construction after its last writer (#10091) by @hyf0 - treeshake: replace inclusion mutual recursion with a worklist engine (#10096) by @IWANABETHATGUY - treeshake: split include_statements.rs into focused modules (#10095) by @IWANABETHATGUY - drop redundant is_user_defined filter on partitioned entries (#10050) by @shulaoda - project the retained export interface out of used_symbol_refs (#10089) by @hyf0 - track used external symbols separately from used_symbol_refs (#10088) by @hyf0 - make module namespace inclusion an explicit linking metadata field (#10087) by @hyf0 - rename statement evaluation metadata (#10078) by @hyf0 ### 📚 Documentation - virtual modules user-facing id convention (#10155) by @sapphi-red - cli: clarify disabling boolean/object flags like codeSplitting (#10153) by @IWANABETHATGUY - chore: remove Vite+ alpha banner (#10105) by @mdong1909 - write down the used_symbol_refs contract (#10090) by @hyf0 - dev/lazy: update design and implementation (#10079) by @h-a-n-a ### ⚡ Performance - ast_scanner: stop order-sensitivity checks once a module is flagged (#10190) by @IWANABETHATGUY - return impl ExactSizeIterator from slice-backed accessors (#10133) by @Boshen - binding: box dev and watcher napi futures (#10103) by @Boshen ### 🧪 Testing - move string_wizard replace unit tests to the JS magic-string suite (#10176) by @IWANABETHATGUY - dev: assert incremental scan state matches a fresh full build after each HMR step (#10115) by @shulaoda - dev: restore runtime assertions of delete_file_not_used_anymore (#10112) by @shulaoda - dev: fix flaky dev server tests in CI (#10152) by @h-a-n-a - add regression test for #10099 (lazyBarrel drops default-import binding but keeps its property reads) (#10109) by @IWANABETHATGUY ### ⚙️ Miscellaneous Tasks - deploy website to Void via GitHub OIDC (#10192) by @Boshen - deps: update oxc to 0.139.0 (#10161) by @shulaoda - deps: update test262 submodule for tests (#10160) by @rolldown-guard[bot] - rolldown_plugin_utils: remove dead asset-url and css scaffolding (#10131) by @shulaoda - deps: revert vite-plus to v0.2.1 (#10148) by @shulaoda - deps: update github actions (#10141) by @renovate[bot] - deps: update dependency rust to v1.96.1 (#10145) by @renovate[bot] - deps: update npm packages (#10142) by @renovate[bot] - deps: update rust crates (#10143) by @renovate[bot] - deps: update napi to v3.10.3 (#10121) by @renovate[bot] - rolldown_utils: remove unused time module (#10138) by @shulaoda - remove dead CopyModulePlugin::is_active method (#10129) by @shulaoda - remove dead LazyCompilationContext::is_lazy_module method (#10128) by @shulaoda - remove dead BuildDiagnostic::downcast_ref method (#10127) by @shulaoda - deps: update dependency vite-plus to v0.2.2 (#10084) by @renovate[bot] - deps: update rust crate oxc_sourcemap to v8.1.0 (#10122) by @renovate[bot] - deps: update crate-ci/typos action to v1.48.0 (#10124) by @renovate[bot] - enable more clippy restriction lints (#10114) by @Boshen - deps: update rust dependencies (#10100) by @Boshen - deps: update oxc resolver to v11.23.0 (#10083) by @renovate[bot] ###◀️ Revert - Revert "chore(deps): revert vite-plus to v0.2.1" (#10157) by @h-a-n-a - "fix(hmr): fall back to full reload when a changed module is not registered as executed (#10132)" (#10151) by @shulaoda ### ❤️ New Contributors * @V1OL3TF0X made their first contribution in [#9271](#9271) Co-authored-by: shulaoda <[email protected]>
refs #10070. ## The behavior to review This PR makes one choice: when a coarse outer map points to a column before the first mapping on the same line of a more detailed inner map, use that first same-line mapping instead of dropping the generated position. Rollup makes the same choice when tracing segments. The fallback never crosses a line. ## Concrete before and after ### User input `main.js` is the source written by the user: ```js export function app() { globalThis.side = 1; return 42; } ``` ### Generated output Rolldown produces this chunk. Region comments and the `sourceMappingURL` line are omitted here, but the code shape is otherwise the fixture's real output: ```js function app() { globalThis.side = 1; return 42; } export { app }; ``` The generated JavaScript is identical before and after this PR. Only its source map changes. ### Before this PR Asking the composed map where the two generated statements came from returns no original position: ```text generated 3:1 globalThis.side = 1 ──X──> null generated 4:1 return 42 ──X──> null ``` In a source-map visualizer, these generated statements have no colored counterpart in the Original pane. A debugger or stack-trace consumer cannot take these positions back to `main.js`. ### After this PR The same generated positions resolve to the corresponding user-written statements: ```text Generated output User input: main.js 3:1 globalThis.side = 1 ─────────────────> 2:2 globalThis.side = 1 4:1 return 42 ─────────────────> 3:2 return 42 ``` Source-map consumer lines are one-based and columns are zero-based. These are the actual positions asserted by the integration fixture. In a visualizer, each generated statement now shares a color with its corresponding statement in the Original pane. Selecting either side can identify the other side. Debuggers and stack traces can return to the correct original line; the column is the best same-line position available from the coarse map. ## Why the lookup changes The map chain contains two different levels of detail: ```text detailed map: intermediate column 2 -> user source coarse map: final column 0 -> intermediate column 0 ``` Before: ```text final column 0 -> intermediate column 0 -> detailed map has no mapping at or before column 0 -> drop the generated position ``` After: ```text final column 0 -> intermediate column 0 -> detailed map's first mapping on this line is column 2 -> use column 2 and preserve the generated position ``` Lookups between or after existing mappings still use the nearest mapping at or before the requested column. Missing and empty lines still fail. One-field explicitly unmapped segments are skipped in this PR, matching Rollup; #10254 separately preserves those boundaries. #10074 remains necessary because it makes Rolldown-owned mutation maps more precise. This PR covers coarse external maps and other cases where the producer did not provide matching columns. ## Why the snapshots change The snapshot visualizer writes mappings as: ```text (original line:column) "original text" --> (generated line:column) "generated text" ``` Unlike the source-map consumer positions above, these snapshot coordinates are zero-based for both lines and columns. The three snapshots only add mappings; no existing mapping is removed or changed. | Snapshot | Newly restored generated mapping | Previous first mapping on that generated line | Original position used | | --- | --- | --- | --- | | `misc/wrapped_esm` | `60:1 "("`, `60:2 "{"` | `60:3 "e} = "` | `foo.js 4:4` | | `misc/wrapped_esm` | `61:1 "("`, `61:2 "{"` | `61:3 "h: "` | `foo.js 7:2` | | `misc/wrapped_esm` | `73:1 "("`, `73:2 "{"` | `73:3 "destructuring} = "` | `foo.js 30:6` | | `wrapped_esm_default_function` | `19:0 "init_foo("` | `19:9 ");"` | `bar.js 0:23` | | `wrapped_esm_export_named_function` | `18:0 "init_foo("` | `18:9 ");"` | `bar.js 0:27` | There are eight added rows in total: six in `misc/wrapped_esm`, one in each deconflict snapshot. Every addition follows the behavior under review: ```text new generated column < previous first mapped column on that line new original position = previous first mapping's original position ``` For example: ```diff +(0:23) ";\n" --> (19:0) "init_foo(" (0:23) ";\n" --> (19:9) ");\n" ``` The coarse map requested generated column 0, while the detailed map's first mapping was at column 9. The fallback associates column 0 with the same original position already used at column 9. It does not move or replace the existing mapping. ## Suggested review order 1. Review the lookup change and `source_id` guards in `crates/rolldown_sourcemap/src/lib.rs`. 2. Review the Rust test where a column-0 coarse token meets a column-2 detailed token. 3. Review the `renderChunk` fixture, which exercises the public plugin path and asserts the exact positions shown above. 4. Check the snapshot table against the eight added snapshot rows. ## User impact - generated JavaScript, runtime behavior, and bundle size do not change - users of source maps regain original locations for lines previously dropped during composition - the change matters when a plugin or another transform provides a less column-detailed map than Rolldown's preceding map - already resolvable mappings are unchanged ## Validation - `cargo test -p rolldown_sourcemap` - `just test-rust` - `just test-node-rolldown-only fixtures-concurrent.test.ts -t plugin/render-chunk/coarse-sourcemap-composition` - `cargo clippy -p rolldown_sourcemap --all-targets -- --deny warnings`
Workaround fix for #10070
Problem. When a CJS module shares a chunk with concatenated ESM modules, the ESM modules lose sourcemap mappings for their indented lines (
globalThis.side = 1/return 42in the repro) — only when the CJS file is present.Cause. A CJS module's require-init is prepended into a concatenated ESM module as a MagicString mutation, and
NormalModule::renderbuilt that mutation map with the defaultHires::False(one token per line, at column 0).collapse_sourcemapscomposes by walking the last map's tokens and looking each up in the codegen map per line; the lookup returnsNone(dropping the token) when the line has no token at column ≤ the query. For indented lines the codegen token sits after the indent, so the column-0 query misses.Fix. Build the mutation map with
Hires::Boundary(a token at every identifier/punctuation boundary — a superset of codegen's columns), so the collapse always finds a match.Not the root fix.
Hiresonly controls maps rolldown generates itself; the same drop still happens whencollapse_sourcemapswalks a coarse map from a plugin'srenderChunk/transformhook. The real fix belongs inoxc_sourcemap: approximate to the line's nearest token on a same-line miss instead of returningNone, as Rollup'straceSegmentalready does.Testing. Added
sourcemap/issue_10070; app.js body lines now map correctly. Only other snapshot change iscjs_compat/mix-cjs-esm— coarse per-line positions become accurate columns (strict improvement).