fix(CString): define byteLength and byteOffset properties

[ObscuritySRL]

What does this PR do?

This PR:

• Addresses CString.byteLength & CString.byteOffset are undefined #22920.
• Defines CString's byteLength and byteOffset properties.
• Makes byteOffset default to 0, and byteLength a lazy, cached getter — it's computed via Buffer.byteLength(…, "utf-8") only on first access when a length wasn't supplied to the constructor, then cached. An explicitly-supplied length is used verbatim, and an empty CString reports 0 without recomputing.
• Fixes the typing to make these properties non-nullable.
• Adds tests for valid values of these properties.

How did you verify your code works?

Added tests to test/js/bun/ffi/ffi.test.js. Rebased onto current main and verified the runtime behavior against a local debug build:

• new CString(ptr(buf)).byteOffset → 0
• new CString(ptr("abc🫠\0")).byteLength → 7 (lazy UTF-8 length)
• explicit byteOffset / byteLength are preserved (new CString(ptr(buf), 4, 2) → offset 4, length 2)
• empty new CString(0) → byteOffset 0, byteLength 0
• repeated byteLength reads return the cached value

(The in-file assertions live inside the ffiRunner suite, which CI exercises with the compiled ffi-test fixture.)

[coderabbitai]

Walkthrough

CString's TypeScript declaration and runtime were adjusted: byteOffset is now required, byteLength is exposed via a getter (cached and computed lazily). The CString constructor defaults byteOffset to 0 and conditionally passes byteLength to BunCString. Tests assert byteOffset and UTF‑8 byteLength.

Changes

Cohort / File(s)	Summary of Changes
Type definitions packages/bun-types/ffi.d.ts	CString declaration: changed byteOffset?: number to byteOffset: number; removed optional byteLength? property and added a get byteLength(): number getter with documentation.
CString implementation src/js/bun/ffi.ts	Constructor now initializes byteOffset to 0 when undefined, sets internal ptr/byteOffset, conditionally calls BunCString(ptr, byteOffset, byteLength) only if a safe integer byteLength is provided, and replaces the public byteLength field with a lazy, cached get byteLength() that computes UTF‑8 byte length. ArrayBuffer and related logic now use the getter. Private #cachedByteLength field added.
Tests test/js/bun/ffi/ffi.test.js	Added assertions expecting byteOffset === 0 and byteLength === 7 for a buffer containing multibyte UTF‑8 characters.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title “fix(CString): define byteLength and byteOffset properties” succinctly highlights the core change of defining and typing these properties on CString, uses conventional commit style, and clearly conveys the primary intent without extraneous detail.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Description check	✅ Passed	The PR description matches the required template and includes the change summary plus verification details.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/js/bun/ffi.ts (2)

120-120: Default with nullish coalescing, not logical OR

||= can clobber legitimate falsy values (e.g., -0, NaN). Use ??= to only default when null/undefined.

-    byteOffset ||= 0;
+    byteOffset ??= 0;

---

128-130: Preserve explicit zero length; avoid truthy check

byteLength || … ignores explicit 0. Use nullish coalescing.

-    this.byteLength = byteLength || Buffer.byteLength(this.toString(), 'utf-8');
+    this.byteLength = byteLength ?? Buffer.byteLength(this.toString(), "utf-8");

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0ea4ce1 and 6dd67b1.

📒 Files selected for processing (3)

• packages/bun-types/ffi.d.ts (1 hunks)
• src/js/bun/ffi.ts (1 hunks)
• test/js/bun/ffi/ffi.test.js (1 hunks)

🧰 Additional context used

📓 Path-based instructions (9)

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

• test/js/bun/ffi/ffi.test.js

test/js/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place JavaScript and TypeScript tests under test/js/

Files:

• test/js/bun/ffi/ffi.test.js

test/js/bun/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)

Files:

• test/js/bun/ffi/ffi.test.js

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

• test/js/bun/ffi/ffi.test.js

test/js/**

📄 CodeRabbit inference engine (test/CLAUDE.md)

Organize unit tests for specific features under test/js/ by module

Files:

• test/js/bun/ffi/ffi.test.js

**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript files with Prettier (bun run prettier)

Files:

• test/js/bun/ffi/ffi.test.js
• src/js/bun/ffi.ts
• packages/bun-types/ffi.d.ts

src/js/bun/**/*.{js,ts}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/

Files:

• src/js/bun/ffi.ts

src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts}: Use .$call and .$apply instead of .call or .apply to avoid user tampering
Use string-literal require("...") only (no dynamic or non-literal specifiers)
Author modules as CommonJS-style with require(...) and export via export default {} (no ESM import/named exports)
Prefer JSC intrinsics and $-prefixed private APIs for performance and safety (e.g., $Array, $newArrayWithSize, $putByIdDirectPrivate, $assert, $debug)
Validate function arguments with $isCallable and throw $ERR_INVALID_ARG_TYPE for invalid callbacks
Use $isObject and throw appropriate TypeErrors for constructor/initializer inputs that must be objects

Files:

• src/js/bun/ffi.ts

src/js/**/*.{js,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

src/js/**/*.{js,ts}: In built-in modules (src/js), require() must use string literals resolved at compile time
In built-in modules (src/js), use export default (converted to a return statement by the preprocessor)

Files:

• src/js/bun/ffi.ts

🧠 Learnings (2)

📓 Common learnings

Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/memory.zig:60-76
Timestamp: 2025-09-02T17:41:07.869Z
Learning: In bun's memory utilities, when handling const pointers in deinit operations, prefer compile-time errors over silent skipping to avoid hard-to-find memory leaks. Users expect explicit failures rather than silent omissions in memory management.

📚 Learning: 2025-09-12T18:16:50.754Z

Learnt from: RiskyMH
PR: oven-sh/bun#22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.

Applied to files:

• src/js/bun/ffi.ts

🧬 Code graph analysis (1)

test/js/bun/ffi/ffi.test.js (1)

src/node-fallbacks/buffer.js (1)

• Buffer (136-145)

🔇 Additional comments (1)

packages/bun-types/ffi.d.ts (1)

1086-1087: Making CString.byteOffset/byteLength non-optional looks good

This aligns runtime guarantees with types and avoids undefined checks downstream.

[ObscuritySRL]

Everything I do uses FFI -- please merge, @Jarred-Sumner 😛

[ObscuritySRL]

Can this get some attention along with #31449, please? 9 months for an FFI fix is frustrating. The FFI API is one of the most powerful parts of Bun and it gets no attention.

[Jarred-Sumner]

Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @ObscuritySRL for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look.

(This comment was written by Claude, on behalf of the Bun team.)