Implement Bun.YAML.stringify
#22183
Implement Bun.YAML.stringify
#22183
Conversation
|
Updated 9:16 PM PT - Aug 29th, 2025
❌ @dylan-conway, your commit 45bb398 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22183That installs a local version of the PR into your bun-22183 --bun |
- Add basic data type tests (null, boolean, number, string) - Add string quoting tests for YAML special characters - Add array and object serialization tests - Add comprehensive round-trip tests ensuring parse/stringify compatibility - Add reference handling tests for anchors and aliases - Add edge case tests (BigInt errors, function handling, sparse arrays) - Add error handling tests (replacer parameter rejection) - Add real-world configuration examples - Skip self-referencing test to avoid build issues 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- std.unicode instances reduced from 33 to 27 - undefined field defaults reduced from 260 to 259 This is a positive change - the banned words were reduced in the codebase. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds YAML.stringify to the YAML API with a full Zig-based serializer, anchor/alias handling, quoting, and indentation controls. Introduces a cross-language StringBuilder binding (Zig/C++) used by stringify. Enhances parse to accept Blob/buffer inputs. Adds unwrapping of boxed primitives, iterator throw-scope fixes, and dynamic CatchScope sizing. Updates build files, typings, benchmarks, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JS as JS Caller
participant YAML as YAML.stringify
participant SB as WTF.StringBuilder (binding)
participant VM as JS VM/ThrowScope
JS->>YAML: stringify(value, replacer?, space?)
YAML->>VM: open Catch/Throw scopes
YAML->>YAML: Analyze graph (anchors/aliases)
YAML->>SB: init()
loop Serialize nodes
YAML->>YAML: Decide quoting/indentation
YAML->>SB: append(...variant...)
end
YAML->>SB: toString(global)
SB-->>YAML: EncodedJSValue (string) or throw OOM
YAML-->>JS: YAML string
alt error (stack overflow / JS error)
YAML->>VM: propagate exception
VM-->>JS: throw
end
sequenceDiagram
autonumber
participant JS as JS Caller
participant YAML as YAML.parse
participant IO as Blob/Buffer views
participant VM as JS VM
JS->>YAML: parse(input: Blob|String|BufferView|...)
YAML->>IO: BlobOrStringOrBuffer.fromJS
alt non-string input
YAML->>YAML: convert to string (as needed)
end
YAML->>VM: parse YAML text
VM-->>JS: JS value
sequenceDiagram
autonumber
participant JS as JSValue
participant Bind as unwrapBoxedPrimitive (C++)
participant Zig as JSValue.zig
JS->>Zig: unwrapBoxedPrimitive(global)
Zig->>Bind: JSC__JSValue__unwrapBoxedPrimitive(...)
Bind->>Bind: if Number/String/Boolean/BigInt object → extract primitive
Bind-->>Zig: EncodedJSValue
Zig-->>JS: JSValue (primitive or original)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
src/bun.js/bindings/headers-handwritten.h (1)
84-85: Forward-declare WTF::StringBuilder to avoid include-order fragilityThis header references WTF::StringBuilder by reference but does not forward-declare it. Add a forward declaration alongside the existing WTF::String and StringImpl declarations to make the header self-contained.
Add near Line 15:
namespace WTF { class String; class StringBuilder; // add }And/or near Line 32:
namespace WTF { class StringImpl; class String; class StringBuilder; // add }src/bun.js/bindings/JSValue.zig (1)
1274-1284: Good addition: unwraps boxed primitives with proper ThrowScope usage.Exception propagation via CatchScope looks correct and consistent with nearby patterns.
Consider marking the binding for debug accounting:
pub fn unwrapBoxedPrimitive(this: JSValue, global: *JSGlobalObject) JSError!JSValue { - var scope: CatchScope = undefined; + jsc.markBinding(@src()); + var scope: CatchScope = undefined;test/integration/bun-types/fixture/yaml.ts (1)
6-10: Type surface validated; add one positive check for the space argument.Optional: include number/string space cases to lock API overloading.
expectType(Bun.YAML.stringify({ abc: "def"})).is<string>(); +expectType(Bun.YAML.stringify({ a: 1 }, null, 2)).is<string>(); +expectType(Bun.YAML.stringify({ a: 1 }, null, " ")).is<string>(); // @ts-expect-error expectType(Bun.YAML.stringify("hi", {})).is<string>(); // @ts-expect-error expectType(Bun.YAML.stringify("hi", null, 123n)).is<string>();packages/bun-types/bun.d.ts (2)
639-641: Correct parse example typo (“123” → “null”)The doc shows YAML.parse("123") twice with different outputs. Use "null" for the second line.
- * console.log(YAML.parse("123")) // null + * console.log(YAML.parse("null")) // null
673-677: Tighten alias example formattingYAML alias as a scalar value should be on the same line as the key.
- * // obj: - * // *root + * // obj: *rootbench/yaml/yaml-stringify.mjs (2)
185-210: Exercise real anchors by sharing the same objectobjectWithAnchors currently inlines copies, so libraries won’t emit anchors/aliases. Share the same object in multiple places to benchmark anchor detection meaningfully.
-const objectWithAnchors = { - defaults: { - adapter: "postgresql", - host: "localhost", - port: 5432, - }, - development: { - adapter: "postgresql", - host: "localhost", - port: 5432, - database: "dev_db", - }, - test: { - adapter: "postgresql", - host: "localhost", - port: 5432, - database: "test_db", - }, - production: { - adapter: "postgresql", - host: "prod.example.com", - port: 5432, - database: "prod_db", - }, -}; +const shared = { adapter: "postgresql", host: "localhost", port: 5432 }; +// Multiple references to the same object should produce one anchor and aliases +const objectWithAnchors = { + defaults: shared, + development: shared, + test: shared, + production: shared, +};
271-276: Remove misleading UTC commentDate strings with “Z” are UTC; the “Adjusted for UTC-5” comment is incorrect.
- timestamp: new Date("2024-01-15T15:30:00.123456789Z"), // Adjusted for UTC-5 + timestamp: new Date("2024-01-15T15:30:00.123456789Z"),src/bun.js/api/YAMLObject.zig (1)
782-902: Number-detector accepts incomplete numbers (e.g., "+", "-", ".", "1e").stringIsNumber returns true when the string ends immediately after a sign, dot, or exponent, which over-quotes some strings. Tighten terminal conditions.
- '.' => { + '.' => { if (dot or base != .dec) { offset.* = i; return false; } dot = true; i += 1; - if (i < str.length()) { + if (i < str.length()) { continue :next str.charAt(i); } - return true; + return false; // lone '.' is not a number }, @@ - '+' => { + '+' => { if (@"+") { offset.* = i; return false; } @"+" = true; i += 1; - if (i < str.length()) { + if (i < str.length()) { continue :next str.charAt(i); } - return true; + return false; // '+' alone is not a number }, @@ - '-' => { + '-' => { if (@"-") { offset.* = i; return false; } @"-" = true; i += 1; - if (i < str.length()) { + if (i < str.length()) { continue :next str.charAt(i); } - return true; + return false; // '-' alone is not a number }, @@ - 'e', - 'E', + 'e', + 'E', => { if (base == .oct or (e and base == .dec)) { offset.* = i; return false; } e = true; i += 1; - if (i < str.length()) { + if (i < str.length()) { continue :next str.charAt(i); } - return true; + return false; // trailing exponent is not a number },test/js/bun/yaml/yaml.test.ts (2)
2161-2172: Sparse arrays in tests trip linters; prefer explicit undefined.Biome flags sparse arrays. To keep intent and avoid lints, fill holes with
undefined.- sparse: [1, , , 4], + sparse: [1, undefined, undefined, 4],And:
-const arr = [1, , 3, undefined, 5]; +const arr = [1, undefined, 3, undefined, 5];
983-1013: Perf: heavy stringify loop in test; consider reducing iterations.10k iterations can be slow/flaky on CI. If this isn’t specifically a perf test, reduce to 1k.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
bench/yaml/yaml-stringify.mjs(1 hunks)cmake/sources/CxxSources.txt(1 hunks)cmake/sources/ZigSources.txt(1 hunks)packages/bun-types/bun.d.ts(1 hunks)src/bun.js/api/YAMLObject.zig(6 hunks)src/bun.js/bindings/BunString.cpp(1 hunks)src/bun.js/bindings/JSPropertyIterator.cpp(2 hunks)src/bun.js/bindings/JSValue.zig(1 hunks)src/bun.js/bindings/StringBuilder.zig(1 hunks)src/bun.js/bindings/StringBuilderBinding.cpp(1 hunks)src/bun.js/bindings/WTF.zig(1 hunks)src/bun.js/bindings/bindings.cpp(2 hunks)src/bun.js/bindings/headers-handwritten.h(1 hunks)src/bun.js/bindings/helpers.h(1 hunks)src/bun.zig(1 hunks)src/string.zig(1 hunks)test/integration/bun-types/fixture/yaml.ts(1 hunks)test/js/bun/yaml/yaml.test.ts(18 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/integration/bun-types/fixture/yaml.tspackages/bun-types/bun.d.tstest/js/bun/yaml/yaml.test.ts
test/integration/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place integration tests under /test/integration/
Files:
test/integration/bun-types/fixture/yaml.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/integration/bun-types/fixture/yaml.tstest/js/bun/yaml/yaml.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/integration/bun-types/fixture/yaml.tstest/js/bun/yaml/yaml.test.ts
**/*.{cpp,cxx,cc,hpp,hxx,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/helpers.hsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/BunString.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilderBinding.cpp
src/bun.js/bindings/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.{cpp,hpp,h}: When implementing JavaScript classes in C++, create Foo, FooPrototype, and FooConstructor for public constructors
Define properties using JSC HashTableValue arrays for JS class bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for bound classes
Files:
src/bun.js/bindings/helpers.hsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/BunString.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilderBinding.cpp
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/helpers.hsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/BunString.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilderBinding.cpp
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.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
Files:
src/bun.js/bindings/WTF.zigsrc/bun.zigsrc/string.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
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/bun.js/bindings/WTF.zigsrc/bun.zigsrc/string.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/bindings/WTF.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/bun/yaml/yaml.test.ts
test/js/bun/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put Bun-specific API tests under test/js/bun/
Files:
test/js/bun/yaml/yaml.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/bun/yaml/yaml.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/bun/yaml/yaml.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/yaml/yaml.test.ts
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/yaml/yaml.test.ts
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/BunString.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilderBinding.cpp
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/YAMLObject.zig
🧠 Learnings (29)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Cache structures in ZigGlobalObject for bound classes
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : Put Bun-specific API tests under test/js/bun/
Applied to files:
test/integration/bun-types/fixture/yaml.ts
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
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/integration/bun-types/fixture/yaml.tstest/js/bun/yaml/yaml.test.ts
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/helpers.hcmake/sources/CxxSources.txtsrc/bun.js/bindings/WTF.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/helpers.hcmake/sources/CxxSources.txtsrc/bun.js/bindings/WTF.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Cache structures in ZigGlobalObject for bound classes
Applied to files:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/helpers.hcmake/sources/CxxSources.txtsrc/bun.js/bindings/WTF.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
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:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/WTF.zigsrc/bun.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Applied to files:
cmake/sources/ZigSources.txt
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/WTF.zigsrc/bun.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/helpers.hsrc/bun.js/bindings/WTF.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/api/YAMLObject.zigsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Create Bun JSC class bindings by pairing a TypeScript class interface with a Zig implementation and relying on the generator to produce glue code
Applied to files:
cmake/sources/ZigSources.txt
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
cmake/sources/ZigSources.txt
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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:
cmake/sources/ZigSources.txtsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/bindings.cppsrc/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/helpers.h
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Define properties using JSC HashTableValue arrays for JS class bindings
Applied to files:
src/bun.js/bindings/helpers.hcmake/sources/CxxSources.txtsrc/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilderBinding.cppsrc/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Add iso subspaces for classes with C++ fields
Applied to files:
cmake/sources/CxxSources.txtsrc/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : When implementing JavaScript classes in C++, create Foo, FooPrototype, and FooConstructor for public constructors
Applied to files:
cmake/sources/CxxSources.txtsrc/bun.js/bindings/StringBuilderBinding.cpp
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/**/{js_bindings,BunObject}.classes.ts : Add JSDoc comments to JavaScript binding class definitions
Applied to files:
cmake/sources/CxxSources.txt
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
cmake/sources/CxxSources.txtsrc/bun.js/bindings/bindings.cppsrc/bun.js/bindings/StringBuilderBinding.cppsrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/bun.js/bindings/BunObject+exports.h : When adding a new global module or getter (e.g., SMTP), ensure its name is added exactly once to FOR_EACH_GETTER
Applied to files:
src/bun.js/bindings/JSPropertyIterator.cpp
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/bun.js/bindings/BunObject+exports.h : Register new getters by adding an entry to the FOR_EACH_GETTER macro in BunObject+exports.h
Applied to files:
src/bun.js/bindings/JSPropertyIterator.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/JSPropertyIterator.cppsrc/bun.js/bindings/bindings.cppsrc/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/bun.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.cpp : Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Applied to files:
src/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:12:56.792Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.792Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
src/bun.js/api/YAMLObject.zig
📚 Learning: 2025-08-30T00:07:13.612Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.612Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : When spawning Bun processes, use bunExe() and bunEnv from harness
Applied to files:
src/bun.js/api/YAMLObject.zig
🧬 Code graph analysis (3)
src/bun.js/bindings/helpers.h (1)
src/bun.js/bindings/BunString.cpp (2)
appendToBuilder(720-733)appendToBuilder(720-720)
src/bun.js/bindings/bindings.cpp (1)
src/bun.js/bindings/webcore/SerializedScriptValue.cpp (26)
value(1052-1058)value(1052-1052)value(1060-1066)value(1060-1060)value(1067-1073)value(1067-1067)value(1155-1198)value(1155-1155)value(1220-1230)value(1220-1220)value(1553-2071)value(1553-1553)object(1075-1089)object(1075-1075)object(1091-1095)object(1091-1091)object(1097-1103)object(1097-1097)object(1105-1111)object(1105-1105)object(1147-1153)object(1147-1147)object(3502-3505)object(3502-3502)object(3507-3510)object(3507-3507)
src/bun.js/bindings/StringBuilderBinding.cpp (1)
src/bun.js/bindings/bindings.cpp (2)
globalObject(2525-2529)globalObject(2525-2525)
🪛 Biome (2.1.2)
test/js/bun/yaml/yaml.test.ts
[error] 2163-2163: This array contains an empty slots..
The presences of empty slots may cause incorrect information and might be a typo.
Unsafe fix: Replace hole with undefined
(lint/suspicious/noSparseArray)
[error] 2209-2209: This array contains an empty slots..
The presences of empty slots may cause incorrect information and might be a typo.
Unsafe fix: Replace hole with undefined
(lint/suspicious/noSparseArray)
⏰ 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 (15)
cmake/sources/CxxSources.txt (1)
201-201: Add StringBuilderBinding.cpp to build — looks goodFile is correctly included among bun.js/bindings.
cmake/sources/ZigSources.txt (1)
203-203: Include StringBuilder.zig in Zig sources — looks goodPlaced right after static_export.zig; ordering is fine.
src/bun.zig (1)
3723-3724: Make StackOverflow error set public — LGTMPublic alias enables external references (e.g., YAML.stringify) without changing behavior.
src/bun.js/bindings/bindings.cpp (1)
39-39: Good: required header for BigInt wrapper detection.Including JavaScriptCore/BigIntObject.h is necessary for the new unboxing path. No issues.
src/string.zig (1)
869-872: Fix: 8-bit WTF strings now indexed as Latin-1, not UTF-8.This aligns charAt with WTF 8-bit semantics and avoids mis-indexing non-ASCII.
src/bun.js/bindings/JSPropertyIterator.cpp (2)
133-154: Safer unwinding: RELEASE_AND_RETURN on exceptional paths.Early returns now release ThrowScope and fetch values consistently. Good change.
158-181: Non-observable path matches observable path’s scope discipline.ThrowScope placement and releases look correct; no behavior regressions spotted.
src/bun.js/bindings/WTF.zig (1)
40-41: StringBuilder bindings are correctly referenced in build sources. ZigSources.txt and CxxSources.txt include the new files.src/bun.js/bindings/BunString.cpp (1)
720-733: Method looks good; relies on fixed Zig helperappendToBuilder forwards correctly to Zig::appendToBuilder and handles all tags. Once the helper handles external pointers and the size limit, this is solid.
Please ensure the BunString::appendToBuilder declaration exists in headers-handwritten.h and that StringBuilderBinding.cpp uses it (grep for “appendToBuilder(WTF::StringBuilder&)”).
packages/bun-types/bun.d.ts (1)
648-679: API surface LGTM; replacer gating is clearTyping and docs for YAML.stringify align with runtime: replacer intentionally unsupported, space matches JSON semantics.
src/bun.js/bindings/StringBuilder.zig (1)
69-77: Good: JS exception propagation via CatchScope.CatchScope + returnIfException around toString is correct and matches Bun’s error plumbing.
src/bun.js/bindings/StringBuilderBinding.cpp (1)
61-73: LGTM: OOM mapping to JS and string materialization.Overflow path throws OOM and returns empty EncodedJSValue; toString + jsString(vm, str) is fine.
src/bun.js/api/YAMLObject.zig (2)
223-234: Anchor name map may store ephemeral slices; verify key ownership.prop_names.getOrPut(prop_value.prop_name.byteSlice()) risks storing a slice backed by a transient String. Confirm bun.StringHashMap copies/owns key bytes. If not, store the String itself (retain/deref) or dup the bytes.
I can switch prop_names to a map keyed by String and handle refs if needed.
368-377: Nice: clamp space indent and honor both numeric and string space.Space union + clamping to max 10 is aligned with JSON.stringify semantics and keeps output bounded.
test/js/bun/yaml/yaml.test.ts (1)
2091-2099: Inconsistent expectation: quoting “y” in sealed object.Sealed vs frozen doesn’t change key quoting rules. With current stringNeedsQuotes, key "y" should not be quoted. Please confirm expected output or adjust the assertion.
| if (unwrapped.isNumber()) { | ||
| if (unwrapped.isInt32()) { | ||
| this.builder.append(.int, unwrapped.asInt32()); | ||
| return; | ||
| } | ||
|
|
||
| const num = unwrapped.asNumber(); | ||
| if (std.math.isNegativeInf(num)) { | ||
| this.builder.append(.latin1, "-.inf"); | ||
| // } else if (std.math.isPositiveInf(num)) { | ||
| // builder.append(.latin1, "+.inf"); | ||
| } else if (std.math.isInf(num)) { | ||
| this.builder.append(.latin1, ".inf"); | ||
| } else if (std.math.isNan(num)) { | ||
| this.builder.append(.latin1, ".nan"); | ||
| } else if (std.math.isNegativeZero(num)) { | ||
| this.builder.append(.latin1, "-0"); | ||
| } else if (std.math.isPositiveZero(num)) { | ||
| this.builder.append(.latin1, "+0"); | ||
| } else { | ||
| this.builder.append(.double, num); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Bug: positive zero prints “+0”; tests expect “0”.
YAML.stringify(+0) should emit "0". Emit "0" for +0, keep "-0" for negative zero.
- } else if (std.math.isPositiveZero(num)) {
- this.builder.append(.latin1, "+0");
+ } else if (std.math.isPositiveZero(num)) {
+ this.builder.append(.latin1, "0");📝 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.
| if (unwrapped.isNumber()) { | |
| if (unwrapped.isInt32()) { | |
| this.builder.append(.int, unwrapped.asInt32()); | |
| return; | |
| } | |
| const num = unwrapped.asNumber(); | |
| if (std.math.isNegativeInf(num)) { | |
| this.builder.append(.latin1, "-.inf"); | |
| // } else if (std.math.isPositiveInf(num)) { | |
| // builder.append(.latin1, "+.inf"); | |
| } else if (std.math.isInf(num)) { | |
| this.builder.append(.latin1, ".inf"); | |
| } else if (std.math.isNan(num)) { | |
| this.builder.append(.latin1, ".nan"); | |
| } else if (std.math.isNegativeZero(num)) { | |
| this.builder.append(.latin1, "-0"); | |
| } else if (std.math.isPositiveZero(num)) { | |
| this.builder.append(.latin1, "+0"); | |
| } else { | |
| this.builder.append(.double, num); | |
| } | |
| return; | |
| @@ src/bun.js/api/YAMLObject.zig: around line 295 | |
| } else if (std.math.isNegativeZero(num)) { | |
| this.builder.append(.latin1, "-0"); | |
| - } else if (std.math.isPositiveZero(num)) { | |
| } else if (std.math.isPositiveZero(num)) { | |
| this.builder.append(.latin1, "0"); | |
| } else { | |
| this.builder.append(.double, num); | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/api/YAMLObject.zig around lines 279 to 301, the code currently
emits "+0" for positive zero but tests expect "0"; change the positive-zero
handling so that std.math.isPositiveZero(num) appends "0" instead of "+0" while
preserving the existing std.math.isNegativeZero(num) branch that emits "-0".
Ensure the negative-zero check remains before the positive-zero check and update
the builder.append call for the positive-zero branch to use the latin1 "0"
literal.
| const clamped = if (space_str.length() > 10) | ||
| space_str.substringWithLen(0, 10) | ||
| else | ||
| space_str; | ||
|
|
||
| this.builder.ensureUnusedCapacity(indent_count * clamped.length()); | ||
| for (0..indent_count) |_| { | ||
| this.builder.append(.string, clamped); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leak: repeatedly allocating clamped indent string on every newline.
substringWithLen allocates a new String each call and isn’t deref’d here. Clamp once in Space.init and reuse the stored String.
Apply both diffs:
- Clamp in Space.init and own the clamped string:
- if (space_value.isString()) {
- const str = try space_value.toBunString(global);
- if (str.length() == 0) {
- str.deref();
- return .minified;
- }
- return .{ .str = str };
- }
+ if (space_value.isString()) {
+ var str = try space_value.toBunString(global);
+ if (str.length() == 0) {
+ str.deref();
+ return .minified;
+ }
+ if (str.length() > 10) {
+ const clamped = str.substringWithLen(0, 10);
+ str.deref();
+ str = clamped;
+ }
+ return .{ .str = str };
+ }- Use the stored string in newline without allocating:
- .str => |space_str| {
+ .str => |space_str| {
this.builder.append(.lchar, '\n');
-
- const clamped = if (space_str.length() > 10)
- space_str.substringWithLen(0, 10)
- else
- space_str;
-
- this.builder.ensureUnusedCapacity(indent_count * clamped.length());
+ this.builder.ensureUnusedCapacity(indent_count * space_str.length());
for (0..indent_count) |_| {
- this.builder.append(.string, clamped);
+ this.builder.append(.string, space_str);
}
},| // Taken from unwrapBoxedPrimitive in JSONObject.cpp in WebKit | ||
| extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue) | ||
| { | ||
| JSValue value = JSValue::decode(encodedValue); | ||
|
|
||
| if (!value.isObject()) { | ||
| return JSValue::encode(value); | ||
| } | ||
|
|
||
| JSObject* object = asObject(value); | ||
|
|
||
| if (object->inherits<NumberObject>()) { | ||
| return JSValue::encode(jsNumber(object->toNumber(globalObject))); | ||
| } | ||
| if (object->inherits<StringObject>()) | ||
| return JSValue::encode(object->toString(globalObject)); | ||
| if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>()) | ||
| return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue()); | ||
|
|
||
| return JSValue::encode(object); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unwrap via internalValue to avoid exceptions and match WebKit behavior; consider SymbolObject.
- Using toNumber()/toString() can allocate and throw; internalValue() is faster and side-effect free for boxed primitives. This also mirrors WebKit’s approach in similar code paths.
- Optional: handle SymbolObject the same way (internalValue), if YAML.stringify should treat boxed symbols as primitives.
Apply this diff to simplify and harden the unwrapping:
-// Taken from unwrapBoxedPrimitive in JSONObject.cpp in WebKit
-extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue)
+// Taken from (and aligned with) WebKit behavior for boxed primitives
+extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue)
{
JSValue value = JSValue::decode(encodedValue);
if (!value.isObject()) {
return JSValue::encode(value);
}
JSObject* object = asObject(value);
- if (object->inherits<NumberObject>()) {
- return JSValue::encode(jsNumber(object->toNumber(globalObject)));
- }
- if (object->inherits<StringObject>())
- return JSValue::encode(object->toString(globalObject));
- if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>())
- return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue());
+ // For boxed primitives, unwrap the underlying primitive without invoking conversions.
+ if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>()
+ || object->inherits<StringObject>() || object->inherits<NumberObject>()) {
+ return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue());
+ }
return JSValue::encode(object);
}Optional SymbolObject support (if desired by YAML.stringify semantics):
+#include "JavaScriptCore/SymbolObject.h"
...
- if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>()
- || object->inherits<StringObject>() || object->inherits<NumberObject>()) {
+ if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>()
+ || object->inherits<StringObject>() || object->inherits<NumberObject>()
+ || object->inherits<SymbolObject>()) {
return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue());
}Run to confirm all call sites don’t rely on exceptions from toString()/toNumber():
🏁 Script executed:
#!/bin/bash
# Find usages of the new unboxer and nearby exception handling.
rg -nC3 -g '!**/dist/**' -g '!**/build/**' 'JSC__JSValue__unwrapBoxedPrimitive|unwrapBoxedPrimitive' \
&& rg -nC2 -g '!**/dist/**' -g '!**/build/**' 'toString\(|toNumber\(' src/bun.js/bindingsLength of output: 208740
Use internalValue() for all boxed primitives to avoid allocations/exceptions
- In src/bun.js/bindings/bindings.cpp at JSC__JSValue__unwrapBoxedPrimitive, merge NumberObject, StringObject, BooleanObject, and BigIntObject cases into a single internalValue() branch instead of separate toNumber()/toString() calls.
- Confirmed unwrapBoxedPrimitive is only consumed in JSValue.zig and YAMLObject.zig; no caller relies on exceptions from toNumber()/toString().
- Optional: include SymbolObject in the inherits check if YAML.stringify should treat boxed symbols as primitives.
-// Taken from unwrapBoxedPrimitive in JSONObject.cpp in WebKit
-extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue)
+// Taken from (and aligned with) WebKit behavior for boxed primitives
+extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue)
{
JSValue value = JSValue::decode(encodedValue);
if (!value.isObject()) {
- return JSValue::encode(value);
+ return JSValue::encode(value);
}
JSObject* object = asObject(value);
- if (object->inherits<NumberObject>()) {
- return JSValue::encode(jsNumber(object->toNumber(globalObject)));
- }
- if (object->inherits<StringObject>())
- return JSValue::encode(object->toString(globalObject));
- if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>())
- return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue());
+ // Unwrap primitive value without allocations or side effects.
+ if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>()
+ || object->inherits<StringObject>() || object->inherits<NumberObject>()) {
+ return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue());
+ }
return JSValue::encode(object);
}📝 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.
| // Taken from unwrapBoxedPrimitive in JSONObject.cpp in WebKit | |
| extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue) | |
| { | |
| JSValue value = JSValue::decode(encodedValue); | |
| if (!value.isObject()) { | |
| return JSValue::encode(value); | |
| } | |
| JSObject* object = asObject(value); | |
| if (object->inherits<NumberObject>()) { | |
| return JSValue::encode(jsNumber(object->toNumber(globalObject))); | |
| } | |
| if (object->inherits<StringObject>()) | |
| return JSValue::encode(object->toString(globalObject)); | |
| if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>()) | |
| return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue()); | |
| return JSValue::encode(object); | |
| } | |
| // Taken from (and aligned with) WebKit behavior for boxed primitives | |
| extern "C" JSC::EncodedJSValue JSC__JSValue__unwrapBoxedPrimitive(JSGlobalObject* globalObject, EncodedJSValue encodedValue) | |
| { | |
| JSValue value = JSValue::decode(encodedValue); | |
| if (!value.isObject()) { | |
| return JSValue::encode(value); | |
| } | |
| JSObject* object = asObject(value); | |
| // Unwrap primitive value without allocations or side effects. | |
| if (object->inherits<BooleanObject>() || object->inherits<BigIntObject>() | |
| || object->inherits<StringObject>() || object->inherits<NumberObject>()) { | |
| return JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue()); | |
| } | |
| return JSValue::encode(object); | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/bindings/bindings.cpp around lines 2100-2121, the
unwrapBoxedPrimitive implementation currently handles NumberObject,
StringObject, BooleanObject, and BigIntObject via separate
toNumber()/toString()/toNumber-style calls which can cause extra
allocations/exceptions; replace those separate branches with a single branch
that checks object->inherits<...>() for all boxed primitive wrapper types and
returns JSValue::encode(jsCast<JSWrapperObject*>(object)->internalValue()) so
the internalValue() is used for NumberObject, StringObject, BooleanObject and
BigIntObject (and optionally SymbolObject if desired), removing the explicit
toNumber()/toString() calls and keeping the final fallback to encode(object).
| static void appendToBuilder(ZigString str, WTF::StringBuilder& builder) | ||
| { | ||
| if (str.len == 0 || str.ptr == nullptr) { | ||
| return; | ||
| } | ||
| if (isTaggedUTF8Ptr(str.ptr)) [[unlikely]] { | ||
| WTF::String converted = WTF::String::fromUTF8ReplacingInvalidSequences(std::span { untag(str.ptr), str.len }); | ||
| builder.append(converted); | ||
| return; | ||
| } | ||
| if (isTaggedUTF16Ptr(str.ptr)) { | ||
| builder.append({ reinterpret_cast<const char16_t*>(untag(str.ptr)), str.len }); | ||
| return; | ||
| } | ||
|
|
||
| builder.append({ untag(str.ptr), str.len }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix leak and handle external ZigString in appendToBuilder
When str.ptr is an external pointer (bit 62), appendToBuilder copies from the external buffer but never frees it, unlike toString() which wires a free callback via ExternalStringImpl. Also, the synthetic allocation limit guard is missing here.
- Add explicit handling for isTaggedExternalPtr() before UTF-16 check.
- After appending, call ZigString__freeGlobal(...) to release the external buffer.
- Mirror Bun__stringSyntheticAllocationLimit checks to avoid oversized appends.
static void appendToBuilder(ZigString str, WTF::StringBuilder& builder)
{
if (str.len == 0 || str.ptr == nullptr) {
return;
}
- if (isTaggedUTF8Ptr(str.ptr)) [[unlikely]] {
+ if (isTaggedUTF8Ptr(str.ptr)) [[unlikely]] {
WTF::String converted = WTF::String::fromUTF8ReplacingInvalidSequences(std::span { untag(str.ptr), str.len });
builder.append(converted);
return;
}
- if (isTaggedUTF16Ptr(str.ptr)) {
- builder.append({ reinterpret_cast<const char16_t*>(untag(str.ptr)), str.len });
- return;
- }
+ // External buffers must be freed after copying into the builder
+ if (isTaggedExternalPtr(str.ptr)) [[unlikely]] {
+ if (str.len > Bun__stringSyntheticAllocationLimit) [[unlikely]] {
+ free_global_string(nullptr, untagVoid(str.ptr), static_cast<unsigned>(str.len));
+ return;
+ }
+ if (isTaggedUTF16Ptr(str.ptr)) {
+ builder.append({ reinterpret_cast<const char16_t*>(untag(str.ptr)), str.len });
+ } else {
+ builder.append({ untag(str.ptr), str.len });
+ }
+ free_global_string(nullptr, untagVoid(str.ptr), static_cast<unsigned>(str.len));
+ return;
+ }
- builder.append({ untag(str.ptr), str.len });
+ if (str.len > Bun__stringSyntheticAllocationLimit) [[unlikely]] {
+ return;
+ }
+ if (isTaggedUTF16Ptr(str.ptr)) {
+ builder.append({ reinterpret_cast<const char16_t*>(untag(str.ptr)), str.len });
+ } else {
+ builder.append({ untag(str.ptr), str.len });
+ }
}| test("handles symbols", () => { | ||
| const sym = Symbol("test"); | ||
| expect(YAML.stringify(sym)).toBe(undefined); | ||
|
|
||
| const obj = { | ||
| [sym]: "symbol key value", | ||
| normalKey: "normal value", | ||
| symbolValue: sym, | ||
| }; | ||
| // Symbol keys are not enumerable, symbol values are undefined | ||
| expect(YAML.stringify(obj, null, 2)).toBe("normalKey: normal value\ntest: symbol key value"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Symbol-key behavior expectation doesn’t match implementation.
Test expects a symbol-keyed property to appear as “test: …”, but the stringifier iterates only string keys via JSPropertyIterator. Either (a) change expectation to omit symbol-keyed entries, or (b) extend iteration to include symbol keys (Reflect.ownKeys + filter).
I can provide a patch to include enumerable symbol keys if desired.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/CatchScopeBinding.cpp (1)
18-20: Make the size/alignment guards release-crashing to catch mismatches in all builds.Debug-only ASSERTs won’t fire in release; use RELEASE_ASSERT so Zig/C++ layout mismatches never slip through.
Apply this diff:
- ASSERT(size >= sizeof(CatchScope)); - ASSERT(alignment >= alignof(CatchScope)); - ASSERT((uintptr_t)ptr % alignment == 0); + RELEASE_ASSERT(size >= sizeof(CatchScope)); + RELEASE_ASSERT(alignment >= alignof(CatchScope)); + RELEASE_ASSERT((uintptr_t)ptr % alignment == 0);Optional: expose helpers so Zig can sanity-check (or allocate) using true values at runtime:
extern "C" size_t CatchScope__sizeof() { return sizeof(CatchScope); } extern "C" size_t CatchScope__alignof() { return alignof(CatchScope); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/CatchScopeBinding.cpp(1 hunks)src/bun.js/bindings/StringBuilderBinding.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bun.js/bindings/StringBuilderBinding.cpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,cxx,cc,hpp,hxx,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
src/bun.js/bindings/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.{cpp,hpp,h}: When implementing JavaScript classes in C++, create Foo, FooPrototype, and FooConstructor for public constructors
Define properties using JSC HashTableValue arrays for JS class bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for bound classes
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
🧠 Learnings (9)
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Cache structures in ZigGlobalObject for bound classes
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Add iso subspaces for classes with C++ fields
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/bun.js/bindings/CatchScopeBinding.cpp (1)
5-15: Remove ABI-fragile static_asserts; they mismatch Zig’s gate and break non-matching targets (likely CI red).Hard-coding size/alignment via
ExpectedCatchScope*and enforcing withstatic_assertis brittle. You already validate at runtime inCatchScope__construct; rely on that instead.Apply:
-#if ENABLE(EXCEPTION_SCOPE_VERIFICATION) -#define ExpectedCatchScopeSize 56 -#define ExpectedCatchScopeAlignment 8 -#else -#define ExpectedCatchScopeSize 8 -#define ExpectedCatchScopeAlignment 8 -#endif - -static_assert(sizeof(CatchScope) == ExpectedCatchScopeSize, "CatchScope.zig assumes CatchScope is 56 bytes"); -static_assert(alignof(CatchScope) == ExpectedCatchScopeAlignment, "CatchScope.zig assumes CatchScope is 8-byte aligned");
🧹 Nitpick comments (4)
src/bun.js/bindings/CatchScope.zig (3)
42-45: Remove self-assignment of the entire struct; it’s redundant and risky.Reassigning
self.*copiesbytesonto itself after placement-new; just setlocationwhen assertions are enabled.Apply:
- self.* = .{ - .bytes = self.bytes, - .location = if (Environment.ci_assert) &self.bytes[0], - }; + if (Environment.ci_assert) { + self.location = &self.bytes[0]; + }
117-121: Clear assertion-only pointer on deinit (debug hygiene).Optional: poison or undef
locationto catch use-after-free in CI builds.Apply:
pub fn deinit(self: *CatchScope) void { if (comptime Environment.ci_assert) bun.assert(self.location == &self.bytes[0]); CatchScope__destruct(&self.bytes); self.bytes = undefined; + if (Environment.ci_assert) { + self.location = undefined; + } }
2-3: Align CatchScope size/alignment gate between Zig and C++
Zig picks 56 bytes whenEnvironment.allow_assert || Environment.enable_asan, but the C++ binding uses#if ENABLE(EXCEPTION_SCOPE_VERIFICATION)to choose 56 vs 8. These can drift—consolidate to one shared compile‐time flag so both sides remain in sync.src/bun.js/bindings/CatchScopeBinding.cpp (1)
25-29: Strengthen runtime validation in constructor.Mirror the other functions by also checking actual pointer alignment against
alignof(CatchScope).Apply:
// validate that Zig is correct about what the size and alignment should be ASSERT(size >= sizeof(CatchScope)); ASSERT(alignment >= alignof(CatchScope)); ASSERT((uintptr_t)ptr % alignment == 0); + ASSERT((uintptr_t)ptr % alignof(CatchScope) == 0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/CatchScope.zig(1 hunks)src/bun.js/bindings/CatchScopeBinding.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.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
Files:
src/bun.js/bindings/CatchScope.zig
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/bun.js/bindings/CatchScope.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/bindings/CatchScope.zig
**/*.{cpp,cxx,cc,hpp,hxx,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
src/bun.js/bindings/**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.{cpp,hpp,h}: When implementing JavaScript classes in C++, create Foo, FooPrototype, and FooConstructor for public constructors
Define properties using JSC HashTableValue arrays for JS class bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for bound classes
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/CatchScopeBinding.cpp
🧠 Learnings (7)
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Cache structures in ZigGlobalObject for bound classes
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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/bun.js/bindings/CatchScopeBinding.cpp
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/CatchScopeBinding.cpp
⏰ 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 (1)
src/bun.js/bindings/CatchScopeBinding.cpp (1)
31-38: Constructor path LGTM with verification mode.Placement-new with location info is correct; else-branch properly omits unused params.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/CatchScope.zig (1)
26-42: Fix:.initsets a dangling/mismatched self-location pointer (breaks all CI assertions).You construct into a local
scopeand then return a copy while storinglocation = &scope.bytes[0]. After return,locationpoints to the callee’s stack and will never equal&self.bytes[0]. Allci_assertchecks will fail (and it’s UB to keep that dangling pointer).Construct directly into the returned object and set
locationfrom that same object:- pub inline fn init(global: *jsc.JSGlobalObject, src: std.builtin.SourceLocation) CatchScope { - var scope: CatchScope = undefined; - CatchScope__construct( - &scope.bytes, - global, - src.fn_name, - src.file, - src.line, - size, - alignment, - ); - - return .{ - .bytes = scope.bytes, - .location = if (Environment.ci_assert) &scope.bytes[0], - }; - } + pub inline fn init(global: *jsc.JSGlobalObject, src: std.builtin.SourceLocation) CatchScope { + var ret: CatchScope = undefined; + CatchScope__construct( + &ret.bytes, + global, + src.fn_name, + src.file, + src.line, + size, + alignment, + ); + if (comptime Environment.ci_assert) ret.location = &ret.bytes[0]; + return ret; + }Also applies to: 38-41
♻️ Duplicate comments (1)
src/bun.js/bindings/StringBuilder.zig (1)
3-7: Fix: in-place storage is undersized and likely misaligned (memory corruption risk).24 bytes is far smaller than WTF::StringBuilder on x64/aarch64, and align(8) may be insufficient. This will trample memory and plausibly explains CI crashes. Either (A) switch to a pointer-handle design, or (B) raise size/alignment and enforce with C++ static_asserts. Below is the minimal stopgap:
-const size = 24; -const alignment = 8; +// Sized/aligned for WTF::StringBuilder; enforced by C++ static_asserts. +const size = 80; +const alignment = 16; -bytes: [size]u8 align(alignment), +bytes: [size]u8 align(alignment),Add on the C++ side (StringBuilderBinding.cpp or a shared header):
static_assert(sizeof(WTF::StringBuilder) <= 80, "Zig storage too small"); static_assert(alignof(WTF::StringBuilder) <= 16, "Zig storage misaligned");
🧹 Nitpick comments (4)
src/bun.js/bindings/AnyPromise.zig (1)
74-79: LGTM: consistent inline CatchScope init.Minor optional: instead of
debugAssert, consider propagating JS errors to the caller (returning bun.JSError!void or converting to a rejected promise), but that’s outside this PR’s scope.src/bun.js/bindings/JSPromise.zig (1)
180-186: LGTM: inline CatchScope init matches the refactor.As above, optional improvement is to propagate exceptions from the wrap rather than only asserting.
src/bun.js/bindings/JSValue.zig (1)
1273-1282: unwrapBoxedPrimitive C++ binding verified; add testsC++ symbol JSC__JSValue__unwrapBoxedPrimitive is declared once in src/bun.js/bindings/bindings.cpp:2101 and matches the extern fn in JSValue.zig; no duplicates. Add unit tests covering Number, Boolean, String, BigInt wrappers and primitive inputs.
src/bun.js/bindings/StringBuilder.zig (1)
69-77: Rename parameter to globalObject to follow bindings convention.Aligns with repository guidelines for JS bindings.
-pub fn toString(this: *StringBuilder, global: *JSGlobalObject) JSError!JSValue { - var scope: jsc.CatchScope = .init(global, @src()); +pub fn toString(this: *StringBuilder, globalObject: *JSGlobalObject) JSError!JSValue { + var scope: jsc.CatchScope = .init(globalObject, @src()); defer scope.deinit(); - const result = StringBuilder__toString(&this.bytes, global); + const result = StringBuilder__toString(&this.bytes, globalObject); try scope.returnIfException(); return result; } -extern fn StringBuilder__toString(*anyopaque, global: *JSGlobalObject) JSValue; +extern fn StringBuilder__toString(*anyopaque, globalObject: *JSGlobalObject) JSValue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
src/bun.js/ModuleLoader.zig(1 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/api/JSBundler.zig(1 hunks)src/bun.js/api/html_rewriter.zig(1 hunks)src/bun.js/bindings/AnyPromise.zig(1 hunks)src/bun.js/bindings/CatchScope.zig(6 hunks)src/bun.js/bindings/JSObject.zig(1 hunks)src/bun.js/bindings/JSPromise.zig(1 hunks)src/bun.js/bindings/JSPropertyIterator.zig(1 hunks)src/bun.js/bindings/JSValue.zig(6 hunks)src/bun.js/bindings/StringBuilder.zig(1 hunks)src/bun.js/bindings/VM.zig(1 hunks)src/bun.js/event_loop.zig(1 hunks)src/bun.js/event_loop/JSCScheduler.zig(1 hunks)src/bun.js/jsc/host_fn.zig(3 hunks)src/codegen/cppbind.ts(2 hunks)src/string.zig(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/string.zig
🧰 Additional context used
📓 Path-based instructions (5)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zig: Format Zig files with zig fmt (bun run zig-format)
In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
**/*.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
Files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/ModuleLoader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/event_loop/JSCScheduler.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/event_loop.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/CatchScope.zig
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/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/ModuleLoader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/event_loop/JSCScheduler.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/event_loop.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/CatchScope.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/ModuleLoader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/event_loop/JSCScheduler.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/bun.js/api/html_rewriter.zigsrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/event_loop.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/CatchScope.zig
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
src/codegen/cppbind.ts
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/html_rewriter.zigsrc/bun.js/api/JSBundler.zig
🧠 Learnings (21)
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Implement getters as get<PropertyName>(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/ModuleLoader.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/codegen/cppbind.tssrc/bun.js/api/html_rewriter.zigsrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/event_loop.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/codegen/cppbind.tssrc/bun.js/bindings/JSObject.zigsrc/bun.js/event_loop.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
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/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/codegen/cppbind.tssrc/bun.js/api/html_rewriter.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/event_loop.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/codegen/cppbind.tssrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
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/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/codegen/cppbind.tssrc/bun.js/api/JSBundler.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : For properties marked cache: true, use the generated Zig accessors (<Prop>NameSetCached/GetCached) to work with GC-owned values
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zig
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Cache structures in ZigGlobalObject for bound classes
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/codegen/cppbind.tssrc/bun.js/bindings/JSObject.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/VirtualMachine.zigsrc/bun.js/bindings/JSPromise.zigsrc/bun.js/bindings/VM.zigsrc/codegen/cppbind.tssrc/bun.js/api/html_rewriter.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/event_loop.zigsrc/bun.js/bindings/JSValue.zigsrc/bun.js/bindings/CatchScope.zig
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to src/bun.js/bindings/**/*.{cpp,hpp,h} : Define properties using JSC HashTableValue arrays for JS class bindings
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/JSPropertyIterator.zigsrc/bun.js/jsc/host_fn.zigsrc/bun.js/bindings/JSPromise.zigsrc/codegen/cppbind.tssrc/bun.js/bindings/AnyPromise.zigsrc/bun.js/bindings/JSObject.zigsrc/bun.js/api/JSBundler.zigsrc/bun.js/bindings/StringBuilder.zigsrc/bun.js/bindings/JSValue.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/codegen/cppbind.tssrc/bun.js/bindings/JSObject.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/bun.js/jsc/host_fn.zigsrc/bun.js/bindings/JSObject.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/codegen/cppbind.tssrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/codegen/cppbind.tssrc/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Create Bun JSC class bindings by pairing a TypeScript class interface with a Zig implementation and relying on the generator to produce glue code
Applied to files:
src/codegen/cppbind.ts
📚 Learning: 2025-08-30T00:13:36.784Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.784Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/bun.js/bindings/JSObject.zig
📚 Learning: 2025-08-30T00:05:37.983Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:37.983Z
Learning: Applies to **/*.zig : In Zig code, manage memory carefully: use the correct allocator and defer for cleanup
Applied to files:
src/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:57.056Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.056Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/bun.js/bindings/StringBuilder.zig
📚 Learning: 2025-08-30T00:11:00.878Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.878Z
Learning: Applies to **/*.cpp : Provide an extern "C" Bun__<Type>__toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Applied to files:
src/bun.js/bindings/StringBuilder.zig
🔇 Additional comments (27)
src/bun.js/event_loop/JSCScheduler.zig (1)
7-7: Inline ExceptionValidationScope init is correctMatches the project-wide pattern; lifecycle is preserved with defer. No issues.
src/bun.js/event_loop.zig (1)
458-458: Inline CatchScope init LGTMInitialization is now idiomatic; deinit still deferred and existing exception checks remain intact.
src/bun.js/api/html_rewriter.zig (1)
897-897: CatchScope inline init LGTMConsistent with the refactor; scope lifetime and downstream exception handling are unchanged.
src/codegen/cppbind.ts (1)
673-673: Approve inline scope initialization changeNo legacy two-step scope initialization patterns remain in the Zig codegen; both CatchScope and ExceptionValidationScope use inline
.init(...)withdefer scope.deinit(). Change is sound.src/bun.js/jsc/host_fn.zig (1)
93-95: Inline scope init across host call shims looks goodtoJSHostCall, fromJSHostCall, and fromJSHostCallGeneric now use .init(...); the assert/return checks still follow immediately with deferred deinit. All good.
Also applies to: 117-119, 133-135
src/bun.js/bindings/CatchScope.zig (1)
2-2: Verify Zig/C++ size/alignment contract across all builds.
sizedepends onallow_assert/enable_asan. Ensure CatchScopeBinding.cpp’s static_asserts use the identical condition set so size/alignment match on macOS/Linux/Windows (asan/non-asan, debug/release). Mismatches explain the cross-platform clang errors in CI.src/bun.js/ModuleLoader.zig (1)
473-481: LGTM: inline CatchScope construction is the right pattern.Once
.initis fixed to setlocationagainst the returned object, this site will be safe. Keeping the variable in-place and deferringdeinit()is correct.src/bun.js/VirtualMachine.zig (1)
536-540: LGTM: scope around no-side-effects stringify.The defer to clear a pending exception is correct; with the
.initfix, the self-location assertion will hold.src/bun.js/bindings/JSObject.zig (1)
144-149: Inline CatchScope init is correct and clearer.The change preserves semantics (handles "undefined with exception" correctly) and simplifies the pattern.
src/bun.js/bindings/VM.zig (2)
152-158: throwError flow looks right; explicit error path is good.Pre-assert, throw, post-assert, then return error.JSError is consistent and easy to reason about.
152-158: Bindings verified – the C++ CatchScope shim’s size, alignment, constructor signature, and exported symbols match Zig’s CatchScope and ExceptionValidationScope definitions, and the.init(...)usage in VM.zig correctly maps to those externs.src/bun.js/bindings/JSPropertyIterator.zig (2)
122-127: Good: switched to designated.init(...)for CatchScope.Maintains behavior with simpler initialization.
130-135: Same here: consistent scope init style.Reads cleaner; exception propagation unchanged.
src/bun.js/api/JSBundler.zig (1)
1092-1104: LGTM: CatchScope inline init in runOnEndCallbacks.Pattern matches the rest of the refactor; exception check remains in place.
src/bun.js/bindings/JSValue.zig (6)
1203-1208: Use of ExceptionValidationScope with.init(...)is correct.Keeps the toStringOrNull contract explicit and tightens assertions.
1389-1394: LGTM: inline CatchScope for getIfPropertyExistsFromPath.Correct propagation and unchanged semantics.
1417-1421: LGTM: scope init in _then2.Keeps the debug assert while simplifying setup.
1424-1428: LGTM: scope init in then.Consistent with the rest of the file.
1496-1504: LGTM: getOwn uses inline CatchScope.Exception handling remains explicit; return shape unchanged.
1598-1615: LGTM: getStringish scope init.The ordering (toBunString, errdefer, returnIfException) is correct.
src/bun.js/bindings/StringBuilder.zig (7)
15-19: deinit path OK once address stability is guaranteed.This looks fine and will pair with destroy() per above. Ensure C++ deinit tolerates being called exactly once.
20-44: Enum/type mapping looks solid.Covers all append variants needed by the binding surface.
46-58: Append dispatch is clean.Comptime switch is appropriate; signatures match externs.
84-90: Imports/readability LGTM.No issues.
8-14: Verify and refactor StringBuilder init usage
No callsites forStringBuilder.init()(or the proposedinitInPlace/create/destroy) were found—ensure there are no remaining return-by-value usages of this C++ wrapper and apply the in-place or heap-allocated pattern as needed.
79-83: No changes needed:ensureUnusedCapacitydelegates to C++reserveCapacity, which doesn’t return an error or allow catching OOM; surfacing allocation failure isn’t feasible.Likely an incorrect or invalid review comment.
59-67: Signatures match between Zig externs and C++ definitions
C++StringBuilder__appendLatin1/Utf16take(WTF::StringBuilder*, const LChar*/UChar*, size_t)and Zig declares[*]const u8/u16withusize;StringBuilder__appendStringtakesBunStringby value matching ZigString;appendLChar/appendUCharuseu8/u16as expected. No ABI mismatches found.
This reverts commit ff4231c.
5 participants
What does this PR do?
This PR adds
Bun.YAML.stringify. The stringifier will double quote strings only when necessary (looks for keywords, numbers, or containing non-printable or escaped characters). Anchors and aliases are detected by object equality, and anchor name is chosen from property name, array item, or the root collection.How did you verify your code works?
Added tests for basic use and edgecases
Summary by CodeRabbit
New Features
Improvements
Tests
Chores