Repository navigation
Conversation
…ition Macro arguments that reference a const whose value is statically known (a literal, or the result of another macro) are documented to work, but two separate limitations made the docs' own worked example fail: - const_values was only populated for a scope's uninterrupted leading declaration run. Any non-declaration statement before the const (e.g. a console.log) flipped is_after_const_local_prefix and later consts were never recorded, so identity(N) after such a statement hit 'Cannot convert identifier to JS'. When a file imports macros, now record const literal initialisers regardless of prefix position; visiting is top-down so entries are only visible to later uses. - handle_identifier only consulted const_values when features.inlining was set. Macro args are visited with should_fold_typescript_constant_expressions set, so consult the map under that flag too. This also makes the bundler (where inlining is gated on minify_syntax) fold const identifiers passed to macros. - data_to_js had no ETemplate arm, so a template literal that didn't fully collapse during visiting (e.g. because a substitution was a UTF-16 EString, which every macro-produced string is) hit 'Cannot convert argument type to JS'. Convert untagged templates by flattening head, part values and tails to a single JS string.
|
Updated 6:58 PM PT - Jul 13th, 2026
❌ @robobun, your commit 5e56d36 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34089That installs a local version of the PR into your bun-34089 --bun |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
Verified the four suggested issues against this branch:
Added |
The previous commit consulted const_values under
should_fold_typescript_constant_expressions, which is also set while
visiting import()/require()/require.resolve() arguments. That collapsed
intentionally-dynamic specifiers like `./a/${x}.js` into static paths
and broke bundler_allow_unresolved.test.ts.
Use a dedicated is_inside_macro_arguments flag instead, set only while
visiting the arguments of a macro call, so dynamic import/require
specifiers are left alone.
![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/p.rs:1816— The pre-existing(Self::ALLOW_MACROS && could_be_macro)branch inserts intoconst_valuesforlet/vartoo, and this PR's newshould_fold_typescript_constant_expressionsgate inhandle_identifiernow reads those entries — solet x = getFive(); x = 10; identity(x)(where both are macros) silently compiles toidentity(5)instead of failing with "Cannot convert identifier to JS". Gate thecould_be_macroinsert onwas_const(already threaded through) so reassignable bindings never enter the map. The PR's "let bindings are still rejected" test uses a literal initializer, socould_be_macrois false and it doesn't cover this shape.Extended reasoning...
What the bug is
visit_declinserts a binding intop.const_valuesunder three disjuncts:if could_be_const_value || (Self::ALLOW_MACROS && could_be_macro) || track_for_macro_args { if let Some(val) = decl.value { if val.can_be_const_value() { self.const_values.put(id_ref, val).expect("oom"); } } }
The first disjunct is gated on
was_const(viacould_be_const_value = was_const && !is_after_const_local_prefix), and the third (track_for_macro_args, added by this PR) is also gated onwas_const. But the middle one —Self::ALLOW_MACROS && could_be_macro— is not.could_be_macrois computed invisit_declsasprev_macro_call_count != self.macro_call_count, i.e. "the initializer contained a macro call", and is passed regardless of the declaration kind. Solet x = getFive()(macro) putsx → 5intoconst_valuesexactly likeconst x = getFive()would.Before this PR that was harmless in the plain
bun run/bun build(no--minify) path:handle_identifieronly consultedconst_valueswhenself.options.features.inliningwas on. This PR changes that check toself.options.features.inlining || self.should_fold_typescript_constant_expressions, andvisit_expr.rs'se_callarm setsshould_fold_typescript_constant_expressions = truefor the duration of visiting a macro call's arguments. The reassignableletentry that was previously inert is now read.Step-by-step proof
// m.ts export function getFive() { return 5; } export function identity(x) { return x; } // entry.ts import { getFive, identity } from "./m.ts" with { type: "macro" }; let x = getFive(); x = 10; console.log(identity(x));
visit_stmtforlet x = getFive()computeswas_const = (kind == KConst) = falseand callsvisit_decls.visit_declsrecordsprev_macro_call_count, visits the initializer (macro expands toENumber(5),macro_call_countincrements), then callsvisit_decl(..., was_const=false, is_after=..., could_be_macro=true).- In
visit_decl:could_be_const_value = false && ... = false;track_for_macro_args = ... && was_const && ... = false; but(Self::ALLOW_MACROS && could_be_macro) = true.ENumberpassescan_be_const_value(), soconst_values[x] = 5. x = 10;is visited outside any macro-arg context; the flag is false and inlining is off, so the assignment is emitted normally (nothing removes or updates theconst_valuesentry).identity(x)is a macro ref, soe_callsetsshould_fold_typescript_constant_expressions = trueand visits the args.handle_identifierforxnow takes the new branch (p.rs:1816), findsx → 5inconst_values, and returnsENumber(5).- The macro receives
5, and the file compiles toconsole.log(5)— printing5instead of erroring. The reassignment to10is silently ignored.
Before this PR the same code failed at build time with
error: cannot coerce EIdentifier to a JS runtime value("Cannot convert identifier to JS") — the correct behavior, sincexis reassignable and not statically known. The PR turns that build-time error into silently wrong output.Why the PR's own negative test doesn't catch it
The "let bindings are still rejected" test uses
let N = 5;— a literal initializer.could_be_macrois therefore false (no macro call in the initializer), andtrack_for_macro_argsis gated onwas_const, soNnever entersconst_valuesand the test correctly errors. The test needs a variant where theletinitializer is itself a macro call.How to fix
Since this PR already threads
was_constintovisit_decl, the one-line fix is to gate the middle disjunct on it too:if could_be_const_value || (Self::ALLOW_MACROS && was_const && could_be_macro) || track_for_macro_args
(and mirror the same
was_constin theis_after_const_local_prefixupdate just below, so alet x = macroCall()still ends the leading-const prefix). Alternatively,handle_identifiercould check the symbol's kind before substituting, but gating the insert is more direct and matches the intent —const_valuesshould only ever contain non-reassignable bindings. -
🟡
src/js_parser/p.rs:1816— The PR description saysshould_fold_typescript_constant_expressionsis only raised for macro/require/enum-init args, but it's actually set file-wide underminify_syntax || inlining(parse_entry.rs:568/757), and a pre-existing typo atvisit_expr.rs:1818(let prev_… = true;instead of saving the actual value) leaves it stuck attrueafter any dynamicimport(). Combined withtrack_for_macro_argspopulatingconst_valuesfor everyconstin a macro-importing file, this now inlines const literals at identifier sites well outside macro arguments. Output is still semantically correct (only immutable primitives passcan_be_const_value()), so not blocking — but since this PR now leans on that flag's save/restore, the one-line fix atvisit_expr.rs:1818(= p.should_fold_typescript_constant_expressions) is worth including here.Extended reasoning...
What
The new guard in
handle_identifier:if self.options.features.inlining || self.should_fold_typescript_constant_expressions { if let Some(replacement) = self.const_values.get(&ref_) { … self.ignore_usage(ref_); … } }
is described in the PR as firing only inside "macro /
require/require.resolveargs, enum member inits" — the positions wheree_calltemporarily raisesshould_fold_typescript_constant_expressions. In practice the flag is true in two additional situations:-
File-wide under
minify_syntax.parse_entry.rs:568-569and757-758setp.should_fold_typescript_constant_expressions = truefor the whole file wheneverminify_syntax || inlining.Bun.Transpiler({ minify: { syntax: true } })setsfeatures.minify_syntax = truewhilefeatures.inliningstaysfalse(it comes from the separateinlineoption,transpiler.rs:1621). So after this PR, that config starts substituting leading-prefixconstliterals at every identifier site — previously this was gated solely onfeatures.inlining. -
After any dynamic
import(), permanently.visit_expr.rs:1818reads:let prev_should_fold_typescript_constant_expressions = true; p.should_fold_typescript_constant_expressions = true;
The "restore" at 1846-1847 / 1851-1852 therefore always writes
trueback, regardless of the prior value. Compare withe_callat ~1958, which correctly saves the actual field. This is a pre-existing bug, but before this PR the leaked flag only affected numeric/template constant folding; now it also drives const-identifier substitution.
Step-by-step (path 2, plain
bun run, no minify)import { f } from "./m.ts" with { type: "macro" }; const p = import("./lazy.js"); const N = 5; console.log(N);
minify_syntax = false,inlining = false⇒ flag startsfalse(parse_entry.rs:568).- File has a macro import ⇒
track_for_macro_argsistruefor everyconst, soconst_values[N] = 5is recorded even thoughNis past the leading prefix. - Visiting
import("./lazy.js")enterse_import, which sets the flag totrueand "restores" it to the hardcodedprev = true. - Visiting
console.log(N)reacheshandle_identifierwith the flag nowtrue⇒Nis replaced by5andignore_usage(N)is called — outside any macro-argument position, in a mode where the user asked for neither minification nor inlining.
Why this doesn't break anything
can_be_const_value()only admits primitives (number/string/boolean/null/undefined/bigint), inlined enums, and macro-frozen arrays/objects — all documented as "no observable difference if duplicated". The declaration-removal DCE loop only runs underminify_syntaxand only in nested scopes, so no exported/module-level decl is dropped in the no-minify scenario. The emitted JS is semantically equivalent; this is a scope-drift / output-shape change, not a correctness bug. For path (1) specifically, the file-wide enablement underminify_syntaxis deliberate per the comment inparse_entry.rs, and it alignsBun.Transpiler({minify:{syntax:true}})withbun build --minify-syntax(which already tiesinlining = minify_syntaxinParseTask.rs).Addressing the counter-argument
One could argue this is purely "PR description understates scope" and not worth a comment. For path (1) I'd agree — the extra inlining under
minify.syntaxis arguably desirable and matches the bundler. But path (2) is a genuine pre-existing save/restore bug (prev = trueis clearly a typo forprev = p.should_fold_typescript_constant_expressions), and this PR increases what that leak affects from "a bit of extra constant folding" to "every trackedconstin a macro-importing file gets inlined after the first dynamic import". Since the new gating inhandle_identifiernow depends on that flag's save/restore being correct, fixing it in the same PR is cheap and directly relevant.Suggested fix
At
src/js_parser/visit/visit_expr.rs:1818:let prev_should_fold_typescript_constant_expressions = p.should_fold_typescript_constant_expressions;
Optionally, if you want the
handle_identifiersubstitution to be strictly limited to macro/require/enum-arg positions as described, guard on a dedicated flag that onlye_callraises rather than the file-wideshould_fold_typescript_constant_expressions— but given the output is correct either way, that's your call. -
The could_be_macro disjunct in visit_decl inserted into const_values regardless of the declaration kind, so 'let x = macroCall()' recorded a value that handle_identifier would then substitute in macro arguments (and, with inlining enabled, on the left-hand side of a later assignment). Gate both the identifier and destructuring paths on was_const so reassignable bindings never enter the map. Also drop the unused getText export from the fixture; the subprocess tests carry their own m.ts.
|
Addressed review findings in ac17fa0:
|
track_for_macro_args records past-prefix consts in const_values for files with macro imports; under --minify-syntax handle_identifier then substitutes every use, dropping use_count_estimate to 0. The 'remove inlined constants' pass only walks the leading declaration prefix and the single-use inliner required use_count_estimate == 1, so the now-dead declaration was left in minified output. Teach the single-use inliner to also drop the trailing decl when the ref is in const_values and the use count is 0; presence in const_values already implies the binding was const with a side-effect-free initialiser.
|
CI status: the diff's own tests (
This is ready for review. |
There was a problem hiding this comment.
No new issues found — both earlier findings (dead getText fixture, the --minify past-prefix DCE regression) are addressed. Deferring to a human because this is a semantic change to the parser's const-folding / DCE passes and adds a new AST→JS conversion path.
Checked this run:
is_inside_macro_argumentssave/restore is balanced and scoped only tois_macro_refcalls;import()/require()specifiers are unaffected (regression test covers it).is_after_const_local_prefixmaintenance is preserved for the DCE pass —track_for_macro_argsdoes not suppress the flag; the only behaviour delta is thatlet x = macroCall()now ends the prefix (previously didn't), which is a correction.- The new zero-use drop in the single-use inliner is safe: entries reach
const_valuesonly viawas_const+can_be_const_value(), so the initialiser is side-effect-free and theKVar/is_using()guards above still apply. template_to_js: tagged templates and raw-cooked parts still error; recursion is stack-checked.
Extended reasoning...
Overview
Three coordinated changes across the parser and JSC bridge: (1) a new is_inside_macro_arguments flag on P so handle_identifier substitutes from const_values inside macro args without --minify; (2) visit_decl now records const literals into const_values past the leading-declaration prefix whenever the file has macro imports, plus a matching zero-use drop in the single-use inliner so those entries don't leave dead decls under --minify; (3) an ETemplate arm in data_to_js that flattens untagged templates with statically-known parts into a UTF-16 JS string. ~180 lines of new tests cover run/build/build-minify and negative let cases.
Security risks
None. No untrusted-input parsing surface is widened — template_to_js only handles AST nodes the parser already produced, and rejects anything it can't stringify with the existing ToJSError variants. No new FFI, no allocation-size arithmetic on external data.
Level of scrutiny
High. src/js_parser/visit/mod.rs is on the hot path for every file Bun parses, and the change touches two interacting optimisation passes (const-value tracking and the single-use inliner) plus the is_after_const_local_prefix state machine that esbuild-derived DCE depends on. The visit_decl signature change and the split of the else-branch into an independent if !could_be_const_value && !track_macro_result subtly changes when the prefix flag is set for non-const macro-initialised bindings; I traced it and believe it's correct (and arguably a fix), but this is exactly the kind of coupling a maintainer should eyeball.
Other factors
- Two prior review rounds from me were addressed (dead fixture export removed in ac17fa0; DCE regression fixed in 09d556e with a
--minify-syntaxtest). - CI is green on the diff's own tests across all lanes; the one red (
test-worker-message-port-transfer-terminate.json x64-asan) is pre-existing on main. - I looked for TDZ hazards from populating
const_valuespast the prefix: visiting is textual-order and function bodies are visited at their declaration site, so an entry is only visible to later-visited code. A hoisted function called before theconstin source would already be a runtime TDZ error, so macro-time substitution there doesn't introduce a new wrong-answer case. - The number-in-template path uses
FormatDouble::dtoa; I did not verify it matches JSToStringfor every edge (e.g.-0), but the only test exercising it is an integer.
|
Correction to the earlier #6865 assessment: that issue is specifically about Merged this branch onto main 028f7a3 and verified Adding |
### Problem - `visit_decl` in `src/js_parser/visit/mod.rs` took three adjacent `bool` parameters (`was_anonymous_named_expr`, `could_be_const_value`, `could_be_macro`), and the second of its two callers passed them positionally: `visit_decl(decl, false, was_const && !is_after, false)`. Nothing at that call says which flag is which. - This is the one `bare_bool_args` finding recorded for this file in `mordant-baseline.toml`. - The shape has already bitten once: the Zig version of this call site passed `was_const and !is_after` in the `was_anonymous_named_expr` slot (`src/js_parser/visit/visit.zig` before d451445); the Rust port put it in the right slot. Two open PRs (#34089, #35548) each add a fourth positional bool to the same call. ### Fix - Adds `VisitDeclOpts` to `src/js_parser/parser.rs`, next to `VisitArgsOpts`, with one documented field per former parameter. Same shape as the existing `VisitArgsOpts` / `ParenExprOpts` option structs; an options struct rather than three enums because `was_anonymous_named_expr` is handed straight on to `maybe_keep_expr_symbol_name` as a `bool`, and `dylint.toml` already treats several-bool structs as this repo's option-bag style. - `visit_decl` now takes `opts: VisitDeclOpts` and destructures it on entry; the rest of its body is unchanged. Both callers build the struct with every field named, using the same expressions as before, in the same order. - Removes the `"bare_bool_args:src/js_parser/visit/mod.rs" = 1` line from `mordant-baseline.toml`. Running the baseline writer scoped to `bun_js_parser` (`MORDANT_BASELINE_WRITE=1 cargo dylint --all -p bun_js_parser`) rewrites the file byte for byte identical to what is committed. - No behavior change is intended. Verified with a debug build: - `bun bd test test/bundler/transpiler/`: 3870 pass, 0 fail. The only red was `jsx-production.test.ts`, whose 32 concurrent parent+child debug-build spawns exceed the 5s per-test timeout on an 8 core box; all 32 pass with `--timeout 120000`. - `bun bd test test/bundler/bundler_minify.test.ts test/bundler/esbuild/dce.test.ts test/regression/issue/26360.test.ts test/regression/issue/22656.test.ts` (keepNames, const inlining, macros): 128 pass, 0 fail. - `bun bd test test/bundler/bundler_edgecase.test.ts`: 134 pass, 0 fail. - `cargo dylint --all -p bun_js_parser -- --keep-going` with the baseline line removed: on the previous code it reports the finding quoted below as over the baseline; on this branch it reports nothing and `target/mordant/over-baseline.txt` is not written. - No test is added: a signature refactor has no observable behavior to pin, so there is nothing a new test could fail on before this change. The existing suites above are the coverage; the lint run is what distinguishes before from after. ### Background - `visit_decls` visits every `decl` of a `let`/`const`/`var` statement. For each one it visits the initializer and then calls `visit_decl`, which does the bookkeeping that depends on facts only the caller observed: whether the initializer was an anonymous function or class before visiting (so the binding's name can be attached to it via `maybe_keep_expr_symbol_name`), whether this is a `const` still inside the scope's leading run of `const`s (so the value may be recorded in `const_values` for inlining), and whether visiting the initializer ran a macro (so the macro result may be recorded or destructured into `const_values`). The three fields of `VisitDeclOpts` are those three facts. - The second caller runs on the `exports.replace` / `exports.eliminate` path of `Bun.Transpiler` for a declaration with no initializer. Its replace/delete branches are inverted today (`eliminate` on `export let x;` hits an `unwrap` on `None`); that is pre-existing and already addressed by #33378, so it is left alone here. - `mordant-baseline.toml` is a ratchet: it records the per (lint, file) counts of findings that predate the lint job, and the job fails only on findings above those counts. Fixing a finding means deleting its line so it cannot come back. <details> <summary>Lint output on the previous code with the baseline line removed</summary> ``` warning: `visit_decl` takes bools `was_anonymous_named_expr`, `could_be_const_value` and `could_be_macro`, and 1 of its 2 calls passes bare `true`/`false` for at least two of them. At `visit_decl(.., false, .., false)` nothing says which is which --> src/js_parser/visit/mod.rs:517:19 | 517 | pub(crate) fn visit_decl( | ^^^^^^^^^^ | note: one of those calls --> src/js_parser/visit/mod.rs:417:29 | 417 | ... self.visit_decl(decl, false, was_const && !is_after, false); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = help: give `was_anonymous_named_expr`, `could_be_const_value` and `could_be_macro` a two-variant enum each, or an options struct, so every call names what it sets = note: `bare_bool_args` over the mordant baseline (0 recorded for src/js_parser/visit/mod.rs) warning: mordant: 1 finding(s) over the baseline in bun_js_parser ``` On this branch the same command finishes with no warnings. </details>
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-13, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#16969) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Problem
The docs at
docs/bundler/macros.md(Arguments section) promise that a macro argument may reference aconstwhose value is statically known, including the result of another macro, and show this as the worked example:That example has always failed with
"Cannot convert argument type to JS" error in macro. Two related shapes expose the same class of bug:Whether a build succeeds depends on where an unrelated statement sits relative to a
const, and template-literal arguments never accept a macro-derived substitution.Cause
Three separate gaps:
const_values(the maphandle_identifieruses to substitute known values) was only populated for a scope's uninterrupted leading declaration run.visit_stmts_switch_onesetsis_after_const_local_prefix = truefor every statement and only restores it for inert/declaration statements, so a singleconsole.log("x")before theconstmeant the identical macro call saw a bare identifier.handle_identifieronly consultedconst_valueswhenfeatures.inliningwas enabled. Macro arguments are visited withshould_fold_typescript_constant_expressions = true, but that flag was not checked, sobun buildwithout--minifynever substituted const identifiers into macro args at all (shapes A and C also fail there today).data_to_jshad noETemplatearm.Run::coerceencodes every macro-returned string as a UTF-16EString, andTemplate::foldonly merges UTF-8 parts, so a template containing a macro-derived (or non-ASCII) substitution stays anETemplateand was rejected asCannot convert argument type to JS.Fix
visit_decl: when the file has macro refs, recordconstinitialisers that passcan_be_const_value()even past the leading-declaration prefix. Visiting is top-down, so an entry is only visible to textually-later uses;is_after_const_local_prefixis still maintained exactly as before for the DCE pass and for files without macros.handle_identifier: also substitute fromconst_valueswhenshould_fold_typescript_constant_expressionsis set (macro /require/require.resolveargs, enum member inits).expr_jsc::data_to_js: handle untaggedETemplateby flattening head, part values (string / number / null / undefined / boolean / inlined enum / nested template) and tails into a single JS string. Tagged templates and unknown part types keep the existing errors.Tests
Added to
test/bundler/transpiler/macro-test.test.ts: each of the four shapes plus the in-function variant, a non-ASCII template substitution, a numeric template substitution, the combined post-statement + macro-result + template case, aletnegative case, andbun build(no--minify) variants. Also un-commented the two template-string tests that were disabled pending this.Fixes #16969
[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file