module loader: evaluate ESM-imported CommonJS at its post-order slot - #35971
Open
robobun wants to merge 24 commits into
Open
module loader: evaluate ESM-imported CommonJS at its post-order slot#35971robobun wants to merge 24 commits into
robobun wants to merge 24 commits into
Claude / Claude Code Review
completed
Jul 26, 2026 in 15m 44s
Code review found 2 important issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/JSCommonJSModule.cpp:1712-1723 |
CJS export getters fire twice under the deferred-eval path |
| 🔴 Important | src/jsc/bindings/headers-handwritten.h:137-139 |
commonjs_export_names BunString leaks on the require() path |
| 🟡 Nit | src/js_parser/visit/visit_expr.rs:2024-2028 |
Embedded NUL in Object.defineProperty export name corrupts the NUL-joined encoding |
Annotations
Check failure on line 1723 in src/jsc/bindings/JSCommonJSModule.cpp
claude / Claude Code Review
CJS export getters fire twice under the deferred-eval path
`evaluateDeferredCommonJSModuleForESM` calls `toSyntheticSource` (which enumerates every own property and invokes each getter via `slot.getValue()`) solely to extract the `default` value, then the `m_staticExportNames` loop reads each name from `exportsObject` a second time — so an enumerable accessor like TypeScript's `Object.defineProperty(exports, "foo", { enumerable: true, get: … })` fires **twice** per ESM import. Pre-PR Bun and Node fire it once, so this is a user-observable regression (de
Check failure on line 139 in src/jsc/bindings/headers-handwritten.h
claude / Claude Code Review
commonjs_export_names BunString leaks on the require() path
The new `ResolvedSource.commonjs_export_names` field (a +1 `BunString`) leaks on the `require()` path: `fetchCommonJSModuleNonBuiltin` → `JSCommonJSModule::evaluate` passes `source` to `Zig::SourceProvider::create` without clearing it, and `~SourceProvider` (ZigSourceProvider.cpp:188-190) was not updated to deref it. Every first `require()` of a freshly-transpiled CJS module leaks one WTFStringImpl. Add `m_resolvedSource.commonjs_export_names.deref();` to `~SourceProvider`, or clear+deref the fi
Check warning on line 2028 in src/js_parser/visit/visit_expr.rs
claude / Claude Code Review
Embedded NUL in Object.defineProperty export name corrupts the NUL-joined encoding
The `Object.defineProperty(exports, "<name>", ...)` recorder accepts string literals containing embedded NUL bytes (e.g. `"a\0b"`), which then collide with the NUL-joined encoding in `join_commonjs_export_names` — `assignStaticExportNames` splits the single name into two, and the wrapper declares `export{$e0 as "a",$e1 as "b"}` with both bindings reading `undefined`. Add `&& !name.contains(&0)` to the guard so such names are skipped (Node's cjs-module-lexer would not detect them anyway).
Loading