Skip to content

refactor(string): remove ZigString public API, rename internal struct to StringView - #27473

Draft
dylan-conway wants to merge 7 commits into
mainfrom
claude/remove-zigstring
Draft

dylan-conway wants to merge 7 commits into
mainfrom
claude/remove-zigstring

Conversation

@dylan-conway

Copy link
Copy Markdown
Member

Summary

Removes the deprecated ZigString type from the public API, migrating all callers to bun.String/BunString. The internal 16-byte {ptr, len} tagged-pointer struct remains as the backing storage for borrowed string views, but is renamed to StringView (Zig) / ZigStringView (C++) and made private.

Net: −1,028 lines across 143 files (3 commits on top of 2222aa9f47)

What changed

Deleted

Item Size
src/bun.js/bindings/ZigString.zig 745 lines
13 ZigString__* C++ FFI functions (bindings.cpp) ~150 lines
bun.ZigString / jsc.ZigString public exports —
ErrorableZigString type (unused) —
Dead functions: getCachedObject, putCachedObject, JSObject.getDirect/putDirect, JSValue.createTypeError/createRangeError, JSValue.putRecord —
jsc.ZigString case from wrapInstanceMethod/wrapStaticMethod —
~140 lines of dead Zig:: helper functions (helpers.h) —

Renamed

Old New Where
ZigString struct StringView Zig (string.zig, private)
ZigString struct ZigStringView C++ (headers-handwritten.h)
Tag.ZigString / .StaticZigString Tag.StringView / .StaticStringView Both
BunStringImpl.zig BunStringImpl.view C++ union field
String.toZigString() (public) asStringViewUnsafe() (private) Zig — safety fix: borrows WTFStringImpl bytes
ZigString__freeGlobal BunString__freeGlobalBytes FFI export
trait.isZigString() trait.isStringSlice() Unrelated naming collision

Converted (~30 FFI functions, ~1000+ call sites)

Zig call-site patterns:

Old New
obj.put(g, ZigString.static("key"), val) obj.put(g, bun.String.static("key"), val) (~350 sites)
ZigString.init(s).withEncoding().toJS(g) try bun.String.createUTF8ForJS(g, s) (~100 sites)
ZigString.init(msg).toErrorInstance(g) bun.String.borrowUTF8(msg).toErrorInstance(g) (~50 sites)
ZigString.Slice bun.String.Slice (~200 sites)
ZigString.init("").toJS(g) jsEmptyString(g)

FFI extern signatures (ZigString* → BunString*):

  • Output params (ref-counted via Bun::toStringRef): getClassName, getNameProperty, getSymbolDescription, symbolKeyFor, getSourceCode, DOMURL.pathname_
  • Output params (borrowed via Bun::toString/toStringView): JSValue.toBunString, JSString.toBunString, FetchHeaders.fastGet_/get_, DOMURL.href_
  • Input params (copied via toWTFString()): createObject2, symbolFor, createAggregateError, deleteModuleRegistryEntry, setTimeZone, deleteProperty, put, FetchHeaders.*, DOMFormData.*, URLSearchParams.*
  • Callbacks: forEachProperty (3 variants), DOMFormData.forEach/toQueryString, URLSearchParams.toString
  • Arrays: putRecord, fromEntries, ServerRouteList.create
  • WebSocket: 14-param connect + writeString + close + didReceiveText
  • Others: Bun__Buffer_fill, Bun__Process__setCwd, Bun__getEnvValue, resolve queryString, CodeString

wrapInstanceMethod params (html_rewriter 27 methods, CryptoHasher, EVP, server.publish, FFI.open): ZigString → bun.String, uses borrowed toString() (no ref overhead, matching old behavior).

Memory & performance audit

Pattern Before After Verdict
INPUT (Zig→C++, .ZigString tag) Zig::toString() borrows latin1, copies UTF8 toWTFString() always copies Safer (no borrow lifetime dep)
OUTPUT w/ toStringRef toZigString() borrows raw ptr Bun::toStringRef() +1 ref, Zig derefs Safer (explicit ownership)
OUTPUT w/ toString (no ref) Borrows impl via COW share Same — relies on source holding ref Identical
OUTPUT w/ toStringView Borrows raw bytes Same — .StringView tag Identical
.put() static keys toIdentifier → copy → atomize toWTFString() → copy → atomize Identical
FetchHeaders get/has/remove Zig::toString() borrow toWTFString(ZeroCopy) borrow Identical (fixed after initial conversion added unnecessary copy)
wrapInstanceMethod params getZigString() borrow (0 ref overhead) toString() borrow (0 ref overhead) Identical
.static().toJS() toStringCopy copies toStringStatic → ASCIILiteral zero-copy Faster

Testing

  • bun run zig:check-all — 61/61 platforms pass
  • bun bd — compiles
  • Smoke tests: fetch (341), inspect (71), serve (189), node:os (52), node:path (116), html-rewriter (41), websocket, FormData, DNS, crypto, ffi, spawn — 0 regressions

Pre-existing failures (unchanged from main): websocket-client.test.ts (needs ws npm pkg), resolve.test.ts (needs tsyringe), expect.test.js stress test timeout.

Architecture

Before:

bun.ZigString / jsc.ZigString  ←── public 16-byte tagged {ptr, len}
       │
       └── used directly by ~1400 call sites + 30+ extern C functions

After:

bun.String  ←── only public string type
    │
    └── private StringView (16-byte {ptr, len})  ←── .StringView / .StaticStringView variants
               ↕ (ABI only)
    ZigStringView  ←── C++ internal struct, only inside BunStringImpl union

The .StringView/.StaticStringView variants mean "Zig-owned bytes, C++ must copy". The .WTFStringImpl variant means "JSC-owned, ref-counted".

- Move bun.String.Slice struct definition into string.zig (was in ZigString.zig); ZigString.Slice is now an alias
- Add toSyntaxErrorInstance() and toDOMExceptionInstance() to bun.String with C++ implementations
- Route JSValue.put() with []const u8 keys through BunString path instead of ZigString
- Add bun.String param type support to wrapInstanceMethod in host_fn.zig
- Change NewRuntimeFunction/NewFunctionWithData to take ?*const bun.String
  - Update JSFFIFunction.cpp to take const BunString* (use toWTFString() copying variant since JSC stores the string long-term in NativeExecutable)
  - Update ffi.zig callers to use bun.String.init()
  - Update bindgen.ts to emit bun.String.static()
…ice + jest.zig static keys

- Replace ZigString.Slice → bun.String.Slice across 48 files (165 line changes)
  - All variants: jsc.ZigString.Slice, JSC.ZigString.Slice, bun.ZigString.Slice, bun.jsc.ZigString.Slice
  - ZigString.Slice now only in string.zig (canonical) and string/wtf.zig (internal)
- Replace all ZigString.static() → bun.String.static() in jest.zig (37 sites)
  - Removed now-unused ZigString import
…tring.static() in .put() keys

Replace ZigString.static() with bun.String.static() for JSValue.put() keys
across 43 files (~312 sites). JSValue.put() already supports bun.String
keys, so this is purely mechanical.

Remaining ZigString.static() occurrences (49 across 11 files) require
different handling:
- createObject2 args (*const ZigString signature) — Phase 2 FFI change
- .toJS() / .toErrorInstance() chains — Pattern B, different replacement
- .* dereferences — Phase 2

Also removed 10 now-unused 'const ZigString = jsc.ZigString' imports.

Stats:
- ZigString.static(): 398 → 49 (88% reduction)
- Total ZigString in Zig: 1400 → 997 (29% reduction)
Phases 1.2 through 4 of the ZigString removal:

## Pattern conversions (~900+ call sites)
- Pattern B: ZigString.init().toJS() → bun.String.createUTF8ForJS()
  - Changed ~30 function signatures from JSValue → bun.JSError!JSValue
- Pattern C: ZigString.init(msg).toErrorInstance() → bun.String.borrowUTF8/static().toErrorInstance()
- Empty strings: ZigString.init("").toJS() → jsEmptyString()

## FFI boundary conversions (30+ functions, ZigString* → BunString*)
- Output params (ref-counted): getClassName, getNameProperty, getSymbolDescription,
  symbolKeyFor, getSourceCode → Bun::toStringRef(), Zig defer .deref()
- Input params (copying): createObject2, symbolFor, createAggregateError,
  deleteModuleRegistryEntry, setTimeZone, deleteProperty → arg->toWTFString()
- Core: JSValue.toZigString → toBunString (borrowed .StringView tag)
- JSString.view now returns bun.String; toSlice → toUTF8WithoutRef
- FetchHeaders: append, get_, has, put, remove, fastGet_, createValue
- DOMURL: href_, pathname_ (ref-counted)
- DOMFormData: all functions + callbacks
- URLSearchParams: create, toString callback
- forEachProperty callbacks (3 variants)
- putRecord/fromEntries: []ZigString → []bun.String
- WebSocket: 14-param connect + writeString + close + didReceiveText
- Bun__Buffer_fill, Bun__Process__setCwd, Bun__getEnvValue
- Zig__GlobalObject__resolve queryString, ServerRouteList, CodeString
- wrapInstanceMethod: all html_rewriter.zig instance methods (27 fns),
  CryptoHasher, EVP, server.publish, FFI.open
  - Uses borrowed toString() (no ref overhead, matches old ZigString behavior)

## Deleted
- src/bun.js/bindings/ZigString.zig (745 lines)
- 13 ZigString__* C++ functions (bindings.cpp)
- bun.ZigString, jsc.ZigString public exports
- getZigString/putZigString deprecated wrappers
- jsc.ZigString case from wrapInstanceMethod/wrapStaticMethod
- ErrorableZigString type (unused)
- Dead: getCachedObject, putCachedObject, JSObject.getDirect/putDirect,
  JSValue.createTypeError/createRangeError, JSValue.putRecord
- ~140 lines of dead Zig:: helpers

## Renamed
- Struct: ZigString → StringView (Zig) / ZigStringView (C++)
- Enum tags: .ZigString/.StaticZigString → .StringView/.StaticStringView
- Union field: BunStringImpl.zig → BunStringImpl.view
- String.toZigString() → private asStringViewUnsafe() (safety: borrows
  WTFStringImpl bytes, dangerous if exposed)
- WTFStringImpl.toZigString() → asStringViewUnsafe()
- export ZigString__freeGlobal → BunString__freeGlobalBytes
- trait.isZigString() → trait.isStringSlice() (was unrelated naming collision)

## Perf fixes (restoring original behavior after initial conversion)
- FetchHeaders get/has/remove: toWTFString() → toWTFString(ZeroCopy)
  (read-only lookups, safe to borrow)
- wrapInstanceMethod: toBunString() → toString() (borrowed, no ref overhead)

## Memory safety audit
- All INPUT conversions (Zig→C++): toWTFString() always copies for
  .StringView tag — same or safer than old Zig::toString/toStringCopy
- All OUTPUT with toStringRef: explicit +1 ref, Zig derefs — safer than
  old borrowed-raw-pointer approach
- All OUTPUT with toString/toStringView: borrowed, relies on source holding
  ref (same invariant as old toZigString)
- Callbacks: borrowed .StringView tag on C++ stack, valid during callback
  only — identical to old pattern

Net: 135 files, +1905/-2935 (-1030 lines)
@robobun

robobun commented Feb 26, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 4:15 AM PT - Feb 27th, 2026

❌ @dylan-conway, your commit 47cb5a3 has 5 failures in Build #38260 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 27473

That installs a local version of the PR into your bun-27473 executable, so you can run:

bun-27473 --bun

dylan-conway and others added 3 commits February 26, 2026 21:54
…-splitting on semicolons

The Buildkite build JSON response includes the commit message in the
"message" field. When that message contains a semicolon (as it did in
c6efe76: "JSString.view now returns bun.String; toSlice → ..."),
passing ${BUILDKITE_BUILD} unquoted to string(JSON ...) causes CMake
to split it into a list at the semicolon, truncating the JSON and
producing "Syntax error: value, object or array expected."

Quote all JSON variable references in string(JSON ...) calls to prevent
this class of bug.
…b-oom test expectations

Three fixes for CI failures on the ZigString removal branch:

1. BunString__toJSON: restore the oversized-string check that was in
   ZigString__toJSONObject. When a StringView exceeds the synthetic
   allocation limit, toWTFString() returns null — without this check
   we fell through to JSONParse(null) and threw a generic "Failed to
   parse JSON" instead of ERR_STRING_TOO_LONG.

2. blob-oom.test.ts: update expected error messages from "2^32-1" to
   the actual WTF::String::MaxLength (2147483647 = 2^31-1). The old
   hardcoded "2^32-1" message was always wrong; the new code path
   uses the correct limit from ErrorCode.cpp STRING_TOO_LONG.

3. node_fs_watcher.zig: remove duplicate "TODO: properly propagate"
   comment added during refactor (ban-words tracking).

4. ban-limits.json: globalThis.hasException 123 → 122 (dead code
   removed by refactor).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants