wtf-bindings: use WTF ASSERT() so debug builds compile#30992
Conversation
The libc assert() here relied on <cassert> being pulled in transitively by a WebKit header. After the WebKit upgrade in #30705 that include is gone, so debug builds fail on the unified-source unit that contains wtf-bindings.cpp: src/jsc/bindings/wtf-bindings.cpp:64:5: error: use of undeclared identifier 'assert' ASSERT() is the WTF-style assertion used elsewhere in the same file and does not depend on a libc header being in scope. Release builds were unaffected because the unified grouping happened to still pull cassert in there. Fixes #30988
|
Updated 11:47 AM PT - May 18th, 2026
✅ @robobun, your commit 491196331d4dd3c584ff7bbf1f776815643d7359 passed in 🧪 To try this PR locally: bunx bun-pr 30992That installs a local version of the PR into your bun-30992 --bun |
WalkthroughReplaces a lowercase ChangesTTY Raw Mode Build and Test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ICANON is 0x2 on Linux/BSD but 0x100 on Darwin (bit 0x2 is ECHOE on macOS, which Bun__ttySetMode mode=1 does not clear). Match the style already used in terminal-spawn.test.ts so macOS CI doesn't fail the new PTY-fd regression test.
There was a problem hiding this comment.
LGTM — the macOS ICANON fix landed; the remaining inline note is a non-blocking comment-wording nit.
Extended reasoning...
Overview
This PR makes a one-line change to src/jsc/bindings/wtf-bindings.cpp (assert → ASSERT) to fix a debug-build compile break introduced when the WebKit upgrade dropped a transitive <cassert> include, plus a ~25-line regression test in test/js/bun/terminal/terminal.test.ts that exercises setRawMode on a real PTY and asserts ICANON/ECHO flip correctly.
Security risks
None. The C++ change swaps one assertion macro for another with identical semantics (debug-only null check on a stack-local pointer). The test change is purely additive in the test suite.
Level of scrutiny
Low. The ASSERT macro is already used in this exact file (line 256) and is provided by <wtf/Assertions.h> which is in scope via existing WTF includes — this is the canonical assertion style for this codebase. The fix is mechanical, well-explained, and several other open PRs carry the same one-liner as a drive-by.
Other factors
- I previously flagged that the test hardcoded
ICANON = 0x2(Linux-only); the author fixed this in 4262bbc with a per-platform value, and that thread is resolved. - The only new finding from this run is a cosmetic nit: the test comment groups "BSD" on the Linux side of the ICANON value when BSDs actually use
0x100like Darwin. The code is correct for every platform Bun targets (linux/darwin, with Windows skipped), so this is purely a wording issue in a comment about platforms Bun doesn't ship — non-blocking. - Neither file is covered by CODEOWNERS.
- This unblocks debug builds on
main, so there's value in landing it promptly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/js/bun/terminal/terminal.test.ts`:
- Around line 296-309: The assertions about the ECHO bit are incorrect given
that Bun.Terminal initializes with echo off; update the expectations in the test
around terminal.localFlags so they assert ECHO is cleared (toBe(0)) for
beforeLflag, rawLflag, and restoredLflag instead of asserting it is set, leaving
the ICANON checks unchanged; locate the assertions near the calls to
terminal.setRawMode(true)/setRawMode(false) and adjust the three expect(... &
ECHO) lines accordingly (references: terminal.localFlags, terminal.setRawMode,
constants ICANON and ECHO, helper enableEcho).
🪄 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: ec3efa42-b21f-4d4a-bb6b-3ec18c4c4906
📒 Files selected for processing (1)
test/js/bun/terminal/terminal.test.ts
| const beforeLflag = terminal.localFlags; | ||
| expect(beforeLflag & ICANON).not.toBe(0); | ||
| expect(beforeLflag & ECHO).not.toBe(0); | ||
|
|
||
| terminal.setRawMode(true); | ||
| const rawLflag = terminal.localFlags; | ||
| expect(rawLflag & ICANON).toBe(0); | ||
| expect(rawLflag & ECHO).toBe(0); | ||
|
|
||
| terminal.setRawMode(false); | ||
| const restoredLflag = terminal.localFlags; | ||
| expect(restoredLflag & ICANON).not.toBe(0); | ||
| expect(restoredLflag & ECHO).not.toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check default ECHO flag state in Bun.Terminal on POSIX
bun -e '
const t = new Bun.Terminal({});
const ECHO = 0x8;
const echoEnabled = (t.localFlags & ECHO) !== 0;
console.log("ECHO enabled by default:", echoEnabled);
t.close();
'Repository: oven-sh/bun
Length of output: 99
🏁 Script executed:
cd /tmp && wc -l test/js/bun/terminal/terminal.test.ts 2>/dev/null || find . -path "*test/js/bun/terminal*" -name "*.ts" 2>/dev/nullRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
head -20 test/js/bun/terminal/terminal.test.tsRepository: oven-sh/bun
Length of output: 769
🏁 Script executed:
# Check the enableEcho helper and surrounding context (lines 1-10)
sed -n '1,20p' test/js/bun/terminal/terminal.test.tsRepository: oven-sh/bun
Length of output: 769
🏁 Script executed:
# Read the entire test file to verify the claims
cat test/js/bun/terminal/terminal.test.tsRepository: oven-sh/bun
Length of output: 35985
🏁 Script executed:
# Let's search for the enableEcho function and its usages
rg -n "enableEcho" test/js/bun/terminal/terminal.test.ts -A 2 -B 2Repository: oven-sh/bun
Length of output: 1367
🏁 Script executed:
# Also check for ECHO and ICANON constants
rg -n "const ECHO|const ICANON" test/js/bun/terminal/terminal.test.tsRepository: oven-sh/bun
Length of output: 198
🏁 Script executed:
# Check lines 290-315 for the disputed test
sed -n '290,315p' test/js/bun/terminal/terminal.test.tsRepository: oven-sh/bun
Length of output: 999
🏁 Script executed:
# Read specific line ranges
head -30 test/js/bun/terminal/terminal.test.tsRepository: oven-sh/bun
Length of output: 992
🏁 Script executed:
cat -n test/js/bun/terminal/terminal.test.ts | sed -n '1,50p'Repository: oven-sh/bun
Length of output: 1766
🏁 Script executed:
python3 << 'EOF'
import os
filepath = 'test/js/bun/terminal/terminal.test.ts'
if os.path.exists(filepath):
with open(filepath, 'r') as f:
lines = f.readlines()
print(f"Total lines: {len(lines)}")
# Show lines 1-20
print("\n=== Lines 1-20 ===")
for i in range(min(20, len(lines))):
print(f"{i+1:3d}: {lines[i]}", end='')
else:
print(f"File not found: {filepath}")
EOFRepository: oven-sh/bun
Length of output: 907
🏁 Script executed:
git ls-files | grep terminal | grep testRepository: oven-sh/bun
Length of output: 189
🏁 Script executed:
ls -la test/js/bun/ 2>&1 | head -20Repository: oven-sh/bun
Length of output: 1221
🏁 Script executed:
# Check if the directory exists
ls -d test/js/bun/terminal 2>&1 || echo "Directory not found"
# List all test files
ls test/js/bun/*.test.ts 2>&1 | headRepository: oven-sh/bun
Length of output: 350
🏁 Script executed:
# Search for the test mentioned in the review
rg "setRawMode" . --type ts --type tsx -lRepository: oven-sh/bun
Length of output: 83
🏁 Script executed:
# Find all test files with "terminal" in the name
find . -name "*terminal*" 2>/dev/null | grep -E "\.(ts|tsx|js)$"Repository: oven-sh/bun
Length of output: 195
🏁 Script executed:
# Also search for ECHO constant definitions
rg "const ECHO" --type ts --type tsxRepository: oven-sh/bun
Length of output: 83
🏁 Script executed:
# Check if there's a Bun.Terminal test file
rg "Bun.Terminal" test --type ts --type tsx -lRepository: oven-sh/bun
Length of output: 83
🏁 Script executed:
# Try to find any reference to the Terminal class or test
rg "class.*Terminal|function.*Terminal|new.*Terminal" test --type ts --type tsx -l | headRepository: oven-sh/bun
Length of output: 83
🏁 Script executed:
# Search for any Terminal-related test
fd -e test.ts -e test.tsx -e test.js test/ | xargs grep -l "Terminal" 2>/dev/null | headRepository: oven-sh/bun
Length of output: 381
Verify ECHO flag initial state—contradicts existing helper documentation.
Lines 298 and 308 assert that the ECHO flag is enabled in the default terminal state and after disabling raw mode:
expect(beforeLflag & ECHO).not.toBe(0); // expects ECHO enabled initially
...
expect(restoredLflag & ECHO).not.toBe(0); // expects ECHO enabled after restoreHowever, the enableEcho helper (line 4) documents that "echo is disabled by default to avoid duplication", and all existing tests explicitly call enableEcho(terminal) before exercising echo behavior (e.g., lines 483, 507, 532, 553, 571, 776, 829, 945), confirming that Bun.Terminal initializes with ECHO off.
The test should instead expect:
expect(beforeLflag & ECHO).toBe(0); // ECHO disabled initially
expect(rawLflag & ECHO).toBe(0); // ECHO still disabled in raw mode
expect(restoredLflag & ECHO).toBe(0); // ECHO remains disabled after restoreWhile the test's primary goal (proving wtf-bindings.cpp compiled by exercising Bun__ttySetMode on a real PTY fd) is sound, these incorrect assertions will cause spurious test failures.
🤖 Prompt for 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.
In `@test/js/bun/terminal/terminal.test.ts` around lines 296 - 309, The assertions
about the ECHO bit are incorrect given that Bun.Terminal initializes with echo
off; update the expectations in the test around terminal.localFlags so they
assert ECHO is cleared (toBe(0)) for beforeLflag, rawLflag, and restoredLflag
instead of asserting it is set, leaving the ICANON checks unchanged; locate the
assertions near the calls to terminal.setRawMode(true)/setRawMode(false) and
adjust the three expect(... & ECHO) lines accordingly (references:
terminal.localFlags, terminal.setRawMode, constants ICANON and ECHO, helper
enableEcho).
There was a problem hiding this comment.
LGTM — both earlier comments addressed; one-line assert → ASSERT build fix plus a well-scoped regression test.
Extended reasoning...
Overview
This PR makes two changes:
src/jsc/bindings/wtf-bindings.cpp: swaps libcassert(tio != NULL)for WTFASSERT(tio != NULL)inuv__tty_make_raw. TheASSERTmacro is already used elsewhere in the same file and is provided by<wtf/Assertions.h>which is already in scope, so no new includes are needed. This fixes the debug-build break (#30988) introduced when the WebKit upgrade dropped the transitive<cassert>include.test/js/bun/terminal/terminal.test.ts: adds a POSIX-only regression test that round-tripssetRawMode(true/false)on a real PTY and assertsICANON/ECHOflip correctly viaterminal.localFlags.
Security risks
None. The C++ change is a debug-only assertion macro swap with identical semantics (no-op in release, abort-on-false in debug). The test is read-only against a freshly-allocated PTY and adds no new attack surface.
Level of scrutiny
Low. The production change is literally one identifier — assert → ASSERT — and the same fix is already being carried as a drive-by in four other open PRs, indicating broad agreement that this is the correct resolution. The accompanying test follows existing patterns in the same file (e.g. the PENDIN masking and skipIf(isWindows) gating already present).
Other factors
I raised two concerns on earlier revisions of this PR, both now resolved:
- The hardcoded
ICANON = 0x2was Linux-only and would have failed on macOS CI → fixed in 4262bbc withprocess.platform === "darwin" ? 0x100 : 0x2. - A comment incorrectly grouped BSD with Linux → fixed in 77974b9.
The current diff reflects both fixes, all inline threads are marked resolved, no CODEOWNERS cover these paths, and the bug-hunting system found no new issues on the latest revision. Given this PR's purpose is to unblock debug builds on main, it's a good candidate to land quickly.
There was a problem hiding this comment.
LGTM — both of my earlier comments were addressed. Re the open CodeRabbit note on ECHO: Terminal.zig:545 initializes the slave termios with .ECHO = true (shared with the master fd that localFlags reads from), so expect(beforeLflag & ECHO).not.toBe(0) is correct as written; the enableEcho helper comment is just stale.
Extended reasoning...
Overview
This PR is a one-line build fix in src/jsc/bindings/wtf-bindings.cpp (assert → ASSERT) plus a ~25-line regression test in test/js/bun/terminal/terminal.test.ts. The C++ change swaps the libc assert macro for WTF's ASSERT, which is already used at line 256 of the same file and is in scope via <wtf/Assertions.h>. This unblocks debug builds broken by the WebKit upgrade in #30705.
Security risks
None. The change is a debug-only assertion macro swap with identical semantics; no inputs, outputs, auth, or data handling are touched.
Level of scrutiny
Low. The C++ change is mechanical and matches an existing pattern in the same file. The test is additive, POSIX-gated, and exercises an existing public API (Bun.Terminal#setRawMode / localFlags). The PR's purpose is to unblock debug builds on main, so it's time-sensitive.
Other factors
- Both inline comments I raised earlier (platform-specific
ICANONvalue; misplaced "BSD" in the comment) were addressed in 4262bbc and 77974b9 and are resolved. - CodeRabbit's unresolved comment claims ECHO is off by default, citing the
enableEchohelper's docstring. I verified againstsrc/runtime/api/bun/Terminal.zig:541-549: the constructor explicitly sets.ECHO = trueon the slave fd, andgetTermiosFlagreads frommaster_fd(line 763), which shares termios with the slave on a PTY pair. The PR author also reports 5/5 passing locally, which would not be possible ifbeforeLflag & ECHOwere 0. I'm treating this as a false positive driven by a stale test-file comment. - The robobun CI "failure" on 77974b9 was
darwin-14-aarch64expiring in the BuildKite queue (infrastructure), not a test failure; an empty retrigger commit (4911963) was pushed. - No CODEOWNERS-sensitive paths;
wtf-bindings.cppis glue code, not core JSC.
Debug/ASAN builds fail with `use of undeclared identifier "assert"` in `uv__tty_make_raw`. libc `<assert.h>` was reaching the TU transitively through a WebKit header that the recent WebKit upgrade dropped. `ASSERT` is the WTF-style macro already used elsewhere in this file (see `uv_tty_reset_mode` below) and is in scope via WTF headers. Drive-by for the same break tracked in #30988 / fixed in #30992 — this PR needs the same one-liner so its debug lane can build.
uv__tty_make_raw() called C assert() without including <cassert>. Worked by accident when unified-source neighbors pulled the header in transitively; breaks when the grouping changes. Use WTF's ASSERT macro (already available via root.h) like the rest of the file. Same fix as #30992; applied here to unblock the debug build.
- wtf-bindings.cpp: assert -> ASSERT (same as #30992; #30705 dropped the transitive <cassert> include so debug builds fail to compile) - semver.test.ts: 30s timeout on the 1e5-iteration memory-churn test and the subprocess regression test; both exceed the 5s default under debug+ASAN on constrained runners
…ing `var;` (#31003) Fixes #31002. ## Repro ```sh $ bun -e "if(0)var[]" panic: internal error: entered unreachable code (src/js_printer/lib.rs:2308:17) Crashed while printing …/[eval] ``` `if (0) var []` / `if (0) var {}` — a destructuring `var` with zero bindings inside a statically-dead `if` body — crashed the printer in `print_decls` where `unreachable!()` guards against empty decl lists ("var;" is invalid syntax). `if(0)` is load-bearing in the reproducer: non-dead branches ship the statement through to JSC, which rejects it with a regular `SyntaxError`; the dead-branch path never reaches JSC. ## Cause `scan_side_effects.rs::should_keep_stmt_in_dead_control_flow` (the DCE helper that decides what to retain in `if (false) …`) handles `var` by hoisting every identifier out of its destructuring pattern so scope semantics are preserved: ```js if (0) var [a, b] = expr(); // becomes: var a, b; (value dropped, names hoisted) ``` For `var []` the destructuring pattern binds no identifiers, so the hoisted vector is empty. The code then installed the empty vec as the new decl list and returned `true`, telling the visitor to keep an `S::Local` carrying zero decls. The next pass handed that to the printer and tripped the invariant. ## Fix `src/js_parser/scan/scan_side_effects.rs`: when the collected identifier list is empty, return `false` and drop the statement entirely — nothing to hoist and no valid `var` output exists. This is symmetric with the existing "omit entirely" branches for `S::Empty`/`S::Expr`/`S::Throw`/etc. ```diff let mut decls: Vec<G::Decl> = Vec::with_capacity(local.decls.len_u32() as usize); for i in 0..(local.decls.len_u32() as usize) { let binding = local.decls.at(i).binding; Self::find_identifiers(binding, &mut decls); } +if decls.is_empty() { + return false; +} + local.decls = G::DeclList::move_from_list(decls); true ``` ## Verification ``` $ bun bd -e 'if(0)var[]' $ echo $? 0 $ bun bd -e 'if(0) var [a, b] = []; console.log(typeof a, typeof b);' undefined undefined ``` New test at `test/regression/issue/31002.test.ts` runs each crash repro in a subprocess and asserts `exitCode === 0` with no output, plus a regression guard that destructuring `var`s with real identifiers still hoist them as `undefined`. Alive-path behavior is unchanged: `var []` / `var {}` at top level or in truthy branches still reach JSC, which rejects them with the expected "Expected an initializer in destructuring variable declaration" SyntaxError. ## Note on debug-build compile error The current debug build on `main` fails in `src/jsc/bindings/wtf-bindings.cpp` with `use of undeclared identifier 'assert'` (unrelated; tracked upstream in #30988, fix already open in #30992). That is not in this diff; rebasing after #30992 lands will unblock the debug lane.
…#31010) A version range with >=256 `||`/whitespace-separated comparators panicked the **debug** build (`attempt to add with overflow`, exit 133) where released Bun fails gracefully. Debug-only: Cargo's dev profile enables `overflow-checks`; the release profile doesn't, so release wrapped (256 -> 0) exactly like Zig's ReleaseFast. `SemverQuery::parse` used `let mut count: u8 = 0;` with `count += 1;` — the 256th increment overflows. `count` is only ever compared `== 0`, never stored or cast, so a wider type is strictly more correct (it also avoids Zig's silent u8 wrap, which was a latent bug that merely didn't crash). Widen `count` to `u32`. Regression test (`Bun.semver.satisfies` with 300 `||` comparators, run in a subprocess since the failure was an uncatchable abort) passes on the debug build and matches released Bun. --- Also carried: - `wtf-bindings.cpp`: `assert` → `ASSERT` — identical to #30992; the WebKit upgrade in #30705 dropped the transitive `<cassert>` include, so debug builds currently fail to compile on `main`. Needed here so the regression test can run under `bun bd`. - 30s timeout on the pre-existing `failures does not cause weird memory issues` test (1e5 iterations) and the new subprocess test — both exceed the 5s default under debug+ASAN. ### Verification ``` $ bun bd test test/cli/install/semver.test.ts 23 pass 0 fail $ ./build/release/bun test test/cli/install/semver.test.ts 23 pass 0 fail ``` With the `u32` change reverted, the new test fails on debug with `panic: attempt to add with overflow (src/semver/SemverQuery.rs:981)` on stderr and exit 132. --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
1 participant
Fixes #30988.
Repro
Cause
uv__tty_make_raw()insrc/jsc/bindings/wtf-bindings.cppcalls the libcassert()macro, but the file never includes<cassert>or<assert.h>. It was reachingasserttransitively through a WebKit header; the WebKit upgrade in #30705 dropped that include, so on current main the unified-source unit containingwtf-bindings.cppfails to compile in debug.Release builds are unaffected because
NDEBUGmakesasserta no-op before name lookup, and because the unified grouping differs from debug — whichever sibling was pulling<cassert>in happens to still land in that bundle.Fix
Swap
assert(tio != NULL)→ASSERT(tio != NULL).ASSERTis the WTF-style assertion already used elsewhere in the same file (line 256) and defined by<wtf/Assertions.h>, which is already in scope via the other WTF includes. No new includes needed.Verification
The added test (
Bun.Terminal > setRawMode > setRawMode toggles on a real PTY fd) exercisesBun__ttySetMode— the same translation unit asuv__tty_make_raw— against a real PTY fd and assertsICANON/ECHOflip off and back on through asetRawMode(true)→setRawMode(false)round-trip. Proves both the file compiled and the runtime path behaves correctly.Several open PRs (#30243, #30975, #30980, #30981) carry this one-line fix as a drive-by; none are focused on #30988. This PR is dedicated to the build break so it can land first and unblock debug builds on
main.