refactor(rolldown_common): model ModuleId as a classified Path/Virtual/Bare enum by Boshen · Pull Request #9927 · rolldown/rolldown

Boshen · 2026-06-22T15:28:09Z

Summary

ModuleId was a struct { inner: ArcStr } that every call site re-interpreted as a filesystem path through SugarPath::as_path() — treating virtual (\0…), bare (react), and URL ids as paths, and round-tripping bytes we already know are UTF-8 through to_string_lossy.

This remodels it as an enum classified once at construction:

pub struct ModuleId { repr: Repr }   // private enum Repr { Path | Virtual | Bare }

Gated path ops — as_path() -> Option<&Path> (Some only for Path), plus is_path(), is_virtual(), kind(), is_in_node_modules(), representative_name(). Non-path ids are no longer silently path-parsed.
ArcStr kept as the backing store — cheap clone, durable cross-build identity, literal! const for the runtime module. ModuleIdx (the u32 per-build handle) stays the hot graph key, so ModuleId optimizes for being a faithful string/path identity rather than for size/hash speed.
Lossless — every to_string_lossy on a ModuleId / StableModuleId-derived path becomes to_str() (module ids are always valid UTF-8). This is done in the shared path_ext helpers, so it also covers ast_scanner and preserve-modules naming.
Hash / Eq / Ord / Borrow<str> are hand-implemented over as_str() (byte-exact, ignoring the discriminant) so &str lookups into module_id_to_idx / user_defined_entry keep working.
StableModuleId::new branches on the cached kind() instead of re-running is_absolute() / \0 detection.

Why

A module id is sometimes a path and sometimes not. Treating it uniformly as a path was wasteful (redundant UTF-8 scans, to_string_lossy on known-UTF-8 data) and a latent correctness hazard. Classifying once makes the path / not-path distinction explicit and confines path semantics to where they belong.

Validation

cargo test — 1756 integration/snapshot tests pass with zero snapshot/fixture churn; behavior is identical.
clippy (pedantic) and fmt clean.
Perf-neutral. Benchmarked main vs branch (criterion, bundle group); a main-vs-main control showed the apparent ~3% delta was a first-run warm-up artifact correlated with case order, not the code — no regression, no real speedup, as expected for a modeling change whose per-module path logic is cost-equivalent.

netlify · 2026-06-22T15:28:27Z

✅ Deploy Preview for rolldown-rs ready!

Name	Link
🔨 Latest commit	`8ed0b76`
🔍 Latest deploy log	https://app.netlify.com/projects/rolldown-rs/deploys/6a3b7ebab8a6600008c969bd
😎 Deploy Preview	https://deploy-preview-9927--rolldown-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54a11609e9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

codspeed-hq · 2026-06-22T15:55:48Z

Merging this PR will improve performance by 3.78%

⚡ 1 improved benchmark
✅ 6 untouched benchmarks
⏩ 10 skipped benchmarks¹

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`bundle@multi-duplicated-top-level-symbol`	262.1 ms	252.5 ms	+3.78%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing model-module-id (1b7cbe4) with main (7f03488)²}

10 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. ↩
No successful run was found on main (d1266c0) during the generation of this report, so 7f03488 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

IWANABETHATGUY · 2026-06-22T16:24:59Z

One behavioral subtlety worth confirming around the new is_in_node_modules():

crates/rolldown/src/stages/generate_stage/detect_ineffective_dynamic_imports.rs:29

!importer_id.is_in_node_modules() && pre_rendered_chunk.module_ids.contains(importer_id)

crates/rolldown_common/src/types/module_id.rs:135

pub fn is_in_node_modules(&self) -> bool {
  self.as_path().is_some_and(rolldown_std_utils::PathExt::is_in_node_modules)
}

For a Virtual/Bare id, as_path() is None, so the string is never scanned for a node_modules component — whereas the old code path-scanned the raw string unconditionally. The behavior therefore diverges only when a non-path importer id literally contains a node_modules component (e.g. a relative foo/node_modules/bar): old → true, new → false, which flips the !.

Since dynamic_importers hold resolved module ids (absolute paths → Path kind, scanned identically; or virtual → no node_modules), a relative id sitting in node_modules doesn't look reachable here, so this seems benign. Just flagging it so the short-circuit is intentional.

Boshen · 2026-06-23T08:00:15Z

@IWANABETHATGUY confirmed intentional — and good catch that the branch was untested.

dynamic_importers only ever hold resolved normal-module ids (importer_path = module.id(), set in module_loader.rs), which are either absolute filesystem paths (Path kind — scanned exactly as before) or virtual \0… ids (which never contain a node_modules path component). A relative id like foo/node_modules/bar isn't a resolved module id, so the diverging branch is unreachable in a real build. The path-gated check is also arguably more correct: it now means "a real path under node_modules", not "any string that happens to contain that substring".

On the coverage gap: the suppression branch has had no test since it was introduced in #8284 — the existing fixtures only exercise the positive-warning path with root-level importers, so nothing guarded the short-circuit either way. I've added tests/rolldown/warnings/dynamic_import_from_node_modules_no_warning: a module dynamically imported from within node_modules while also statically imported, with expectWarning: false. It passes with the guard and, with the guard removed, emits exactly the INEFFECTIVE_DYNAMIC_IMPORT warning — so the short-circuit is now locked in.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff0253ea81

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Boshen · 2026-06-23T09:01:00Z

Addressed both Codex review threads in bedba1345:

1. Stale module-id internal doc (internal-docs/module-id/implementation.md) — updated the ### ModuleId section to reflect the new classified repr: Path | Virtual | Bare (ArcStr) model, and clarified that equality/ordering/hashing remain raw string comparison over as_str() (so &str map lookups still work) while the classification only gates path logic.

2. sideEffects for non-absolute plugin-resolved ids (ecma_module_view_factory.rs) — good catch, this was a real behavioral regression. A plugin resolveId hook can return a virtual/bare id together with a packageJsonPath, so resolved_id.package_json is populated even when the id isn't a Path kind. The new as_path()? short-circuited the package sideEffects check for those ids. Restored the historical behavior by computing the relative path from the raw id string (resolved_id.id.as_str().relative(...) — exactly what the old unconditional as_path() did), so sideEffects: false is honored again for such plugin-resolved modules. The package_json (121) and side_effect (58) integration tests pass.

Boshen · 2026-06-23T13:40:06Z

@codex review

chatgpt-codex-connector · 2026-06-23T13:47:12Z

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 1b7cbe4074

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

IWANABETHATGUY · 2026-06-24T06:50:44Z

Merge activity

Jun 24, 6:50 AM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Jun 24, 6:50 AM UTC: IWANABETHATGUY added this pull request to the Graphite merge queue.
Jun 24, 6:57 AM UTC: Merged by the Graphite merge queue.

…l/Bare enum (#9927) ## Summary `ModuleId` was a `struct { inner: ArcStr }` that every call site re-interpreted as a filesystem path through `SugarPath::as_path()` — treating virtual (`\0…`), bare (`react`), and URL ids as paths, and round-tripping bytes we already know are UTF-8 through `to_string_lossy`. This remodels it as an enum classified **once** at construction: ```rust pub struct ModuleId { repr: Repr } // private enum Repr { Path | Virtual | Bare } ``` - **Gated path ops** — `as_path() -> Option<&Path>` (`Some` only for `Path`), plus `is_path()`, `is_virtual()`, `kind()`, `is_in_node_modules()`, `representative_name()`. Non-path ids are no longer silently path-parsed. - **`ArcStr` kept** as the backing store — cheap clone, durable cross-build identity, `literal!` const for the runtime module. `ModuleIdx` (the `u32` per-build handle) stays the hot graph key, so `ModuleId` optimizes for being a faithful string/path identity rather than for size/hash speed. - **Lossless** — every `to_string_lossy` on a `ModuleId` / `StableModuleId`-derived path becomes `to_str()` (module ids are always valid UTF-8). This is done in the shared `path_ext` helpers, so it also covers `ast_scanner` and preserve-modules naming. - **`Hash` / `Eq` / `Ord` / `Borrow<str>`** are hand-implemented over `as_str()` (byte-exact, ignoring the discriminant) so `&str` lookups into `module_id_to_idx` / `user_defined_entry` keep working. - **`StableModuleId::new`** branches on the cached `kind()` instead of re-running `is_absolute()` / `\0` detection. ## Why A module id is sometimes a path and sometimes not. Treating it uniformly as a path was wasteful (redundant UTF-8 scans, `to_string_lossy` on known-UTF-8 data) and a latent correctness hazard. Classifying once makes the path / not-path distinction explicit and confines path semantics to where they belong. ## Validation - `cargo test` — 1756 integration/snapshot tests pass with **zero snapshot/fixture churn**; behavior is identical. - `clippy` (pedantic) and `fmt` clean. - **Perf-neutral.** Benchmarked main vs branch (criterion, `bundle` group); a main-vs-main control showed the apparent ~3% delta was a first-run warm-up artifact correlated with case order, not the code — no regression, no real speedup, as expected for a modeling change whose per-module path logic is cost-equivalent.

@shulaoda

## [1.1.3] - 2026-06-24 ### 🐛 Bug Fixes - `defer_drop` crashes the browser main thread (#9942) by @shulaoda - camel-case: correct camel case for nested values (#9933) by @kb019 - cli: display --help options in camelCase (#9941) by @IWANABETHATGUY - preserve used re-exports under preserveModules (#9122) (#9934) by @IWANABETHATGUY - watch: make close reentrant in event callbacks (#9904) by @hyf0 - git for windows treats symlink files as regular files (#9915) by @AliceLanniste - dev: cancel pending full reload on build error (#9903) by @h-a-n-a - chunking: pass plugin meta to codeSplitting groups name function (#9267) by @Kyujenius - dev: serve assets emitted during HMR/lazy compile (vite#22596) (#9815) by @h-a-n-a - release: dry-run step no longer publishes binding packages (#9866) by @Boshen ### 🚜 Refactor - rolldown_common: model ModuleId as a classified Path/Virtual/Bare enum (#9927) by @Boshen - remove unused LegacyModuleIdx (#9872) by @shulaoda - remove unused StmtInfos::get_namespace_stmt_info (#9870) by @shulaoda - remove unused Module::as_external_mut (#9871) by @shulaoda - remove unused EcmaAst::is_body_empty (#9869) by @shulaoda - drop dead is_css_module handling in resolve_dependencies (#9867) by @shulaoda - drop redundant with_commonjs on cjs source type (#9868) by @shulaoda ### 📚 Documentation - clarify on drafting PRs (#9952) by @h-a-n-a - update contribution guidelines (#9944) by @fubhy - note Rust crates don't follow semver in AGENTS.md (#9905) by @IWANABETHATGUY - add feedback form (#9159) by @TheAlexLichter ### ⚡ Performance - utils: avoid allocation in default_sanitize_file_name for clean names (#9928) by @Boshen - binding: box once-per-build futures before spawn_future (#9864) by @Boshen - utils: avoid wasted allocation in legitimize_identifier_name (#9926) by @Boshen - rolldown: fuse the canonical-name dedup and insert in the renamer (#9900) by @Boshen - rolldown: probe the name map once in ConflictResolver::resolve (#9899) by @Boshen - cut two heap allocations from wrapped ESM init finalize (#9901) by @Boshen - rolldown_plugin_vite_reporter: hoist invariant out_dir prefix out of reporter loop (#9873) by @shulaoda - drop throwaway Vec in wrapped esm init stmt (#9878) by @shulaoda - borrow owner_filename in build-import-analysis AddDeps (#9874) by @shulaoda ### 🧪 Testing - cover preserveModules named export via namespace re-export (#6010) (#9937) by @IWANABETHATGUY ### ⚙️ Miscellaneous Tasks - deps: update napi to v3.9.4 (#9954) by @shulaoda - reduce noise from CODEOWNERS for trival changes (#9953) by @h-a-n-a - deps: update mimalloc-safe to 0.1.64 (#9950) by @shulaoda - deps: update rollup submodule for tests to v4.62.2 (#9931) by @rolldown-guard[bot] - deps: test mimalloc-safe upstream-mimalloc switch in CI (#9930) by @shulaoda - rolldown_plugin_vite_build_import_analysis: remove unused v2 code path (#9917) by @shulaoda - rolldown_plugin_vite_manifest: remove unused is_enable_v2 code path (#9916) by @shulaoda - rolldown_plugin_vite_asset_import_meta_url: remove unexposed native vite plugin (#9896) by @shulaoda - rolldown_plugin_vite_asset: remove unexposed native vite plugin (#9895) by @shulaoda - rolldown_plugin_vite_css_post: remove unexposed native vite plugin (#9894) by @shulaoda - rolldown_plugin_vite_css: remove unexposed native vite plugin (#9893) by @shulaoda - rolldown_plugin_vite_html_inline_proxy: remove unexposed native vite plugin (#9892) by @shulaoda - rolldown_plugin_vite_html: remove unexposed native vite plugin (#9891) by @shulaoda - deps: update github actions (#9909) by @renovate[bot] - deps: update rust crate oxc_sourcemap to v8.0.2 (#9910) by @renovate[bot] - deps: update npm packages (#9912) by @renovate[bot] - deps: update github actions to v7 (#9913) by @renovate[bot] - deps: update rolldown-plugin-dts to ^0.26.0 (#9897) by @renovate[bot] - remove rolldown_filter_analyzer crate (#9865) by @Boshen ### ❤️ New Contributors * @fubhy made their first contribution in [#9944](#9944) Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>

Boshen force-pushed the model-module-id branch from bff7218 to 54a1160 Compare June 22, 2026 15:33

Boshen marked this pull request as ready for review June 22, 2026 15:48

Boshen requested review from IWANABETHATGUY, hyf0 and shulaoda as code owners June 22, 2026 15:48

Boshen assigned IWANABETHATGUY and hyf0 Jun 22, 2026

chatgpt-codex-connector Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread crates/rolldown_common/src/types/module_id.rs

Boshen force-pushed the model-module-id branch from 54a1160 to ff0253e Compare June 23, 2026 07:59

chatgpt-codex-connector Bot reviewed Jun 23, 2026

View reviewed changes

Comment thread crates/rolldown/src/ecmascript/ecma_module_view_factory.rs Outdated

Boshen force-pushed the model-module-id branch from ff0253e to bedba13 Compare June 23, 2026 09:00

Boshen requested a review from h-a-n-a as a code owner June 23, 2026 09:00

Boshen force-pushed the model-module-id branch from 8b12ecb to 1b7cbe4 Compare June 23, 2026 11:17

IWANABETHATGUY approved these changes Jun 23, 2026

View reviewed changes

hyf0 approved these changes Jun 23, 2026

View reviewed changes

IWANABETHATGUY added the graphite: merge-when-ready label Jun 24, 2026

graphite-app Bot force-pushed the model-module-id branch from 1b7cbe4 to 8ed0b76 Compare June 24, 2026 06:52

graphite-app Bot merged commit 8ed0b76 into main Jun 24, 2026
34 checks passed

graphite-app Bot removed the graphite: merge-when-ready label Jun 24, 2026

graphite-app Bot deleted the model-module-id branch June 24, 2026 06:57

rolldown-guard Bot mentioned this pull request Jun 24, 2026

release: v1.1.3 #9958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(rolldown_common): model ModuleId as a classified Path/Virtual/Bare enum#9927

refactor(rolldown_common): model ModuleId as a classified Path/Virtual/Bare enum#9927
graphite-app[bot] merged 1 commit into
mainfrom
model-module-id

Boshen commented Jun 22, 2026

Uh oh!

netlify Bot commented Jun 22, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

codspeed-hq Bot commented Jun 22, 2026 •

edited

Loading

Uh oh!

IWANABETHATGUY commented Jun 22, 2026

Uh oh!

Boshen commented Jun 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Boshen commented Jun 23, 2026

Uh oh!

Boshen commented Jun 23, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 23, 2026

Uh oh!

IWANABETHATGUY commented Jun 24, 2026 •

edited by graphite-app Bot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Boshen commented Jun 22, 2026

Summary

Why

Validation

Uh oh!

netlify Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs ready!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

codspeed-hq Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will improve performance by 3.78%

Performance Changes

Footnotes

Uh oh!

IWANABETHATGUY commented Jun 22, 2026

Uh oh!

Boshen commented Jun 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Boshen commented Jun 23, 2026

Uh oh!

Boshen commented Jun 23, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jun 23, 2026

Uh oh!

IWANABETHATGUY commented Jun 24, 2026 • edited by graphite-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify Bot commented Jun 22, 2026 •

edited

Loading

codspeed-hq Bot commented Jun 22, 2026 •

edited

Loading

IWANABETHATGUY commented Jun 24, 2026 •

edited by graphite-app Bot

Loading