Conversation
…c of, and print (let)[0] at a statement start
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe parser updates import attributes and TypeScript ChangesSyntax parsing and printing
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The syntax fixes have no identified merge-blocking regression, so the PR is ready for normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 11:25 PM PT - Sep 14th, 2026
❌ @robobun, your commit 7298519 has some failures in 🧪 To try this PR locally: bunx bun-pr 41676That installs a local version of the PR into your bun-41676 --bun |
|
CI on 7ea4e15 (build 111721): every lane passed except |
![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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js_printer/lib.rs— A pure-annotated call in a for-loop head still prints aslet[…], sofor (/* @ __PURE__ */ (let)[0]();;)round-trips to invalid output.ECallre-anchors onlystmt_startafterprint_pure()(lib.rs:3520-3524) and never re-anchors the newfor_init_start, so the innerEIndexat lib.rs:3783 no longer sees the head start and skips the(let)wrap. Fix: mirror thestmt_startsave/restore forfor_init_startaroundprint_pure()so every start marker thewrap_letcheck reads survives a leading DCE annotation. Pre-existing on the base too, but this PR adds the marker without the matching preservation.Extended reasoning...
Input in sloppy-mode JS with
print_dce_annotationsenabled:for (/* @ __PURE__ */ (let)[0]();;) ;.print_for_loop_initsetsfor_init_start = Nat lib.rs:6562 and callsprint_expron theECall. InECallat lib.rs:3509has_pure_commentis true; at lib.rs:3520was_stmt_startis false becausestmt_startwas set for the enclosingforstatement, not the head.print_pure()writes the annotation,written()advances past N, and neitherstmt_startnorfor_init_startis updated. The call targetEIndex(let, 0)is then printed; at lib.rs:3782n = written()equals neitherstmt_startnorfor_init_start, sowrap_letis false. Output:for (/* @ __PURE__ */ let[0]();; ). On re-parse the comment is skipped and the head begins with the token pairlet [, which theforgrammar takes as a lexical declaration;let [0]is then rejected as an invalid binding pattern. The fix already exists forstmt_startat lib.rs:3520-3524; the same two lines are needed forfor_init_start.Verification: pre-existing — the mechanism is exactly as described.
print_for_loop_initsetsself.for_init_start = self.writer.written()(lib.rs:6562) before recursing into theECall. In theECallarm, whenhas_pure_commentis true, onlystmt_startis captured/restored aroundprint_pure()(lib.rs:3520-3524);for_init_startis not re-anchored. After the annotation bytes are emitted,… | nit — the…
|
Took the optional suggestion in 7298519. |
|
CI on 7298519 (build 115821): 180 of 181 jobs passed. The one failed job is the darwin aarch64 test job, which hit the job time limit. No test is red. Every test failure in the build passed on retry ( |
This branch has not been deployed
2 participants
Problem
import a from "./m.mjs"followed bywith { type: "json" }on the next line fails withExpected "(" but found "{".for await (async of xs)fails withExpected "=>" but found "[". Sloppy code(let)[0] = 1parses, but the printer emitslet[0] = 1, aletdeclaration, and JSC rejects the output withSyntaxError: Unexpected number '0'.(let)+ line break +[a] = bprints aslet[a] = b, which still parses, as a destructuring declaration. Since js_printer: start the output position at 0 so position sentinels cannot match the empty output #40814 this also happens at the very start of a file.parse_path(src/js_parser/parse/mod.rs:1345) requires no newline beforewith, but only the legacyassertform has that restriction.parse_async_prefix_expr(mod.rs:1625) commits to an async arrow for everyasync <identifier>in JS mode. The printer wrapsletonly at a for-of head (src/js_printer/lib.rs:4258) and not at the other positions with alet [lookahead restriction.Fix
parse_pathapplies toassertonly.withon the next line starts the attributes clause, as in esbuild.async of, JS mode now does the=>lookahead that TypeScript mode already does.for await (async of xs)is a for-of over the identifierasync.for (async of xs)now reports the existing errorFor loop initializers cannot start with "async of".async of => {}is still an arrow.for_init_startmark next tostmt_start. AnEIndexwhose target is the identifierletprints as(let)[...]at a statement start or at the start of a for, for-in, or for-of head. esbuild 0.28 still has this printer bug, so the precedent is the printer's own for-of rule.test/bundler/transpiler/transpiler.test.js(three new blocks, all fail on 1.4.3 and on main 627e407) and one new case intest/bundler/bundler_edgecase.test.ts(a/* @__PURE__ */comment before(let)[0]()in a for head). Alsotest/bundler/esbuild/{default,ts,lower}.test.ts,test/bundler/bundler_edgecase.test.ts,test/js/bun/transpiler/.Background
[no LineTerminator here]beforeassertin the legacy import assertions grammar. The import attributes grammar (with) has no such restriction.[lookahead ∉ { let, async of }]before the left-hand side.for awaithas noasync ofrestriction becauseasynccannot start an arrow there.[lookahead ≠ let []. So a transpiler must keep the parentheses in(let)[0]at those positions. The printer re-derives parentheses from position, which is why the parser does not keep them.parse_path) also appears in Carry import attributes through the parser, printer, bundler and runtime; add the bytes loader #40836. The hunks are identical, so whichever lands second rebases cleanly.Notes
Reported by the parser conformance ledger (entries 24993, 24992, 24994).
Self-reviewed: 4 concerns raised, 2 addressed (the overlap with #40836 and the esbuild precedent claim are stated above). Two rejected: "split into three PRs" (the changes are three lines of parser and one printer mark, one test file), and "measure the printer hot path" (the check is one integer compare on
written()before any symbol lookup, and the name lookup only runs at a statement or for-head start).Repros on bun 1.4.3:
A newline before
assertstill ends the import statement.import json from "./foo.json"followed byassert { type: "json" }on the next line is thenassertfollowed by{on the same line, which is a syntax error in every engine. The test asserts that it still fails.[human-review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file