Conversation
|
Updated 1:26 AM PT - Sep 8th, 2026
❌ @robobun, your commit d798cdd has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32173That installs a local version of the PR into your bun-32173 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRewrites top-level ChangesTop-level this undefined substitution tests and compiler updates
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_parser/p.rs (1)
5616-5623:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
module_type == Esmin this branch too.Line 5616 still keys only on
has_es_module_syntax.prepare_for_visit_pass()separately marks files as ESM whenself.options.module_type == options::ModuleType::Esm, so a.mjs/type: "module"file with no explicitimport/exporttokens will still fall through to the CommonJSexportsrewrite instead of gettingundefined.💡 Suggested fix
- if self.has_es_module_syntax && self.commonjs_named_exports.count() == 0 { + if (self.has_es_module_syntax + || self.options.module_type == options::ModuleType::Esm) + && self.commonjs_named_exports.count() == 0 + { // In an ES6 module, "this" is supposed to be undefined. Instead of // doing this at runtime using "fn.call(undefined)", we do it at // compile time using expression substitution here. return Some(Expr { loc, data: js_ast::ExprData::EUndefined(E::Undefined), }); } else {As per coding guidelines, "Fix the whole bug class in the same PR - grep for every sibling site sharing the pattern."
🤖 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 `@src/js_parser/p.rs` around lines 5616 - 5623, The branch that substitutes "this" for undefined only checks self.has_es_module_syntax and misses files marked ESM via options; update the condition to also consider self.options.module_type == options::ModuleType::Esm (or an equivalent module_type == Esm flag) so that when has_es_module_syntax OR the module_type is Esm and commonjs_named_exports.count() == 0 you return the EUndefined Expr; search for other sibling sites that check has_es_module_syntax (e.g., other CommonJS rewrites called after prepare_for_visit_pass()) and apply the same module_type check to fix the whole bug class.Source: Coding guidelines
🤖 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/bundler/transpiler/runtime-transpiler.test.ts`:
- Around line 236-239: The test captures stderr but never asserts it; update the
subprocess tests that read Promise.all([proc.stdout.text(), proc.stderr.text(),
proc.exited]) to assert stderr before checking exitCode—either add
expect(stderr).toBe("") (or the expected stderr string) immediately after the
stdout assertion or include stderr in the same object assertion (e.g., expect({
file, stdout, stderr }).toEqual({ file, stdout: "undefined true\n", stderr: ""
})), then keep expect(exitCode).toBe(0); apply the same change for the other
occurrence around the variables stdout, stderr, exitCode.
---
Outside diff comments:
In `@src/js_parser/p.rs`:
- Around line 5616-5623: The branch that substitutes "this" for undefined only
checks self.has_es_module_syntax and misses files marked ESM via options; update
the condition to also consider self.options.module_type ==
options::ModuleType::Esm (or an equivalent module_type == Esm flag) so that when
has_es_module_syntax OR the module_type is Esm and
commonjs_named_exports.count() == 0 you return the EUndefined Expr; search for
other sibling sites that check has_es_module_syntax (e.g., other CommonJS
rewrites called after prepare_for_visit_pass()) and apply the same module_type
check to fix the whole bug class.
🪄 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: 2aa73306-b322-4419-9f80-9e0f1ad174c4
📒 Files selected for processing (4)
src/js_parser/p.rstest/bundler/esbuild/default.test.tstest/bundler/transpiler/runtime-transpiler.test.tstest/bundler/transpiler/transpiler.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/js_parser/p.rs`:
- Around line 5616-5621: The strict-mode check in P::prepare_for_visit_pass
should use the same forced-ESM predicate as P::value_for_this: replace the
current branch that only checks
esm_import_keyword/esm_export_keyword/top_level_await_keyword with a predicate
that uses self.has_es_module_syntax || self.options.module_type ==
options::ModuleType::Esm (and still ensure commonjs_named_exports.count() == 0
if that was part of the original module detection), so files forced to ESM (e.g.
.mjs or package.json "type":"module") enable implicit strict mode and match the
behavior of value_for_this; update the branch in prepare_for_visit_pass
accordingly, referencing prepare_for_visit_pass, value_for_this,
has_es_module_syntax, and options::ModuleType::Esm.
In `@src/jsc/RuntimeTranspilerCache.rs`:
- Line 46: Update the runtime-transpiler cache version constant so both modules
agree: change the RUNTIME_TRANSPILER_CACHE_VERSION constant in the bundler cache
to match EXPECTED_VERSION (23) defined in RuntimeTranspilerCache.rs; locate the
RUNTIME_TRANSPILER_CACHE_VERSION symbol in src/bundler/cache.rs and set its
value to 23 to prevent existing cache entries from being treated as stale.
🪄 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: d0e6225c-9432-42eb-b1d3-c5f940a4c837
📒 Files selected for processing (5)
src/js_parser/p.rssrc/js_printer/lib.rssrc/jsc/RuntimeTranspilerCache.rstest/bundler/transpiler/runtime-transpiler.test.tstest/bundler/transpiler/transpiler.test.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/js_parser/p.rs (1)
5616-5620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep forced-ESM detection in sync with implicit strict-mode setup.
Lines 5619-5620 now classify syntax-less
.mjs/"type":"module"files as ESM for top-levelthis, butprepare_for_visit_pass()still only enables implicit strict mode from import/export/top-level-await. That leaves the same file with modulethissemantics and sloppy-mode parsing rules, which is an observable correctness break for forced-ESM files. Please drive both sites from the same forced-ESM predicate/helper.As per coding guidelines, "Rust code: fix the whole bug class in the same PR - grep for every sibling site sharing the pattern."
🤖 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 `@src/js_parser/p.rs` around lines 5616 - 5620, The code currently treats files as forced-ESM in the TLA/`this` check using the condition (self.has_es_module_syntax || self.options.module_type == options::ModuleType::Esm) && self.commonjs_named_exports.count() == 0 but `prepare_for_visit_pass()` still enables implicit strict mode only when import/export/top-level-await are present, causing mismatched semantics; extract that forced-ESM logic into a single helper (e.g., is_forced_esm(&self) or ModuleType::is_forced_esm) and replace both the top-level `this`/TLA check and the implicit strict-mode decision in `prepare_for_visit_pass()` to call this helper so forced `.mjs`/package-"type":"module" files get both module `this` semantics and implicit strict parsing consistently (ensure to reference and remove the duplicated conditionals: has_es_module_syntax, options.module_type == options::ModuleType::Esm, and commonjs_named_exports.count()).Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@src/js_parser/p.rs`:
- Around line 5616-5620: The code currently treats files as forced-ESM in the
TLA/`this` check using the condition (self.has_es_module_syntax ||
self.options.module_type == options::ModuleType::Esm) &&
self.commonjs_named_exports.count() == 0 but `prepare_for_visit_pass()` still
enables implicit strict mode only when import/export/top-level-await are
present, causing mismatched semantics; extract that forced-ESM logic into a
single helper (e.g., is_forced_esm(&self) or ModuleType::is_forced_esm) and
replace both the top-level `this`/TLA check and the implicit strict-mode
decision in `prepare_for_visit_pass()` to call this helper so forced
`.mjs`/package-"type":"module" files get both module `this` semantics and
implicit strict parsing consistently (ensure to reference and remove the
duplicated conditionals: has_es_module_syntax, options.module_type ==
options::ModuleType::Esm, and commonjs_named_exports.count()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49a8f0a4-b2af-4de9-85fc-49152ae63652
📒 Files selected for processing (3)
src/js_parser/p.rssrc/js_printer/lib.rssrc/jsc/RuntimeTranspilerCache.rs
There was a problem hiding this comment.
All my earlier feedback has been addressed and I didn't find anything new, but since this changes transpiler semantics (the module_type == Esm expansion of value_for_this, the printer's delete-operand wrap, and the runtime cache key derivation) it's worth a human maintainer's sign-off.
Extended reasoning...
Overview
This PR fixes #32167 by changing top-level this substitution in ES modules from null to undefined, matching ECMA-262 and Node. It touches five source files in the parser/printer/cache layer plus four test files:
src/js_parser/p.rs—value_for_thisreturnsEUndefinedinstead ofENull, and now also treatsoptions.module_type == Esmas ESM (so.mjs/"type": "module"files with no import/export syntax getundefinedinstead of falling through toexports). The now-unusednull_value_exprhelper is removed (verified no remaining references).src/js_printer/lib.rs—EUndefinedadded to the delete-operand wrap predicate so substituteddelete thisprints asdelete (0, undefined)instead of the strict-mode-invaliddelete undefined.src/jsc/RuntimeTranspilerCache.rs—EXPECTED_VERSIONbumped 22→23 with a changelog line.src/bundler/cache.rs—RUNTIME_TRANSPILER_CACHE_VERSIONmirror synced 20→23.src/js_parser/parse/parse_entry.rs—module_type(which is#[repr(u8)]with stable discriminants) folded into the runtime-transpiler features hash, closing a pre-existing cache-aliasing gap where byte-identical files in ESM vs CJS packages shared a cache entry.
All three of my earlier inline comments (cache version bump, delete undefined printer wrap, module_type in features hash) and CodeRabbit's comments have been addressed and resolved across commits 84e22c4, 9f8b044, 63de048, and 1a6c6fa. The bug-hunting pass on the current revision found no new issues.
Security risks
None identified. No auth, crypto, permissions, or untrusted-input parsing surface is touched. The cache-key change strictly narrows which entries are considered hits, and the version bump invalidates all prior on-disk entries.
Level of scrutiny
This is core transpiler/parser infrastructure — value_for_this runs on every top-level this in every transpiled file, and the features-hash change affects cache keying for every cached module. The primary fix (ENull→EUndefined) is a one-liner that restores the upstream esbuild behavior, but the PR also includes a deliberate behavioral expansion: the || module_type == Esm clause changes what bare .mjs / "type": "module" files (with no import/export syntax) see for top-level this from {} (via the exports substitution) to undefined. That is the spec-correct outcome and is well-tested here, but it is a user-visible semantic change beyond the original bug report.
Other factors
- None of the changed files are CODEOWNER-protected.
- Test coverage is thorough: transform-output unit tests, runtime subprocess tests across import-only/export-only/bare-
.mjs/type:module/CJS, an updated bundler expectation, and a transpiler-cache integration test that exercises the ESM/CJS cache-poisoning scenario in both orders. - The author appropriately scoped out the related implicit-strict-mode question to a separate issue (#32175).
Given the transpiler-semantics scope and the multi-commit evolution across review rounds, I'm deferring to a human maintainer rather than auto-approving.
There was a problem hiding this comment.
All my earlier feedback is addressed and I found nothing new on this revision — deferring only because this is a transpiler-semantics change (widened ESM predicate, printer delete-operand wrap, cache-key shape) that's worth a maintainer glance before merge.
Extended reasoning...
Overview
This PR fixes #32167 by changing the compile-time substitution of top-level this in ES modules from null to undefined (matching ECMA-262 and Node). What started as a one-line ENull → EUndefined swap in value_for_this (src/js_parser/p.rs) grew through review into five source-file changes:
src/js_parser/p.rs— returnEUndefinedinstead ofENull; widen the ESM predicate to includemodule_type == Esm(so syntax-less.mjs/"type": "module"files also getundefined); remove the now-unusednull_value_exprhelper (verified no remaining references).src/js_printer/lib.rs— addEUndefinedtois_identifier_or_numeric_constant_or_property_accesssodelete thisin a module printsdelete (0, undefined)instead of the strict-mode-invaliddelete undefined.src/jsc/RuntimeTranspilerCache.rs— bumpEXPECTED_VERSION22 → 23.src/bundler/cache.rs— sync the mirror constant 20 → 23.src/js_parser/parse/parse_entry.rs— foldmodule_typeinto the runtime-transpiler features hash so byte-identical files in ESM vs CJS package contexts no longer alias one cache entry.
Plus four test files: bundler expectation update, new runtime subprocess tests, new transform-output tests, and a new transpiler-cache integration test.
Security risks
None. No auth, crypto, network, or filesystem-permission surface is touched. The cache-key change only adds a discriminator; it cannot cause stale entries to be accepted that weren't before.
Level of scrutiny
Moderate-to-high. The parser, printer, and runtime transpiler cache are on the hot path for every JS/TS file Bun executes. Each individual change is small and well-reasoned, but together they alter (a) ESM classification for this-substitution purposes, (b) printer output for an edge-case delete operand, and (c) the shape of the on-disk cache key. None of these are mechanical; they're semantic decisions about transpiler behavior. The widened ESM predicate in particular (treating module_type == Esm as ESM even with no import/export syntax) is a deliberate behavior change beyond the original bug report — it's correct per spec, but a maintainer should confirm it doesn't conflict with any intentional Bun-specific interop heuristic.
Other factors
- All three of my earlier inline comments (cache version bump,
delete undefinedwrap,module_typein features hash) were addressed in commits 84e22c4 and 1a6c6fa, with tests covering each. - The bug-hunting system found nothing on the latest revision.
- No CODEOWNERS paths are touched.
- The author appropriately scoped out the related implicit-strict-mode question to a follow-up issue (#32175) rather than expanding this PR further.
- Test coverage is thorough: transform-output assertions, runtime subprocess checks across four ESM-forcing variants plus a CJS negative control, and a cache-aliasing integration test that fails on the pre-fix build.
This is good work and I'd expect it to merge cleanly — I'm deferring purely because parser/cache semantics changes of this scope benefit from a maintainer sign-off rather than bot-only approval.
|
CI status: the PR's own tests (transpiler, runtime-transpiler, transpiler-cache, esbuild suites) pass on all lanes in both runs. The remaining red lanes are unrelated to this diff and differ between the two runs:
No failures overlap between runs and none touch the parser, printer, or transpiler cache. Ready for review. |
24c6b77 to
4b66440
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and the change looks correct to me, but since this alters transpiler output semantics for every ES module (and broadens the ESM classification for this-substitution via module_type), it's worth a human sign-off.
Extended reasoning...
Overview
This PR fixes #32167 by changing the compile-time substitution of top-level this in ES modules from null to undefined (per ECMA-262 §16.2.1.6 and matching Node/esbuild). It touches:
src/js_parser/p.rs—value_for_thisreturnsEUndefinedinstead ofENull; the ESM predicate is broadened to includeoptions.module_type == Esm(so.mjs/"type": "module"files without import/export syntax also getundefinedinstead of falling through to the CJSexportssubstitution). The now-unusednull_value_exprhelper is removed.src/js_printer/lib.rs— addsEUndefinedto the delete-operand wrap helper sodelete this→delete (0, undefined)instead of strict-mode-invaliddelete undefined.src/jsc/RuntimeTranspilerCache.rs— bumpsEXPECTED_VERSION22→23.src/bundler/cache.rs— syncs the mirror constant 20→23.src/js_parser/parse/parse_entry.rs— foldsmodule_typeinto the features hash so byte-identical files in ESM vs CJS contexts no longer alias one cache entry.- Tests: transform-output assertions, runtime subprocess tests across import-only/export-only/bare-
.mjs/type:modulefixtures, a CJS negative control, and a transpiler-cache test for the module_type aliasing.
All three issues I raised on earlier revisions (cache version bump, delete undefined strict-mode SyntaxError, module_type missing from the features hash) have been addressed and resolved, as has CodeRabbit's feedback. The bug-hunting pass on the current revision found nothing.
Security risks
None. No auth, crypto, filesystem permissions, or untrusted-input parsing surface is changed. The transpiler-cache changes only add a discriminant byte to an existing hash and bump a version constant.
Level of scrutiny
Moderate-to-high. The diff is small and each piece is well-reasoned, but the JS parser/printer is on the hot path for every module Bun executes, and this is a runtime-observable semantics change. Two aspects in particular warrant a maintainer's eye:
- Broadening the ESM predicate (
|| module_type == Esm) changes what.mjs/"type":"module"files with no import/export syntax see for top-levelthis— fromexports({}) toundefined. That's spec-correct and matches Node, but it's a behavior change for a class of files beyond the original bug report, and it interacts with Bun's intentional CJS-interop leniency for forced-ESM files (the author explicitly scoped out the related strict-mode question into #32175). - Cache-key change — adding
module_typetofeatures_hashis correct, but it's a structural change to how cache entries are keyed.
Other factors
- Test coverage is thorough (transform output, runtime behavior across all four ESM-detection paths, CJS control, delete-operand edge case, cache-aliasing regression test).
- CI is green on the PR's own tests across all lanes; remaining failures are documented as unrelated flakes.
- The
default/ThisUndefinedWarningESMexpectation update aligns with esbuild's original fixture. - No outstanding unresolved review threads.
I'm not approving solely because transpiler-semantics changes are the kind of thing a Bun maintainer should sign off on, not because I see anything wrong.
|
Post-rebase CI (build 63529): the PR's own tests pass on all lanes. The two red lanes are unrelated to this diff and, again, differ from the previous runs' failures:
Neither touches the parser, printer, or transpiler cache. Across three CI runs (62071, 62073, 63529) no failure has overlapped and none has involved this change. The branch is rebased onto current main and mergeable; ready for a maintainer. |
4b66440 to
e13bbdf
Compare
|
Rebased head CI (build 68404): the PR's own tests pass on all lanes. The only annotation is CI's "flaky" bucket (all lanes retried), none touching this diff:
Across four CI runs (62071, 62073, 63529, 68404) no failure has overlapped and none has involved the parser, printer, or transpiler cache. The branch is rebased onto current main and mergeable; ready for a maintainer. |
In a file with ES module syntax, value_for_this substituted E::Null for a top-level this, so typeof this was "object" and this === undefined was false. The spec, Node, and esbuild give undefined. Substitute E::Undefined instead. The CommonJS branch (this is exports) is unchanged. Bump the runtime transpiler cache version so cached output with the old substitution is not served, and hash module_type into the features hash: exports_kind for a file with no module syntax is decided by module_type, so byte-identical sources in an ESM package and a CommonJS package must not share a cache entry. Fixes #32167
e13bbdf to
0e61e64
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto current main as one commit (0e61e64) and narrowed. #41515 was a duplicate of this PR and is closed. Reproduced on 1.4.3: What changed since the last revision:
Verified with a debug build: |
|
CI on the rebased head (build 112509): the PR's own tests pass on all lanes. The red lanes are unrelated to this diff:
None involve the parser, printer, or transpiler cache. Mergeable with no open review threads; ready for a maintainer. |
There was a problem hiding this comment.
LGTM — all three earlier concerns are addressed in the current diff: EXPECTED_VERSION is bumped to 30, module_type is folded into the features hash (with a test proving byte-identical ESM/CJS files no longer alias), and the delete this → delete (0, undefined) case is covered by the printer and locked in by a transpiler test.
Checked that null_value_expr has no remaining call sites, that options::ModuleType is #[repr(u8)] so the as u8 hash write is well-defined, and that the runtime test matrix covers both the sync entry-point and async imported-dep transpiler paths plus .cjs-under-"type":"module" staying exports.
Extended reasoning...
Overview
The PR fixes #32167: top-level this in an ES module was being substituted with null instead of undefined, contrary to the ECMAScript spec, Node, and the esbuild code this was ported from. The core change is one line in src/js_parser/p.rs (ENull → EUndefined), plus removal of the now-dead null_value_expr() helper. Supporting changes: EXPECTED_VERSION bumped 29→30 in RuntimeTranspilerCache.rs to invalidate stale on-disk entries, and module_type added to the features hash in parse_entry.rs so byte-identical .js files under "type": "module" vs "type": "commonjs" no longer share a cache entry. Four test files are extended: bundler snapshot updated to match esbuild, transpiler printed-output tests (including constant folding, arrow capture, delete this, and nested-function this left alone), a describe.concurrent runtime matrix over ESM/CJS entry and dep paths, and a transpiler-cache test proving the ESM/CJS cache separation via strict-vs-sloppy observable behavior.
Security risks
None. This is a spec-correctness fix to a compile-time literal substitution in the transpiler. No user input parsing, no auth, no crypto, no filesystem or network surface. The module_type as u8 hash write is safe — options::ModuleType is #[repr(u8)] with three variants.
Level of scrutiny
Moderate. The parser is a critical path, but the semantic change is a one-token substitution that aligns with the spec and the reference implementation (the comment above the changed line already said "undefined"). The remaining changes are the cache-invalidation infrastructure I asked for in earlier review rounds. I verified null_value_expr has no remaining callers repo-wide, the ModuleType enum has a stable #[repr(u8)], and the new test file already imports tempDir/bunExe/bunEnv from harness.
Other factors
All three of my prior inline concerns (cache version bump, delete undefined strict-mode SyntaxError, module_type missing from the cache key) are demonstrably fixed in the current diff — verified from code, not from the author's self-resolution. All coderabbitai threads were resolved by non-authors. Test coverage is thorough and follows the CLAUDE.md/REVIEW.md conventions: describe.concurrent for independent subprocess tests, Promise.all draining stdout/stderr/exited, combined-object assertion before exit code, tempDir from harness, real issue number cited, and the transpiler-cache test reuses that file's existing temp_dir/dummyFile fixture pattern. The PR description transparently documents scope that was tried and backed out (widening module_type == Esm into the branch condition) with a clear reason and a locking test for the current .cjs behavior.
There was a problem hiding this comment.
LGTM — all three concerns from earlier rounds are addressed: EXPECTED_VERSION is bumped to 30, module_type is now folded into the features hash, and the printer already wraps EUndefined for delete (verified at src/js_printer/lib.rs:1544, covered by the new delete (0, undefined) assertion).
Also checked: null_value_expr has no remaining callers, the module_type as u8 cast is sound (options_types::bundle_enums::ModuleType is #[repr(u8)]), and the earlier module_type == Esm widening is indeed dropped from value_for_this — the imports-cjs.js test locks in .cjs-under-"type":"module" keeping this === module.exports.
Extended reasoning...
Overview
The PR swaps a single literal in value_for_this (ENull → EUndefined) so top-level this in ES modules matches Node and the spec, deletes the now-orphaned null_value_expr() helper, bumps the runtime transpiler cache EXPECTED_VERSION from 29 to 30, and adds module_type to the features hash so byte-identical .js files under different package.json "type" settings no longer alias one cache entry. Test coverage spans the Bun.Transpiler printed output (including typeof/equality folding, arrow capture, delete this, and function-body this left alone), runtime spawns for entry-point and imported ESM plus CJS-under-ESM, an updated esbuild bundler snapshot, and a transpiler-cache test proving the ESM/CJS split.
Security risks
None. This is a compile-time expression substitution in the parser and a hash-input addition; no untrusted-input parsing, no auth/crypto/permissions, no I/O surface.
Level of scrutiny
Low-to-moderate. The core change is a one-token literal fix that brings Bun in line with the esbuild reference (js_parser.go emits EUndefined here) and Node. The two knock-on cache concerns — stale on-disk entries and the module_type key gap — were the substantive review risk, and both are handled with the version bump plus the hash addition (backed by a targeted test). Tests are added alongside existing cases in the right files, use tempDir/bunExe/bunEnv, drain pipes concurrently, assert output before exit code, and run under describe.concurrent.
Other factors
This is a follow-up push after my earlier inline comments; every point raised is now visible in the diff. I confirmed null_value_expr has zero remaining references, is_identifier_or_numeric_constant_or_property_access already includes EUndefined on main (so delete this prints delete (0, undefined)), and bundle_enums::ModuleType is #[repr(u8)] so the as u8 hash write is well-defined. The value_for_this condition is unchanged from base (has_es_module_syntax && commonjs_named_exports.count() == 0), so the dropped module_type == Esm widening described in the PR notes is not present — and the imports-cjs.js runtime test guards against a future regression there. No outstanding third-party CHANGES_REQUESTED reviews; the coderabbit threads are non-author-resolved. Exit reason is dry_streak.
|
CI on the current head d798cdd (build 112520): the PR's own tests pass on all lanes. Same picture as the previous run:
Nothing here involves the parser or transpiler cache. Mergeable, no open review threads, ready for a maintainer. |
This branch has not been deployed
1 participant
Fixes #32167
Problem
thisevaluates tonullin bun:typeof thisis"object"andthis === undefinedisfalse. Node and the spec giveundefined. An arrow at module top level captures the same value, which is how the issue'sEventEmitterrepro shows it.value_for_thisinsrc/js_parser/p.rs:6029. The parser substitutes a top-levelthisat compile time. For a file with ES module syntax it builtE::Null. The comment above it saysundefined, and so does the esbuild code it was ported from.Fix
E::Undefined. The CommonJS branch (thisisexports), the REPL path, andthisnested in functions and class bodies are unchanged. The unusednull_value_exprhelper is removed.EXPECTED_VERSIONinsrc/jsc/RuntimeTranspilerCache.rs(29 to 30). The cache key does not include the bun version, so without the bump a cached module keeps thenulloutput.module_typeinto the transpiler cache features hash.exports_kindfor a file with no module syntax is decided bymodule_type, so byte-identical files in a"type": "module"package and a"type": "commonjs"package shared one entry, and the first to run decided strict ESM or sloppy CommonJS for both.test/bundler/transpiler/transpiler.test.js,runtime-transpiler.test.ts,test/cli/run/transpiler-cache.test.ts(the new tests fail on 1.4.3),esbuild/default.test.ts(ThisUndefinedWarningESMnow expects esbuild's output). Also ranbundler_cjs,bundler_edgecase,esbuild/{dce,importstar,packagejson}. Self-reviewed: 2 concerns raised, 2 addressed (see Notes).Background
E::Thisoutside a function. In an ES module it becomes a literal, sotypeof thisandthis === undefinedconstant-fold from it. That is why the wrong literal also showed in folded output.src/js_parser/parser.rsasks for a version bump on any parser change that affects output.module_typeisEsm,Cjs, orUnknown, from the file extension or the package.json"type". The parser reads it only to pickexports_kindwhen the file content does not decide.Notes
E::NulltoE::Undefinedchange without the cache version bump.module_type == Esm, so that a.mjsor"type": "module"file with no import/export syntax getsundefinedinstead ofexports(Node printsundefinedthere, bun prints{}). The self-review found that the runtime loader does not reportmodule_typeconsistently yet: the asyncimportpath derives it from package.json only (src/jsc/RuntimeTranspilerStore.rs:335), so a.cjsfile imported under"type": "module"reportsEsm, and a nameless nested{"type":"commonjs"}package.json is only honored for files in its own directory (read_dir_info_package_jsoninsrc/runtime/jsc_hooks.rs). With the widening,import dep from "./dep.cjs"in a"type": "module"package gavethis === undefinedinsidedep.cjs(verified), where Node and current bun givemodule.exports. That case is dropped from this PR. parser: .mjs/"type":"module" is authoritative over module/exports identifier refs #33807 makesmodule_typeauthoritative at runtime and carries the samevalue_for_thischange together with the loader fixes it needs. The newimports-cjs.jsruntime test locks in the current.cjsbehavior.EUndefinedto the printer's delete-operand wrap (delete thiswould printdelete undefined, a strict-mode SyntaxError). That has since landed on main inis_identifier_or_numeric_constant_or_property_access. The transpiler test still coversdelete (0, undefined).RUNTIME_TRANSPILER_CACHE_VERSIONmirror constant insrc/bundler/cache.rs. That constant no longer exists on main.try { undeclaredVariable = 1; "sloppy" } catch { "strict" }: run under"type": "module"then"type": "commonjs"printsstricttwice. In the other order it printssloppytwice. Node printsstrictthensloppy. Withmodule_typein the features hash bun matches Node in both orders.export {}; console.log(typeof this, this === undefined, this === null)printsobject false true. Node printsundefined true false.