Conversation
…me for cjs
- parse_stmt: call mark_strict_mode_feature(WithStatement) in t_with so
bundling a with-statement to --format=esm errors instead of emitting
strict-mode-invalid output, and "use strict"; with(x){} is rejected.
Sloppy cjs output and bun run of .cjs are unchanged.
- fold: include import.meta.filename in the cjs inlining set (same value
as import.meta.path); previously emitted verbatim, which is a syntax
error under Node.js CommonJS.
- visit_expr: emit esbuild's direct-eval debug note when bundling.
WalkthroughChangesThe bundler now inlines Import Meta CJS Handling
Bundled Eval Diagnostics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:53 PM PT - Jul 26th, 2026
❌ @robobun, your commit fad5bfd has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35961That installs a local version of the PR into your bun-35961 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
robobun
changed the title
|
Dropped the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/parse/parse_stmt.rs:308-310— Themark_strict_mode_feature(WithStatement, ...)call is placed in the parse pass, but esbuild performs this check in the visit pass (case *js_ast.SWith:invisitStmts). At parse timeis_strict_mode()only seesExplicitStrictMode—ImplicitStrictModeClass(visit/mod.rs:805) andImplicitStrictModeImport/Export/TopLevelAwait(p.rs:2796–2805,prepare_for_visit_pass) are set after parsing — soclass C { m() { with(x){} } }andexport {}; with(x){}bundled to--format=cjsslip through and still produce output every engine rejects. Moving the call intos_with()at visit_stmt.rs:1620 (usingjs_lexer::range_of_identifier(p.source, stmt.loc)for the range) matches esbuild's control flow, aligns with every othermark_strict_mode_featurecaller in this codebase, and covers the whole bug class in one place.Extended reasoning...
What the bug is
The PR adds
p.mark_strict_mode_feature(StrictModeFeature::WithStatement, with_range, b"")insidet_with()in parse_stmt.rs — the parse pass. The reference implementation (esbuild) performs this check in the visit pass instead, undercase *js_ast.SWith:invisitStmts(js_parser.go ~10885). Per REVIEW.md, "for ported code, the reference implementation (esbuild, Node) is the spec — diff control flow against it".Why the layer matters
mark_strict_mode_featurereadsself.current_scope().strict_modeviais_strict_mode()(p.rs:4331, 4383). Several strict-mode kinds are only populated after the parse pass completes:ImplicitStrictModeClassis set at visit/mod.rs:805 viarecursive_set_strict_mode(StrictModeKind::ImplicitStrictModeClass)— during class visit.ImplicitStrictModeImport/Export/TopLevelAwaitare set at p.rs:2796–2805 inprepare_for_visit_pass, after parsing completes.
The only strict-mode kind set during parsing is
ExplicitStrictMode(a leading"use strict"directive, parse/mod.rs:1486), inherited by child scopes at push time (p.rs:3261). So at parse time,is_strict_mode()insidet_with()returnsfalsefor every implicit-strict scope.Step-by-step proof
Take
bun build --format=cjs --target=nodeon:class C { m() { with (x) {} } }
- Parse pass hits
t_with()at parse_stmt.rs:308. At this pointcurrent_scope().strict_mode == SloppyMode— the class body'sImplicitStrictModeClasswon't be set until visit/mod.rs:805. is_strict_mode()(p.rs:4331) →false.is_strict_mode_output_format()(p.rs:4368, 4388) →bundle && output_format.is_esm()→falsefor--format=cjs.mark_strict_mode_featurereturnsOk(())with no diagnostic.- Visit pass reaches
s_with()at visit_stmt.rs:1620 — no check there. - Output contains
class C { m() { with (x) {} } }, which Node.js (and every engine) rejects: "SyntaxError: Strict mode code may not include a with statement".
Same walkthrough for
export {}; with (Math) {}under--format=cjs:ImplicitStrictModeExportis set at p.rs:2801 inprepare_for_visit_pass, aftert_with()has already run and returned without erroring.esbuild rejects both of these; Bun with this patch silently emits unparseable output — the exact symptom the PR title says it fixes.
Why existing code doesn't prevent it
The two things that do work at parse time — the ESM-output-format check (
is_strict_mode_output_format()readsp.options, populated before parsing) and explicit"use strict"(ExplicitStrictModeset at parse/mod.rs:1486) — happen to cover only the two test cases the PR added. Every othermark_strict_mode_featurecaller in the codebase (visit/mod.rs:99, visit/mod.rs:620, visit_expr.rs:187, p.rs:4535 which is called from the visit pass) lives in the visit pass precisely so it sees the fully-resolvedstrict_mode.Impact
Per REVIEW.md, "Fix the whole class in the same PR" and "Fix bugs at the layer that owns the violated invariant, never where the symptom appears". The PR's stated bug class is "
bun buildemits output Node.js cannot parse due towith", and the wrong-layer placement leaves that class open for class bodies and implicit-ESM modules bundled to non-ESM output.Fix
Move the call from
t_with()(parse_stmt.rs:310) intos_with()at visit_stmt.rs:1620, beforep.visit_expr(&mut data.value):fn s_with(p: &mut Self, stmts: &mut StmtList<'a>, stmt: &mut Stmt, data: &mut S::With) -> Result<(), Error> { p.mark_strict_mode_feature( StrictModeFeature::WithStatement, js_lexer::range_of_identifier(p.source, stmt.loc), b"", )?; p.visit_expr(&mut data.value); ...
This is the same one-line change, matches esbuild exactly, and covers
ExplicitStrictMode,ImplicitStrictModeClass, andImplicitStrictModeImport/Export/TopLevelAwaitin one place. Worth adding a test for the class-body case (class C { m() { with(x){} } }under--format=cjsshould error) alongside the existing three.
|
Re the CI on build 82576 (fad5bfd, finished): 195/196 jobs passed. The only failure is the binary-size check, which compares against a stale canary baseline (main #79916, ~2600 builds back) and shows the same +~550 KB on unrelated PR builds 82552 and 82554; this diff is three source lines. The four test annotations ( |
There was a problem hiding this comment.
LGTM — the two inline notes are non-blocking test-strength nits; the change itself is correct.
What was reviewed:
- fold.rs:
import.meta.filenamejoins the existing.patharm and inlines top.source.path.text(full path), matching Node's__filenamesemantics; the sibling.dir/.dirname/.file/.urlarms already covered the rest of the set. - visit_expr.rs: the direct-eval note follows the identical
range_of_identifier→add_range_debugpattern used ~150 lines below for the unbundled-require note, and is gated onp.options.bundlelike esbuild. - The pre-existing
module.filenamebasename fold flagged last run has been acknowledged and handed off; the CI failure (worker-transfer-terminate-stressSIGABRT) is unrelated to the bundler.
Extended reasoning...
Overview
Two hunks in the JS parser plus one bundler test:
src/js_parser/fold.rs(+1/-1): adds|| name == b"filename"to the existingname == b"path"arm inside theEImportMetacjs/Bake fold block, soimport.meta.filenameinlines top.source.path.textalongsideimport.meta.path. This closes the last hole in the{dir,dirname,file,path,url}set that already prevented rawimport.metafrom reaching--format=cjsoutput.src/js_parser/visit/visit_expr.rs(+8/-1): replaces a TODO inside the direct-eval detection block with esbuild's debug-level note, gated onp.options.bundle. The call shape (js_lexer::range_of_identifier(p.source, e_.target.loc)→p.log().add_range_debug(Some(p.source), r, b"...")) is byte-for-byte the pattern already used at visit_expr.rs:2056-2062 for the unbundled-require note.test/bundler/bundler_edgecase.test.ts(+20):edgecase/ImportMetaFilenameCjsbundlesimport.meta.{filename,dirname,path}withformat: "cjs", target: "node", asserts noimport.metatoken remains, and runs the output under Node checkingtypeof === "string"for each.
Security risks
None. No untrusted-input parsing, no allocation, no lifetime changes. The fold arm reuses an existing e_string_init(p.source.path.text) expression already emitted for the sibling .path case; the eval note is a debug-level log message with a static byte literal.
Level of scrutiny
Low. The fold change is a one-token addition to an existing conditional whose consequent is unchanged; correctness reduces to "is import.meta.filename semantically the full path?" — yes, per Node's docs it equals __filename. The eval note is a TODO-to-implementation swap that copies an in-file pattern verbatim and only fires at debug log level, so worst-case regression is a spurious build note.
Other factors
- The two inline findings this run are both nits about test strength (no coverage for the debug note;
typeofrather than value equality), not correctness. The primary fix — rawimport.metano longer reaches cjs output — is covered by the new test's.not.toContain("import.meta")+runtime: "node"assertions. - My prior-run comment on the pre-existing
module.filename→ basename fold (fold.rs:364) was acknowledged; the author is handling it (andmodule.path) in a follow-up. That thread is resolved and not merge-blocking here. - The
with-statement hunk was already dropped in favor of #35959, so no overlap remains. - CI shows
worker-transfer-terminate-stress.test.tsSIGABRT on x64-asan and a binary-size delta vs an older canary; neither is attributable to a 9-line bundler diff.
![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
🤖 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/bundler_edgecase.test.ts`:
- Around line 2155-2156: Update the assertions around import.meta.filename and
import.meta.path to compare the actual full source path against the fixture’s
expected absolute /entry.ts path, using platform-safe path construction. Replace
the current shape-only length and equality checks while preserving the existing
test context.
🪄 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: 43c310cd-c8bc-4426-9ea8-a0de4e709e75
📒 Files selected for processing (3)
src/js_parser/fold.rssrc/js_parser/visit/visit_expr.rstest/bundler/bundler_edgecase.test.ts
There was a problem hiding this comment.
LGTM — the test-strength nit is addressed in fad5bfd (.filename === .path and .length > .file.length pin the full-path value), and the direct-eval note gap is acknowledged with a reasonable rationale (debug-level, no harness knob, matches the untested sibling at :2058).
Extended reasoning...
Overview
Three small changes: fold.rs adds || name == b"filename" to the existing import.meta.path fold arm so cjs/Bake output inlines import.meta.filename to the full path instead of leaving raw import.meta (a Node syntax error); visit_expr.rs replaces a TODO with an add_range_debug note for direct eval() under p.options.bundle, matching esbuild and the existing add_range_debug pattern at :2058; bundler_edgecase.test.ts gains edgecase/ImportMetaFilenameCjs.
Security risks
None. This is a compile-time constant-folding tweak in the parser plus a debug-level diagnostic string. No user input reaches new allocation or side-effecting paths.
Level of scrutiny
Low. The fold.rs change is a one-token addition to an existing branch that already handles path with the correct value (p.source.path.text). The visit_expr.rs change is a non-load-bearing advisory diagnostic guarded by p.options.bundle, using the same helper (js_lexer::range_of_identifier + p.log().add_range_debug) as neighboring code.
Other factors
I reviewed this PR twice previously. All three inline threads are resolved: the pre-existing module.filename basename bug was handed off separately (correct — different failure class); the test-strength nit was fixed in fad5bfd; the direct-eval-note coverage gap was acknowledged with the harness-limitation explanation. The new test asserts no import.meta token remains in cjs output, runs the result under Node, and now pins .filename to the full path via equality with .path and a length check against .file. No new findings from the bug-hunting system this run.
This branch has not been deployed
2 participants
Problem
The cjs/Bake fold pass in
fold.rsalready inlinesimport.meta.{dir,dirname,file,path,url}for--format=cjs, butfilename(Node's__filenameequivalent, same value asimport.meta.path) was missing from the set, so the rawimport.metaexpression reached the output and Node rejected it.Fix
fold.rs: treatimport.meta.filenamethe same asimport.meta.pathin the cjs/Bake inlining branch.visit_expr.rs: emit esbuild's debug-level note for directeval()when bundling (replaces the TODO at that site).Verification
edgecase/ImportMetaFilenameCjsintest/bundler/bundler_edgecase.test.tsasserts noimport.metaremains in cjs output and that the result runs undernode. It fails on the released binary and passes with this change.bundler_edgecase.test.ts,bundler_cjs.test.ts,bundler_minify.test.ts,transpiler/transpiler.test.js,transpiler/runtime-transpiler.test.tsandesbuild/default.test.ts -t DirectEvalcontinue to pass.Related
import.meta.mainprecedence wrap: Parenthesizerequire.main === moduleper its surrounding precedence #33447Bun.build({env:"disable"})NODE_ENV handling: Bun.build: honorenv: "disable"for process.env.NODE_ENV #35952withstatement rejected for ESM output: bundler: error on 'with' statements that would land in strict-mode ESM output #35959 (thewithhunk was dropped from this PR in its favor; bundler: error on 'with' statements that would land in strict-mode ESM output #35959 runs the check in the visit pass so it also covers implicit-strict ESM sources bundled to non-ESM output)no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts