Repository navigation
Conversation
…ding in the file captures it The printer writes a folded NaN as the identifier `NaN`, and without minify-syntax an EUndefined as `undefined`. The bundler renamer reserves those names (#41909), but `bun run`, `bun build --no-bundle` and `Bun.Transpiler` do not rename, so a block-level `let NaN`, a parameter named `undefined`, a class named `NaN` and the like captured the printed value. `print_ast`, the transpiler entry point, now checks the file's symbols for a bound `undefined`, `NaN` or `Infinity`. Only when there is none does the printer use the names; otherwise it writes `void 0`, `0 / 0` and `1 / 0`. Inside a `with` body it always uses those forms, as esbuild does. Unshadowed non-minified output now prints an infinite literal as `Infinity` instead of `1 / 0`, like the bundler.
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 37 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: ready for review. Reproduced on 1.4.3 ( |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the default printed form of infinite literals for the transpiler path and reworks the precedence/wrapping logic in print_number, a human look at the output-format shift would still be worthwhile.
What was reviewed:
print_numberNaN/Infinity branches — wrapping atLevel::Multiplymatches the property-access/**cases in the tests;-1 / 0and-(0/0)still evaluate to the right values unwrapped.print_astsymbol scan — filtersKind::Unboundso a bare reference to globalNaNdoesn't trip it; covers non-parser producers (TOML loader, class-expression names) as claimed.with_nesting— incremented only around theSWithbody, not the head expression; reset via the struct init inPrinter::init.- Cache version bump to 30 covers the changed output per REVIEW.md's cache-key rule.
Extended reasoning...
Overview
This PR fixes a correctness bug in the JS printer where folded constants (NaN, Infinity, undefined) were emitted as bare identifiers even when a local binding with that name existed, causing the local to capture the constant. The fix renames has_run_symbol_renamer → global_value_properties_unshadowed, adds a symbol-table scan in print_ast to set it for the transpiler path, adds a with_nesting counter so with bodies always fall back to arithmetic forms, and teaches print_number to emit 0 / 0 / 1 / 0 (and print_undefined to keep void 0) when the names are unsafe. The runtime transpiler cache version is bumped to 30. Five files changed: two bundler linker sites (mechanical rename), the printer, the cache version, and transpiler.test.js.
Security risks
None. This is a code-generation correctness fix with no auth, crypto, network, or filesystem surface. The only input is the AST/symbol table already produced by the parser; no new untrusted-input parsing is introduced.
Level of scrutiny
Medium-high. The JS printer is on the hot path for every file Bun runs or builds, and the change alters default output for the non-bundled transpiler: infinite literals that previously printed as 1 / 0 now print as Infinity when no shadowing binding exists (the existing "numeric property keys that overflow to Infinity" test block was updated to reflect this). That is a user-visible output-format change beyond the shadowed-name bugfix itself, and the precedence/wrapping rework in the Infinity branch (by_name boolean replacing the old has_run_symbol_renamer || minify_syntax condition) deserves a maintainer's eye even though it tests out correctly. The new per-file symbols_for_source.iter().flatten().any(...) scan is O(symbols) on every print_ast call; the PR description quantifies it (~4 ns/symbol) but a reviewer familiar with the printer's perf budget should confirm that's acceptable.
Other factors
The test coverage is unusually thorough: let/class/switch/label/param/class-expression-name/with binding forms, TOML data-loader synthesized bindings, the module-level export const NaN = 0 / 0 TDZ case, parenthesization at ** and property-access precedence, and end-to-end bun run / bun build --no-bundle spawns evaluated via new Function to bypass a second transpile. Tests follow harness conventions (bunExe/bunEnv/tempDir, await using, Promise.all on pipes, stdout/stderr asserted before exitCode, test.concurrent for the subprocess cases). The cache-version bump satisfies REVIEW.md's "any change to cached/serialized output bumps the format version constant" rule. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. No CODEOWNERS cover the changed paths. I checked the print_space_before_identifier() call that now precedes 0 / 0 / (0 / 0) — it's conservative (may add an unneeded space) but never produces wrong adjacency, and the .x / ** follow-on cases are wrapped at Level::Multiply so no 0.x ambiguity arises.
Problem
bun runandbun build --no-bundleprint a folded NaN (0 / 0,+"abc", a macro result) as the identifierNaN, so a localNaNcaptures it:for (let NaN = 0; NaN < 1; NaN++) console.log(0 / 0)prints0, andexport const NaN = 0 / 0is a TDZ error (the NaN twin of ReferenceError: Cannot access uninitialized variable. #7263).--no-bundlealso printsvoid 0asundefinednext to a localundefined.print_number/print_undefined(src/js_printer/lib.rs): they name the global with no scope check. renamer: reserve NaN, Infinity and undefined so inlined constants reach the globals #41909 reserved the names in the bundler renamer, but these paths do not rename.Fix
print_ast, the transpiler entry point, looks for a bound symbol namedundefined,NaNorInfinityin the file and sets the printer option from that. The bundler still sets it from its renamer.withbody (esbuild'swithNesting), the printer writes0 / 0,1 / 0andvoid 0, in parentheses where precedence needs them.Infinity, like the bundler. The runtime transpiler cache version moves to 30.test/bundler/transpiler/transpiler.test.js(new block; 7 tests fail on 1.4.3), plus the suites listed in the notes.Background
+"abc",void 0,1e999, an unboundNaNidentifier, and underbun runalso0 / 0, into constant nodes with no symbol. The printer must spell the value.bun runprints withminify_syntax, which already writesvoid 0and1 / 0; onlyNaNwas exposed there.Symbolin the file's table: declarations from the parser, or exports that data loaders (TOML keys) synthesize. Unresolved references areKind::Unbound.Notes
0 / 0andvoid 0where a local binding orwithobject shadowsNaN/undefined#36124 (July 27). Both conflict with main since renamer: reserve NaN, Infinity and undefined so inlined constants reach the globals #41909 landed the renamer half of js_printer: guard undefined/NaN/Infinity identifiers against local shadows #36122. js_printer: print0 / 0andvoid 0where a local binding orwithobject shadowsNaN/undefined#36124 used the same symbol-table check; this version adds thewithhandling, the property-key case and tests for each door.print_astand not a parser flag: the parser is not the only producer of bindings on this path.Bun.Transpiler().transformSync(src, "toml")(and JSON, YAML, JSON5) turns top-level keys intovarbindings intranspiler.rswithout the parser, so a TOML file with keysNaN/Infinityand anan/infvalue printedvar NaN = "n", x = NaN. A class expression's own name also bypassesdeclare_symbol. The scan covers every producer. Cost: about 4 ns per symbol (0.2 ms for 50k symbols), once per file.{ class NaN {} String(0 / 0) }gave the class source,switch (1) { case 1: let NaN = "local"; console.log(0 / 0) }gavelocal, and the macro faceconst g = (NaN = nan()) => NaNprinted(NaN = NaN) => NaN(a TDZ error underbun run); it now prints(NaN = 0 / 0) => NaN. A function-levelconst NaNonly looked handled because DCE dropped the unused binding; a keptlet NaNcaptured there too.0 / 0and1 / 0are wrapped atLevel::Multiplyand above (x / (0 / 0),(0 / 0).toFixed(),2 ** (0 / 0)),void 0atLevel::Prefixand above, as before.0 / 0/1 / 0prints computed (number_property_key_must_be_computed).minify_identifiersthe transpiler path runsMinifyRenamer, which reserves these names since renamer: reserve NaN, Infinity and undefined so inlined constants reach the globals #41909, but a named exportexport let NaNismust_not_be_renamed, so the scan is used there too.withbody in the source stay identifiers (the parser does not fold them there), sowith_nestingonly affects synthesized constants.eval("var NaN = 1")introducing the binding at runtime.new Function(no Bun transpile) in both Bun and Node:f64[0] = 0 / 0and-(0 / 0)store7ff8000000000000, the same as theNaNglobal; only a runtimez / zgives the x86 defaultfff8…. On aarch64 the hardware default NaN is7ff8…already. SoFloat64Array/Buffer.writeDoubleBEoutput does not change.transpiler.test.js"numeric property keys that overflow to Infinity": the whitespace-only minifier now prints{Infinity:1}(valid), so that block usesminify: { whitespace, syntax }to keep covering the computed[1/0]form, and gains alet Infinity;variant for the non-minified fallback.new Function, which hands it to JavaScriptCore without a second pass through Bun's transpiler, so the--no-bundledoor is checked on its own.export const NaNcase; both are addressed above.test/bundler/transpiler/transpiler.test.js,test/bundler/bundler_edgecase.test.ts,test/bundler/bundler_minify.test.ts,test/bundler/bundler_loader.test.ts,test/bundler/esbuild/{default,dce,lower}.test.ts,test/js/bun/transpiler/,test/js/bun/toml/,test/bundler/transpiler/{decorators,ts-use-define-for-class-fields}.test.ts.