Repository navigation
Conversation
Karma's `di` package and AngularJS's $injector parse
`Function.prototype.toString()` to discover DI tokens from
comment-annotated parameters:
exports.create = function (/* config */ c, /* config.proxies */ p) {}
Bun's runtime transpiler was discarding these comments entirely, so
`di` saw bare parameter names and failed with
"No provider for \"proxies\"!" when starting Karma.
This change makes the lexer capture comments while scanning a function
parameter list, stores them on `G::Arg`, and re-emits them in
`print_fn_args` (skipped under `--minify`). Line comments are
rewritten as block comments so they don't swallow the rest of the
argument list.
Fixes #13451
|
Warning Review limit reached
Next review available in: 15 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 (4)
Comment |
|
Updated 9:29 PM PT - Aug 1st, 2026
❌ @robobun, your commit 3ff12cd has 4 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.657eb2be-5c1e-48d2-8626-885926f93a1e
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.d67c870e-75f0-48bf-b059-7c6ca5c1f2d0
🧪 To try this PR locally: bunx bun-pr 36705That installs a local version of the PR into your bun-36705 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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/parse/parse_fn.rs:205-207—preserve_all_comments_beforeis heldtruefor the entire(...)loop, so comments scanned while parsing an argument's binding pattern, TS type annotation, or default-value expression leak into the next argument (or into nested statement lists). For examplefunction(a = /* x */ 1, b) {}now prints asfunction(a = 1, /* x */ b) {}, andfunction(a /* note */, b) {}prints asfunction(a, /* note */ b) {}— the comment moves into the exact DI-token position forb, which is a regression vs. the pre-PR behavior of simply dropping it. The flag should only be high between the comma/(and the start of the binding: turn it off (and truncate back tocomments_base) beforeparse_binding/skip_type_script_type/parse_exprfor the default, then re-enable before advancing past the comma. This also avoids two related leaks from the same over-broad scope:parse_stmts_up_todraining ordinary comments inside a block-bodied default (e.g.a = () => { return 1 /* mid */ + 2; }) into misplacedS::Commentstatements, andparse_import_exprunconditionally clearing the buffer / setting the flag tofalsewhen a default containsimport(), which drops the DI comment on the following arg.Extended reasoning...
What the bug is
parse_fnsetsp.lexer.preserve_all_comments_before = trueat line 207 and only restores it at line 375, wrapping the entire argument loop. Inside that window, each iteration callstake_arg_leading_comments(comments_base)once — at the top, before parsing the binding — and then parses the binding pattern, optional?, TS type annotation, and default-value expression with the flag still high. Any comment the lexer scans during those sub-parses is pushed intocomments_to_preserve_before(the lexer gate atlexer.rs:2244ishas_legal_annotation || preserve_all_comments_before) and nothing consumes or truncates it before the next iteration. That over-broad scope produces three distinct symptoms.Symptom 1 — comments migrate to the next argument (regression)
Concrete trace for
function(a = /* x */ 1, b) {}:- Iter 1:
take_arg_leading_commentsdrains nothing (buffer atcomments_base).parse_bindingconsumesa. Lexer sees=,next()scans/* x */and pushes it (flag is true), token becomes1.parse_exprconsumes1.args.push({binding: a, default: 1, leading_comments: []}). - Iter 2:
take_arg_leading_commentsdrains[/* x */]and attaches it tob. - Printer emits
function(a = 1, /* x */ b) {}.
The same happens for a trailing comment on the previous arg (
function(a /* note */, b)→function(a, /* note */ b)), for destructuring (function({/* p */ x}, y)→function({x}, /* p */ y)), and for TS annotations (function(a: /* T */ number, b)→function(a, /* T */ b)).Why this is a regression: before this PR the flag was
falsehere, so/* x *///* note */were dropped anddi.annotate()fell back to the parameter nameb. After this PR the comment is relocated into the DI-token position of a different parameter, sodi.annotate()returns['a', 'x']/['a', 'note']forb— a wrong DI token where previously there was none. That's the exact class of behavior this PR exists to make semantically meaningful, so producing it in the wrong place is worse than the pre-PR baseline.Symptom 2 — nested brace bodies inside defaults emit misplaced
S::CommentstatementsIf a default value contains a block body — arrow with a block, function/class expression, object method — that body is parsed via
parse_fn_body → parse_stmts_up_towhile the flag is still true (arrows reachparse_arrow_bodywithout going throughparse_fn; nestedparse_fnsavesold_preserve_comments = trueand restores it totruebefore callingparse_fn_body).parse_stmts_up_to(mod.rs:1454-1458) drainscomments_to_preserve_beforeintoS::Commentstatements at the top of every iteration. With the flag high, every ordinary comment inside the body is pushed and then emitted as a statement at the nearest following statement boundary:function outer(a = () => { return 1 /* mid */ + 2; }) {}
Pre-PR output strips
/* mid */. Post-PR output is:function outer(a = () => { return 1 + 2; /* mid */ }) {}
The
SCommentprinter arm has nominify_whitespacegate (it exists to keep legal comments through minification), so these non-legal comments now also survive into minified output. This is not the same as symptom 1 — hereparse_stmts_up_toconsumes the buffer before the outer loop resumes, so a truncate-after-args.push()fix alone would not address it.Symptom 3 —
import()in a default disables capture for later argsparse_import_expr(parse_import_export.rs:45-51) unconditionally setspreserve_all_comments_before = falseand.clear()s the buffer with no save/restore. It's reachable from a default value viaparse_expr(Level::Comma). So forfunction(a = import('./x'), /* token */ b) {}, afterimport()returns the flag isfalse, the lexer does not push/* token */, andb.leading_commentsstays empty. The.clear()also wipes entries belowcomments_base(legal comments the PR's own comment says are 'left in place'). This is a new interaction created by the PR — nothing previously enabled the flag around expression parsing that could reachimport().Why existing code doesn't prevent it
The only truncate is at line 374, after the whole loop.
take_arg_leading_commentsis called beforeparse_binding, not after the default is parsed, so it cannot discard comments captured mid-argument. Nothing lowers the flag around the sub-parses.Fix
Scope the flag to the gap between arguments, not the whole list. Concretely, right after
take_arg_leading_comments(...):let leading_comments = p.take_arg_leading_comments(comments_base); p.lexer.preserve_all_comments_before = old_preserve_comments; // off during binding/type/default
and re-enable + truncate just before advancing to the next argument:
p.lexer.comments_to_preserve_before.truncate(comments_base); p.lexer.preserve_all_comments_before = true; p.lexer.next()?; // consume ','
This fixes all three symptoms at once: comments inside the binding/type/default aren't captured (symptom 1), nested
parse_stmts_up_tosees the flagfalseso ordinary comments aren't turned intoS::Comment(symptom 2), andparse_import_expr's hard reset is harmless because the flag was alreadyfalseand the buffer already atcomments_base(symptom 3). Independently, per the 'fix the whole class' rule,parse_import_exprshould save/restore the flag andtruncate-to-a-saved-base rather than.clear()now that callers can enter it with the flag already true. - Iter 1:
-
🟡
src/js_parser/parse/parse_fn.rs:372-374— The unconditionaltruncate(comments_base)here also discards legal comments (/*! ... */,//! ...) that the lexer would have pushed tocomments_to_preserve_beforeeven without this PR (viahas_legal_annotationat lexer.rs:2244), and that previously survived into the function body asS::Commentstatements. Concretely,function foo(/*! @license MIT */) {}andfunction foo(x /*! @preserve */) {}used to keep the legal comment in the output; now it is silently dropped. Consider filtering the tail (retain entries whosetext[2] == b'!') instead of blanket-truncating — legal comments in a param list are rare enough that this need not block, but it is a regression.Extended reasoning...
What the bug is
Before this PR,
preserve_all_comments_beforewasfalsewhile parsing a function's parameter list, so the only comments that could land inlexer.comments_to_preserve_beforeduring arg parsing were legal comments — those whose third byte is!(/*! ... */or//! ...). The lexer pushes those unconditionally, regardless ofpreserve_all_comments_before:// src/js_parser/lexer.rs:2198, 2244 let has_legal_annotation = text.len() > 2 && text[2] == b'!'; ... if has_legal_annotation || self.preserve_all_comments_before { self.comments_to_preserve_before.push(...); }
Those legal comments then flowed through to
parse_fn_body→parse_stmts_up_to, which drains the buffer intoS::Commentstatements at the top of the body (mod.rs:1454-1458). So a/*!comment inside a parameter list was preserved in the printed output — relocated into the body, but present.This PR adds an unconditional
p.lexer.comments_to_preserve_before.truncate(comments_base)after the argument loop. The intent is to discard the newly captured ordinary comments between the last arg and)(which would otherwise surface as body-levelS::Comments). But because legal comments are pushed via the same buffer, any legal comment that lands pastcomments_baseafter the lasttake_arg_leading_commentsdrain — or in the empty-parens case wheretake_arg_leading_commentsnever runs at all — is deleted along with them.Step-by-step trace:
function foo(/*! @license MIT */) {}Pre-PR:
p.lexer.expect(TOpenParen)callsnext(), which scans the/*! @license MIT */comment.has_legal_annotation = true→ pushed tocomments_to_preserve_before(index 0).- Arg loop condition:
token == TCloseParen→ loop body never runs. expect(TCloseParen), thenparse_fn_body→expect(TOpenBrace)→parse_stmts_up_to.- First iteration of
parse_stmts_up_todrainscomments_to_preserve_before→ emitsS::Comment { text: "/*! @license MIT */" }intofunc.body.stmts. - Printer emits the comment inside the function body. Comment preserved.
Post-PR:
comments_base = 0,preserve_all_comments_before = true.expect(TOpenParen)→next()scans the legal comment → pushed at index 0 (same as before;has_legal_annotationORpreserve_all_comments_before— both true now).- Arg loop:
token == TCloseParen→ loop body never runs, sotake_arg_leading_commentsis never called. truncate(comments_base)=truncate(0)→ the legal comment is deleted.parse_stmts_up_tosees an empty buffer. Comment lost.
Second trace:
function foo(x /*! @preserve */) {}comments_base = 0.expect(TOpenParen)→next()→ token =x, buffer empty.- Arg loop iteration:
take_arg_leading_comments(0)runs — buffer is empty → returnsEMPTY. parse_bindingconsumesx; itsnext()scans/*! @preserve */→ pushed at index 0.token == TCloseParen→break.truncate(0)deletes the legal comment. Pre-PR, this same comment was pushed viahas_legal_annotationand survived intoS::Comment.
The same applies to
function foo(x = 1 /*! @license */) {}— the legal comment is scanned after the default expression, past the point wheretake_arg_leading_commentsalready ran for that arg.Why existing code doesn't prevent it
take_arg_leading_commentsonly runs at the start of each arg-loop iteration, beforeparse_binding. Any comment scanned after that point (after the binding, after a default value, or in empty parens where the loop never enters) sits in the buffer until the post-looptruncate. The truncate makes no distinction between ordinary comments (which this PR intentionally captures and wants to drop here) and legal comments (which were captured pre-PR too and should survive).Note: legal comments that appear before an arg binding are actually fine post-PR — they're drained into
arg.leading_commentsand re-emitted by the printer. Only the empty-parens and after-last-arg-drain positions regress.Impact
This is a genuine behavioral regression:
/*! ... */and//! ...legal-annotation comments inside a function parameter list were previously preserved in output and are now silently dropped. That said, legal/license comments placed inside a parameter list (as opposed to at file or function-header scope) are essentially nonexistent in real code, and the pre-PR "preservation" was itself a relocation into the body rather than positionally-correct output. Marking as a nit rather than blocking.Fix
Instead of a blanket
truncate(comments_base), filter the tail to retain legal-annotation entries:// Drop ordinary comments captured between the last arg and `)`, but keep // legal (`/*!` / `//!`) comments — those were pushed regardless of // preserve_all_comments_before and should still surface as S::Comment. let buf = &mut p.lexer.comments_to_preserve_before; let mut i = comments_base; while i < buf.len() { let t = buf[i].text.slice(); if t.len() > 2 && t[2] == b'!' { i += 1; } else { buf.remove(i); } } p.lexer.preserve_all_comments_before = old_preserve_comments;
(or equivalently
buf.drain(comments_base..)and re-push the!-prefixed ones).
Comments inside a binding pattern, TS type, or default value were being captured and migrated to the next argument, and legal comments drained into leading_comments were dropped under --minify. Restrict capture to the whitespace between `(`/`,` and the next binding, and leave legal (`/*!` / `//!`) comments on the existing S::Comment path.
|
Addressed the review findings in 5fb70a2:
I did not change |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/parse/parse_fn.rs:261—preserve_all_comments_beforestays on acrossparse_binding,skip_type_script_type, and the default-valueparse_expr, so a comment written inside one argument is captured and then re-emitted somewhere else:function(a = /* opts */ {}, b) {}prints asfunction(a = {}, /* opts */ b) {}(di now extracts"opts"forb— a new wrong-provider failure), andfunction(a = /* note */ () => { return 1 }) {}drains/* note */into the arrow body as anS::Commentthat survives--minify. Restorepreserve_all_comments_before = old_preserve_commentsaround the binding/type/default parses so the flag only covers the gap from(/,to the start of the binding — truncating after eachargs.pushfixes the first case but not the nested-body leak.Extended reasoning...
What the bug is
parse_fnenablesp.lexer.preserve_all_comments_before = trueat line 201 and does not restore it until line 368, after the wholewhile p.lexer.token != T::TCloseParenloop. Inside each iteration,take_arg_leading_comments(comments_base)runs first (line 261) and then the loop goes on to callparse_binding, optionallyskip_type_script_type, and optionallyparse_expr(Level::Comma)for the default value — all while the flag is stilltrue. Any ordinary comment scanned during those calls is pushed intocomments_to_preserve_beforeeven though it is not a "leading comment before the parameter binding". Nothing betweenargs.push(...)and the next iteration'stake_arg_leading_commentsclears the buffer, so those stray comments end up in one of two wrong sinks.Sink (a): misattributed to the next argument
Step-by-step for
function(a = /* opts */ {}, b) {}:- Line 199:
comments_base = 0; line 201 setspreserve_all_comments_before = true;expect(TOpenParen)advances toa. - Iter 1:
take_arg_leading_comments(0)— buffer is empty →EMPTY.parse_bindingconsumesa. Token is=;p.lexer.next()scans/* opts */and, because the flag is on, pushes it (index 0).parse_exprconsumes{}.args.push(a)withleading_comments = EMPTY.p.lexer.next()past,. - Iter 2:
take_arg_leading_comments(0)seesbuf.len() == 1 > 0and drains[/* opts */]→ attached tob. - Printer at
lib.rs:2111emits it beforeb:function(a = {}, /* opts */ b) {}.
Pre-PR,
preserve_all_comments_beforewasfalsehere so/* opts */was never pushed and the output wasfunction(a = {}, b) {}.di'sFN_ARGregex on the second comma-split segment now matches/* opts */and extracts"opts"instead of"b"— the exact wrong-provider failure mode this PR was opened to fix, freshly introduced for an adjacent shape. The same mechanism relocatesfunction({/* c */ x}, y)→function({x}, /* c */ y)and, in TS,function(x: /* T */ number, y)→function(x, /* T */ y).Sink (b): leaked into a nested body as
S::Comment, surviving--minifyStep-by-step for
function outer(a = /* note */ () => { return 1 }) {}:- Iter 1:
take_arg_leading_comments(0)→EMPTY.parse_binding(a).next()past=pushes/* note */.parse_expr→ arrow →parse_arrow_body→parse_fn_body→parse_stmts_up_to(TCloseBrace). parse_stmts_up_to(mod.rs:1454-1458) iteratescomments_to_preserve_before, emitsS::Comment("/* note */")at the top of the arrow body, and clears the buffer — before the outer loop ever pushesaor reaches the post-looptruncate.- The printer's
SCommentarm (lib.rs:4901-4904) callsprint_indented_commentwith nominify_whitespacegate.
Under
--minifythe output isfunction outer(a=()=>{/* note */return 1}){}; pre-PR it wasfunction outer(a=()=>{return 1}){}. The same happens forfunction(a = /* c */ function(){})(the innerparse_fnsnapshotscomments_base_inner = 1, so itstruncate(1)leaves/* c */at index 0 in place and the innerparse_fn_body → parse_stmts_up_toswallows it), and for// noteline comments, whichprint_indented_commentfollows with a hard\neven when minifying.Why existing code doesn't prevent it
The only consumers of
comments_to_preserve_beforeareparse_stmts_up_to(mod.rs:1454-1458),parse_import_export.rs:49, lexerrestore(), the new post-looptruncate(comments_base), andtake_arg_leading_commentsitself. None of those run between finishing one argument's binding/type/default and the next iteration's drain — exceptparse_stmts_up_towhen a nested brace body appears in a default, which is precisely sink (b). Truncating tocomments_baseright after eachargs.pushwould fix (a) but cannot fix (b), because the drain intoS::Commenthappens duringparse_exprfor the current argument, before the push.Impact
This PR's purpose is to make
Function.prototype.toString()reflect the authored comment positions. For any comment inside a destructuring pattern, TS type, or default value it instead relocates the comment to a position it was never written — either in front of the next parameter (breakingdi-style annotation parsing in a way that did not happen before this PR) or inside a nested function body. Under--minify, ordinary non-legal comments now survive in nested default bodies where pre-PR they were stripped. All of this is new behavior introduced by this change and belongs to the same bug class being fixed here.Fix
Scope the flag to only the gap between
(/,and the start of the binding. Concretely, right afterlet leading_comments = p.take_arg_leading_comments(comments_base);restorep.lexer.preserve_all_comments_before = old_preserve_comments;, and re-enable it just beforep.lexer.next()?that advances past the comma (and before the loopcontinuein theTThisbranch). Alternatively, wrapparse_binding/skip_type_script_type/ the default-valueparse_exprin a save-restore of the flag. Either approach fixes both sinks; the "truncate after each push" band-aid fixes only (a). - Line 199:
-
🔴
src/js_parser/parse/parse_fn.rs:366-368— The blankettruncate(comments_base)here drops legal comments (/*! ... */,//! ...) that were scanned inside the parens but never drained into anyarg.leading_comments— the empty-list casefunction foo(/*! @license */) {}(loop body never runs) and the trailing casefunction foo(x /*! @license */) {}(take_arg_leading_commentsruns beforeparse_binding, so the comment lands after the drain). Pre-PR these flowed through toparse_stmts_up_to→S::Commentand printed even under minify; now they're silently discarded. This is the companion to the printer-gate issue atlib.rs:2111— that one covers legal comments that were drained; this covers the ones that weren't, so the "skip legal comments intake_arg_leading_comments" fix suggested there also needs the truncate to preservetext[2] == b'!'entries.Extended reasoning...
What the bug is
The lexer pushes legal comments (
/*! ... *////! ...) intocomments_to_preserve_beforeunconditionally —lexer.rs:2244gates onhas_legal_annotation || self.preserve_all_comments_before, andhas_legal_annotationistext[2] == b'!'(lexer.rs:2198). So a legal comment inside a parameter list lands in the buffer regardless of this PR'spreserve_all_comments_before = truetoggle — it landed there pre-PR too.Pre-PR, nothing between
expect(TOpenParen)andparse_fn_bodytouched that buffer, so the legal comment survived toparse_stmts_up_to(mod.rs:1454-1458), which drains the buffer into anS::Commentat the top of the loop before theeendcheck — meaning even an empty{}body emits theS::Comment. The printer'sSCommentarm callsprint_indented_commentwith nominify_whitespacegate, so the legal comment printed even under--minify.Post-PR, the new
truncate(comments_base)after the arg loop deletes any buffer entry at index>= comments_basethat wasn't already drained bytake_arg_leading_comments. Two shapes hit this.Step-by-step: empty parameter list
Input:
function foo(/*! @license MIT */) {}comments_base = buf.len()(say 0),preserve_all_comments_before = true,expect(TOpenParen).- Advancing past
(scans/*! @license MIT */;has_legal_annotationis true → pushed at index 0. - Lexer lands on
).while p.lexer.token != T::TCloseParen— condition false, loop body never runs,take_arg_leading_commentsnever called. buf.truncate(0)— the legal comment is deleted.parse_fn_body→parse_stmts_up_to: buffer is empty, noS::Comment.- Output (any mode):
function foo() {}— the/*! @license MIT */is gone.
Pre-PR, step 4 didn't exist; the comment survived to step 5 and became
function foo() {/*! @license MIT */}.Step-by-step: trailing after the last arg
Input:
function foo(x /*! @license */) {}comments_base = 0,expect(TOpenParen)→ token isx, buffer empty.- Loop iteration:
take_arg_leading_comments(0)runs first — buffer is empty, returnsEMPTY. parse_bindingconsumesx; the lexer'snext()advancing pastxscans/*! @license */and pushes it at index 0.- Token is
), not,→break. buf.truncate(0)— the legal comment is deleted.
Pre-PR, again, step 5 didn't exist and the comment reached
S::Comment.Why existing code doesn't prevent it
take_arg_leading_commentsonly runs at the top of each loop iteration, so it can only capture comments scanned before the current arg's first token. Anything scanned while advancing past the binding, the type annotation, the default expression, or the final,-vs-)lookahead sits in the buffer at loop exit and is unconditionally truncated. Legal comments are the only class that would have been in that buffer pre-PR (non-legal ones weren't pushed withoutpreserve_all_comments_before), so they're the only regression here.Relationship to the
lib.rs:2111commentThe already-posted comment at
lib.rs:2111covers legal comments that were drained intoarg.leading_comments(leading position, e.g.function foo(/*! ... */ x) {}) and then dropped by the printer's!minify_whitespacegate. This bug covers legal comments that never reachleading_commentsbecause they're truncated first. Fixing only the printer gate leaves both shapes above broken. And notably, one of the fixes suggested there — "havetake_arg_leading_commentsskip legal comments so they continue down theS::Commentpath" — is defeated by thistruncateunless it's fixed too: the skipped legal comment would sit in the buffer and get truncated on loop exit.Impact
Functional regression in the bundler's legal-comment preservation guarantee. Legal comments are the one class contractually kept across minification; a
/*! @license ... */placed inside an empty or trailing-position parameter list silently disappears from bundled output where it previously survived. Edge-case placement in practice, but a real behavioral change from pre-PR.Fix
Instead of a blanket
truncate(comments_base), retain entries at index>= comments_basewhosetext[2] == b'!'(e.g.buf.drain(comments_base..).filter(is_legal).collect()back, or in-placeretainover the tail). Equivalently — and this composes cleanly with thelib.rs:2111fix — havetake_arg_leading_commentsleave legal comments in the buffer and have the post-loop cleanup drop only non-legal entries, so legal comments flow toS::Commentexactly as they did pre-PR.
|
The two findings in the latest automated review (flag held across
Spot-checked on the current build: |
…ase before each gap Comments inside `this: /* c */ T` or `@dec(/* c */)` were captured with the flag still raised and relocated onto the next real argument. Lower the flag (and discard any non-legal buffer entries) before entering the TThis branch, and around parse_type_script_decorators. Also re-snapshot comments_base before each comma scan so a nested .clear() during default-value parsing can't cause a later arg's leading comment to be mis-indexed.
A block-bodied arrow inside a decorator argument reaches parse_stmts_up_to which .clear()s comments_to_preserve_before, so a comment captured before @dec was being relocated into the nested body. Stash it into the leading-comment accumulator before decorator parsing (and re-snapshot comments_base after), then drain again after `...` for anything captured in the remaining gap.
There was a problem hiding this comment.
No further findings on 3a58b80 — the decorator-body drain issue from the last round is addressed by draining into leading before parse_type_script_decorators(). This still touches lexer-state save/restore across every branch of parse_fn's argument loop, so worth a human pass before merge.
What was reviewed:
- Flag scoping:
preserve_all_comments_beforeis raised only across the(/,→binding gap and restored beforeparse_binding, type annotations, default values, decorators, and thethis-param branch. - Legal comments:
is_legal_commentfiltering leaves/*!///!in the buffer so they still flow toS::Commentunder--minify; post-loop cleanup usesdiscard_non_legal_comments_fromrather than truncate. comments_baseis re-snapshotted before each re-raise (both comma paths, post-decorator, post-this), and both helpers floorbaseatbuf.len()so a nested.clear()can't strand later captures below the base.- Printer: line comments are wrapped as
/* … */and dropped if they contain*/; the whole block is gated on!minify_whitespace, which is safe because legal comments never reacharg.leading_comments.
Extended reasoning...
Overview
The PR adds a leading_comments field to G::Arg, captures inline comments between (/, and each parameter binding in parse_fn, and re-emits them in print_fn_args. This restores Karma/AngularJS-style DI-token comments in Function.prototype.toString() output. Four files: src/ast/g.rs (+5), src/js_parser/parse/parse_fn.rs (+58), src/js_printer/lib.rs (+17), plus a new regression test.
Security risks
None. This is comment preservation in the transpiler; no untrusted-input parsing beyond what the lexer already does, no auth/crypto/permissions.
Level of scrutiny
High. parse_fn runs for every function in every transpiled file, and the change threads mutable lexer state (preserve_all_comments_before, comments_to_preserve_before) through a loop with five distinct exit/continue shapes (this-param continue, this-param break, decorator path, rest-arg path, comma vs. break). Three prior review rounds each found a comment-migration or legal-comment regression on a path the previous fix missed; the current revision addresses all of them, but the number of interacting branches is exactly the kind of thing a maintainer familiar with the parser should eyeball.
Other factors
- All previously-flagged issues (legal-comment loss under minify, migration from default values / trailing positions /
thisannotations / decorator arguments, stalecomments_baseafter nested.clear()) are covered by the current diff and by the three tests in13451.test.ts. - I checked that the flag is restored on the loop's normal exit and on the
this-parambreak; error-path (?) exits leave it raised, but those abort the parse so lexer state is moot. Arg::deep_clonecopiesleading_commentsby value (StoreSliceisCopy), matching how otherStoreSlicefields are handled in the same file.- The
leadingArenaVecis allocated per-iteration even whenallow_ts_decoratorsis false and stays empty until the second drain — a minor allocation-in-hot-loop concern a reviewer may want to weigh in on, thoughArenaVec::new_inis cheap when never pushed to. - One residual quirk I noted but did not flag:
function(... /* c */ x)will print asfunction(/* c */ ...x)(comment moves before the...), since the drain happens after advancing pastTDotDotDot. Cosmetic and not a regression.
There was a problem hiding this comment.
Beyond the inline nit, I re-verified the prior-round findings against the current diff: legal comments in take_arg_leading_comments are skipped and survive --minify via the S::Comment path; the TThis and decorator branches now discard/drain before nested parsing so comments can't migrate onto the wrong arg; comments_base is re-snapshotted at each gap so a nested .clear() can't strand entries below the base. Also confirmed every other G::Arg construction site uses ..Default::default(), so the new field needs no call-site updates. Arrow-function args go through a separate parse path and are intentionally out of scope (Karma's di regex only matches the function keyword).
Extended reasoning...
This parser change has been through four review rounds; every substantive finding I raised (legal-comment minify regression, TS this-param/decorator comment migration, stale comments_base after nested .clear(), decorator-body swallowing pre-@ comments) is addressed in the current diff and the fixes trace correctly. Only a test-harness nit remains this run. I'm not approving because this is state-heavy logic in the JS parser hot path — a maintainer should eyeball the final flag-toggle shape in parse_fn before merge.
There was a problem hiding this comment.
No new issues this pass, but this threads mutable lexer state through the parse_fn hot path and took four review rounds to stabilize — worth a maintainer's eye before merge.
What was reviewed:
- Legal-comment (
/*!,//!) preservation under--minify—is_legal_commentmatches the lexer's own predicate and legal entries stay on theS::Commentpath. - Comment migration onto adjacent args via
this:params, decorators, default values, trailing position, and nested block bodies — flag is now scoped to the(/,-to-binding gap andcomments_basere-snapshotted at each raise. - All other
G::Argconstruction sites use..Default::default(), so the new field isEMPTYeverywhere else.
Extended reasoning...
Overview
Adds G::Arg::leading_comments and populates it in parse_fn by toggling lexer.preserve_all_comments_before around each argument gap, then re-emits those comments in print_fn_args. Fixes Karma/AngularJS DI which parses Function.prototype.toString() for /* token */ annotations. Four files touched: AST struct, parser, printer, plus a new regression test.
Security risks
None. Pure transpiler-output change; no untrusted-input parsing beyond what the lexer already does, no new syscalls or FFI.
Level of scrutiny
High. parse_fn runs for every function/method in every file the runtime transpiles or the bundler builds. The change is a small state machine (flag raise/lower + buffer-index snapshotting) interleaved with existing recursive parse calls (parse_type_script_decorators, skip_type_script_type, parse_binding, default-value parse_expr), several of which can themselves clear or append to comments_to_preserve_before. Earlier revisions of this PR had four separate correctness issues in exactly those interactions (legal comments dropped under minify; comments migrating from erased this/decorators onto real args; stale comments_base after a nested .clear(); decorator-body swallowing a pre-captured comment). All were addressed and now have test coverage, but the density of edge cases is why I'm not auto-approving.
Other factors
- Error paths: several
?early-returns can leavepreserve_all_comments_before = trueon the lexer, but a syntax error aborts the parse entirely so the stale flag is not observable. Not flagging as a bug. - Verified every
G::Arg { .. }site in the tree uses..Default::default(), so no site silently drops the new field. - The printer converts
//line comments to/* */and drops the pathological// ... */case rather than emitting broken output — reasonable. - Arrow-function parameter comments are still not preserved (
parse_arrow_bodydoesn't go throughparse_fn); that's fine for the Karma use case and out of scope here. - Test file uses
test.concurrent, hermetictempDirs, drains both pipes, and asserts exit code last.
|
CI status for this diff: Build #87489: 191 jobs passed. Remaining failures are unrelated to this change:
Earlier builds #87281 and #87353 failed with every build job expired (no agents picked them up). The new test Ready for review. |
|
Closing this PR because it has been inactive for more than 90 days. |
|
#47678 replaces this PR with another design: the parser keeps the source text of a parameter list that holds a comment in a side table, and the printer writes that text. It adds no field to |
Problem
Karma fails to start under Bun with:
Karma's
dipackage (and AngularJS's$injector) discover dependency-injection tokens by parsingFunction.prototype.toString()and extracting/* token */comments from the parameter list:Under Node,
fn.toString()returns the source verbatim, sodi's regex sees["config", "config.proxies", "emitter"]. Under Bun the runtime transpiler discards those comments, sodisees["config", "proxies", "emitter"]and there is no provider registered under the bare name"proxies".Fix
js_parser::parse_fnnow enablespreserve_all_comments_beforewhile scanning the(...)list and drains the captured comments into a newG::Arg::leading_commentsfield for each argument.js_printer::print_fn_argsre-emits those comments before each binding (skipped underminify_whitespace). Line comments are converted to block comments so they don't swallow the rest of the single-line argument list; line comments containing*/are dropped rather than emitted incorrectly.G::Argconstruction sites already use..Default::default(), so no call-site churn.Verification
Before:
After, with the debug build, karma starts and runs until killed by
timeout(no DI error).di's annotate parser now produces the same result under Bun as under Node:Regression test added at
test/regression/issue/13451/13451.test.ts; existingtranspiler.test.js,bundler_edgecase,bundler_minify,bundler_cjs2esm,esbuild/defaultand decorator tests all pass locally.Fixes #13451
[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 1
evidence per changed file