Conversation
|
Warning Review limit reached
Next review available in: 3 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 |
|
Updated 10:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 22be795 has some failures in 🧪 To try this PR locally: bunx bun-pr 38021That installs a local version of the PR into your bun-38021 --bun |
|
Status: reproduced and fixed in this PR; ready for review. Reproduced by adding CI on 22be795 (build 94543): 177 of 179 jobs passed, no test failures. The remaining 2 jobs are the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — build-time codegen guard with byte-identical output on the current tree.
What was reviewed:
- Traced the
[(<]stop-regex change and the$anchor onfunction_regexpthroughsliceSourceCode/applyReplacements; the chunk always ends exactly at the macro name, so anchoring cannot skip a real call and correctly stops$debug && $debug(...)from splicing out the prefix. - Confirmed no
$macro<or generic-arg call sites exist insrc/js/, so theprivate.d.tsde-genericization and the new throw path do not affect any current input. - Checked that
$macro < x(comparison with whitespace) does not match the new stop pattern, so no false positives on non-call<. - Test file placement in
test/internal/source-lints/and the workflow path filter follow the existing pattern for build-script unit tests.
Extended reasoning...
Overview
This PR hardens the builtin-JS macro preprocessor (src/codegen/builtin-parser.ts + src/codegen/replacements.ts) so that $macro<T>(...) — which the regexp-based parser previously ignored, leaving an unexpanded @macro(...) in the bundle and omitting the native-side registration — now fails the build with a clear error. It also anchors function_regexp to the end of the chunk (fixing a latent splice bug when a bare $debug reference precedes a $debug(...) call in the same chunk), removes the generic type parameters from the five native-macro declarations in src/js/private.d.ts so tsc rejects the form too, adds a dedicated source-lints test covering all ten macros, and adds src/codegen/** to the source-lints workflow path filter.
Security risks
None. This is purely build-time TypeScript that preprocesses src/js/** before bundling; it runs during bun bd, not at runtime, and the only new behavior is throwing an error where it previously silently mis-emitted.
Level of scrutiny
Low-to-medium. The failure mode of a mistake here is a broken build (loud, immediate), not a mis-behaving binary. The PR description states — and the robobun repro comment confirms — that bundle-modules.ts output for the unmodified tree is byte-identical before and after, which is the strongest evidence that the anchor and stop-regex changes do not perturb any existing macro expansion. I traced the chunk boundary logic: when match[2] is set, i lands on the (/<, so the slice handed to applyReplacements ends exactly at __intrinsic__<name> and the anchored regex matches; when the chunk ends for any other reason (string, comment, bracket), rest cannot start with ( or <, so the anchored regex not matching is equivalent to the old rest.startsWith("(") guard failing. The new < stop requires no whitespace between the macro name and <, so a hypothetical $debug < x comparison would not trigger it.
Other factors
Grep confirms no call site in src/js currently passes type arguments to any of the five native macros or writes $macro<, so both the private.d.ts change and the new throw are inert on today's tree. The new test file exhaustively covers every entry in function_replacements (with a guard test that fails if a new macro is added without coverage), the positive expansion, the type-argument rejection, string/comment immunity, bare-reference passthrough, and the anchoring fix. Placement in test/internal/source-lints/ matches sibling build-script tests that run against the released bun without needing the built binary, and the workflow path-filter addition ensures it runs when the preprocessor changes. No outstanding reviewer comments; no CODEOWNER-gated paths.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested build-tooling change that turns a silent codegen miss into a loud bundle-time error.
What was reviewed:
- Traced the
[(<]stop-regex change and the$anchor onfunction_regexpthroughsliceSourceCode— the chunk always ends immediately after the macro name whenmatch[2]fires, so the anchor is sound and also fixes the$debug && $debug(...)splicing bug. - Checked that no
src/js/**call site passes type arguments to$cpp/$rust/$new*Function/$bindgenFn, so theprivate.d.tsde-genericization is a no-op for existing code. - Considered the
$macro < xfalse-positive case (comparison, not type args) — not a real pattern for any of these names, and the byte-identicalbundle-modulesoutput confirms nothing in-tree hits it. - The seven
comment-copflags are the advisory heuristic firing on any 2+ line comment in claude-labeled PRs; five are JSDoc in a.d.tsfile and two are brief "why" notes on non-obvious regex behavior — none are workaround justifications.
Extended reasoning...
Overview
This PR tightens the builtin-JS macro preprocessor (src/codegen/builtin-parser.ts + src/codegen/replacements.ts) so that $macro<T>(...) — which previously fell through unexpanded and only surfaced as a distant cargo dead-code error or a runtime module-load failure — is now a hard bundle-time error naming the macro and the fix ($macro(...) as T). It also anchors function_regexp to the end of the chunk (fixing a latent $debug && $debug(...) mis-splice), removes the misleading generic type parameters from the five native-macro declarations in src/js/private.d.ts, adds src/codegen/** to the source-lints workflow path filter, and ships a 24-case unit test that exercises every macro through the actual preprocessor.
Security risks
None. This is build-time TypeScript tooling that runs on the repo's own src/js/** sources during the build; it takes no external input and produces no runtime code paths of its own. The change strictly tightens acceptance (rejects a form that previously passed silently).
Level of scrutiny
Medium-low. The preprocessor feeds every builtin module, but a mistake here fails the build loudly rather than shipping wrong runtime behavior, and the PR author verified bundle-modules.ts output is byte-identical on the current tree (all 197 modules, GeneratedJS2Native.h, generated_js2native.rs, the module blob). The two behavioral changes are each a one-token regex edit whose effect I traced through sliceSourceCode: match[2].length - 1 leaves i pointing at the (/<, so the chunk always ends exactly at the macro name, making the $ anchor correct; and none of the replacements[] rules can rewrite a trailing macro name away from the chunk end.
Other factors
- The new test file lives alongside a directory of similar preprocessor/source-tree lints, imports the codegen scripts directly, covers both stop-regex variants (via
$assert(...)end-on-comma and template-substitution re-entry), asserts the exact error text, and checks that macro-lookalikes inside strings/comments/template-text are untouched. Thefunction_replacementscompleteness test guards against a future macro being added without coverage. - I grepped
src/jsfor$(cpp|rust|newCppFunction|newRustFunction|bindgenFn)\s*<— zero hits, so removing the generics fromprivate.d.tscannot introduce new tsc errors. - The only theoretical over-reach —
$debug < xnow stops the chunk and throws — is not a pattern that makes sense for any of these ten names, and would fail the build with a clear message rather than miscompile. - No CODEOWNERS cover these paths. The seven
comment-copinline comments are from an advisory bot that flags every 2+ added comment lines undersrc/on claude-labeled PRs; five target JSDoc in a.d.ts(where doc comments belong) and two target 2–3 line "why" notes on non-obvious regex structure. None are the "paragraph-long workaround justification" the bot's message describes, so I do not treat them as blocking.
…ipping them
The builtin JS preprocessor only recognizes a macro when the name is
directly followed by "(", so a call written with TypeScript type
arguments ($newRustFunction<() => number>(...)) was left in the bundle
unexpanded and its native call was never registered. Nothing reported
this at bundle time; it surfaced later as a dead-code error in cargo or
a module that fails to load.
builtin-parser.ts now also stops at "$macro<" and applyReplacements
throws there, naming the macro and the "$macro(...) as T" form. The
macro regexp is anchored to the end of the chunk, since the chunk ends
right after the macro name being expanded; a bare reference to a macro
name earlier in the same chunk no longer hijacks the expansion.
private.d.ts drops the type parameters from the native macros so tsc
rejects the form as well. No call site in src/js used them, and the
bundle output for the tree is unchanged.
…the if ($debug) fall-through
b82e1a1 to
22be795
Compare
There was a problem hiding this comment.
LGTM — turns a silent codegen footgun into a loud build-time error, with no change to generated output for the current tree.
What was reviewed:
- Traced the stop-regex change:
[(<]only adds$macro<(no space), so comparisons like$debug < xare unaffected; verified no$macro<exists insrc/js/today. - Verified the
$anchor onfunction_regexpis safe: when the stop regex ends a chunk on a macro, the name is at the end; when it ends on anything else,restnever starts with(so the old unanchored match was discarded anyway. - Checked the test's native-macro fixtures (
node_util_binding.rs,NodeValidator.cpp,bindgen_test.bind.ts) all resolve throughresolveNativeFileId, and thefunction_replacementscompleteness assertion guards against future macro additions. - The comment-cop bot firings look like noise — they fire at line 0 including on
private.d.tswhere the diff adds no comments; the two comments actually added are one-liners explaining non-obvious invariants.
Extended reasoning...
Overview
This PR touches build-time codegen tooling only — no runtime code. It changes src/codegen/builtin-parser.ts (the stop regex now also breaks at $macro<), src/codegen/replacements.ts (applyReplacements throws on $macro< with a helpful message; function_regexp is anchored to the end of the chunk to fix a latent bug where an earlier bare macro reference in the same chunk hijacked the expansion), src/js/private.d.ts (drops the generic type parameters from the five native-binding macros so tsc also rejects the form), .github/workflows/source-lints.yml (adds src/codegen/** to the path filter), and adds test/internal/source-lints/codegen-builtin-macros.test.ts.
Security risks
None. This is a build-time preprocessor for internal builtin JS modules; no user input, network, filesystem writes, auth, or crypto is involved. The change strictly tightens what the preprocessor accepts.
Level of scrutiny
Low-to-medium. The preprocessor runs at build time and any regression would fail the build loudly rather than shipping broken code. The author verified bundle-modules.ts output is byte-identical for the unmodified tree (all 197 modules, generated headers, and the module blob). I independently traced the two behavioral changes: (1) the [(<] addition to the stop regex only fires on $macro< with no intervening whitespace, so $debug < x and similar are untouched — and grep confirms no such pattern exists in src/js/; (2) the $ anchor on function_regexp cannot lose a real expansion because when match[2] is set the chunk always ends exactly at the macro name, and when it isn't set rest never starts with ( so the old unanchored match was a no-op anyway.
Other factors
The test file is thorough: it covers all 10 macros (with a completeness assertion against function_replacements so future additions are caught), both stop-regex variants (via $assert's end-on-comma slice and template substitutions), strings/comments/template-text pass-through, the if ($debug) bare-value case, and the anchoring fix. The native-macro test fixtures resolve against real files in the tree (rustIdentifierPaths['node_util_binding.rs'], NodeValidator.cpp, bindgen_test.bind.ts), and the expansion assertions use /^__intrinsic__lazy\\(\\d+\\)$/ so they don't depend on registration order. The .d.ts change is type-only, keeps the previous default return types, and no call site in src/js currently passes type arguments (grep confirmed).
The seven github-actions[bot] comment-cop comments appear to be automated file-level firings (all at line 0) rather than substantive review — four of them are on private.d.ts where the diff adds no comments at all. The two comments the PR does add (in builtin-parser.ts and replacements.ts) are single-line notes explaining non-obvious invariants (why < is a stop and why the anchor is safe), which is exactly what REVIEW.md asks for.
This branch has not been deployed
1 participant
Problem
timerClockMs: $newRustFunction<() => number>("runtime/timer/Timer.rs", "internal_bindings.timerClockMs", 0)insrc/js, bundles without any error: the bundled module keeps a literal@newRustFunction(...)(a private name that does not exist) and the call is missing fromGeneratedJS2Native.h/generated_js2native.rs.bun bdfails in cargo witherror: function `timer_clock_ms` is never usedbecause the thunk was never generated; if the symbol is used elsewhere, or for a C++ target, the build passes and the module fails when it is first loaded.src/codegen/builtin-parser.ts:12only ends a chunk at$macro(, andsrc/codegen/replacements.ts(applyReplacements) only expands when the rest of the source starts with(.$macro<...>(matches neither, so the call falls through the plain-text path. This applies to every macro infunction_replacements.src/js/private.d.tsdeclared$cpp,$rust,$newCppFunction,$newRustFunctionand$bindgenFnas generic, so tsc accepted exactly this form. The$cpp/$rustdoc comments have said since feat: addJSObjectconstructors #15742 that a type parameter "will break codegen"; nothing enforced it, and it was hit again while working on node:http: measure the keep-alive idle period on the monotonic clock #37974.Fix
builtin-parser.tsalso ends a chunk at$macro<;applyReplacementsthrows there with the macro name, the source it found, and the supported spelling ($macro(...) as T).bundle-modulesexits 1 and names the module (While processing: internal-for-testing.ts).replacements.tsis anchored to the end of the chunk. The chunk ends right after the macro name whose(/<ended it, so that is the only name that can be the call being expanded; previously the first macro name anywhere in the chunk won, sox = $debug && $debug("..")spliced out the$debug &&prefix.$debugas a value (if ($debug), defined by--define) still passes through unchanged.private.d.tsdrops the type parameters from the five native macros, so tsc reports the form too (TS2558: Expected 0 type arguments, but got 1). The return types are the old defaults (anyand(...args: any) => any), and no call site insrc/jspasses type arguments, so nothing else changes.$rust()registry comments ingenerate-js2native.ts, and greps for$name(all agree on, andas Talready covers typing the result (it is what the existing call sites do).bundle-modules.tsoutput for the current tree is byte-identical before and after (all 197 modules, the builtin functions,GeneratedJS2Native.h,generated_js2native.rs, and the module blob were diffed), and thetsc -p src/jserror set is unchanged.test/internal/source-lints/codegen-builtin-macros.test.ts: for each of the 10 macros, the plain call still expands and the type-argument form throws; type arguments inside strings/comments are left alone; the bare-reference and anchoring cases above. 11 of 24 cases fail without thesrc/change. The file goes insource-lints/because it imports the build scripts directly and never needs the built binary;src/codegen/**is added to that workflow's path filter so it runs when the preprocessor changes.bun bd teston the new file plustest/internal/internal-module-blob.test.tsandtest/internal/bindgen.test.ts, the wholetest/internal/source-lints/directory,bun run lint, and the temporary repro above throughbundle-modules.ts(fails at bundle time now, passed silently before).Background
src/js/**) is preprocessed bybundle-modules.ts/bundle-functions.tsbefore being bundled:$namebecomes the JSC private name@name, and a few$names are macros expanded byreplacements.ts($rust,$cpp,$newRustFunction,$newCppFunction,$bindgenFn,$debug,$assert,$isPromise*).generate-js2native.tsand are replaced by@lazy(id); that registry is what emits the C++ dispatch table and the Rust thunks. A call the preprocessor does not see is therefore missing from the native side as well, not just left as-is in the JS.builtin-parser.ts(sliceSourceCode) is regexp-based: it walks the source in chunks, ending a chunk at strings, comments, brackets or a macro call, and hands each code chunk toapplyReplacements(src, chunkLength), which sees the chunk plus everything after it.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file