Treat BunTranspiledModule as Module in debugger parse data, cached types and completion - #405
Conversation
There was a problem hiding this comment.
LGTM — mechanical addition of BunTranspiledModule to three Module switch arms under USE(BUN_JSC_ADDITIONS), matching the existing pattern in InspectorDebuggerAgent.cpp. The inline nit about the reinterpret_cast in CachedSourceProvider::encode is worth cleaning up but is not a blocker: only base-class virtuals are reached and the same cast pattern already ships for Program/Module.
Extended reasoning...
Overview
The PR adds case SourceProviderSourceType::BunTranspiledModule: (guarded by USE(BUN_JSC_ADDITIONS)) as a fallthrough into the existing Module arm in three switch statements: gatherDebuggerParseDataForSource in DebuggerParseData.cpp, CachedSourceProvider::encode/decode in CachedTypes.cpp, and getSourceType in Completion.cpp. Net diff is +12 lines, all conditional-compilation-guarded case labels with no new logic.
Security risks
None. No new inputs, no parsing changes, no auth/crypto/permissions. The change routes an existing Bun-internal enum value to the same code path already used for Module; before this PR two of the three sites would either return a benign default or hit RELEASE_ASSERT_NOT_REACHED().
Level of scrutiny
Low-to-moderate. The change is mechanical and mirrors the identical treatment already applied at InspectorDebuggerAgent.cpp:99-101 and asserted in ModuleProgramExecutable.cpp:40-42. I grepped for all SourceProviderSourceType::Module switch sites and confirmed no other unguarded switch was missed. The CachedStringSourceProvider::decode path already handles the sibling-provider case explicitly (returning SourceProvider* and reusing decoder.provider()), so the newly-reachable decode arm is consistent with prior Bun-side work in this file.
Other factors
The one finding is a nit: the encode-side reinterpret_cast<const StringSourceProvider&> now binds a sibling type. It is technically UB but harmless in practice — under USE(BUN_JSC_ADDITIONS) the callee only invokes Base::encode (which takes const SourceProvider&) and virtual source(), and the same reinterpret pattern is upstream WebKit code that has shipped for years. The suggested cleanup (widen the parameter to const SourceProvider& under the Bun guard) is straightforward and can land here or as a follow-up without changing behaviour. This does not warrant blocking a 12-line pattern-following change that unblocks breakpoint resolution and bytecode caching for Bun-transpiled modules.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughChangesBunTranspiledModule support
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
dc3159e to
4e25722
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM — thanks for addressing the reinterpret_cast nit; the encode side now mirrors decode and takes const SourceProvider& directly.
What was reviewed:
- The three switch fall-throughs route
BunTranspiledModuleto the existingModulearm, matching the pattern already inInspectorDebuggerAgent::scriptTypeForScript. CachedStringSourceProvider::encodeunderBUN_JSC_ADDITIONSonly touches base-class API (Base::encodetakesconst SourceProvider&;source()is pure-virtual onSourceProvider), so widening the parameter is safe.- All changes are gated on
USE(BUN_JSC_ADDITIONS), so non-Bun builds are unaffected.
Extended reasoning...
Overview
This PR adds SourceProviderSourceType::BunTranspiledModule as a fall-through to the Module case in three switch statements: gatherDebuggerParseDataForSource (DebuggerParseData.cpp), CachedSourceProvider::encode/decode (CachedTypes.cpp), and getSourceType (Completion.cpp). It also widens CachedStringSourceProvider::encode to accept const SourceProvider& under USE(BUN_JSC_ADDITIONS) and drops the reinterpret_cast<const StringSourceProvider&> at the call site — the exact fix suggested in my earlier inline comment on the previous revision, which the author applied in commit ff42fd0 and marked resolved.
Security risks
None. The change routes an existing Bun-internal source-provider type through existing module-handling code paths. No user input parsing, auth, crypto, or permissions are involved. Everything is guarded by #if USE(BUN_JSC_ADDITIONS) so stock WebKit is byte-identical.
Level of scrutiny
Low-to-moderate. The DebuggerParseData and Completion changes are pure case-label additions that fall through to existing logic — mechanically identical to what InspectorDebuggerAgent::scriptTypeForScript already does for this enum value. The CachedTypes change deserved a closer look because of the sibling-type cast, but the follow-up commit removed the UB by widening the parameter type; I confirmed Base::encode at CachedTypes.cpp:1614 already takes const SourceProvider& and source() is pure-virtual on SourceProvider (SourceProvider.h:75), so the widened body compiles and dispatches correctly with no StringSourceProvider-specific member access remaining.
Other factors
- The prior review's only finding (nit-level) was fully addressed and the thread resolved.
- The bug hunting system found no issues on the current revision.
- The decode path was already prepared for this:
CachedStringSourceProvider::decodereturnsSourceProvider*and reusesdecoder.provider()when the source type and length match, so the newBunTranspiledModuledecode case slots in without further change. - A test exercising the debugger breakpoint path lives in the paired oven-sh/bun#35605 per the description.
Preview Builds
|
`447082ab6897` → `3997b59485da` is three commits on the fork: - oven-sh/WebKit#405: JSC's `DebuggerParseData`, `CachedTypes` and `Completion` switches get a `BunTranspiledModule` arm next to `Module`. Runtime ESM that Bun hands to JSC with a prebuilt module record (`bun test --isolate` / `--parallel`, `bun build --compile` output) uses that source type, and `gatherDebuggerParseDataForSource` returned false for it, so `Debugger.setBreakpoint` replied "Could not resolve breakpoint" and `Debugger.setBreakpointByUrl` returned no locations for those files. - oven-sh/WebKit#406: exception checks on the inspector's pause / `evaluateOnCallFrame` / `Runtime.evaluate` paths (`JSJavaScriptCallFrame`, `JSInjectedScriptHost`, `jsToInspectorValue`). Under `validateExceptionChecks` (the ASAN lane) a paused inspectee used to abort at `JSJavaScriptCallFrame::scopeChain`, which is why `inspector.test.ts` stripped the flag for its children and `inspect.test.ts` sat in `no-validate-exceptions.txt`. Both workarounds are removed here; all of `test/cli/inspect/*` and `test/js/node/inspector/inspector.test.ts` pass locally with the flag forced on for every child. - oven-sh/WebKit#403 also landed in between: `ALWAYS_INLINE` is `__always_inline__` again in release builds, so `NodeUtilTypesModule.cpp` now includes `ObjectPrototypeInlines.h` for `objectPrototypeToString` (the only out-of-line use that relied on the old stopgap; release links locally against the new tarball). `test/cli/inspect/debugger-buntranspiledmodule.test.ts` (from #35754, updated for the `scriptType` param the Aug 2 upgrade introduced) drives `bun test --isolate` under `--inspect-wait` and asserts both breakpoint calls resolve; it fails on `447082ab` and passes here. #35605 makes every `bun run` module take the `BunTranspiledModule` path; splitting the bump out so it can land first. Supersedes #35754. --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Runtime-transpiled ESM in Bun uses
SourceProviderSourceType::BunTranspiledModulewhenever the transpiler hands JSC a prebuilt module record. Three switches only had aModulearm, so those providers fell intodefault:gatherDebuggerParseDataForSourcereturned false, leavingpausePositionsempty, soDebugger::resolveBreakpointnever resolved aDebugger.setBreakpointByUrlline breakpoint on such a file (onlydebugger;statements paused).CachedSourceProvider::encode/decodewouldRELEASE_ASSERT_NOT_REACHED().getSourceTypein Completion.cpp mapped it toScriptFetchParameters::Type::Noneinstead ofJavaScript.Each now shares the
Modulearm underUSE(BUN_JSC_ADDITIONS), the same wayscriptTypeForScriptin InspectorDebuggerAgent.cpp already does. Supersedes #345 (same change, rebased past the 3722912 upgrade which already covered the InspectorDebuggerAgent site).Today this is only reachable under
bun test --isolateand--compileoutput; oven-sh/bun#35605 makes everybun runmodule take this path, and carries a test that sets a line breakpoint over the inspector protocol and fails without this change.