Repository navigation
types: add offset and size options to Bun.mmap - #34573
Conversation
Bun.mmap has accepted offset and size options in the native implementation, but MMapOptions only declared sync and shared, so TypeScript rejected them. Declare both with doc comments and cover them in the bun-types fixture.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesBun.mmap Type Support
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:49 AM PT - Jul 18th, 2026
✅ @robobun, your commit 57f9c8f9562cb4dcf0639db164f37cbad6ca9ba4 passed in 🧪 To try this PR locally: bunx bun-pr 34573That installs a local version of the PR into your bun-34573 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — declares two existing runtime options in the type surface.
Checked mmap_file in src/runtime/api/BunObject.rs:1781-1823 — it reads "offset" and "size" from the options object, coerces both to int64 with non-negative validation, and defaults to 0 / rest-of-file, matching the new JSDoc. The fixture asserts Uint8Array<ArrayBuffer>, which matches the existing mmap declaration's return type.
Extended reasoning...
Overview
Adds offset?: number and size?: number to the MMapOptions interface in packages/bun-types/bun.d.ts, and adds a type-check case to test/integration/bun-types/fixture/bun.ts exercising all four options and the return type. No runtime code is touched.
Security risks
None. Type declarations only; no executable change.
Level of scrutiny
Low. This is a .d.ts-only addition exposing options the native implementation already reads. I verified against src/runtime/api/BunObject.rs: the mmap_file host function calls opts.get(global_this, "size") and opts.get(global_this, "offset"), coercing each via coerce_to_int64 and rejecting negatives — so number with a non-negative default is the correct surface. The offset doc comment stays neutral on page alignment as the PR description notes (semantics are being adjusted in a separate PR), which is reasonable.
Other factors
The fixture return-type assertion Uint8Array<ArrayBuffer> matches the existing function mmap(...): Uint8Array<ArrayBuffer> declaration, so it will not introduce a fixture failure. The PR description confirms the bun-types integration test passes with the change and fails without it. No prior reviewer comments to address.
The in-process LanguageService cases skip on debug builds, so add a small tsc-CLI check that runs there too.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/bun-types/bun-types.test.ts`:
- Around line 384-388: Update the compiler subprocess assertion around the
Promise.all result to validate the structured stdout, stderr, and exitCode
outcome without requiring stderr.trim() to be empty. Preserve the successful
exitCode assertion and stdout validation, while allowing benign compiler
diagnostics on stderr.
In `@test/integration/bun-types/fixture/bun.ts`:
- Around line 82-92: Add independent Bun.mmap type assertions in
test/integration/bun-types/fixture/bun.ts:82-92 for options containing only
offset and only size, preserving the existing combined assertion. Also add
independent offset-only and size-only declarations in
test/integration/bun-types/bun-types.test.ts:372-373 to cover the complete
optional-flag matrix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0eb83bae-5c88-42e2-8122-4c7e44d10770
📒 Files selected for processing (3)
packages/bun-types/bun.d.tstest/integration/bun-types/bun-types.test.tstest/integration/bun-types/fixture/bun.ts
There was a problem hiding this comment.
LGTM — the timeout nit was addressed in 8d5c47a.
What was reviewed:
- Confirmed
src/runtime/api/BunObject.rs(mmap_file) reads"offset"and"size"from the options object with the documented defaults, so the newMMapOptionsfields match the runtime. - Checked the
mmapdeclaration returnsUint8Array<ArrayBuffer>, matching the fixture'ssatisfies/.is<>assertions. - New spawned-tsc test follows the adjacent tsgo test's pattern (concurrent pipe drain, stderr/stdout before exitCode,
bunEnv); offset-only and size-only cases were added per earlier feedback.
Extended reasoning...
Overview
This PR adds two optional properties, offset?: number and size?: number, to the MMapOptions interface in packages/bun-types/bun.d.ts. It also adds type-level coverage in two places: tsd.expectType(...) assertions in the existing fixture/bun.ts, and a new standalone test in bun-types.test.ts that spawns tsc over a minimal file exercising all four MMapOptions fields (plus offset-only and size-only variants). No runtime code is touched.
Security risks
None. This is a pure type-declaration change plus test additions. The new test spawns tsc from the fixture's already-installed node_modules/typescript under bunEnv in a temp dir — same pattern as the pre-existing tsgo test above it.
Level of scrutiny
Low. .d.ts additions that expose already-shipping runtime options are among the most mechanical changes in the repo, and CLAUDE.md explicitly calls out that bun-types edits do not require a native build. I verified against src/runtime/api/BunObject.rs:1803-1824 that the native mmap_file implementation reads exactly "size" and "offset" (coerced to non-negative int64, offset defaulting to 0, size defaulting to None = rest of file), so the new declarations and doc comments accurately describe the existing behavior. The PR description notes that offset page-alignment semantics are being addressed separately in #34120, and the doc comment here is appropriately neutral on that point.
Other factors
All prior review threads are resolved: CodeRabbit's optionality-matrix suggestion was addressed in 32cfc92 (offset-only and size-only cases added to both test sites), CodeRabbit withdrew its stderr concern after the author explained bunEnv sets BUN_DEBUG_QUIET_LOGS=1 and tsc diagnostics go to stdout, and my own note about the explicit 60_000 per-test timeout was addressed in 8d5c47a. The new test mirrors the harness conventions of the adjacent tsgo test (Promise.all pipe drain, stderr → stdout → exitCode assertion order, await using for the subprocess). The author reports the test fails with the .d.ts change reverted, satisfying the "prove it fails for the right reason" requirement.
There was a problem hiding this comment.
LGTM — types-only change exposing runtime-supported offset/size on MMapOptions; the earlier timeout nit was addressed in 8d5c47a.
What was reviewed:
- Confirmed the runtime reads both options (
src/runtime/api/BunObject.rs:1803-1813) andbun_sys::mmap_fileclampssizetostat_size - offset, matching the new doc comment. - Fixture and spawned-tsc test cover combined, offset-only, and size-only calls; assertion order and
bunEnvmatch the adjacent tsgo test. - Both CodeRabbit threads are resolved (stderr assertion withdrawn; independent-optionality coverage added in 32cfc92).
Extended reasoning...
Overview
This PR adds two optional number properties — offset and size — to the MMapOptions interface in packages/bun-types/bun.d.ts, and adds type-level test coverage in test/integration/bun-types/fixture/bun.ts (tsd assertions) plus a spawned-tsc check in test/integration/bun-types/bun-types.test.ts. No runtime code is touched.
Security risks
None. This is a .d.ts-only change plus type-check tests; nothing is executed with the mmap'd file at test time, and the declared shape matches what the native implementation already accepts.
Level of scrutiny
Low. Per the repo's own docs guidance, type-declaration changes don't even require a build. I verified the declared options against the actual native implementation: mmap_file in src/runtime/api/BunObject.rs reads opts.get(global_this, "size") and opts.get(global_this, "offset"), and bun_sys::mmap_file in src/sys/lib.rs computes stat_size.saturating_sub(offset) then .min(wanted_size) — exactly what the new JSDoc describes ("Clamped to the file size (minus offset)"). The doc comment deliberately stays neutral on page-alignment semantics, which are being addressed separately in #34120.
Other factors
All three prior review threads are resolved: CodeRabbit withdrew the empty-stderr concern (tsc emits diagnostics on stdout, bunEnv sets BUN_DEBUG_QUIET_LOGS=1, and the pattern matches the adjacent tsgo test); the optionality-matrix suggestion was addressed in 32cfc92 with offset-only and size-only assertions in both the fixture and the spawned test; and my own earlier note about the explicit 60_000 per-test timeout was addressed in 8d5c47a. The new spawned-tsc test drains stdout/stderr concurrently, uses await using for the subprocess, asserts exit code last, and reuses BASE_FIXTURE_DIR's packed types via typeRoots so it exercises the packed .d.ts rather than the source tree. The PR description includes fails-on-main / passes-on-PR evidence for both debug and release builds.
2 participants
What does this PR do?
Bun.mmaphas acceptedoffsetandsizeoptions in the native implementation for a long time, butMMapOptionsin bun-types only declaressyncandshared, so TypeScript rejects them:This declares both options with doc comments and adds a type check to the bun-types fixture.
Surfaced by #34572, which asks for offset-based random access into large mmap'd files; the runtime already supports it, the types just did not expose it.
Note: offset view semantics are being fixed separately in #34120; the doc comment here stays neutral on page alignment.
How did you verify your code works?
bun test test/integration/bun-types/bun-types.test.tspasses with the change and fails (9 fixture type errors) with the.d.tschange reverted.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file
root cause · written by the author bot
The runtime implementation of Bun.mmap already read optional offset and size fields from its options object, but the MMapOptions interface in the published type declarations never declared them, so valid calls failed type checking. The fix adds offset and size as optional number properties to MMapOptions in bun.d.ts, with doc comments matching the native behavior of offset defaulting to zero and size clamping to the file size minus the offset. Type-level assertions in the fixture and a spawned tsc test over the packed declarations cover the combined, offset-only, and size-only cases to keep…