Skip to content

bundler: fix ESMConditions capacity miscomputation with custom conditions #30535

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

bundler: fix ESMConditions capacity miscomputation with custom conditions #30535

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
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 @@ -1087,9 +1087,9 @@
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);
try default_condition_amp.ensureTotalCapacity(defaults.len + 2 + @as(usize, if (allow_addons) 1 else 0) + conditions.len);
try import_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, if (allow_addons) 1 else 0) + conditions.len);
try require_condition_map.ensureTotalCapacity(defaults.len + 2 + @as(usize, if (allow_addons) 1 else 0) + conditions.len);

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

View check run for this annotation

Claude / Claude Code Review

Same if-else precedence footgun remains in loadersFromTransformOptions 

Nit (pre-existing): the same if-else precedence footgun exists ~480 lines down in `loadersFromTransformOptions`'s `total_capacity` argument — when `target.isBun()` is true, `default_loader_ext.len` (and the browser term) get absorbed into the `else` branch and dropped. It's harmless there since only the user-provided extensions use `putAssumeCapacity` and the defaults use `getOrPutValue` which grows the map, so it's just a missed pre-sizing — but since this PR is specifically about this pattern
Comment on lines +1090 to +1092

@claude claude Bot May 12, 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): the same if-else precedence footgun exists ~480 lines down in loadersFromTransformOptions's total_capacity argument — when target.isBun() is true, default_loader_ext.len (and the browser term) get absorbed into the else branch and dropped. It's harmless there since only the user-provided extensions use putAssumeCapacity and the defaults use getOrPutValue which grows the map, so it's just a missed pre-sizing — but since this PR is specifically about this pattern in this file, you may want to give it the same @as(usize, ...) treatment.

Extended reasoning...

Summary

This PR fixes the Zig if-else precedence bug in ESMConditions.init where A + if (cond) 1 else 0 + B parses as A + (if (cond) 1 else (0 + B)). However, the identical anti-pattern still exists in the same file in loadersFromTransformOptions (around src/bundler/options.zig:1574-1577):

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,

How it parses

Because Zig's if-else binds tighter on the else-branch than the surrounding +, the trailing additions are absorbed into the preceding else. So when target.isBun() is true, the whole expression evaluates to just input_loaders.extensions.len + default_loader_ext_bun.len (= extensions.len + 2), silently dropping both default_loader_ext.len (16) and the browser term. When target.isBun() is false and target == .browser, it evaluates to extensions.len + 0 + default_loader_ext_browser.len (= extensions.len + 1), again dropping default_loader_ext.len. Only the fall-through case (else 0 + else 0 + default_loader_ext.len) yields the +16 the author intended as the baseline.

Why it isn't a correctness bug here

Unlike ESMConditions.init, this miscomputation cannot corrupt memory or assert:

  • stringHashMapFromArrays (src/bundler/options.zig:41-51) only calls putAssumeCapacity for the user-provided keys (input_loaders.extensions). The miscomputed total_capacity is always >= input_loaders.extensions.len because that term is unconditional and first, and every if-branch contributes a non-negative addend. So the putAssumeCapacity loop is always covered.
  • The subsequent default-extension inserts (default_loader_ext, default_loader_ext_bun, default_loader_ext_browser) all use try loaders.getOrPutValue(...), which grows the map on demand.

So the only effect is a missed pre-sizing optimization — the map will rehash a couple of times during the default-extension inserts instead of being sized up-front.

Step-by-step example

Take target = .bun, input_loaders.extensions.len = 0:

  1. Author intent: 0 + 2 (bun) + 0 (browser) + 16 (default) = 18.
  2. Actual parse: outer if takes the then branch → 0 + default_loader_ext_bun.len = 0 + 2 = 2. The entire else 0 + if (...) ... + default_loader_ext.len tail is the unevaluated else-branch.
  3. stringHashMapFromArrays is called with total_capacity = 2 and keys.len = 0, so ensureTotalCapacity isn't even called (gated on keys.len > 0).
  4. The 16 getOrPutValue calls for default_loader_ext then grow the map from empty — correct, just not pre-sized.

Suggested fix

Apply the same treatment as in this PR:

input_loaders.extensions.len +
    @as(usize, if (target.isBun()) default_loader_ext_bun.len else 0) +
    @as(usize, if (target == .browser) default_loader_ext_browser.len else 0) +
    default_loader_ext.len,

This is pre-existing and nit-level — it has no correctness impact and shouldn't block the PR. It's only worth mentioning because the PR's entire purpose is fixing this exact precedence footgun in this exact file, so applying it consistently removes a known-misleading capacity computation.

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

import_condition_map.putAssumeCapacity("import", {});
Expand Down
15 changes: 15 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 @@ -649,6 +649,21 @@ describe("Bun.build", () => {
expect(await html?.text()).toContain("<meta name='injected-by-plugin' content='true'>");
},
);

test.concurrent("many custom conditions does not crash", async () => {
// The capacity reserved for the ESM conditions map was miscomputed when
// node-addons were enabled (the default), so enough custom conditions
// would overflow the reserved capacity and assert/corrupt.
const dir = tempDirWithFiles("bun-build-conditions", {
"entry.ts": `export const x = 1;`,
});
const build = await Bun.build({
entrypoints: [join(dir, "entry.ts")],
conditions: ["a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l"],
});
expect(build.success).toBe(true);
expect(build.outputs).toHaveLength(1);
});
});

test.concurrent("macro with nested object", async () => {
Expand Down
Loading