bundler: fix ESMConditions capacity with user-provided conditions - #30470
bundler: fix ESMConditions capacity with user-provided conditions#30470robobun wants to merge 1 commit into
Conversation
The expression `if (allow_addons) 1 else 0 + conditions.len` parses as `if (allow_addons) 1 else (0 + conditions.len)`, so when allow_addons is true the user-provided conditions were not counted towards the reserved capacity. With enough custom conditions this tripped the addOneAssumeCapacity assertion in debug builds and wrote past the allocation in release builds.
WalkthroughThis PR refactors capacity preallocation in ChangesBundler Conditions Capacity Optimization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/bundler/bun-build-api.test.ts`:
- Around line 819-830: Replace the nested for-loops that iterate over target and
n with parameterized test cases using describe.each or test.each so each
(target, n) combo is reported independently; specifically, create a table/array
of tuples for the matrix of targets ["bun","browser","node"] and ns
[1,4,6,8,12,20], then use describe.each or test.each to iterate those tuples and
inside each test call Bun.build with the same entrypoints and compute conditions
= Array.from({ length: n }, (_, i) => `cond${i}`) and assert build.success;
remove the outer for (const target ...) and inner for (const n ...) loops and
replace them with the parameterized test harness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf7fe81a-c1ba-4954-aa3b-e4388fdf9285
📒 Files selected for processing (2)
src/bundler/options.zigtest/bundler/bun-build-api.test.ts
| for (const target of ["bun", "browser", "node"] as const) { | ||
| for (const n of [1, 4, 6, 8, 12, 20]) { | ||
| const conditions = Array.from({ length: n }, (_, i) => `cond${i}`); | ||
| const build = await Bun.build({ | ||
| entrypoints: [join(dir, "entry.js")], | ||
| target, | ||
| conditions, | ||
| }); | ||
| expect(build.success).toBe(true); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use parameterized cases instead of nested loops for this matrix.
Please convert the (target, n) combinations into parameterized tests so each combo reports as an independent case in failures.
♻️ Suggested refactor
-test("many custom conditions does not crash", async () => {
+describe.each(["bun", "browser", "node"] as const)("many custom conditions does not crash (%s)", target => {
+ test.concurrent.each([1, 4, 6, 8, 12, 20])("n=%i", async n => {
const dir = tempDirWithFiles("bun-build-many-conditions", {
"entry.js": `console.log(1);`,
});
- for (const target of ["bun", "browser", "node"] as const) {
- for (const n of [1, 4, 6, 8, 12, 20]) {
- const conditions = Array.from({ length: n }, (_, i) => `cond${i}`);
- const build = await Bun.build({
- entrypoints: [join(dir, "entry.js")],
- target,
- conditions,
- });
- expect(build.success).toBe(true);
- }
- }
-});
+ const conditions = Array.from({ length: n }, (_, i) => `cond${i}`);
+ const build = await Bun.build({
+ entrypoints: [join(dir, "entry.js")],
+ target,
+ conditions,
+ });
+ expect(build.success).toBe(true);
+ });
+});As per coding guidelines: "Use describe.each() for parameterized tests".
🤖 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/bundler/bun-build-api.test.ts` around lines 819 - 830, Replace the
nested for-loops that iterate over target and n with parameterized test cases
using describe.each or test.each so each (target, n) combo is reported
independently; specifically, create a table/array of tuples for the matrix of
targets ["bun","browser","node"] and ns [1,4,6,8,12,20], then use describe.each
or test.each to iterate those tuples and inside each test call Bun.build with
the same entrypoints and compute conditions = Array.from({ length: n }, (_, i)
=> `cond${i}`) and assert build.success; remove the outer for (const target ...)
and inner for (const n ...) loops and replace them with the parameterized test
harness.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #30466, which also fixes a second instance of the same precedence bug in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward operator-precedence fix with a regression test.
Extended reasoning...
Overview
This PR fixes an operator-precedence bug in ESMConditions.init (src/bundler/options.zig). The original expression defaults.len + 2 + if (allow_addons) 1 else 0 + conditions.len parsed such that conditions.len bound to the else branch, so when allow_addons was true (the default) the user-provided conditions were never counted toward the reserved capacity. The subsequent putAssumeCapacity calls could then overflow the map. The fix hoists the conditional into const extra: usize = if (allow_addons) 1 else 0; and adds it explicitly, making the arithmetic unambiguous. A regression test in test/bundler/bun-build-api.test.ts exercises 1–20 custom conditions across all three targets.
Security risks
None. This is a capacity-reservation arithmetic fix; if anything it removes a potential out-of-bounds write in release builds. No auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
Low. The change is four lines of obviously-correct arithmetic refactoring whose intent and effect are easy to verify by inspection. I checked that the new reserved capacity (defaults.len + 2 + extra + conditions.len) is ≥ the actual number of putAssumeCapacity calls for each of the four maps, and it is. The style_condition_map line is unchanged and was already correct.
Other factors
The only finding from the bug-hunting pass is a nit that the new test should use test.concurrent for consistency with its neighbors — purely stylistic and not a reason to withhold approval. The PR includes a targeted regression test that would have caught the original bug.
| expect(text).toContain(" globalThis."); | ||
| }); | ||
|
|
||
| test("many custom conditions does not crash", async () => { |
There was a problem hiding this comment.
🟡 Nit: this should be test.concurrent to match the neighboring top-level tests in this file (and per test/CLAUDE.md's guidance to prefer concurrent tests when they write files / spawn builds with no shared state).
Extended reasoning...
Summary
The new test "many custom conditions does not crash" is declared with plain test(...) rather than test.concurrent(...). This is inconsistent with both the surrounding code in this file and the project's documented testing conventions.
Project convention
test/CLAUDE.md states:
Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with
test.concurrentordescribe.concurrentunless it's very difficult to make them concurrent.
This test writes files via tempDirWithFiles and runs 18 Bun.build calls — exactly the kind of test the guideline targets.
Local consistency
Looking at the immediate neighbors at the top level of this file (outside the describe("Bun.build") block):
test.concurrent("macro with nested object", ...)test.concurrent("regression/NODE_PATHBuild api", ...)test.concurrent("regression/GlobalThis", ...)— directly abovedescribe.concurrent("sourcemap boolean values", ...)— directly below
Every adjacent top-level test uses .concurrent. The only nearby block that does not is describe("tsconfig option"), and that one has a clear reason: it calls process.chdir(), which is process-global and cannot safely run concurrently. The new test has no such constraint.
Why concurrency is safe here
Step-by-step:
- The test creates an isolated temp directory with
tempDirWithFiles("bun-build-many-conditions", ...)— no path collisions with other tests. - It calls
Bun.build({ entrypoints, target, conditions })with nooutdir, so it doesn't write to any shared location. - It does not call
process.chdir(), mutateprocess.env, or touch any other process-global state. - The loop variables (
target,n,conditions) are all locals.
There is therefore no obstacle to running it concurrently with the other tests in this file.
Impact
This is purely a consistency / test-suite-throughput nit — the test is functionally correct as written and will pass either way. It just runs sequentially when it could run in parallel with its neighbors, and it diverges from the established pattern at this location in the file.
Fix
-test("many custom conditions does not crash", async () => {
+test.concurrent("many custom conditions does not crash", async () => {1 participant
What does this PR do?
ESMConditions.initcomputes how much capacity to reserve for each condition map before inserting withputAssumeCapacity. The expression used was:which Zig parses as:
So when
allow_addonsis true (the default), the number of user-provided conditions was never counted towards the reserved capacity. With roughly 6+ custom conditions passed toBun.build({ conditions })orbun build --conditions, the subsequentputAssumeCapacitycalls exceed the allocation — tripping a debug assertion and writing past the buffer in release builds.Fix by hoisting the conditional into a local so the addition is unambiguous.
How did you verify your code works?
Added a regression test that builds with 1–20 custom conditions across
bun,browser, andnodetargets. Before this change the test panics withreached unreachable codeinMultiArrayList.addOneAssumeCapacity; after, it passes.Fuzzer fingerprint:
2ca2ef9e67423f31