Remove redundant exception checks that follow already-checked calls

Merged
DY
3mo ago
+439-1053

What does this PR do?

Removes exception checks that can never fire.

The rule in this codebase is that every call that can throw is followed on the next line by its check — RETURN_IF_EXCEPTION(scope, …) in C++, ? on a JsResult in Rust — and nothing else checks. An audit turned up a batch of guards that break that rule in the harmless direction: if global.has_exception() { return … } blocks sitting right after a call that was already ?-checked, and RETURN_IF_EXCEPTION lines placed after statements that cannot throw (callFrame->argument(n), jsString(vm, …), putDirect(vm, …), dynamicDowncast, JSSetIterator::create, JSArrayBuffer::create(vm, …), plain field assignments) where the preceding throwing call already has its own check. They never fire, but they make it unclear which call a check belongs to, and a few of them were !has_exception() conditions guarding a fresh throw that read as if the preceding infallible code could leave something pending. This deletes them so each remaining check identifies its call.

Almost all of it is mechanical deletion. The handful that are not:

  • fetch() option parsing (tls, redirect) and Response.json()'s init parsing matched Err(_) / Err(Thrown) and returned Ok(.zero), re-deriving the error from the pending exception and collapsing OutOfMemory/Terminated into it. They now propagate the JsError with ?. reject_on_exception no longer needs its empty-Ok arms since fetch_impl no longer returns .zero.
  • Body::Value::from_js, CryptoHasher digest, Bun.inspect.table, and PBKDF2 re-derived an already-returned Err via has_exception(); they use ? / key the async unprotect guard on the error path instead.
  • setTimeout/setInterval/setImmediate and NodeVM's moduleLoaderImportModuleInner returned the result of a throwing call from inside an unreleased ThrowScope; they use RELEASE_AND_RETURN. NodeVMSourceTextModule::initializeImportMeta releases before JSC::call rather than after.
  • JSCookieMap.delete, JSSQLStatement columnTypes (putDirectIndex) / deserialize (toWTFString), jsFunctionToClass (toObject), process.dlopen (toWTFString), process.getBuiltinModule, Buffer.byteLength, JSValue.fromEntries, and the Worker constructor had their check hoisted a statement or two away from the throwing call; it now sits directly under it.
  • tty.isatty, makeGetterTypeErrorForBuiltins, makeDOMExceptionForBuiltins, JSValue.stringIncludes, JSValue.isInstanceOf, and Bun__ErrorCode__inspectForErrorMessage declared a TopExceptionScope but only ever return-if-exception; they use a ThrowScope (their callers already check).
  • encodeURIComponent's internal encode declared a ThrowScope it never used; removed, along with the now-pointless scope in CookieMap__writeFetchHeadersToUWSResponse.

No behaviour change intended.

Five guards that looked redundant turned out to be the only check for a callee that throws without returning Err (the console formatter for Bun.inspect / Bun.inspect.table; Blob::from_dom_form_data under new Response/new Request/fetch with a FormData body). Those are kept, each with a comment naming the callee; fixing those signatures is separate work.

JSString::to_slice / view / get_zig_string / to_slice_clone now return JsResult: JSC__JSString__toZigString resolves ropes via JSString::value(), which throws on OOM, and neither side surfaced that — four of the removed guards (h2_frame_parser.rs header pairs / value arrays, fetch.rs unix socket path) had been the only thing catching it. With the signature fixed they are genuinely dead.

Every removed check was then re-audited by tracing each preceding call to its throwing leaves (JSC FFI / WebKit source); the per-site evidence is in the review thread.

How did you verify your code works?

Debug build, then ran these under BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1 with no unchecked-exception reports:

  • test/js/web/fetch/fetch.test.ts
  • test/js/web/fetch/body.test.ts
  • test/js/bun/http/serve.test.ts
  • test/js/bun/http/bun-serve-args.test.ts
  • test/js/node/crypto/node-crypto.test.js
  • test/js/bun/util/password.test.ts
  • test/js/bun/spawn/spawn.test.ts
  • test/js/node/fs/fs.test.ts
  • test/js/bun/ffi/ffi.test.js
  • test/js/bun/test/expect.test.js
  • test/js/node/vm/vm.test.ts
  • test/js/bun/plugin/plugins.test.ts
  • test/js/bun/sqlite/sqlite.test.js
  • test/js/node/http/node-http.test.ts

plus a script driving the touched argument-validation paths (fetch/Request/Response/Bun.serve options, CryptoHasher/pbkdf2/password, process.memoryUsage/getBuiltinModule, Buffer.byteLength/copy, expect toEqual on Set/Map and toThrow, vm codeGeneration options, sqlite as()/columnTypes, timers, spawn/kill, fs arg parsing, Bun.plugin, Blob parts, Bun.color, Transpiler.scan) under the same flags.