Skip to content

Fix capacity miscalculation in ESMConditions.init #30604

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 1 commit into from
Closed

Fix capacity miscalculation in ESMConditions.init #30604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions src/bundler/options.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1087,9 +1087,10 @@
var require_condition_map = ConditionsMap.init(allocator);
var style_condition_map = ConditionsMap.init(allocator);

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

Check notice on line 1093 in src/bundler/options.zig

View check run for this annotation

Claude / Claude Code Review

Same if-else precedence pattern remains in loadersFromTransformOptions 

Nit (pre-existing, not introduced here): the same if-else precedence trap also lives ~480 lines down in `loadersFromTransformOptions` — `input_loaders.extensions.len + if (target.isBun()) ... else 0 + if (...) ... else 0 + default_loader_ext.len`. That one is harmless because the subsequent inserts use `getOrPutValue` (which grows), so it just under-reserves and triggers a couple of extra reallocations rather than overflowing. But since it's the identical root cause in the same file, you may wan
Comment on lines +1090 to +1093

@claude claude Bot May 13, 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.

🟣 Nit (pre-existing, not introduced here): the same if-else precedence trap also lives ~480 lines down in loadersFromTransformOptions — input_loaders.extensions.len + if (target.isBun()) ... else 0 + if (...) ... else 0 + default_loader_ext.len. That one is harmless because the subsequent inserts use getOrPutValue (which grows), so it just under-reserves and triggers a couple of extra reallocations rather than overflowing. But since it's the identical root cause in the same file, you may want to give it the same const x: usize = if (...) ... else 0; treatment while you're here.

Extended reasoning...

What the issue is

This PR correctly fixes the Zig if-else precedence bug in ESMConditions.init, where if (allow_addons) 1 else 0 + conditions.len parses as if (allow_addons) 1 else (0 + conditions.len). However, the exact same pattern exists further down in the same file, in loadersFromTransformOptions:

var loaders = try stringHashMapFromArrays(
    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,
    input_loaders.extensions,
    loader_values,
);

In Zig, the else-branch of an if expression greedily consumes a full expression. So a + if (c) x else 0 + y parses as a + (if (c) x else (0 + y)), not (a + (if (c) x else 0)) + y. The chained ifs here therefore do not produce the sum the indentation suggests.

Step-by-step proof

Take target = .bun (so target.isBun() is true and target == .browser is false), with input_loaders.extensions.len = 3:

  1. The inner if (target == .browser) default_loader_ext_browser.len else 0 + default_loader_ext.len parses with the else-branch consuming 0 + default_loader_ext.len. Since the condition is false, it evaluates to 0 + 16 = 16.
  2. The outer if (target.isBun()) default_loader_ext_bun.len else 0 + (16) — condition is true, so it evaluates to default_loader_ext_bun.len = 2. The else-branch (0 + 16) is discarded entirely.
  3. total_capacity = input_loaders.extensions.len + 2 = 5.

The intended value was 3 + 2 + 0 + 16 = 21. So the map is reserved for 5 entries instead of 21.

Why it doesn't crash (unlike the ESMConditions case)

  • stringHashMapFromArrays only does putAssumeCapacity for keys.len items (the user-supplied extensions). Since extensions.len is always a term in the sum regardless of which if-branch is taken, the reserved capacity is always >= extensions.len, so those inserts never overflow.
  • The subsequent default_loader_ext / default_loader_ext_bun / default_loader_ext_browser inserts all use getOrPutValue, which grows the map on demand.
  • Additionally, when input_loaders.extensions.len == 0 (no custom loaders), stringHashMapFromArrays skips ensureTotalCapacity entirely, so the reservation hint is ignored anyway.

So the only impact is a few unnecessary reallocations during build setup when custom loaders are passed — no correctness or safety issue.

Why mention it

It's the identical root cause this PR is fixing (Zig if-else precedence in a capacity calculation), in the same file, and the same fix style applies cleanly:

const bun_count: usize = if (target.isBun()) default_loader_ext_bun.len else 0;
const browser_count: usize = if (target == .browser) default_loader_ext_browser.len else 0;
... input_loaders.extensions.len + bun_count + browser_count + default_loader_ext.len ...

This is pre-existing — the PR doesn't touch, call, or otherwise interact with loadersFromTransformOptions — so it's purely a non-blocking "while you're here" suggestion, not something that should hold up the merge.

try style_condition_map.ensureTotalCapacity(defaults.len + 2 + conditions.len);

import_condition_map.putAssumeCapacity("import", {});
Expand Down
14 changes: 14 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 @@ -682,6 +682,20 @@ export function testMacro(val: any) {
);
});

test.concurrent("Bun.build does not crash with many conditions", async () => {
const dir = tempDirWithFilesAnon({
"entry.js": `console.log("hi");`,
});
for (let n = 0; n <= 16; n++) {
const conditions = Array.from({ length: n }, (_, i) => `cond${i}`);
const result = await Bun.build({
entrypoints: [join(dir, "entry.js")],
conditions,
});
expect(result.success).toBe(true);
}
});

// Since NODE_PATH has to be set, we need to run this test outside the bundler tests.
test.concurrent("regression/NODE_PATHBuild api", async () => {
const dir = tempDirWithFiles("node-path-build", {
Expand Down
Loading