transpiler: preserve function-body "use strict" in CJS - #29539
Conversation
|
Updated 1:09 PM PT - May 4th, 2026
❌ @robobun, your commit 15dd048 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29539That installs a local version of the PR into your bun-29539 --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:
WalkthroughParser restricts directive-prologue recognition to entry/function-body scopes, preserves non-module directives as Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fix touches core CJS transpilation infrastructure (parse.zig and visit.zig). The root cause and fix are well-analyzed, but the interaction between the parser's directive handling and the CJS wrapper in P.zig is non-trivial enough to warrant a human look.
Extended reasoning...
Overview
The PR fixes a regression where function-body "use strict" directives were silently dropped during CJS transpilation. Two files are modified: src/ast/parse.zig (parser directive-prologue handling) and src/ast/visit.zig (minification/DCE pass). A regression test with 3 cases is added.
Security risks
None. This is a correctness fix for strict-mode semantics; no auth, crypto, or permission code is involved.
Level of scrutiny
The fix is small and surgical (< 20 lines of logic changed), well-documented, and accompanied by solid test coverage including a guard against regression in the opposite direction. However, parse.zig and visit.zig are core transpiler infrastructure — the change relies on the invariant that the CJS wrapper in P.zig re-emits the module-level directive, which is not verified in the diff itself. That cross-file invariant is worth a human confirming.
Other factors
No CODEOWNERS entries cover these files. The bug causes real-world breakage (bluebird, random-number-csprng). The fnBodyContainsUseStrict helper at the bottom of visit.zig already handles s_directive nodes, so the parse-side conversion is consistent with the existing visitor design. The secondary fix (preserving s_directive in the joinWithComma optimization loop) is a one-liner and clearly correct.
|
Re: the CJS-wrapper invariant — the re-emit lives in const preserve_strict_mode = ...
!(parts.items.len > 0 and
parts.items[0].stmts.len > 0 and
parts.items[0].stmts[0].data == .s_directive);
...
if (preserve_strict_mode) {
remaining_stmts[0] = p.s(
S.Directive{ .value = "use strict" },
p.module_scope_directive_loc,
);
...
}Gated on |
![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 the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29533.test.ts`:
- Around line 1-3: Remove the extra bug-history prose from the test header and
leave only the single-line GitHub issue URL comment; specifically delete the two
trailing comment lines that start with "Bun 1.3.11..." and "so IIFEs (e.g.
bluebird's..." in the header of test/regression/issue/29533.test.ts so the file
contains only the issue URL comment as the header.
🪄 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: 15a0d1c5-5791-4281-9d2b-5cdca20b7ca8
📥 Commits
Reviewing files that changed from the base of the PR and between f91def2a5f0271b1b2bd36739aafc18e3d1e4e82 and c22271af9c838eef0193ba295e7069ef267d408e.
📒 Files selected for processing (5)
src/ast/parse.zigsrc/ast/visit.zigtest/regression/issue/29533-fn.fixture.cjstest/regression/issue/29533-module.fixture.cjstest/regression/issue/29533.test.ts
💤 Files with no reviewable changes (1)
- src/ast/visit.zig
There was a problem hiding this comment.
The fix logic looks correct, but this touches core CJS transpilation in parse.zig and visit.zig affecting all CJS files, and there's a flagged potential duplicate PR (#27185) that should be checked before merging.
Extended reasoning...
Overview
This PR modifies two core AST processing files: src/ast/parse.zig (directive-prologue parsing in parseStmtsUpTo) and src/ast/visit.zig (minification/DCE statement visitor). The parse change gates the skip = true behavior on p.current_scope == p.module_scope, preserving function-body directives as S.Directive nodes. The visit change removes the .s_directive => continue line that was silently dropping all directive nodes during bundling passes. Two fixture files and a test file are also added.
Security risks
No security-sensitive code is touched. The change restores correct strict-mode semantics, which if anything improves correctness rather than introducing risk.
Level of scrutiny
High. parseStmtsUpTo and visitStmts are hot paths executed for every CJS file Bun processes. A subtle off-by-one or scope-comparison error could affect directive handling across all CJS transpilation. The change is small and well-reasoned, but the blast radius of a mistake is large.
Other factors
- A potential duplicate PR (#27185) was flagged by the bot. A human should verify whether this was previously attempted and reverted, or whether the approaches differ.
- The PR description claims 3 tests but only 2 were committed (nit flagged by the inline bug report — the missing test covers the
.js-extension CJS variant, which is not a correctness problem but a coverage gap). - CI failures in
bundler_npm.test.ts,bun-install.test.ts, andleak.test.tsappear unrelated to these changes (different subsystems, present on the autofix.ci commit), but they are still outstanding on the PR.
5f3a5d4 to
9bad376
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ast/parse.zig`:
- Around line 1194-1211: The code is promoting any leading string literal to
S.Directive regardless of scope; change the promotion in parseStmtsUpTo() so you
only allocate/assign S.Directive (and set skip/module_scope_directive_loc or
stmt.data = Prefill.Data.SEmpty) when p.current_scope.kind is a module or a
function scope (e.g., .entry or the function kinds like
.function_body/.function_args); for all other kinds (blocks, catch, finally,
etc.) leave the stmt as a normal expression statement so block-level string
literals are not converted to directives. Ensure the is_strict and "use asm"
branches still apply but only after verifying p.current_scope.kind permits
directive promotion.
In `@test/regression/issue/29533-fn.fixture.cjs`:
- Around line 11-19: Replace the brittle undeclared assignment in the
exports.mode probe with a unique global sentinel name (e.g.,
"__issue_29533_sentinel__") and ensure the sentinel is deleted before and after
the probe so prior tests can't affect the result; specifically, in the
exports.mode IIFE, delete the sentinel from the global object, attempt to assign
to global[<uniqueSentinel>] inside the try/catch to detect sloppy vs strict
mode, and finally clean up by deleting global[<uniqueSentinel>] in both success
and failure paths.
🪄 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: 2fbc8e74-b435-4b25-b375-db35675969e8
📥 Commits
Reviewing files that changed from the base of the PR and between 5f3a5d4ae72b2fc3ff0fab4569963a3c46d321bc and 9bad376f876fc719b33e94f04ed5b2f4268848f6.
📒 Files selected for processing (5)
src/ast/parse.zigsrc/ast/visit.zigtest/regression/issue/29533-fn.fixture.cjstest/regression/issue/29533-module.fixture.cjstest/regression/issue/29533.test.ts
💤 Files with no reviewable changes (1)
- src/ast/visit.zig
637ce0d to
eb22e9e
Compare
There was a problem hiding this comment.
LGTM — all previously raised concerns (block-scope directive promotion, P.zig guard narrowing, missing .js-extension test) are addressed in 637ce0d.
Extended reasoning...
Overview
The PR fixes a regression introduced in Bun 1.3.11 where function-body "use strict" directives were silently dropped during CJS transpilation. Two source files are changed (parse.zig, visit.zig) plus a guard in P.zig, with five regression tests added.
Security risks
None. This is a parser/transpiler correctness fix with no auth, crypto, or permission surface.
Level of scrutiny
The changes are targeted and well-understood: a scope-kind guard in parseStmtsUpTo, removal of a continue in the DCE pass, and a narrowed string-equality check in the CJS-wrapper guard. Previous review rounds surfaced and addressed every non-trivial edge case (block-scope promotion, non-strict directive suppressing re-emission, missing .js test). The fix is now complete.
Other factors
All inline comments from both CodeRabbit and my prior reviews are marked resolved and addressed in commit 637ce0d. The CI failures shown by robobun are in unrelated tests (shell leak, file loader, bun-install, bundler_npm) and are not caused by the directive-handling changes.
eb22e9e to
d907990
Compare
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR fixes a CJS transpiler regression where function-body "use strict" directives were silently dropped. Changes touch src/ast/parse.zig (directive-prologue scoping), src/ast/visit.zig (remove blanket s_directive skip in DCE pass), src/ast/P.zig (tighten CJS-wrapper guard to check directive value, not just tag), and new regression tests.
Security risks
None. The changes operate purely on directive-handling in the parser and bundler — no auth, crypto, or permissions code is touched.
Level of scrutiny
Moderate. The fix is confined to a well-understood regression path with a clear root cause and surgical changes. The three-way interaction between parse.zig, visit.zig, and P.zig required careful reasoning, and the review cycle caught real secondary bugs (block-scope promotion, directive-value guard, missing test) which were all addressed.
Other factors
Five regression tests cover the main scenario (.cjs), the .js+package.json classifier path, the "use client" + "use strict" ordering edge case, and the block-scope minification check. CI failures visible in the timeline (bundler_npm.test.ts, ASAN SIGILL) appear to be infrastructure noise unrelated to directive handling. The bug hunting system found no new bugs in the final state.
ad34019 to
7e4e09f
Compare
7e4e09f to
9a09fc1
Compare
There was a problem hiding this comment.
LGTM — all issues from prior review rounds have been addressed.
Extended reasoning...
Overview
This PR fixes a regression (introduced in Bun 1.3.11) where function-body "use strict" directives were silently dropped during CJS transpilation. The changes touch three core parser/visitor files (parse.zig, visit.zig, P.zig) and add a regression test suite.
Security risks
None. The changes affect JS directive-prologue parsing and CJS wrapper emission — no auth, crypto, or permission code is involved.
Level of scrutiny
This PR received several rounds of iterative review with multiple critical and minor bugs caught and fixed:
- The S.Directive promotion was correctly gated to
.entryand.function_bodyscopes only (block/catch/finally remain as SExpr for DCE) - The P.zig CJS-wrapper guard was narrowed to check the directive value (
"use strict") not just the AST tag - Test coverage now includes function-body
.cjs, function-body.js(package type=commonjs), module-level directive preservation, non-strict directive guard, and block-scope DCE — all via concurrent subprocess tests
Other factors
All inline comments from prior review rounds are marked resolved and confirmed in commits 5f3a5d4, 637ce0d, eb22e9ed, d907990, and 9a09fc1. The bundler_npm snapshot offset delta (+52 bytes) is expected — the minified React bundle is slightly larger because function-body "use strict" directives are now preserved. No new concerns remain.
9a09fc1 to
dc1d48e
Compare
There was a problem hiding this comment.
All previously raised issues have been addressed; LGTM.
Extended reasoning...
Overview
This PR fixes a regression (Bun 1.3.11+) where "use strict" directives inside function bodies were silently dropped during CJS transpilation. Three files are touched: src/ast/parse.zig (directive-prologue gating), src/ast/visit.zig (removes the .s_directive => continue that discarded all directives in the minify+DCE pass), and src/ast/P.zig (narrows the CJS-wrapper guard to match specifically "use strict", not any arbitrary directive). The bundler_npm.test.ts snapshot offset updates (+52 bytes) are a mechanical consequence of directives now surviving into React's CJS output.
Security risks
None. The changes affect directive parsing and CJS wrapper emission only; no security-sensitive code paths (auth, crypto, permissions) are involved.
Level of scrutiny
Medium-high: the parser and visitor are core infrastructure, but the changes are narrowly scoped — each change site is well-commented and the logic is straightforward to verify. All five rounds of review feedback were addressed with targeted fixes and corresponding regression tests.
Other factors
All my prior inline comments were resolved in commits 5f3a5d4, 637ce0d, d907990, and 9a09fc1:
- Block-scope string literals no longer promoted to
S.Directive(scope-kind guard added) - P.zig wrapper guard checks directive value, not just tag
- Third
.js-extension CJS test added - Subprocess tests converted to
test.concurrent - Prohibited panic/ASSERTION-FAILED stderr checks removed
The test suite covers all four correctness axes: function-body "use strict" in .cjs, in .js with "type":"commonjs", module-level re-emission via the CJS wrapper, non-strict directive interplay, and block-scope DCE. No concerns remain.
|
@robobun resume |
dc1d48e to
4003f1f
Compare
|
Build #51265 (
(Build #51112 was a CI outage — 30 expired + 252 waiting_failed, 0 actual failures; |
4003f1f to
4ce7edd
Compare
parseStmtsUpTo unconditionally skipped "use strict" during directive-prologue
parsing. That was safe at module scope — the CJS wrapper in P.zig re-emits
the directive at the top of the wrapper IIFE — but at function scope there
was no later pass that put it back, so the function body silently ran sloppy.
Visible fallout: bluebird's ES5 feature detector
var isES5 = (function(){ "use strict"; return this === undefined; })();
returned false, and promisifyAll(require("crypto")) then read
X509Certificate.prototype.ca with the wrong `this`:
TypeError: Can only call X509Certificate.ca on instances of
X509Certificate [ERR_INVALID_THIS]
Changes:
- parse.zig: only skip at module scope; at function scope fall through to
the existing generic directive-preservation branch so the printer
re-emits it. Also gate the S.Directive promotion itself to module/
function-body scopes per ES2015 §14.1.1 — block-scope strings stay
S.SExpr so DCE can drop them.
- visit.zig: stop dropping S.Directive nodes in joinWithComma
(minify_syntax + DCE). Directives carry semantics; the parse-side
scope gate above keeps block-scope strings out of this path.
- P.zig: the CJS-wrapper "don't double-emit use strict" guard now
checks the directive VALUE, not just the tag — otherwise a module
that begins with e.g. "use client" would suppress re-emission and
the wrapper IIFE would run sloppy.
- bundler_npm.test.ts: the ReactSSR bundle test hard-codes source-map
positions and the exact filesize. React's `react.development.js`
has a function-body `'use strict'` that was previously dropped; now
that it is preserved the bundle grows by 52 bytes (four 13-byte
directives) and the first two mappings shift by 13 columns. Update
the snapshot.
Fixes #29533
Fixes #14251
4ce7edd to
15dd048
Compare
There was a problem hiding this comment.
All prior feedback has been addressed and the bug hunter found nothing new, but this changes directive-prologue semantics across the parser/visitor/CJS-wrapper and shifts minified output offsets, so it's worth a human sign-off.
Extended reasoning...
Overview
This PR fixes a regression where function-body "use strict" directives were dropped during CJS transpilation. It touches three coupled pieces of the JS parser: parse.zig (gates directive-prologue recognition to .entry/.function_body scopes and rewrites function-level directives to S.Directive instead of skipping them), visit.zig (removes the .s_directive => continue that dropped directives during the minify+DCE merge loop), and P.zig (narrows the CJS-wrapper "already has use strict" guard to check the directive value, not just the tag). It also updates bundler_npm.test.ts source-map column offsets and the exact-filesize assertion (+52 bytes) because preserved directives shift minified output, and adds 5 regression tests with 2 fixtures.
Security risks
None identified. This is a transpiler-output correctness fix; it does not touch auth, crypto, permissions, network, or filesystem boundaries. The only behavioural change is that some user code now runs in strict mode where it previously (incorrectly) ran sloppy — which is the intended fix and matches Node/spec semantics.
Level of scrutiny
High. The JS parser/transpiler is on the hot path for every file Bun loads or bundles. The three changes are individually small but have subtle cross-cutting interactions — several of which surfaced during review (block-scope string literals being wrongly promoted, non-strict module directives like "use client" suppressing strict-mode re-emission, the React Fast Refresh / decorator before-list reorder edge case). All raised issues except the last (acknowledged as a non-regression follow-up) were fixed in subsequent commits. The bundler_npm.test.ts offset/filesize churn is expected but is exactly the kind of thing a human reviewer should sanity-check against the actual build output.
Other factors
- All 7 of my prior inline comments and CodeRabbit's comments are resolved.
- CI on the most recent fully-tested commit was green for the new test on every lane; remaining failures were documented as pre-existing on
main. - One known incomplete-fix edge case (directive displaced by
nearest_stmt_listhoisting under React Fast Refresh / standard decorators) was deliberately scoped out as a follow-up — behaviour there is unchanged from before this PR. - No human reviewer has approved yet; alii pinged
resumebut did not review.
Given the parser-core scope and the output-size/sourcemap snapshot updates, I'm deferring rather than approving.
|
This targets the pre-Rust-rewrite |
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
2 participants
What does this PR do?
Restores correct strict-mode semantics for
"use strict"directives inside function bodies during CJS transpilation. Regressed in 1.3.11.Reproduction (from #29533)
Minimal:
Root cause
parseStmtsUpTo(src/ast/parse.zig) unconditionallyskip = true'd"use strict"during directive-prologue parsing at every scope. That was safe at module scope — the CJS wrapper inP.zigre-emits the directive at the top of the wrapper IIFE — but at function scope there is no later pass, so the function body silently ran in sloppy mode.With bluebird's
isES5detector returningfalse,util.inheritedDataKeystook its pre-ES5for..inpath, which fails to filter getter-only properties.promisifyAll(require("crypto"))then readX509Certificate.prototype.cawiththis = the prototype, tripping Node'sERR_INVALID_THISguard.Secondary bug: the
joinWithCommaoptimisation insrc/ast/visit.zig(runs underminifySyntax+ DCE) dropped everyS.Directivenode — so even if the parser kept the directive, the bundler would re-drop it.Fix
src/ast/parse.zig: only skip atp.current_scope == p.module_scope. At function scope, rewrite theS.SExprcarrying the string literal into anS.Directiveso the printer re-emits"use strict";verbatim at the top of the function body.src/ast/visit.zig: append.s_directivestatements to the output instead ofcontinueing past them.Verification
test/regression/issue/29533.test.ts(3 tests):"use strict"in.cjs→this === undefinedinside the IIFE."use strict"in CJS.js→typeof this === "string"after.call("hello"), undeclared assignment throws."use strict"in.cjs→ still enforced via the CJS wrapper (regression in the other direction).Gate:
Existing
test/bundler/transpiler/preserve-use-strict-cjs.test.tsand the"does not preserve use strict (for now)"transpiler test (which checks the module-level behaviour) still pass.Closes #29533
Fixes #14251