Repository navigation
feat(minify): optimize Error constructors by removing 'new' keyword - #22493
Conversation
Refactored `maybeMarkConstructorAsPure` to `minifyGlobalConstructor` to return an optional expression instead of mutating in place. Added optimization that converts `new Error(...)` to `Error(...)` for all Error constructor types, saving 4 bytes per call. Changes: - Renamed and refactored `maybeMarkConstructorAsPure` to `minifyGlobalConstructor` - Function now returns `?Expr` instead of void for better composability - Added Error, TypeError, SyntaxError, RangeError, ReferenceError, EvalError, and URIError to the optimization list - These constructors behave identically with or without 'new', so we can safely remove it - Preserves existing pure constructor marking for Date, Map, Set, etc. Tests added to verify: - All Error types are correctly optimized - Runtime behavior is preserved - Other constructors still get @__PURE__ annotations This optimization is particularly useful in minified production code where Error constructors are commonly used. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 8:09 PM PT - Sep 8th, 2025
❌ @dylan-conway, your commit ae2bf8a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22493That installs a local version of the PR into your bun-22493 --bun |
WalkthroughAdds extensive global-constructor minification: expands KnownGlobal enums, replaces maybeMarkConstructorAsPure with minifyGlobalConstructor that may return an inline js_ast.Expr (allocator, loc, minify_whitespace); visitExpr substitutes returned expressions; parser/runtime flags updated and numerous tests/snapshots/sourcemaps rebased. Changes
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (3)
test/bundler/bundler_minify.test.ts (1)
759-800: Runtime-behavior check is useful; keep tests data-drivenConsider table-driven cases (describe.each) for the three Error types to reduce duplication and make it easier to extend.
src/ast/KnownGlobal.zig (2)
186-222: Minor: remove unused aliasThe alias below appears unused and could be dropped to avoid noise.
Apply this diff:
-const Map = std.AutoHashMapUnmanaged;
21-41: Optional: consider AggregateError (with verification first)If spec-conformant across targets (ECMA262/engines), AggregateError might also be safely callable without new. If validated, you could add it to KnownGlobal and the switch list; otherwise, keeping it out is safer.
Would you like me to check spec/engine behavior for AggregateError() vs new AggregateError() before we include it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/ast/KnownGlobal.zig(13 hunks)src/ast/visitExpr.zig(1 hunks)test/bundler/bundler_minify.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/visitExpr.zigsrc/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/visitExpr.zigsrc/ast/KnownGlobal.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_minify.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_minify.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_minify.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_minify.test.ts
🧠 Learnings (7)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-09-08T00:41:12.041Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.041Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
test/bundler/bundler_minify.test.tssrc/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-09-08T00:41:12.041Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.041Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add new V8 API mangled symbols (without leading underscore) to src/symbols.txt
Applied to files:
src/ast/KnownGlobal.zig
🧬 Code graph analysis (1)
test/bundler/bundler_minify.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1694-1728)
🔇 Additional comments (6)
src/ast/visitExpr.zig (1)
1494-1496: LGTM: early return on minified constructor is correctRewriting here and short-circuiting when minify_syntax is enabled looks good. Flags/args/loc are preserved via KnownGlobal.minifyGlobalConstructor, and DCE semantics are maintained.
test/bundler/bundler_minify.test.ts (2)
693-757: Good coverage for optimization and non-Error constructorsSolid assertions for all Error variants and for preserving /* @PURE */ on Date/Map/Set. This should catch regressions in both rewrite and purity marking.
802-849: Semantics parity test is on pointNice verification that call vs. construct is indistinguishable for Error-family instances and stacks.
src/ast/KnownGlobal.zig (3)
11-17: Enum additions look rightAdding the Error-family variants matches the new optimization surface. No conflicts with existing names.
21-41: Safe rewrite: new Error(...) → Error(...)The guardrails (identifier-only target + unbound symbol + KnownGlobal match) make this safe. Copying args, loc, and can_be_unwrapped_if_unused preserves behavior and DCE. One small check: confirm the struct field names are correct across types:
- E.Call uses close_paren_loc
- E.New provides close_parens_loc
If these differ (paren vs. parens), compilation will fail; otherwise, all good.
42-127: Purity marking preserved for Weak{Map,Set}/Set/Date/etc.Logic mirrors prior behavior while returning null to indicate “no unwrap-expression.” This keeps the optimization focused and non-invasive.
Enhanced the minifier to convert constructors to more efficient forms:
- `new Object()` → `{}`
- `new Object({a: 1})` → `{a: 1}` (unwraps the argument)
- `new Object(null/undefined)` → `{}`
- `new Array()` → `[]`
- `new Array(1, 2, 3)` → `[1, 2, 3]`
- `new Array(n)` → `Array(n)` (preserved for sparse arrays)
- Added Array, Object, Function, and RegExp to the list of constructors that work without 'new'
These optimizations provide even better size savings compared to just removing 'new'.
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/ast/KnownGlobal.zig (2)
73-80: Reduce duplication: helper to build Call from NewYou repeat the New→Call construction in several branches. A tiny helper improves readability and reduces error surface for field names.
Example (outside the switch):
inline fn callFromNew(e: *E.New, loc: logger.Loc) js_ast.Expr { const call = E.Call{ .target = e.target, .args = e.args, .close_paren_loc = e.close_parens_loc, .can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused, }; return js_ast.Expr.init(E.Call, call, loc); }Then replace repeated blocks with return callFromNew(e, loc);
Also applies to: 98-106, 111-118
311-314: Remove unused aliasconst Map = std.AutoHashMapUnmanaged is unused and may trigger Zig’s unused-constant error; drop it.
-const Map = std.AutoHashMapUnmanaged;test/bundler/bundler_minify.test.ts (2)
759-800: Avoid mixing compile-time capture with runtime captureThis test defines a capture function and also uses the capture: [...] expectation. The compile-time capture harness won’t apply when capture is shadowed. Prefer asserting only run.stdout here.
itBundled("minify/ErrorConstructorWithVariables", { @@ - capture: [ - "val", - "e1.message", - "e2.message", - "e3.message", - "e1 instanceof Error", - "e2 instanceof TypeError", - "e3 instanceof SyntaxError", - "e.message", - ], minifySyntax: true, target: "bun",
802-849: Same here: prefer runtime assertions onlyThis test also defines capture(). Keep only run.stdout (it’s already present and sufficient).
itBundled("minify/ErrorConstructorPreservesSemantics", { @@ - capture: [ - "val", - "e1 instanceof Error", - "e2 instanceof Error", - 'e1.message === "with new"', - 'e2.message === "without new"', - 'typeof e1.stack === "string"', - 'typeof e2.stack === "string"', - "withNew.constructor === withoutNew.constructor", - ], minifySyntax: true, target: "bun",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/ast/KnownGlobal.zig(13 hunks)test/bundler/bundler_minify.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_minify.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_minify.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_minify.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_minify.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/KnownGlobal.zig
🧠 Learnings (8)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-09-08T00:41:12.041Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.041Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-09-08T00:41:12.041Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.041Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add new V8 API mangled symbols (without leading underscore) to src/symbols.txt
Applied to files:
src/ast/KnownGlobal.zig
🧬 Code graph analysis (1)
test/bundler/bundler_minify.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1694-1728)
🔇 Additional comments (5)
src/ast/KnownGlobal.zig (4)
11-21: Enum extensions look goodAdding Error, TypeError, SyntaxError, RangeError, ReferenceError, EvalError, URIError, Array, Object, Function, RegExp is consistent with the new optimizer scope.
35-45: Safe 'new Error(...)' → 'Error(...)' rewriteThis transformation is spec-safe for all built-in Error constructors and the implementation looks correct.
47-80: Object constructor handling is sound
- 0 args → {} and special-casing {}/[]/null/undefined are correct.
- The generic “remove new” fallback for other cases preserves semantics.
25-33: Verified:minifyGlobalConstructorunwrapping andclose_parens_locmapping are correct
All call sites useif (…)?|minified|to handle the optional return, ande.close_parens_loc(plural) correctly assigns toE.Call.close_paren_loc(singular).test/bundler/bundler_minify.test.ts (1)
694-757: Good coverage of Error constructor minificationCovers all built-in Error types, complex args, and preserves @PURE for Date/Map/Set. Nice.
| .Function, .RegExp => { | ||
| // Just remove 'new' for Function and RegExp | ||
| // RegExp literal conversion would require parsing the pattern string | ||
| const call = E.Call{ | ||
| .target = e.target, | ||
| .args = e.args, | ||
| .close_paren_loc = e.close_parens_loc, | ||
| .can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused, | ||
| }; | ||
| return js_ast.Expr.init(E.Call, call, loc); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: RegExp rewrite can change semantics for RegExp inputs
new RegExp(re) creates a copy; RegExp(re) returns re itself when flags are undefined. Rewriting new RegExp(/abc/) → RegExp(/abc/) returns the same instance, changing identity and lastIndex behavior.
Guard the optimization:
- .Function, .RegExp => {
- // Just remove 'new' for Function and RegExp
+ .Function => {
+ // Just remove 'new' for Function
// RegExp literal conversion would require parsing the pattern string
const call = E.Call{
.target = e.target,
.args = e.args,
.close_paren_loc = e.close_parens_loc,
.can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused,
};
return js_ast.Expr.init(E.Call, call, loc);
},
+ .RegExp => {
+ const n = e.args.len;
+ if (n == 1) {
+ const arg0 = e.args.ptr[0];
+ // If the first arg is a RegExp literal/expression and no flags, preserve 'new'
+ if (arg0.data == .e_reg_exp) {
+ return null;
+ }
+ }
+ const call = E.Call{
+ .target = e.target,
+ .args = e.args,
+ .close_paren_loc = e.close_parens_loc,
+ .can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused,
+ };
+ return js_ast.Expr.init(E.Call, call, loc);
+ },Also add tests to cover new RegExp(/a/) identity vs RegExp(/a/).
📝 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.
| .Function, .RegExp => { | |
| // Just remove 'new' for Function and RegExp | |
| // RegExp literal conversion would require parsing the pattern string | |
| const call = E.Call{ | |
| .target = e.target, | |
| .args = e.args, | |
| .close_paren_loc = e.close_parens_loc, | |
| .can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused, | |
| }; | |
| return js_ast.Expr.init(E.Call, call, loc); | |
| }, | |
| .Function => { | |
| // Just remove 'new' for Function | |
| // RegExp literal conversion would require parsing the pattern string | |
| const call = E.Call{ | |
| .target = e.target, | |
| .args = e.args, | |
| .close_paren_loc = e.close_parens_loc, | |
| .can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused, | |
| }; | |
| return js_ast.Expr.init(E.Call, call, loc); | |
| }, | |
| .RegExp => { | |
| const n = e.args.len; | |
| if (n == 1) { | |
| const arg0 = e.args.ptr[0]; | |
| // If the first arg is a RegExp literal/expression and no flags, preserve 'new' | |
| if (arg0.data == .e_reg_exp) { | |
| return null; | |
| } | |
| } | |
| const call = E.Call{ | |
| .target = e.target, | |
| .args = e.args, | |
| .close_paren_loc = e.close_parens_loc, | |
| .can_be_unwrapped_if_unused = e.can_be_unwrapped_if_unused, | |
| }; | |
| return js_ast.Expr.init(E.Call, call, loc); | |
| }, |
🤖 Prompt for AI Agents
In src/ast/KnownGlobal.zig around lines 108 to 118, the rewrite that strips
'new' from Function and RegExp calls incorrectly converts new RegExp(re) into
RegExp(re), changing semantics for RegExp inputs because new RegExp(re) produces
a copy while RegExp(re) returns the same instance when flags are undefined. Fix
by guarding the optimization: only remove 'new' for RegExp when the first
argument is absent or provably not a RegExp (e.g., not a RegExp literal and not
statically typed as RegExp); if the first arg may be a RegExp value, leave the
Call as new RegExp(...). Add unit tests asserting identity and lastIndex
behavior that new RegExp(/a/) !== /a/ and new RegExp(/a/).lastIndex is
independent from the original, whereas RegExp(/a/) returns the same instance.
| itBundled("minify/AdditionalGlobalConstructorOptimization", { | ||
| files: { | ||
| "/entry.js": /* js */ ` | ||
| // Test Array constructor | ||
| capture(new Array()); | ||
| capture(new Array(3)); | ||
| capture(new Array(1, 2, 3)); | ||
|
|
||
| // Test Object constructor | ||
| capture(new Object()); | ||
| capture(new Object(null)); | ||
| capture(new Object({ a: 1 })); | ||
|
|
||
| // Test Function constructor | ||
| capture(new Function("return 42")); | ||
| capture(new Function("a", "b", "return a + b")); | ||
|
|
||
| // Test RegExp constructor | ||
| capture(new RegExp("test")); | ||
| capture(new RegExp("test", "gi")); | ||
| capture(new RegExp(/abc/)); | ||
|
|
||
| // Test with variables | ||
| const pattern = "\\d+"; | ||
| capture(new RegExp(pattern)); | ||
|
|
||
| // Test that other constructors are preserved | ||
| capture(new Date()); | ||
| capture(new Map()); | ||
| capture(new Set()); | ||
| `, | ||
| }, | ||
| capture: [ | ||
| "[]", // new Array() -> [] | ||
| "Array(3)", // new Array(3) stays as Array(3) because it creates sparse array | ||
| `[ | ||
| 1, | ||
| 2, | ||
| 3 | ||
| ]`, // new Array(1, 2, 3) -> [1, 2, 3] | ||
| "{}", // new Object() -> {} | ||
| "{}", // new Object(null) -> {} | ||
| "{ a: 1 }", // new Object({ a: 1 }) -> { a: 1 } | ||
| 'Function("return 42")', | ||
| 'Function("a", "b", "return a + b")', | ||
| 'RegExp("test")', | ||
| 'RegExp("test", "gi")', | ||
| "RegExp(/abc/)", | ||
| "RegExp(pattern)", | ||
| "/* @__PURE__ */ new Date", | ||
| "/* @__PURE__ */ new Map", | ||
| "/* @__PURE__ */ new Set", | ||
| ], | ||
| minifySyntax: true, | ||
| target: "bun", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a guard case for RegExp(RegExp) semantics
Please add a test to ensure new RegExp(/abc/) is NOT rewritten (it must clone, not return the same instance), while RegExp("abc") and RegExp("abc","g") can be safely de-newed.
itBundled("minify/AdditionalGlobalConstructorOptimization", {
@@
// Test RegExp constructor
capture(new RegExp("test"));
capture(new RegExp("test", "gi"));
capture(new RegExp(/abc/));
+ // Semantics: new RegExp(re) must clone; RegExp(re) returns the same instance
+ const re = /xyz/g;
+ const a = new RegExp(re);
+ const b = RegExp(re);
+ console.log(a === re, b === re);
@@
capture: [
@@
"RegExp(/abc/)",
+ // Runtime check: expect "false\ntrue"
],
minifySyntax: true,
target: "bun",
+ run: { stdout: "false\ntrue" },
});Committable suggestion skipped: line range outside the PR's diff.
The minifier optimizations reduced the output size by 463 bytes, which shifted source map column positions. Updated the expected values to match the new optimized output. Changes: - Updated source map column positions (shifted left due to smaller output) - Updated expected file size from 222174 to 221711 bytes - All tests now pass with the new optimizations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/bundler/bundler_npm.test.ts (1)
70-70: Exact filesize assertion may churn; add rationale or consider a less brittle checkThe new 221711 value makes sense given the minification, but exact byte assertions tend to change with upstream deps. At minimum, add a short comment to capture why the size changed.
Apply this diff to document the rationale:
expectExactFilesize: { + // Bundle shrank due to removing "new " from Error constructors (minifySyntax). "out/entry.js": 221711, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/bundler/bundler_npm.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_npm.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_npm.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_npm.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_npm.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_npm.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_npm.test.ts
🧠 Learnings (7)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : sourcemap.test.ts should verify source map correctness in dev
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-09-08T04:44:59.085Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.085Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer snapshot tests over exact stdout equality assertions
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-09-08T04:44:59.085Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.085Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/css.test.ts : css.test.ts should contain CSS bundling tests in dev mode
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-09-08T00:41:12.041Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.041Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/react-spa.test.ts : react-spa.test.ts should contain React SPA, react-refresh, and basic server component transform tests
Applied to files:
test/bundler/bundler_npm.test.ts
🔇 Additional comments (1)
test/bundler/bundler_npm.test.ts (1)
60-66: Sourcemap offsets updated — consistent with Error constructor minification. LGTMThe decreased column offsets align with dropping the 'new ' prefix on Error constructors. Anchors still target the same source symbols/strings, so this snapshot update looks correct.
Updated source map positions and file sizes to account for combined optimizations: - typeof undefined optimization from main (saves 60 bytes) - constructor optimizations from this PR (saves additional 403 bytes) - Total savings: 463 bytes on React SSR bundle 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/bundler/bundler_npm.test.ts (1)
70-70: Exact filesize assertion is brittle; consider softening it.Exact byte counts tend to vary with unrelated tweaks and across platforms. Prefer a snapshot or a ≤ threshold (if harness supports it) to reduce churn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/ast/visitExpr.zig(1 hunks)test/bundler/bundler_minify.test.ts(1 hunks)test/bundler/bundler_npm.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/bundler/bundler_minify.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_npm.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_npm.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_npm.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_npm.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_npm.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_npm.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/visitExpr.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/visitExpr.zig
🧠 Learnings (7)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/sourcemap.test.ts : sourcemap.test.ts should verify source map correctness in dev
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-09-08T04:44:59.085Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.085Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer snapshot tests over exact stdout equality assertions
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-09-08T04:44:59.085Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.085Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/css.test.ts : css.test.ts should contain CSS bundling tests in dev mode
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-09-08T00:41:12.041Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.041Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/bundler/bundler_npm.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/react-spa.test.ts : react-spa.test.ts should contain React SPA, react-refresh, and basic server component transform tests
Applied to files:
test/bundler/bundler_npm.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
test/bundler/bundler_npm.test.ts (1)
60-66: Snapshot mappings updated to reflect size/offset shifts — looks correct.Offsets shrink consistently with the constructor minification change; no red flags.
src/ast/visitExpr.zig (1)
1495-1497: minifyGlobalConstructor preserves the originalcan_be_unwrapped_if_unusedflag on returnedE.Call(includingnew Error …), matching previous behavior; no changes required.
- Removed unused Map alias in KnownGlobal.zig - Added AggregateError to the list of Error constructors that can be optimized - Verified AggregateError behaves identically with and without 'new' - Added tests for AggregateError optimization 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Per CodeRabbit feedback, extracted repeated New→Call conversion pattern into a helper function to improve readability and reduce error surface. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/ast/KnownGlobal.zig (2)
81-99: Bug: new Array(x) → [x] can change semantics when x may be a number.With
n == 1,xbeing numeric at runtime must not become[x]. Current guardarg.data != .e_numberis insufficient (e.g.,const n = 3is an identifier, not.e_number).Use a stricter proof that the single arg is definitely non‑numeric (or a literal object/array), else strip only
new:- // new Array(1, 2, 3) -> [1, 2, 3] - // But NOT new Array(3) which creates an array with 3 empty slots - if (n > 1 or (n == 1 and e.args.ptr[0].data != .e_number)) { - var array = E.Array{}; - array.items = e.args; - return js_ast.Expr.init(E.Array, array, loc); - } + // new Array(1, 2, 3) -> [1, 2, 3] + if (n > 1) { + var array_lit = E.Array{}; + array_lit.items = e.args; + return js_ast.Expr.init(E.Array, array_lit, loc); + } + // n == 1: only emit literal when arg is provably non-numeric + if (n == 1) { + const arg0 = e.args.ptr[0]; + switch (arg0.knownPrimitive()) { + .string, .null, .undefined, .boolean => { + var array_lit = E.Array{}; + array_lit.items = e.args; + return js_ast.Expr.init(E.Array, array_lit, loc); + }, + else => {}, + } + switch (arg0.data) { + .e_object, .e_array => { + var array_lit = E.Array{}; + array_lit.items = e.args; + return js_ast.Expr.init(E.Array, array_lit, loc); + }, + else => {}, + } + } // For new Array(number), just remove 'new' return callFromNew(e, loc);Please add tests covering
const n = 3; new Array(n)andlet x = 'a'; new Array(x).
101-105: Bug: Removing ‘new’ from RegExp can change identity semantics.
new RegExp(re)clones;RegExp(re)returnsrewhen flags are undefined. Don’t stripnewfor the 1‑arg RegExp‑literal case.Split the cases:
- .Function, .RegExp => { - // Just remove 'new' for Function and RegExp - // RegExp literal conversion would require parsing the pattern string - return callFromNew(e, loc); - }, + .Function => { + // Safe to strip 'new' + return callFromNew(e, loc); + }, + .RegExp => { + const n = e.args.len; + if (n == 1 and e.args.ptr[0].data == .e_reg_exp) { + // Preserve 'new' to avoid returning the same instance + return null; + } + return callFromNew(e, loc); + },Add tests asserting
new RegExp(/a/) !== RegExp(/a/)and independentlastIndex.
🧹 Nitpick comments (1)
src/ast/KnownGlobal.zig (1)
36-44: Unused allocator parameter.
allocatoris not used (explicitly discarded). Either drop it from the signature or put it to use; keeping unused params adds churn to all call sites.Apply this if you decide to remove it:
-pub noinline fn minifyGlobalConstructor(allocator: std.mem.Allocator, noalias e: *E.New, symbols: []const Symbol, loc: logger.Loc) ?js_ast.Expr { - _ = allocator; +pub noinline fn minifyGlobalConstructor(noalias e: *E.New, symbols: []const Symbol, loc: logger.Loc) ?js_ast.Expr {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ast/KnownGlobal.zig(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/KnownGlobal.zig
🧠 Learnings (4)
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/ast/KnownGlobal.zig
🔇 Additional comments (12)
src/ast/KnownGlobal.zig (12)
11-22: Good expansion of KnownGlobal with Error family and others.Covers all standard Error constructors and common globals needed for rewrites.
26-34: Nice helper extraction (callFromNew).Keeps emission consistent and reduces duplication.
47-50: Safe ‘new’-stripping for Error constructors.Gate on
.unboundensures we only touch the real globals. This rewrite is semantically safe and size‑positive.
52-79: Object() rewrite looks correct.
- new Object() → {}
- new Object(obj/arr/null/undefined) handled precisely
- other cases fall back to call form
All sound.
106-137: Purity markings for WeakSet/WeakMap look consistent.Zero-arg and trivial arg cases marked
.if_unused; others preserved.
141-166: Date purity handling LGTM.Only marks pure where arguments are known primitives; otherwise conservatively impure.
168-191: Set purity handling LGTM.Matches existing strategy; avoids risky iterator cases.
193-203: Headers: conservative and fine.Marks zero-arg pure; no unsafe rewrites.
205-233: Response: conservative and fine.Zero-arg and primitive‑arg cases marked pure; no rewrites.
234-248: TextEncoder/Decoder purity LGTM.No risky transforms; purity only for zero-arg.
250-286: Map purity handling LGTM.Guards iterator side effects; handles nested array literal tuples precisely.
293-297: Imports setup is correct.
stdneeded for the allocator type;loggeralias used for loc typing.
When optimizing `new Array(x)` where x is a number, we must not convert it to `[x]` because they have different semantics: - `new Array(5)` creates a sparse array with 5 empty slots - `[5]` creates an array with one element: the number 5 This fix uses .knownPrimitive() to detect when the single argument is a number and preserves the Array(x) form in that case, while still converting non-numeric single arguments to array literals for better minification. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/ast/KnownGlobal.zig (2)
81-99: Bug: single-arg new Array(x) may be rewritten unsafelyUsing
knownPrimitive() != .numberwill treat unknown/indeterminate values as “non-number” and incorrectly rewritenew Array(x)to[x](changes semantics whenxis numeric at runtime). Guard only when we can prove the single arg is definitely not a number (string/null/undefined/boolean). Leave other single-arg cases as a call form.Apply this fix:
- // new Array(1, 2, 3) -> [1, 2, 3] - // But NOT new Array(3) which creates an array with 3 empty slots - if (n > 1 or (n == 1 and e.args.ptr[0].knownPrimitive() != .number)) { - var array = E.Array{}; - array.items = e.args; - return js_ast.Expr.init(E.Array, array, loc); - } - - // For new Array(number), just remove 'new' - return callFromNew(e, loc); + // new Array(1, 2, 3) -> [1, 2, 3] + if (n > 1) { + var array_lit = E.Array{}; + array_lit.items = e.args; + return js_ast.Expr.init(E.Array, array_lit, loc); + } + + // n == 1: only emit literal when we can prove it's not a number at runtime + const arg0 = e.args.ptr[0]; + switch (arg0.knownPrimitive()) { + .string, .null, .undefined, .boolean => { + var array_lit = E.Array{}; + array_lit.items = e.args; + return js_ast.Expr.init(E.Array, array_lit, loc); + }, + // .number or unknown -> preserve semantics by only removing 'new' + else => { + return callFromNew(e, loc); + }, + }
101-105: Bug: RegExp rewrite can change semantics for RegExp inputs
new RegExp(re)clones;RegExp(re)returnsreitself when flags are undefined. Always strippingnewbreaks identity/lastIndex behavior.Split Function and RegExp, and guard the single-arg RegExp-literal case:
- .Function, .RegExp => { - // Just remove 'new' for Function and RegExp - // RegExp literal conversion would require parsing the pattern string - return callFromNew(e, loc); - }, + .Function => { + // Just remove 'new' for Function + return callFromNew(e, loc); + }, + .RegExp => { + const n = e.args.len; + if (n == 1) { + const arg0 = e.args.ptr[0]; + // If arg is a RegExp literal, preserve 'new' to keep clone semantics + if (arg0.data == .e_reg_exp) { + return null; + } + } + return callFromNew(e, loc); + },Add a runtime test (see test file suggestion) to lock this in.
test/bundler/bundler_minify.test.ts (2)
857-934: Fix test to preserve 'new' for RegExp(/re/) and add a runtime identity checkAs-is, this test expects
new RegExp(/abc/)to de-new, which is incorrect (should clone, not return the same instance).Apply:
@@ - // Test RegExp constructor + // Test RegExp constructor capture(new RegExp("test")); capture(new RegExp("test", "gi")); - capture(new RegExp(/abc/)); + // If first arg is a RegExp, 'new' must be preserved to clone + capture(new RegExp(/abc/)); + // Identity semantics check + const re = /xyz/g; + const a = new RegExp(re); + const b = RegExp(re); + console.log(a === re, b === re); @@ - 'RegExp("test", "gi")', - "RegExp(/abc/)", + 'RegExp("test")', + 'RegExp("test", "gi")', + "new RegExp(/abc/)", "RegExp(pattern)", @@ - minifySyntax: true, - target: "bun", + minifySyntax: true, + target: "bun", + run: { stdout: "false\ntrue" },This pairs with the Zig fix to guard RegExp.
936-991: Add safety test: single-arg Array(variable) must not become a literalCovers the common regression where
new Array(n)is rewritten to[n]whennis unknown at build time.Suggested patch:
@@ const a1 = new Array(1, 2, 3); const a2 = Array(1, 2, 3); capture(JSON.stringify(a1) === JSON.stringify(a2)); capture(a1.constructor === a2.constructor); + // Single-arg variable: must preserve sparse semantics when numeric + const n = 3; + const a3 = new Array(n); + const a4 = Array(n); + capture(a3.length === a4.length && a3.length === 3 && 0 in a3 === false); + @@ - "a1.constructor === a2.constructor", + "a1.constructor === a2.constructor", + "a3.length === a4.length && a3.length === 3 && 0 in a3 === !1", @@ - run: { - stdout: "true\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue", - }, + run: { stdout: "true\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue" },This will fail with the current Zig logic, surfacing the bug early.
🧹 Nitpick comments (1)
test/bundler/bundler_minify.test.ts (1)
808-855: Optional: Add AggregateError parity checkTo fully exercise the Error family, add a quick AggregateError new vs call comparison.
Proposed addition inside this test:
@@ const errors = [ [new TypeError("t1"), TypeError("t2")], [new SyntaxError("s1"), SyntaxError("s2")], [new RangeError("r1"), RangeError("r2")], + [new AggregateError([], "a1"), AggregateError([], "a2")], ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/ast/KnownGlobal.zig(12 hunks)test/bundler/bundler_minify.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_minify.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_minify.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_minify.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_minify.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/KnownGlobal.zig
🧠 Learnings (6)
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-09-07T05:41:52.546Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.546Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Prefer JSC intrinsics and `$`-prefixed private APIs for performance and safety (e.g., `$Array`, `$newArrayWithSize`, `$putByIdDirectPrivate`, `$assert`, `$debug`)
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-09-07T05:41:52.546Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-09-07T05:41:52.546Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts} : Use `$isObject` and throw appropriate TypeErrors for constructor/initializer inputs that must be objects
Applied to files:
test/bundler/bundler_minify.test.ts
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/ast/KnownGlobal.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/ast/KnownGlobal.zig
🧬 Code graph analysis (1)
test/bundler/bundler_minify.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1694-1728)
🔇 Additional comments (9)
src/ast/KnownGlobal.zig (6)
11-22: LGTM: Adding Error-family and other globals to KnownGlobalEnum expansion looks correct and aligns with the new optimization goals.
26-34: LGTM: Helper to convert New → CallcallFromNew cleanly centralizes the emission; preserves can_be_unwrapped_if_unused.
36-44: API refactor to return ?Expr is soundEarly-exit on unbound symbols and optional rewrite make the visitor simpler.
47-50: Safe to strip 'new' for Error constructorsSpec allows calling Error, TypeError, SyntaxError, RangeError, ReferenceError, EvalError, URIError, AggregateError without new.
If we want belt-and-suspenders, add a small runtime test for AggregateError to ensure cross-engine parity (see test suggestion below).
52-79: LGTM: Object constructor rewrites preserve semantics
- 0 args → {}
- 1 arg object/array → identity
- null/undefined → {}
All other cases: remove 'new'. Looks correct.
293-297: Minor: imports/logger alias look goodConsistent with surrounding modules.
test/bundler/bundler_minify.test.ts (3)
695-763: LGTM: Error constructors are optimized and pure annotations preserved for othersGood coverage across Error types and interactions with other constructors.
765-806: LGTM: Variable/instanceof/try-catch paths validate behaviorRuntime assertions are helpful here.
808-855: LGTM: Error() vs new Error() equivalence is validatedNice constructor equality checks and stack presence assertions.
The global constructor optimization was being applied to all code when minification was enabled, including node_modules loaded at runtime. This caused issues with packages like proxy-addr (used by Express) that rely on specific array creation patterns. Added check to skip minification for node_modules code to ensure runtime compatibility while still optimizing user code during bundling.
The minification optimization changes source map positions and file sizes in the bundler_npm test. Updated expected values to match the new output.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/ast/visitExpr.zig (1)
1495-1497: Minify guard present (matches prior guidance)The minification is correctly guarded by p.options.features.minify_syntax as suggested in prior feedback.
test/bundler/bundler_minify.test.ts (2)
936-991: Add safety test for single-arg Array(variable) caseGuard against regressions that might rewrite
new Array(n)into[n]whennis unknown at build time.Apply:
@@ capture(a1.constructor === a2.constructor); @@ const sparse = new Array(5); capture(sparse.length === 5); capture(0 in sparse === false); // No element at index 0 capture(JSON.stringify(sparse) === "[null,null,null,null,null]"); + + // Single-arg variable case: must preserve sparse semantics + const n = 3; + const a3 = new Array(n); + const a4 = Array(n); + capture(a3.length === a4.length && a3.length === 3 && a3[0] === undefined); @@ capture: [ "val", "JSON.stringify(a1) === JSON.stringify(a2)", "a1.constructor === a2.constructor", "sparse.length === 5", "0 in sparse === !1", 'JSON.stringify(sparse) === "[null,null,null,null,null]"', + "a3.length === a4.length && a3.length === 3 && a3[0] === undefined", "typeof o1 === typeof o2", "o1.constructor === o2.constructor", "typeof f1 === typeof f2", "f1() === f2()", "r1.source === r2.source", "r1.flags === r2.flags", ], @@ - run: { - stdout: "true\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue", - }, + run: { + stdout: "true\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue", + },
858-934: Preserve RegExp identity semantics; don’t rewrite new RegExp(/abc/)Rewriting
new RegExp(/abc/)toRegExp(/abc/)changes identity and lastIndex behavior. Keepnewwhen the first arg is a RegExp literal; add a runtime test to lock this down.Apply:
@@ - // Test RegExp constructor + // Test RegExp constructor capture(new RegExp("test")); capture(new RegExp("test", "gi")); - capture(new RegExp(/abc/)); + // Literal RegExp: must NOT de-new (identity differs) + capture(new RegExp(/abc/)); + + // Semantics: new RegExp(re) must clone; RegExp(re) returns the same instance + const re = /xyz/g; + const a = new RegExp(re); + const b = RegExp(re); + console.log(a === re, b === re); @@ - 'RegExp("test", "gi")', - "RegExp(/abc/)", + 'RegExp("test", "gi")', + // Preserve 'new' for literal RegExp + "new RegExp(/abc/)", "RegExp(pattern)", @@ - target: "bun", + target: "bun", + run: { stdout: "false\ntrue" },
🧹 Nitpick comments (2)
test/js/bun/test/test-error-code-done-callback.test.ts (1)
52-52: Column offsets updated across snapshotsThe higher column values reflect the removal of “new” in constructor calls. Consider normalizing columns (like you already normalize times and paths) to reduce future churn, but this is optional.
Also applies to: 62-62, 72-72, 82-82, 92-92, 102-102, 112-112, 122-122, 132-132
test/js/bun/test/test-test.test.ts (1)
736-736: Avoid hard-coding exact column in first frame (optional)To make this resilient to future minifier tweaks, normalize the “:line:column” bit before asserting (similar to console-log test). Example diff:
- if (process.platform === "win32") { - expect(stackLines[0]).toContain(`<dir>\\my-test.test.js:5:15`.replace("<dir>", test_dir)); - } + if (process.platform === "win32") { + expect(stackLines[0].replace(/:\d+:\d+$/, ":NN:NN")).toContain( + `<dir>\\my-test.test.js:5:NN`.replace("<dir>", test_dir), + ); + } - if (process.platform !== "win32") { - expect(stackLines[0]).toContain(`<dir>/my-test.test.js:5:15`.replace("<dir>", test_dir)); - } + if (process.platform !== "win32") { + expect(stackLines[0].replace(/:\d+:\d+$/, ":NN:NN")).toContain( + `<dir>/my-test.test.js:5:NN`.replace("<dir>", test_dir), + ); + }Also applies to: 739-739
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/ast/KnownGlobal.zig(12 hunks)src/ast/visitExpr.zig(1 hunks)test/bundler/bundler_minify.test.ts(1 hunks)test/js/bun/test/stack.test.ts(1 hunks)test/js/bun/test/test-error-code-done-callback.test.ts(9 hunks)test/js/bun/test/test-test.test.ts(1 hunks)test/js/bun/util/inspect-error.test.js(5 hunks)test/js/bun/util/inspect.test.js(1 hunks)test/js/bun/util/reportError.test.ts(1 hunks)test/js/web/console/console-log.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/bun/util/inspect.test.jstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/bundler/bundler_minify.test.tstest/js/bun/util/inspect-error.test.js
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/bun/util/inspect.test.jstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/js/bun/util/inspect-error.test.js
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/bun/util/inspect.test.jstest/js/bun/util/reportError.test.tstest/js/bun/util/inspect-error.test.js
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/bun/util/inspect.test.jstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/bundler/bundler_minify.test.tstest/js/bun/util/inspect-error.test.js
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/bundler/bundler_minify.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/bun/util/inspect.test.jstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/js/bun/util/inspect-error.test.js
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/bundler/bundler_minify.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/js/bun/util/inspect.test.jstest/js/web/console/console-log.test.tstest/js/bun/util/reportError.test.tstest/bundler/bundler_minify.test.tstest/js/bun/util/inspect-error.test.js
test/js/web/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Web API tests under test/js/web/, separated by category
Files:
test/js/web/console/console-log.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_minify.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/visitExpr.zigsrc/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/visitExpr.zigsrc/ast/KnownGlobal.zig
🧬 Code graph analysis (1)
test/bundler/bundler_minify.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1694-1728)
🔇 Additional comments (15)
test/js/bun/test/stack.test.ts (1)
81-81: Snapshot column update looks correctThe shift to originalColumn: 18 matches the new constructor-minification behavior. No action needed.
test/js/web/console/console-log.test.ts (1)
133-133: Caret alignment snapshot update is fineThe adjusted indentation under the error line reflects the new stack formatting. Looks good.
test/js/bun/util/inspect.test.js (1)
664-664: Inline snapshot caret shift acknowledgedUpdated caret position aligns with changed column offsets. No further changes needed.
test/js/bun/util/reportError.test.ts (1)
24-27: Snapshot now points to updated locationsThe caret and stack frame positions match the new formatting. Approved.
test/js/bun/util/inspect-error.test.js (1)
16-16: Snapshot position adjustments look consistentAll updated line/column references align with constructor-call minification and minified fixture shifts. Good to go.
Also applies to: 18-18, 24-24, 26-26, 121-121, 154-154, 169-169
src/ast/visitExpr.zig (2)
1495-1497: Early-returning the minified expression is fine; verify call-visit side effects aren’t neededReturning a Call/other expression from e_new is correct, but double-check:
- Flags like can_be_unwrapped_if_unused are preserved (previously set by maybeMarkConstructorAsPure).
- Any metadata (loc/close_paren_loc) needed for accurate sourcemaps is set on the returned node.
If either isn’t guaranteed by KnownGlobal.minifyGlobalConstructor, consider re-visiting the returned expr once (p.visitExpr) or setting the flags explicitly in the returned node.
1495-1497: Runtime/node_modules gatingThe PR mentions not applying runtime minification to node_modules. Since this call site doesn’t pass source-context, please confirm minifyGlobalConstructor internally checks runtime vs bundling and node_modules paths before transforming; otherwise, we may need to thread that context in.
test/bundler/bundler_minify.test.ts (3)
695-763: LGTM: Error constructors “de-newed” with correct capturesCovers all Error types and includes sanity checks for non-Error constructors. Looks good.
765-806: LGTM: Variable-based Error construction and instanceof checksRuntime assertions verify semantics; concise and solid.
808-855: LGTM: Parity between Error() and new Error()Good coverage of message, stack presence, and constructor equality.
src/ast/KnownGlobal.zig (5)
26-34: Helper extraction is clean
callFromNewreduces duplication and improves clarity.
46-50: LGTM: Error-family “de-new” is semantics-preservingCalling Error constructors without
newis equivalent; good byte savings.
125-156: Pure annotations for WeakSet/WeakMap (0-arg and some 1-arg) are fineConservative and safe; no change requested.
157-210: Date/Set purity marking is reasonablePure when args are empty or trivially coercible; returns null to let DCE work. Looks good.
224-305: Headers/Response/TextDecoder/TextEncoder/Map: conservative purityApproach is sound; maintains side-effect safety.
- Add safety test for single-arg Array(variable) case to ensure sparse array semantics are preserved
- Extend single-argument Array() handling in KnownGlobal.zig to convert object/array literals (e.g., new Array({}) -> [{}], new Array([1]) -> [[1]])
- Refactor code style to use inline struct initialization for cleaner code
- Update sourcemap positions and file size in bundler_npm.test.ts to reflect optimization changes
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/ast/KnownGlobal.zig (3)
145-151: Incorrectly markingnew WeakSet(null)/new WeakMap(null)as pure (these throw)Passing
nulltriggers a TypeError via iterator lookup; it’s not safe to DCE.Apply:
- .e_null, .e_undefined => { - // "new WeakSet(null)" is pure - // "new WeakSet(void 0)" is pure - e.can_be_unwrapped_if_unused = .if_unused; - }, + .e_undefined => { + // "new WeakSet(void 0)" / "new WeakMap(void 0)" is pure + e.can_be_unwrapped_if_unused = .if_unused; + }, + .e_null => { + // "new WeakSet(null)" / "new WeakMap(null)" throws; do not mark pure + },Please mirror the comment text for WeakMap as well.
206-213:new Set(null)is not pure (throws); refine purity for array inputs
nullthrows via iterator access; don’t mark pure.- Consider only treating
new Set([])as pure; non-empty arrays perform iteration/add which we typically avoid DCE-ing.Apply:
- .e_array, .e_null, .e_undefined => { - // "new Set([a, b, c])" is pure - // "new Set(null)" is pure - // "new Set(void 0)" is pure - e.can_be_unwrapped_if_unused = .if_unused; - }, + .e_undefined => { + // "new Set(void 0)" is pure + e.can_be_unwrapped_if_unused = .if_unused; + }, + .e_null => { + // "new Set(null)" throws; not pure + }, + .e_array => |array| { + if (array.items.len == 0) { + // "new Set([])" is pure + e.can_be_unwrapped_if_unused = .if_unused; + } + },Add tests asserting
(() => new Set(null))throws, and thatnew Set([])may be DCE’d.
289-296:new Map(null)is not pure (throws); keepundefinedas pureSame iterator semantics as Set; please don’t mark
nullas pure. Optionally restrict array-literal handling to[]only for consistency with Set/WeakSet.Apply:
- .e_null, .e_undefined => { - // "new Map(null)" is pure - // "new Map(void 0)" is pure - e.can_be_unwrapped_if_unused = .if_unused; - }, + .e_undefined => { + // "new Map(void 0)" is pure + e.can_be_unwrapped_if_unused = .if_unused; + }, + .e_null => { + // "new Map(null)" throws; not pure + },And (optional) narrow the
.e_arraycase to only marknew Map([])as pure.
♻️ Duplicate comments (1)
src/ast/KnownGlobal.zig (1)
129-133: Do not stripnewfrom RegExp when the first arg is a RegExp (identity/lastIndex semantics change)
new RegExp(re)creates a copy;RegExp(re)returnsrewhen flags are undefined. This changes identity andlastIndex. Guard this case.Apply:
- .Function, .RegExp => { - // Just remove 'new' for Function and RegExp - // RegExp literal conversion would require parsing the pattern string - return callFromNew(e, loc); - }, + .Function => { + // Just remove 'new' for Function + return callFromNew(e, loc); + }, + .RegExp => { + const n = e.args.len; + if (n >= 1) { + const arg0 = e.args.ptr[0]; + if (arg0.data == .e_reg_exp) { + // If flags are absent or explicitly undefined, RegExp(re[, undefined]) returns `re` (no copy) + if (n == 1) return null; + const flags = e.args.ptr[1]; + switch (flags.knownPrimitive()) { + .undefined => return null, + // If flags is definitely a string (or any non-undefined), removing `new` is safe. + .string, .boolean, .number, .bigint, .null => {}, + else => return null, // unknown: be conservative + } + } + } + return callFromNew(e, loc); + },Please add tests covering:
const r = /a/g; const a = new RegExp(r); a !== r;const r = /a/g; const b = RegExp(r); b === r;RegExp(r, "i") !== r.
🧹 Nitpick comments (2)
src/ast/KnownGlobal.zig (2)
81-127: Array single-arg handling is now safe; consider also literalizing SymbolsThe “definitely-not-a-number” check is solid. Optional: treat
.symbolfromknownPrimitive()as safe to literalize too (Symbols are never numbers).Apply:
- switch (primitive) { - .null, .undefined, .boolean, .string, .bigint => { + switch (primitive) { + .null, .undefined, .boolean, .string, .bigint, .symbol => { // These are definitely not numbers, safe to convert return js_ast.Expr.init(E.Array, .{ .items = e.args }, loc); },
278-314: Map array-arg purity heuristic may be too liberalToday it allows any non-empty array-of-arrays. To be conservative and consistent with Set/WeakSet, consider limiting to
[]only (optional).If you want, I can draft tests to measure the impact and catch edge-cases around iterator monkey-patching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ast/KnownGlobal.zig(12 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/KnownGlobal.zig
🔇 Additional comments (10)
src/ast/KnownGlobal.zig (10)
11-22: Enum additions look good and cover the targeted constructorsSet is comprehensive for this PR’s scope.
26-34: Good helper extraction (callFromNew)Removes duplication and preserves call metadata.
36-44: Guarding on unbound globals is correctSkipping shadowed bindings avoids semantic changes.
45-51: Safe to dropnewfor Error-family constructorsSpec preserves behavior when called as functions; nice win.
52-80: Object constructor transforms are sound
- new Object() → {} and unwrapping object/array args are semantics-preserving.
- null/undefined → {} is correct.
167-174: Date purity notes LGTM0-arg and primitive single-arg cases marked pure only-if-unused is reasonable.
221-231: Headers purity marking LGTMConservative and safe.
233-261: Response purity marking LGTM0-arg and primitive single-arg: fine to DCE if unused.
262-276: TextEncoder/TextDecoder purity marking LGTMOnly 0-arg treated as pure-if-unused; sensible.
321-329: Imports/aliasing look correctNo issues.
- new RegExp(re) creates a copy, but RegExp(re) returns the same instance - This affects object identity and lastIndex behavior - The semantics are too complex to safely optimize - Added detailed comment explaining why RegExp optimization is disabled - Updated test expectations to preserve new RegExp() calls 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/bundler/bundler_minify.test.ts (1)
857-934: Add explicit RegExp identity test (clone vs same-instance).To guard against future regressions if RegExp de-new is reintroduced, add an identity check:
new RegExp(re)must not be rewritten, whileRegExp(re)returns the same instance.itBundled("minify/AdditionalGlobalConstructorOptimization", { @@ capture(new RegExp(/abc/)); + // Identity semantics: new RegExp(re) must clone; RegExp(re) returns the same instance + const re = /xyz/g; + const a = new RegExp(re); + const b = RegExp(re); + console.log(a === re, b === re); @@ capture: [ @@ "new RegExp(/abc/)", + // Expect: "false\ntrue" ], minifySyntax: true, target: "bun", + run: { stdout: "false\ntrue" }, });
🧹 Nitpick comments (2)
src/ast/KnownGlobal.zig (2)
52-80: Object() transformations are safe; add tiny readability tweak.
- Returning the arg for
.e_object/.e_arrayretains identity semantics.null/undefined→{}is correct.
Optional: early-return onn == 1to reduce nesting..Object => { const n = e.args.len; if (n == 0) { // new Object() -> {} return js_ast.Expr.init(E.Object, E.Object{}, loc); } - if (n == 1) { + if (n == 1) { const arg = e.args.ptr[0]; switch (arg.data) { .e_object, .e_array => { // new Object({a: 1}) -> {a: 1} // new Object([1, 2]) -> [1, 2] return arg; }, .e_null, .e_undefined => { // new Object(null) -> {} // new Object(undefined) -> {} return js_ast.Expr.init(E.Object, E.Object{}, loc); }, else => {}, } - } - - // For other cases, just remove 'new' - return callFromNew(e, loc); + // Other primitives: just remove 'new' + return callFromNew(e, loc); + } + // n > 1: just remove 'new' + return callFromNew(e, loc); },
133-140: Consider re-enabling safe RegExp de-new only where semantics can’t change.You currently skip all RegExp optimizations to avoid identity pitfalls. That’s safe but leaves bytes on the table. We can safely strip
newexcept when there is exactly one argument that is a RegExp literal/expression with no flags.Apply within this block:
- .RegExp => { - // Don't optimize RegExp - the semantics are too complex: - // - new RegExp(re) creates a copy, but RegExp(re) returns the same instance - // - This affects object identity and lastIndex behavior - // - The difference only applies when flags are undefined - // Keep the original new RegExp() call to preserve correct semantics - return null; - }, + .RegExp => { + const n = e.args.len; + if (n == 1) { + const arg0 = e.args.ptr[0]; + // If the arg is a RegExp literal/expression and no flags, preserve 'new' to clone + if (arg0.data == .e_reg_exp) return null; + } + // 0 args or 1+ args where first is not a RegExp: safe to remove 'new' + return callFromNew(e, loc); + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/ast/KnownGlobal.zig(12 hunks)test/bundler/bundler_minify.test.ts(1 hunks)test/bundler/bundler_npm.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/bundler/bundler_npm.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_minify.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_minify.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_minify.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_minify.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/KnownGlobal.zig
🧬 Code graph analysis (1)
test/bundler/bundler_minify.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1694-1728)
🔇 Additional comments (11)
src/ast/KnownGlobal.zig (7)
11-22: Enum expansion looks correct and aligns with intended minifications.Covers all built-in Error types plus Array/Object/Function/RegExp. No concerns.
26-34: Helper extraction LGTM; improves reuse and keeps emit logic consistent.Reusing
can_be_unwrapped_if_unusedis important for DCE. Nice.
36-45: Good shadowing guard via.unbound.Prevents breaking user-shadowed globals. Returning
?js_ast.Expris a clean interface for replacements vs purity-only cases.
46-51: Safe ‘de-new’ for Error constructors.Built-in Error constructors are callable and produce equivalent instances; replacing with calls preserves behavior and saves bytes.
81-127: Array single-arg handling looks careful; one edge-case note.
- 0 args →
[]and multi-arg → literal are correct.- Single-arg: only literalize when definitely not a number or when object/array literal — good.
Note: BigInt is correctly treated as “not a Number” in the single-arg path (Array(1n) → [1n]). All good.
141-226: Purity markings for WeakSet/WeakMap/Date/Set/Headers/Response/TextEncoder/Decoder/Map are consistent with esbuild-style rules.Marking as
if_unusedonly, and avoiding replacements, keeps semantics. Looks good.Also applies to: 228-321
328-333: Imports and aliases OK.No unused symbols after
_ = allocator;. Formatting consistent.test/bundler/bundler_minify.test.ts (4)
695-763: Covers all Error constructors and ensures only intended constructors are minified.Matches implementation (RegExp remains with
new, Date/Map/Set pure-annotated). Good.
765-806: Variable/instanceof coverage for Error constructors is solid.Validates semantics after de-newing. LGTM.
808-855: Behavioral parity test for Error vs new Error is helpful.Covers stack/message/constructor parity. Nice.
936-998: Nice: sparse array semantics and single-arg variable guarded.Covers the tricky
Array(n)path and ensures no literalization whennis unknown. LGTM.
| } | ||
|
|
||
| // For new Array(number), just remove 'new' | ||
| return callFromNew(e, loc); |
There was a problem hiding this comment.
this is unreachable. i think this should be switch (n) { 0 => {...}, 1 => {...}, else => {...}}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ast/KnownGlobal.zig (1)
151-162: Function: OK to strip ‘new’; RegExp: good to skip
new Function(...)→Function(...)is safe.- Skipping RegExp rewrite avoids identity/lastIndex traps. Good call.
🧹 Nitpick comments (2)
src/ast/KnownGlobal.zig (2)
36-45: Avoid double-return pattern around switch
return switch (...) { ... return ...; }mixes expression-return with early returns, making the outerreturn switchredundant and potentially confusing. Prefer returning per-arm without wrapping theswitchin areturn.Example:
- return switch (constructor) { + switch (constructor) { .Error, ... => { - return callFromNew(e, loc); + return callFromNew(e, loc); }, // ... - }; + }
350-357: Import of js_parser only for Prefill — may be removableIf you switch to
E.Missingas suggested, thejs_parserimport can likely be dropped from this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/ast/KnownGlobal.zig(12 hunks)src/ast/visitExpr.zig(1 hunks)src/bundler/ParseTask.zig(1 hunks)src/runtime.zig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ast/visitExpr.zig
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bundler/ParseTask.zigsrc/runtime.zigsrc/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bundler/ParseTask.zigsrc/runtime.zigsrc/ast/KnownGlobal.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (5)
src/bundler/ParseTask.zig (1)
1174-1180: Propagate whitespace-minify flag — LGTMHooking
opts.features.minify_whitespacetotranspiler.options.minify_whitespaceis correct and scoped to parsing.src/ast/KnownGlobal.zig (4)
46-49: Error-family: correct and safe to strip ‘new’
new Error(...)≡Error(...)across engines; byte-saving with identical semantics.
51-78: Object constructor rewrites look good
new Object()→{}is correct.new Object(objOrArray)→ argument is returned unchanged — correct.new Object(null|undefined)→{}— correct fallback.- Other cases: removing
newpreserves semantics.Please ensure tests cover:
new Object(Symbol('x')),new Object(1n),new Object('s')(all should remain a wrapper when strippingnew).
164-343: Purity flags for WeakSet/WeakMap/Date/Set/Map/Headers/Response/TextEncoder/Decoder — LGTMMarking known pure cases with
.if_unusedenables DCE without changing semantics. Coverage looks conservative.Add tests proving purity for:
new Set()andnew Map()dropped when unused.new WeakSet([])doesn’t throw and is removable when unused.new Response('')unused is removable.
80-149: Verify array hole representation and mutation safety
- Confirm that using
js_parser.Prefill.Data.EMissinghere produces valid hole elements or switch toE.Missingif parser‐internal types aren’t permitted.- Ensure
listManagedreturns a cloned list—clearing and appending holes must not mutate the originale.args.- Add a brief comment explaining the 0–10 threshold choice for inlining holes.
- Add tests for
Array(3).length === 3,[,, ,].length === 3, and0 in Array(3) === falsevs[,,].
| minify_syntax: bool = false, | ||
| minify_identifiers: bool = false, | ||
| minify_whitespace: bool = false, | ||
| dead_code_elimination: bool = true, | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Include minify_whitespace in the runtime transpiler cache key
Features.minify_whitespace affects emitted code but isn’t part of hash_fields_for_runtime_transpiler. Toggling the flag may return stale cached output.
Apply:
const hash_fields_for_runtime_transpiler = .{
.top_level_await,
.auto_import_jsx,
.allow_runtime,
.inlining,
.commonjs_named_exports,
- .minify_syntax,
- .minify_identifiers,
+ .minify_syntax,
+ .minify_identifiers,
+ .minify_whitespace,
.dead_code_elimination,
.set_breakpoint_on_first_line,
.trim_unused_imports,
.dont_bundle_twice,
.commonjs_at_runtime,
.emit_decorator_metadata,
.lower_using,Run to ensure no other cache-key lists exist that also need this field:
Also applies to: 212-230
🏁 Script executed:
#!/bin/bash
rg -nP --type=zig -C2 'hash_fields_for_runtime_transpiler|Wyhash|hashForRuntimeTranspiler'Length of output: 40602
Include minify_whitespace in runtime transpiler cache key
src/runtime.zig lines 212–230:
@@ 212
.minify_syntax,
.minify_identifiers,
+ .minify_whitespace,
.dead_code_elimination,📝 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.
| minify_syntax: bool = false, | |
| minify_identifiers: bool = false, | |
| minify_whitespace: bool = false, | |
| dead_code_elimination: bool = true, | |
| .minify_syntax, | |
| .minify_identifiers, | |
| .minify_whitespace, | |
| .dead_code_elimination, |
🤖 Prompt for AI Agents
In src/runtime.zig around lines 169-173 and 212-230, the minify_whitespace
boolean option is defined but not included in the runtime transpiler cache key;
add minify_whitespace to the cache key construction where minify_syntax,
minify_identifiers, and dead_code_elimination are included so the cache
differentiates builds with different whitespace minification. Ensure you append
a deterministic representation of the boolean (e.g.,
"minify_whitespace=true"/"minify_whitespace=false" or a single byte) into the
same buffer or hash input used for the key, update any key-length or hash
calculations accordingly, and run/update tests that assert cache behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/bundler/bundler_minify.test.ts (1)
857-934: Add explicit RegExp identity semantics testPlease include the identity/clone distinction to lock this in:
new RegExp(re)must not be rewritten whileRegExp(re)returns the same instance when flags are omitted. This guards future regressions.Apply within this test block:
@@ // Test RegExp constructor capture(new RegExp("test")); capture(new RegExp("test", "gi")); capture(new RegExp(/abc/)); + + // Identity semantics: new RegExp(re) clones; RegExp(re) returns the same instance (no flags) + const re = /xyz/g; + const a = new RegExp(re); + const b = RegExp(re); + console.log(a === re, b === re); @@ capture: [ @@ 'new RegExp("test")', 'new RegExp("test", "gi")', "new RegExp(/abc/)", "new RegExp(pattern)", "/* @__PURE__ */ new Date", "/* @__PURE__ */ new Map", "/* @__PURE__ */ new Set", ], minifySyntax: true, target: "bun", + run: { stdout: "false\ntrue" },
🧹 Nitpick comments (3)
test/bundler/bundler_minify.test.ts (2)
936-972: Nice whitespace-gated holes optimization matrixGreat set of cases. Consider adding 0.0 and -0 to document integer detection and that both collapse to
[]in this mode.
974-1036: Good semantics lock for Array/Object/Function/RegExpOptional: add a primitive-wrapper case to ensure
new Object(1)→Object(1)only (and not a literal), and still matches constructor/type semantics.Apply inside this test:
@@ // Test Object semantics const o1 = new Object(); const o2 = Object(); capture(typeof o1 === typeof o2); capture(o1.constructor === o2.constructor); + + // Primitive wrapper semantics preserved + const p1 = new Object(1); + const p2 = Object(1); + capture(p1 instanceof Number && p2 instanceof Number); + capture(Object.prototype.toString.call(p1) === Object.prototype.toString.call(p2)); @@ capture: [ "val", "JSON.stringify(a1) === JSON.stringify(a2)", "a1.constructor === a2.constructor", "sparse.length === 5", "0 in sparse === !1", 'JSON.stringify(sparse) === "[null,null,null,null,null]"', "a3.length === a4.length && a3.length === 3 && a3[0] === void 0", "typeof o1 === typeof o2", "o1.constructor === o2.constructor", + "p1 instanceof Number && p2 instanceof Number", + "Object.prototype.toString.call(p1) === Object.prototype.toString.call(p2)", "typeof f1 === typeof f2", "f1() === f2()", "r1.source === r2.source", "r1.flags === r2.flags", ],src/ast/KnownGlobal.zig (1)
80-149: Array rules look right; simplify the 0–10 check for maintainabilityLogic preserves sparse semantics and only literalizes when provably non-number (including object/array literals). The 0–10 numeric whitelist is correct but verbose; use an integer-range check to reduce churn.
Apply within the
.numberarm:- if ( - // only want this with whitespace minification - minify_whitespace and - (val == 0 or - val == 1 or - val == 2 or - val == 3 or - val == 4 or - val == 5 or - val == 6 or - val == 7 or - val == 8 or - val == 9 or - val == 10)) - { + if (minify_whitespace) { + const intval: usize = @intFromFloat(val); + if (@as(f64, @floatFromInt(intval)) == val and intval <= 10) { const arg_loc = arg.loc; var list = e.args.listManaged(allocator); list.clearRetainingCapacity(); - bun.handleOom(list.appendNTimes(js_ast.Expr{ .data = js_parser.Prefill.Data.EMissing, .loc = arg_loc }, @intFromFloat(val))); - return js_ast.Expr.init(E.Array, .{ .items = .fromList(list) }, loc); - } + bun.handleOom(list.appendNTimes( + js_ast.Expr{ .data = js_parser.Prefill.Data.EMissing, .loc = arg_loc }, + intval, + )); + return js_ast.Expr.init(E.Array, .{ .items = .fromList(list) }, loc); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/ast/KnownGlobal.zig(12 hunks)test/bundler/bundler_minify.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/ast/KnownGlobal.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast/KnownGlobal.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/bundler/bundler_minify.test.ts
test/bundler/**/*
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place bundler/transpiler/CSS/bun build tests under test/bundler/
Files:
test/bundler/bundler_minify.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/bundler/bundler_minify.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/bundler/bundler_minify.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/bundler/bundler_minify.test.ts
🧬 Code graph analysis (1)
test/bundler/bundler_minify.test.ts (1)
test/bundler/expectBundled.ts (1)
itBundled(1694-1728)
🔇 Additional comments (10)
test/bundler/bundler_minify.test.ts (3)
695-763: Solid coverage for de-newing Error constructorsThe expectations align with the implementation (strip
newfor Error-family; keep pure annotations on Date/Map/Set). Looks good.
765-806: Runtime assertions validate message/instanceof semanticsGood sanity checks; the stdout expectations exercise real behavior beyond snapshots.
808-855: Parities verified for Error() vs new Error()Nice cross-check of constructor parity and stack presence. No issues.
src/ast/KnownGlobal.zig (7)
11-22: Enum expansion looks correctNew variants cover the constructors used by the minifier. No issues spotted.
26-34: Helper extraction is clean
callFromNewpreserves args/paren loc/unwrapping flags; good reuse point.
36-43: Safe gating on unbound global identifiersEarly-exit on non-unbound symbols avoids rewriting shadowed constructors. Good.
44-49: De-newing Error-family is safe and wins bytesCorrectly funnels through
callFromNew. Matches JS semantics.
51-79: Object() rewrites preserve semantics; unwraps for literal args are correct
{}for 0-arg and null/undefined is valid.- Returning the single object/array arg maintains identity.
Fallback toObject(...)for other primitives is appropriate.
151-162: Leaving RegExp alone avoids subtle semantic trapsGood call to not de-new RegExp. Tests should include identity behavior to prevent regressions (suggested in test comments).
If desired, I can add the corresponding test case in this PR.
166-314: Purity marking for built-ins is conservative and safeThe
.can_be_unwrapped_if_unused = .if_unusedannotations look correct across Date/Set/Map/Weak* and web APIs. No rewrite is attempted where semantics can vary.
…1828) ### Problem - With `minify_syntax`, `new Array(5, ...rest)` becomes `[5, ...rest]`. When `rest` is empty the original means `new Array(5)`, five holes, and the fold is `[5]`. For `const none = []; const a = new Array(5, ...none); console.log(a.length, 0 in a)` Node prints `5 false`. Bun 1.4.3 prints `1 true` under `bun run` and in `bun build --minify-syntax` output. - The cause is the more-than-one-argument branch of `KnownGlobal::minify_global_constructor` (`src/ast/known_global.rs:244`). It folds the arguments into a literal without checking for a spread, so the argument count can differ at runtime. ### Fix - If any argument is a spread, emit `Array(5, ...rest)` instead of a literal. This is the `call_from_new` form the single-argument branch already uses when the argument may be a number. - Correct because `Array` called as a function behaves like `new Array` (ECMA-262 23.1.1). Only the literal fold was unsound. - `EXPECTED_VERSION` in `RuntimeTranspilerCache.rs` moves to 29: the runtime transpiler enables `minify_syntax`, so its cached output changes. - Verified: `test/bundler/bundler_minify.test.ts` (one case captures the output, one runs it, both fail on 1.4.3). Also `bundler_npm.test.ts` and `minify-new-array-with-if.test.ts`. ### Background - `minify_global_constructor` is the byte-saving rewrite from #22493: `new Object()` to `{}`, `new Array(1, 2)` to `[1, 2]`, and `new` dropped from constructors that behave the same when called. - `new Array(n)` with one number makes a sparse array of length `n`. Any other argument list makes an array of the arguments. A spread hides which case applies until runtime. - This hunk comes from #37388, closed in favour of #41580. It is independent of the stack-frame change there. <details><summary>Notes</summary> - `new Array(...xs)` alone was already safe: it is the single-argument branch and stays a call. - #41580 and #41159 also bump the transpiler cache version. Whichever lands second bumps again. - #41580 gates `minify_global_constructor` on `bundle`, which hides this under `bun run` but not in `bun build --minify` output. This PR fixes the fold itself, so it is needed either way. </details> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
Summary
maybeMarkConstructorAsPuretominifyGlobalConstructorthat returns?Exprnewnew Object()→{},new Array()→[], etc.Details
This PR refactors the existing
maybeMarkConstructorAsPurefunction tominifyGlobalConstructorand changes it to return an optional expression. This enables powerful minification optimizations for global constructors.Optimizations Added:
1. Error Constructors (4 bytes saved each)
new Error(...)→Error(...)new TypeError(...)→TypeError(...)new SyntaxError(...)→SyntaxError(...)new RangeError(...)→RangeError(...)new ReferenceError(...)→ReferenceError(...)new EvalError(...)→EvalError(...)new URIError(...)→URIError(...)new AggregateError(...)→AggregateError(...)2. Object Constructor
new Object()→{}(11 bytes saved)new Object({a: 1})→{a: 1}(11 bytes saved)new Object([1, 2])→[1, 2](11 bytes saved)new Object(null)→{}(15 bytes saved)new Object(undefined)→{}(20 bytes saved)3. Array Constructor
new Array()→[](10 bytes saved)new Array(1, 2, 3)→[1, 2, 3](9 bytes saved)new Array(5)→Array(5)(4 bytes saved, preserves sparse array semantics)4. Function and RegExp Constructors
new Function(...)→Function(...)(4 bytes saved)new RegExp(...)→RegExp(...)(4 bytes saved)Important Fixes:
new Array(number)callFromNewhelper to reduce code duplicationSize Impact:
Test plan
✅ All tests pass:
bundler_minify.test.tsnewbundler_npm.test.ts🤖 Generated with Claude Code