Conversation
…S module The parser replaces a top-level `this` in an ES module at compile time. It used `null`, so `this === undefined` was false and `typeof this` was "object". The spec and Node give undefined.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe parser now lowers ES module top-level ESM top-level
Suggested reviewers: Merge Risk: ⚪ Minimal · up to ES modules now evaluate top-level 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:21 AM PT - Sep 6th, 2026
❌ @robobun, your commit c8f3c28 has 6 failures in
Add 🧪 To try this PR locally: bunx bun-pr 41515That installs a local version of the PR into your bun-41515 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I grepped the test suite for other assertions on the old null output — default/ThisOutsideFunctionRenamedToExports and default/ThisOutsideFunctionNotRenamed don't depend on the substituted value, and no other snapshot or stdout expectation matches the old ESM this → null behavior. The CommonJS and function-scoped this paths in value_for_this are untouched.
Extended reasoning...
The confirmed inline finding covers the only cleanup issue in the diff. I additionally verified the REVIEW.md rule "grep the suite for assertions on the old behavior and update them in the same PR": the two neighboring ThisOutsideFunction* bundler tests at test/bundler/esbuild/default.test.ts:1659-1702 assert on rewriting-vs-not rather than the substituted literal, so they remain correct, and no other test expects null for ESM top-level this. The parser change itself is a one-line literal swap matching the existing comment and esbuild's behavior; the CommonJS exports branch and REPL bypass are unchanged.
There was a problem hiding this comment.
LGTM — the orphaned #[inline] from the earlier review is now cleaned up, and the deletion is tidy.
What was reviewed: the value_for_this substitution now emits EUndefined (matches ES spec §10.2.1.2 and esbuild's js_parser.go valueForThis); confirmed null_value_expr had no other callers; the updated ThisUndefinedWarningESM snapshot matches esbuild's expected output rather than weakening a test; the new transpiler cases cover constant-folding of typeof this / this === undefined and leave function-body this untouched.
Extended reasoning...
Overview
This PR is a one-line semantic fix in src/js_parser/p.rs: the compile-time substitution of top-level this in an ES module now produces E::Undefined instead of E::Null. The now-orphaned null_value_expr() helper is deleted (with its #[inline] attribute, after the follow-up commit). One bundler snapshot in test/bundler/esbuild/default.test.ts is updated to expect undefined instead of null, and a new test block in test/bundler/transpiler/transpiler.test.js covers the raw substitution, typeof/=== constant folding, and confirms function-body this is not rewritten.
Security risks
None. This is a parser-output correctness fix affecting what literal a top-level this lowers to in ESM. No untrusted input handling, auth, crypto, or filesystem paths are touched.
Level of scrutiny
Low-to-medium. The change is a single expression swap whose correctness is directly verifiable against the ES spec (module Environment Record's [[ThisValue]] is undefined) and against esbuild's reference implementation, which the ported test (ThisUndefinedWarningESM) already tracks. The comment immediately above the changed line already documented the intended undefined behavior, so the code is being brought in line with its own stated contract. The CommonJS branch and function-scope this handling are untouched, and the new test explicitly guards the function-body case.
Other factors
The prior review's only note (orphaned #[inline] left on is_require_marker) was addressed in commit 77be223 — verified at lines 10270-10284 that the deletion is now clean. The updated snapshot is a correction toward spec/esbuild parity, not a weakening. Dead code (null_value_expr) is deleted in the same PR per REVIEW.md. No CODEOWNERS gate on these paths and no outstanding third-party objections in the timeline.
|
Status: the diff is ready. Two CI runs (110868 and 110986) fail only on lanes this one-line parser change does not touch: test-crypto-dh-leak.js (RSS threshold on x64-asan), bun-patch.test.ts and isolated-install.test.ts (Windows install lanes), import-meta.test.js (worker crash in a parallel batch on ubuntu aarch64, passes locally with this branch), fetch-backpressure.test.ts (timeout on Windows aarch64), serve-error-handler-stream.test.ts (a concurrent fixture got a 404 on darwin aarch64), and node-dgram.test.js (pre-existing on darwin x64). These are reported for main-break triage. The transpiler, esbuild/default, and esbuild/ts suites pass locally with the fix. |
|
Closing in favor of #32173, which fixes the same bug (#32167) with the same |
2 participants
Problem
thisevaluates tonullin bun.typeof thisgives"object"andthis === undefinedgivesfalse. Node and the spec giveundefined. This affectsbun run,bun build, andBun.Transpiler.value_for_thisinsrc/js_parser/p.rs:6029. The parser substitutes a top-levelthisat compile time. The comment says it must beundefined, but the code substitutedE::Null.Fix
E::Undefinedinstead ofE::Null. Thenull_value_exprhelper had no other caller, so it is removed.thisisexports) andthisinside functions and classes are not changed.test/bundler/transpiler/transpiler.test.js(new blocktop-level this in an ES module is undefined, fails on main).test/bundler/esbuild/default.test.tsThisUndefinedWarningESMnow expectsundefined, which is what the esbuild test expects. The transpiler,esbuild/default, andesbuild/tssuites pass.Background
value_for_thisruns in the visit pass for everyE::Thisthat is not nested inside a function. For a file with ES module syntax it returns a replacement expression, so no runtimefn.call(undefined)is needed.typeof thisandthis === undefinedthen constant-fold from the substituted literal, which is why the wrong literal showed up in folded output as well.[auto-merge] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file