Skip to content

Fix capacity miscount in ESMConditions.init #30692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 3 commits into from
Closed

Fix capacity miscount in ESMConditions.init #30692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e2c22c
Fix capacity miscount in ESMConditions.init when allow_addons is true
robobun May 14, 2026
e3e0878
Add runtime --conditions regression test for #30619
robobun May 14, 2026
28128c6
Also fix if/else precedence in loadersFromTransformOptions capacity hint
robobun May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/bundler/options.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1572,9 +1572,9 @@
bun.StringArrayHashMap(Loader),
allocator,
input_loaders.extensions.len +
if (target.isBun()) default_loader_ext_bun.len else 0 +
if (target == .browser) default_loader_ext_browser.len else 0 +
default_loader_ext.len,
(if (target.isBun()) default_loader_ext_bun.len else 0) +
(if (target == .browser) default_loader_ext_browser.len else 0) +
default_loader_ext.len,

Check failure on line 1577 in src/bundler/options.zig

View check run for this annotation

Claude / Claude Code Review

options.zig edit is a no-op: file is not compiled, and ESMConditions.init wasn't even changed 

This PR's only source change is to `src/bundler/options.zig`, but per `src/CLAUDE.md` the `.zig` files are non-compiled porting references — the shipped binary is built from `src/bundler/options.rs`, where `ESMConditions::init` already hoists `addon_extra` and uses growing `.insert()`, so the bug described in the PR body cannot occur there. On top of that, commit 28128c64 reverted the `ESMConditions.init` hunk from 1e2c22c7, so the net diff doesn't even touch that function — only the cosmetic pa
Comment on lines +1575 to +1577

@claude claude Bot May 14, 2026

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.

🔴 This PR's only source change is to src/bundler/options.zig, but per src/CLAUDE.md the .zig files are non-compiled porting references — the shipped binary is built from src/bundler/options.rs, where ESMConditions::init already hoists addon_extra and uses growing .insert(), so the bug described in the PR body cannot occur there. On top of that, commit 28128c6 reverted the ESMConditions.init hunk from 1e2c22c, so the net diff doesn't even touch that function — only the cosmetic parenthesization of loadersFromTransformOptions (which uses getOrPutValue and was never UB) survives. Net effect: the .zig edit changes nothing in the built binary and the title/description are misleading; only the two new regression tests add value. Please verify whether #30619 still reproduces on main (post-Rust-rewrite) — if not, retitle as a tests-only PR; if it does, the fix needs to go in .rs.

Extended reasoning...

What this PR actually changes

The PR title is "Fix capacity miscount in ESMConditions.init" and the description claims to fix #30619 by hoisting if (allow_addons) 1 else 0 into a typed local. But the net diff against base for src/bundler/options.zig shows zero changes to ESMConditions.init — only loadersFromTransformOptions (lines 1574-1577) is modified, adding parentheses around two if/else expressions in a capacity hint.

This happened because commit 28128c6 ("Also fix if/else precedence in loadersFromTransformOptions capacity hint") accidentally reverted the ESMConditions.init fix from 1e2c22c. git show 28128c64 -- src/bundler/options.zig shows it removing const addons_extra: usize = if (allow_addons) 1 else 0; and restoring the original buggy expression:

-        const addons_extra: usize = if (allow_addons) 1 else 0;
-        try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + addons_extra + conditions.len);
+        try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len);

At HEAD, src/bundler/options.zig:1090-1092 still reads the unparenthesized form, which Zig parses as defaults.len + 2 + (if (allow_addons) 1 else (0 + conditions.len)).

Why none of this affects the binary

More fundamentally, src/CLAUDE.md (lines 15-17) states:

You will see .zig siblings next to many .rs files — those are the original implementation kept as a porting reference for behavior; they are not compiled and are not where new code goes.

The base of this PR is one commit after 23427db "Rewrite Bun in Rust (#30412)". There is no top-level build.zig, and all CI build failures in this PR's timeline are in .rs files (Blob.rs, server/mod.rs, h2_frame_parser.rs, JSSecrets.rs, VirtualMachine.rs, process.rs) — there is no Zig build step. The compiled implementation is src/bundler/options.rs.

Inspecting src/bundler/options.rs:832-835, the Rust port of ESMConditions::init already computes capacity correctly:

let addon_extra = if allow_addons { 1 } else { 0 };
default_condition_amp.reserve(defaults.len() + 2 + addon_extra + conditions.len());

and at line ~838 it uses .insert() (which grows on demand) rather than an assume-capacity variant — the comment // PERF(port): was assume_capacity confirms this was intentionally changed during the port. So even if the reserve hint were wrong, the Rust code could not exhibit the putAssumeCapacity overflow / debug assert described in the PR body. Similarly, loaders_from_transform_options in options.rs already sums its terms correctly (Rust if/else with braces doesn't have Zig's precedence trap).

Step-by-step proof

  1. Net source diff: git diff 19d8ade2..HEAD -- src/bundler/options.zig touches only lines 1574-1577 in loadersFromTransformOptions. ESMConditions.init is byte-identical to base.
  2. Surviving change is cosmetic: loadersFromTransformOptions calls stringHashMapFromArrays (which only putAssumeCapacity's input_loaders.extensions — always ≤ the hint regardless of precedence) and then uses getOrPutValue for the default extensions, which grows the map safely. The added parentheses fix the hint arithmetic but there was never UB here.
  3. File is not compiled: src/bundler/options.zig is a reference-only sibling of src/bundler/options.rs per project conventions. Editing it cannot change the built binary.
  4. Rust path is already correct: ESMConditions::init in options.rs already hoists addon_extra and uses growing .insert(). With 12 user conditions and allow_addons=true, it reserves 2 + 2 + 1 + 12 = 17 and inserts via .insert() — no overflow possible.
  5. Therefore: the two new regression tests should pass on main with or without this PR's .zig edit, because they exercise the Rust runtime. The .zig change has zero runtime effect.

Impact and how to fix

The PR as written does not deliver what its title and "Fixes #30619" claim. The actionable outcomes are:

input_loaders.extensions,
loader_values,
);
Expand Down
13 changes: 13 additions & 0 deletions test/bundler/bun-build-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,19 @@ describe("Bun.build", () => {
expect(await bundle.outputs[0].text()).toBe("var o=/*@__PURE__*/console.log(1);export{o as OUT};\n");
});

test.concurrent("many user conditions does not crash", async () => {
const fixture = tempDirWithFiles("build-many-conditions", {
"entry.ts": `export const x = 1;`,
});

const bundle = await Bun.build({
entrypoints: [join(fixture, "entry.ts")],
target: "bun",
conditions: ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l"],
});
expect(bundle.success).toBe(true);
});

test.concurrent(
"you can write onLoad and onResolve plugins using the 'html' loader, and it includes script and link tags as bundled entrypoints",
async () => {
Expand Down
19 changes: 19 additions & 0 deletions test/js/bun/resolve/import-custom-condition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,22 @@ it("custom condition when don't match condition should resolves to default", asy

expect(exitCode).toBe(1);
});

// https://github.com/oven-sh/bun/issues/30619
it("many custom conditions resolve correctly", async () => {
for (const n of [4, 5, 6, 8, 12, 20]) {
const flags = [];
for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
flags.push("--conditions=first");
const { exitCode, stdout, stderr } = Bun.spawnSync({
cmd: [bunExe(), ...flags, `${dir}/test.js`],
env: bunEnv,
cwd: import.meta.dir,
});
expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
n,
exitCode: 0,
stdout: "1\n",
});
}
});
Comment on lines +172 to +188

@coderabbitai coderabbitai Bot May 14, 2026

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.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Use parameterized test cases instead of a manual loop.

This should be split into parameterized cases so each n reports independently and can run concurrently across the matrix.

♻️ Suggested rewrite 
-it("many custom conditions resolve correctly", async () => {
-  for (const n of [4, 5, 6, 8, 12, 20]) {
-    const flags = [];
-    for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
-    flags.push("--conditions=first");
-    const { exitCode, stdout, stderr } = Bun.spawnSync({
-      cmd: [bunExe(), ...flags, `${dir}/test.js`],
-      env: bunEnv,
-      cwd: import.meta.dir,
-    });
-    expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
-      n,
-      exitCode: 0,
-      stdout: "1\n",
-    });
-  }
-});
+it.concurrent.each([4, 5, 6, 8, 12, 20])("many custom conditions resolve correctly (n=%i)", async n => {
+  const flags = [];
+  for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
+  flags.push("--conditions=first");
+
+  const { exitCode, stdout, stderr } = Bun.spawnSync({
+    cmd: [bunExe(), ...flags, `${dir}/test.js`],
+    env: bunEnv,
+    cwd: import.meta.dir,
+  });
+
+  expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
+    n,
+    exitCode: 0,
+    stdout: "1\n",
+  });
+});

Based on learnings, in test/js/bun/**/*.test.ts, prefer test.each()/test.concurrent.each() over manual loops when each parameter value maps to a single assertion.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("many custom conditions resolve correctly", async () => {
for (const n of [4, 5, 6, 8, 12, 20]) {
const flags = [];
for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
flags.push("--conditions=first");
const { exitCode, stdout, stderr } = Bun.spawnSync({
cmd: [bunExe(), ...flags, `${dir}/test.js`],
env: bunEnv,
cwd: import.meta.dir,
});
expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
n,
exitCode: 0,
stdout: "1\n",
});
}
});
it.concurrent.each([4, 5, 6, 8, 12, 20])("many custom conditions resolve correctly (n=%i)", async n => {
const flags = [];
for (let i = 1; i < n; i++) flags.push(`--conditions=c${i}`);
flags.push("--conditions=first");
const { exitCode, stdout, stderr } = Bun.spawnSync({
cmd: [bunExe(), ...flags, `${dir}/test.js`],
env: bunEnv,
cwd: import.meta.dir,
});
expect({ n, exitCode, stdout: stdout.toString("utf8"), stderr: stderr.toString("utf8") }).toMatchObject({
n,
exitCode: 0,
stdout: "1\n",
});
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/bun/resolve/import-custom-condition.test.ts` around lines 172 - 188,
Replace the manual for-loop inside the "many custom conditions resolve
correctly" test with a parameterized test so each `n` is its own case: convert
the current it(...) block in import-custom-condition.test.ts to use
test.concurrent.each([4,5,6,8,12,20]) (or test.each if concurrency is undesired)
and move the body that builds `flags`, calls `Bun.spawnSync({...})`, and asserts
the result into the per-case callback; keep the same `flags` construction logic,
same `Bun.spawnSync` call and the expect comparing `{ n, exitCode, stdout,
stderr }` to the expected object, but run it once per `n` so failures report
independently.

Loading