Support tagged template literals in macros#30545
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds end-to-end support for tagged template literals as macro call syntax. Previously, using a template literal to invoke a macro would panic. The changes expose the lexer's escape-decoding method, implement template-to-JS conversion helpers in the macro runner, update the template visitor to properly handle macro invocation with parts pre-visiting and constant folding, and validate the feature with comprehensive tests. ChangesTagged Template Literal Macros
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related: #21624 takes a similar approach but targets the old
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/js_parser_jsc/Macro.zig`:
- Around line 557-586: The template arrays are left mutable and `.raw` is
defined enumerable; update the end of buildTemplateObject to define the `.raw`
property on cooked_array as non-enumerable (use the JS
defineProperty/put-with-attributes API rather than a plain put) and then freeze
both cooked_array and raw_array using the JS engine's object freeze API so they
become immutable like real tagged-template objects; refer to cooked_array,
raw_array, cooked_array.put(... jsc.ZigString.static("raw"), raw_array) and the
createEmptyArray/protect/unprotect sequence to locate where to add the
defineProperty-with-non-enumerable-attribute and the freeze calls.
In `@src/js_parser/ast/visitExpr.zig`:
- Around line 411-415: The `${...}` substitution visit is placed after
early-return checks so branches like is_control_flow_dead, no_macros, and
node_modules skip visiting substitutions; move the loop that iterates over
e_.parts and assigns part.value = p.visitExpr(part.value) earlier in visitExpr
(or ensure it's executed before each early return path) so substitutions are
always visited even when the macro is rejected (similar to how .e_call handles
folding and symbol reporting).
In `@test/regression/issue/18047.test.ts`:
- Line 32: The unconditional expect(stderr).toBe("") assertions are flaky under
ASAN/debug builds because benign warnings may be printed to stderr; remove or
relax those assertions in test/regression/issue/18047.test.ts (the two
occurrences around the expect(stderr).toBe("") statements) and rely on the
subprocess exit code assertion (e.g., expect(code).toBe(0)) as the primary
success signal, or replace the strict empty check with a more permissive pattern
match if you must validate stderr contents.
🪄 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: 99cabb17-0332-4527-890e-3ee36adb2b79
📒 Files selected for processing (6)
src/js_parser/ast/visitExpr.zigsrc/js_parser/lexer.zigsrc/js_parser_jsc/Macro.zigtest/bundler/transpiler/macro-test.test.tstest/bundler/transpiler/macro.tstest/regression/issue/18047.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
StatusRebased onto main (714e3de) and ported the implementation to the new Rust Remaining CI red on build #54261 is unrelated flake, none of it touches the parser/macro/lexer code in this diff:
All review threads (CodeRabbit + claude[bot]) resolved. Ready for maintainer review/merge. |
Calling a macro as a tagged template (``ico`hello```) previously hit @Panic("TODO: support template literals in macros") in Macro.Runner.run. The e_template caller case now builds a TemplateStringsArray-shaped first argument (cooked segments, with a .raw property) and passes the substitution values as the remaining args, matching tagged-template call semantics. Raw segments are cooked via the lexer's escape decoder; segments with undecodable escapes become undefined per spec. visitExpr now visits substitution values before the macro call so literals are resolved, and reports macro errors the same way the e_call path does. Fixes #18047
Mirrors the .e_call path so `"a" + "b"` folds to a literal before toJS. Also: mark the two regression subprocess tests concurrent.
bun.String.createUninitialized asserts len > 0; return String.empty when decodeEscapeSequences produces zero code units (e.g. a segment that is only a backslash-newline). Also drop the dead e_.fold() call on the macro fall-through path since E.Template.fold is a no-op when tag != null.
The macro runner and e_template visitor moved to Rust (Macro.rs,
visit_expr.rs); the Rust e_template path emitted a hard error instead
of the old Zig panic. Port the implementation:
- Macro.rs: cook_raw_template_segment + make_template_strings_array,
and fill the ExprData::ETemplate match arm in Runner::run.
- visit_expr.rs: visit substitution values (with constant-folding
flags set, matching .e_call) before the macro call; report errors
like .e_call does; skip the trailing fold/re-visit when the macro
returns the original template.
- lexer.rs: expose decode_escape_sequences (was private).
The parallel .zig changes are kept in sync as the port reference.
Adjust the constant-folding test to avoid string-addition ropes,
which trip a pre-existing debug_assert in string_to_js (unrelated
to this change; affects identity("a"+"b") too).
robobun
force-pushed
the
farm/46de06ae/macro-tagged-template
branch
from
916955a to
714e3de
Compare
There was a problem hiding this comment.
All my earlier inline feedback is addressed and I found nothing new on this pass — but this is ~300 lines of new macro-execution / JSC-interop logic across parallel Zig and Rust paths (plus a couple of acknowledged deferred spec gaps), so it's worth a maintainer's eyes before merge.
Extended reasoning...
Overview
This PR implements tagged-template-literal support for Bun macros, replacing a @panic("TODO"). It adds two helpers (cookRawTemplateSegment, makeTemplateStringsArray) and a new .e_template arm in Runner.run in both src/js_parser_jsc/Macro.zig and the parallel Rust port Macro.rs; updates the .e_template visitor in visit_expr.{zig,rs} to pre-visit substitutions with constant-folding flags and to report macro errors like the .e_call path; flips decodeEscapeSequences to pub in the lexer; and adds ~90 lines of tests across macro-test.test.ts, macro.ts, and a new 18047.test.ts.
Security risks
Macros execute arbitrary user JS at build time, so the macro runner is a security-relevant subsystem. That said, this PR doesn't change the trust boundary — it adds a new call shape into the existing runner, reusing the same toJS, protect/unprotect, and node_modules / no_macros gates already applied to the .e_call path. The throwaway lexer for escape decoding operates on source bytes already in memory and writes only to a local log that's discarded. I don't see new injection, auth, or data-exposure surface here.
Level of scrutiny
Moderate-to-high. This is not a config tweak or mechanical refactor — it's a feature implementation with non-trivial pieces: building a TemplateStringsArray-shaped JS object via JSC bindings, GC protect/unprotect bookkeeping, a throwaway-lexer pattern for cooking raw segments, UTF-16 string materialization, and a parallel Rust port that must stay behaviorally identical. The author has also explicitly deferred three known minor gaps (no Object.freeze / non-enumerable .raw; legacy-octal / \\8-\\9 escapes don't produce undefined cooked values; a pre-existing one-GC-root leak on the toJS error path when javascript_object != .zero, copied verbatim from .e_call). Those are reasonable scope calls for a crash-fix PR, but a maintainer should sign off on them.
Other factors
- All six of my prior inline comments (two rounds) are resolved:
test.concurrentapplied, constant-folding flags added around the substitution-visit loop, thebuf.items.len == 0line-continuation crash guarded, and the deadfold()block removed. The remaining two were acknowledged as intentional deferrals. - Test coverage is good: in-process cases for no-subs, subs, escapes, unicode, invalid escapes →
undefined, line continuations, constant folding, namespace imports; plus subprocessbun build/bun runregression tests. - CI is green on the touched tests per the author's status comment; remaining red is unrelated flake.
- No human reviewer has weighed in yet; the author has marked it ready for maintainer review.
|
Heads-up: #31693 fixes the "Scope mismatch while visiting" panic from macro tagged templates by visiting the template parts before the macro dispatch (covers the dead-flow / macros-disabled / node_modules returns this PR's pre-call visit doesn't reach). If #31693 lands first, the rebase here is dropping the inserted visit loop and keeping the fold-flag wrapper around the (now earlier) parts visit. |
…emplates (#31693) ### Repro ```ts // macro.ts export function mac(...args: any[]) { return "x"; } // index.ts import { mac } from './macro.ts' with { type: 'macro' }; mac`a${() => { let q = 1; }}b`; function g() { { let y = 1; } } ``` ``` $ bun index.ts panic: Scope mismatch while visiting ``` Any tagged-template macro invocation whose interpolations contain a scope-creating expression (arrow, function, class) panics instead of reporting the intended `template literal macro invocations are not supported` error. The dead-code variant (`false && mac`…`` `), the macros-disabled path, and the node_modules path crash the same way. Panics on release builds (kind mismatch) and debug builds (loc mismatch); same structure existed in the Zig-era `visitExpr.zig`, so this predates the Rust port. Crash signature matches Sentry [BUN-3BYK](https://bun-p9.sentry.io/issues/7504990169/) (`Scope mismatch while visiting`, also seen in Zig-era releases). ### Cause The parser records every scope pushed during the parse pass in `scopes_in_order`; the visit pass replays them in the same order and panics on divergence. In `e_template` (`src/js_parser/visit/visit_expr.rs`), when the tag resolves to a macro ref, **every** dispatch outcome returns before the `for part in e_.parts_mut()` visit loop: - `is_control_flow_dead` → replaced with `undefined` - `no_macros` → error + `undefined` - `node_modules` → error + `undefined` - `macro_context.call(...)` failure → plain `return` (and template invocations currently always fail with `template literal macro invocations are not supported`, `src/js_parser_jsc/Macro.rs`) The scopes recorded for arrows/functions inside the interpolations are never consumed, so the next scope the visit pass pushes reads a stale entry and trips the `Scope mismatch while visiting` check. Same bug family as #31231 / #31340 / #31533 (constructs dropped without consuming/discarding their recorded scopes). ### Fix Visit the template parts right after visiting the tag, **before** the macro dispatch — the same ordering `e_call` uses (arguments are visited before its macro handling). All dispatch paths may then freely replace the expression: the parts' scope entries have already been consumed. The fall-through case no longer re-visits parts. Also syncs `Cargo.lock` with `bun_bin`'s manifest (`bstr` was added to `Cargo.toml` in 90f334a without the lock update; any local cargo invocation regenerates this line). ### Verification - 3 new tests in `test/bundler/transpiler/scope-mismatch-panic.test.ts` (live tag, dead-flow, namespace-member tag). All three panic without the fix and pass with it. - `test/bundler/transpiler/{scope-mismatch-panic,macro-test,transpiler,template-literal}.test.ts` and `test/bundler/bundler_string.test.ts` all pass. Note: #30545 (tagged-template macro support, feature) inserts a parts visit before the macro *call*, which would cover the live path but not the dead-flow / macros-disabled / node_modules returns. This fix is independent and minimal; #30545 rebases on top by dropping its duplicate visit loop (its fold-flag wrapper can stay).
1 participant
What does this PR do?
Fixes #18047 — calling a macro as a tagged template literal no longer panics.
Repro
Cause
Macro.Runner.runonly handled.e_callexpressions; the.e_templatecase was@panic("TODO: ...").Fix
Macro.zig: implement the.e_templatecase. Build aTemplateStringsArray-shaped first argument — a JS array of cooked string segments with a.rawproperty holding the raw segments — and pass substitution values as the remaining arguments, matching tagged-template call semantics. Raw segments are cooked via the lexer'sdecodeEscapeSequences; segments whose escapes don't decode becomeundefinedin the cooked array per spec.visitExpr.zig: visit template substitution values before the macro call so literals are resolved fortoJS, and report macro errors the same way the.e_callpath does instead of silently returning the original expression.lexer.zig: makedecodeEscapeSequencespubso the macro runner can reuse it.After
Verification
test/bundler/transpiler/macro-test.test.tscovering: no substitutions, substitutions, escape sequences (\n,\t,\`),\u{…}unicode escapes, invalid escapes (cooked →undefined), and namespace imports.test/regression/issue/18047.test.tscovers bothbun buildandbun run.bundler_string/template-literaltests still pass.