Skip to content

bake: give each DevServer module its own renamer scope - #39375

Open
undeemed wants to merge 1 commit into
oven-sh:mainfrom
undeemed:claude/dev-server-per-file-renamer
Open

bake: give each DevServer module its own renamer scope#39375
undeemed wants to merge 1 commit into
oven-sh:mainfrom
undeemed:claude/dev-server-per-file-renamer

Conversation

@undeemed

Copy link
Copy Markdown

What does this PR do?

Fixes #18017. In DevServer, two modules that both declare a top-level App ended up with the second one renamed to App2, so App.name came back as "App2". The same thing happens to module, which is why the screenshot on the issue shows a numbered module.

DevServer prints every module inside its own closure - generateCodeForFileInChunkJS.rs:72-174 wraps each file in (hmr, module, exports) => { ... } - but rename_symbols_in_chunk still pushed every file's top-level symbols into the single chunk-level NumberScope through add_top_level_declared_symbols. Two files declaring the same name therefore collided at the chunk root, even though neither name is actually at the chunk's top level.

For Format::InternalBakeDev, each file now gets its own NumberScope parented to the chunk root. Its live parts' top-level symbols and their nested scopes are numbered inside it. Nothing can collide with a name already taken at the chunk root, because NumberScope lookup walks the parent chain.

The branch predicate is deliberately the same one the codegen uses (is_dev_server && !source_index.is_runtime()). The runtime file stays on the chunk-root path because it prints unwrapped, and it is only in the chunk at all under bun build --format=internal_bake_dev. Everything else is untouched: the new code sits behind output_format == Format::InternalBakeDev, so production bundling runs exactly the path it ran before.

Two things worth a second look in review:

  • The file scope is seeded from part.declared_symbols, not from the module scope the way WrapKind::Cjs does it. React Fast Refresh temp refs (var _s = $RefreshSig$()) are registered on the enclosing function scope but print at the file's top level, and part.declared_symbols is the only thing that carries them to the renamer. Seeding from the module scope instead drops them and two components in one file both end up as _s. test/bake/dev/react-spa.test.ts catches that.
  • The three new methods on NumberRenamer exist because NumberScope::{parent, name_counts} and assign_name are pub(crate) to bun_js_printer, so the bundler cannot build a child scope or write into one itself.

One existing test needed updating. test/bake/dev/server-sourcemap.test.ts asserted the buggy name, doSomething2, and the column that went with it. It now asserts doSomething. The frame still resolves to the same file and the same line, only the column moves, 28 to 31, because the identifier is a character shorter so the frame's generated column lands in the neighbouring mapping segment. In export function doSomething() {, 28 is the ( and 31 is the {.

This is the // TODO: instead of running a renamer per chunk, run it per file at generateChunksInParallel.rs:61 that the issue points at, scoped to DevServer only.

How did you verify your code works?

Debug build (bun bd) on Linux x64 throughout. Steps 3 and 5 ran with the branch rebased onto c3995e43d5; steps 2 and 4 ran two commits earlier at 8326d1bd39, and the two commits in between are a WebKit bump and a CI allowlist revert that touch neither the bundler nor test/bake.

1. Reproduced it first on the released bun. Two files that each export function App:

// a.js
import { App as Other } from "./other";
export function App() {}
console.log(App.name, Other.name);

// other.js
export function App() {}
$ bun --version
1.3.14
$ bun build --format=internal_bake_dev a.js | grep -E 'function App|hmr, module'
  ], [], (hmr, module, exports) => {
    function App() {}
  ], [], (hmr, module2, exports) => {
    function App2() {}

Same command on this branch:

$ bun bd build --format=internal_bake_dev a.js | grep -E 'function App|hmr, module'
  ], [], (hmr, module, exports) => {
    function App() {}
  ], [], (hmr, module, exports) => {
    function App() {}

2. The new test fails without the fix. It runs those two modules through a real DevServer client and asserts App.name + " " + Other.name === "App App". With the test applied but src/ reverted to main:

$ bun bd test test/bake/dev/bundle.test.ts
  [
-   "App App",
+   "App App2",
  ]
(fail)  DEV:bundle-3: same top-level name in two modules keeps Function.prototype.name (#18017)
 21 pass
 1 fail

3. Targeted runs with the fix applied.

$ bun bd test test/bake/dev/bundle.test.ts test/bake/dev/server-sourcemap.test.ts \
      test/bake/dev/react-spa.test.ts test/bundler/bundler_loader.test.ts
 91 pass
 0 fail
Ran 91 tests across 4 files. [252.92s]

bundler_loader.test.ts is in there for its internal_bake_dev lazy exports block, and react-spa.test.ts for React Fast Refresh. Both are 0 fail on main too (58 pass and 6 pass respectively).

4. Full test/bake/ suite, main vs this branch.

main         180 pass, 10 fail, 190 tests
this branch  181 pass, 10 fail, 191 tests

The two failure sets are identical, name for name. All 10 are production > ... in dev-and-prod.test.ts, and every one of them is the 5000 ms default test timeout on a debug build on this machine, not an assertion.

5. Lints.

$ cargo fmt --all --check      # clean
$ bun run lint                 # 0 warnings, 0 errors
$ bun test test/internal/source-lints/    # 170 pass, 0 fail
$ prettier --check test/bake/dev/bundle.test.ts test/bake/dev/server-sourcemap.test.ts   # clean
$ bun run rust:clippy          # 0 warnings, 0 errors

DevServer prints every module inside its own closure, but
rename_symbols_in_chunk still pushed every file's top-level symbols into
the single chunk-level NumberScope. Two files declaring the same
top-level name collided at the chunk root, so the second one came out as
`App2` and `App.name` returned "App2".

For Format::InternalBakeDev, each non-runtime file now gets its own
NumberScope parented to the chunk root, seeded from its live parts'
declared symbols. Name lookup still walks the parent chain, so nothing
can collide with a name already taken at the chunk root.

Fixes oven-sh#18017

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ccbc47eb-0e66-4203-96b4-2ff3d46a5598

📥 Commits

Reviewing files that changed from the base of the PR and between c3995e4 and 0499e7c.

📒 Files selected for processing (4)
  • src/bundler/linker_context/renameSymbolsInChunk.rs
  • src/js_printer/renamer.rs
  • test/bake/dev/bundle.test.ts
  • test/bake/dev/server-sourcemap.test.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.


Walkthrough

Changes

Dev-server symbol scopes

Layer / File(s) Summary
NumberRenamer scope management
src/js_printer/renamer.rs
NumberRenamer can allocate child scopes, assign declared symbols, and return scopes to the pool.
Per-module dev-server renaming and validation
src/bundler/linker_context/renameSymbolsInChunk.rs, test/bake/dev/bundle.test.ts, test/bake/dev/server-sourcemap.test.ts
Non-runtime development-server modules use isolated scopes. Tests verify that duplicate top-level App functions retain their names and update source-map expectations.

Possibly related PRs

  • oven-sh/bun#34361: Both PRs modify symbol renaming and add DevServer bundling tests, but this PR addresses scoped naming while that PR reserves restricted identifiers.

Suggested reviewers: robobun, jarred-sumner

Merge Risk: ⚪ Minimal · up to 0499e

This localized DevServer renaming change is supported by targeted tests and lint checks, with no actionable merge-blocking risk remaining.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: separate renamer scopes for DevServer modules.
Description check ✅ Passed The description explains the bug, implementation, scope, regression coverage, and verification results in the required sections.
Linked Issues check ✅ Passed The changes address [#18017] by preserving duplicate component names across DevServer modules, including the reported App.name behavior.
Out of Scope Changes check ✅ Passed The source changes, tests, and sourcemap update are directly related to the DevServer renaming fix and its regression coverage.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HMR suffixes component function names with a number

1 participant