diff --git a/src/ast/g.rs b/src/ast/g.rs index 92cd31a4712b..b4e5907308fb 100644 --- a/src/ast/g.rs +++ b/src/ast/g.rs @@ -337,6 +337,9 @@ pub struct Arg { pub is_typescript_ctor_field: bool, pub ts_metadata: TypeScript::Metadata, + + // `function(/* token */ arg)` — kept for Function.prototype.toString() + pub leading_comments: StoreSlice, } impl Default for Arg { @@ -347,6 +350,7 @@ impl Default for Arg { default: None, is_typescript_ctor_field: false, ts_metadata: TypeScript::Metadata::MNone, + leading_comments: StoreSlice::EMPTY, } } } @@ -368,6 +372,7 @@ impl Arg { }, is_typescript_ctor_field: self.is_typescript_ctor_field, ts_metadata: self.ts_metadata.clone(), + leading_comments: self.leading_comments, }) } } diff --git a/src/js_parser/parse/parse_fn.rs b/src/js_parser/parse/parse_fn.rs index 6a58ff9ae02c..1b7bfec6be30 100644 --- a/src/js_parser/parse/parse_fn.rs +++ b/src/js_parser/parse/parse_fn.rs @@ -14,6 +14,11 @@ use bun_ast::{E, Expr, Flags, G, S, Stmt}; type Error = crate::Error; +#[inline] +fn is_legal_comment(text: &[u8]) -> bool { + text.len() > 2 && text[2] == b'!' +} + impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_ONLY> { /// This assumes the "function" token has already been parsed pub(crate) fn parse_fn_stmt( @@ -160,6 +165,34 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O Ok(p.s(S::Function { func }, loc)) } + fn drain_arg_leading_comments_into( + &mut self, + out: &mut bun_alloc::ArenaVec<'a, G::Comment>, + base: usize, + ) { + let buf = &mut self.lexer.comments_to_preserve_before; + let mut i = base.min(buf.len()); + while i < buf.len() { + if is_legal_comment(buf[i].text.slice()) { + i += 1; + } else { + out.push(buf.remove(i)); + } + } + } + + fn discard_non_legal_comments_from(&mut self, base: usize) { + let buf = &mut self.lexer.comments_to_preserve_before; + let mut i = base.min(buf.len()); + while i < buf.len() { + if is_legal_comment(buf[i].text.slice()) { + i += 1; + } else { + buf.remove(i); + } + } + } + pub(crate) fn parse_fn( &mut self, name: Option, @@ -185,6 +218,11 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O open_parens_loc: p.lexer.loc(), ..Default::default() }; + + // Keep `/* ... */` inside `(...)` for Function.prototype.toString(). + let old_preserve_comments = p.lexer.preserve_all_comments_before; + let mut comments_base = p.lexer.comments_to_preserve_before.len(); + p.lexer.preserve_all_comments_before = true; p.lexer.expect(T::TOpenParen)?; // Await and yield are not allowed in function arguments @@ -216,6 +254,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O while p.lexer.token != T::TCloseParen { // Skip over "this" type annotations if Self::IS_TYPESCRIPT_ENABLED && p.lexer.token == T::TThis { + p.discard_non_legal_comments_from(comments_base); + p.lexer.preserve_all_comments_before = old_preserve_comments; p.lexer.next()?; if p.lexer.token == T::TColon { p.lexer.next()?; @@ -225,13 +265,21 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O break; } + comments_base = p.lexer.comments_to_preserve_before.len(); + p.lexer.preserve_all_comments_before = true; p.lexer.next()?; continue; } + let mut leading = bun_alloc::ArenaVec::::new_in(p.arena); + let mut ts_decorators = bun_alloc::AstAlloc::vec(); if opts.allow_ts_decorators { + p.drain_arg_leading_comments_into(&mut leading, comments_base); + p.lexer.preserve_all_comments_before = old_preserve_comments; ts_decorators = p.parse_type_script_decorators()?; + comments_base = p.lexer.comments_to_preserve_before.len(); + p.lexer.preserve_all_comments_before = true; if ts_decorators.len_u32() > 0 { arg_has_decorators = true; } @@ -244,6 +292,14 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O func.flags.insert(Flags::Function::HasRestArg); } + p.drain_arg_leading_comments_into(&mut leading, comments_base); + let leading_comments = if leading.is_empty() { + bun_ast::StoreSlice::EMPTY + } else { + bun_ast::StoreSlice::from_bump(leading) + }; + p.lexer.preserve_all_comments_before = old_preserve_comments; + let mut is_typescript_ctor_field = false; let is_identifier = p.lexer.token == T::TIdentifier; let mut text = p.lexer.identifier; @@ -325,6 +381,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O // We need to track this because it affects code generation is_typescript_ctor_field, ts_metadata, + leading_comments, }); if p.lexer.token != T::TComma { @@ -343,9 +400,14 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O break; } + comments_base = p.lexer.comments_to_preserve_before.len(); + p.lexer.preserve_all_comments_before = true; p.lexer.next()?; rest_arg = false; } + p.lexer.preserve_all_comments_before = old_preserve_comments; + p.discard_non_legal_comments_from(comments_base); + if !args.is_empty() { func.args = bun_ast::StoreSlice::new_mut(args.into_bump_slice_mut()); } diff --git a/src/js_printer/lib.rs b/src/js_printer/lib.rs index ea6bc77a3ae1..b1aa34d5434c 100644 --- a/src/js_printer/lib.rs +++ b/src/js_printer/lib.rs @@ -2108,6 +2108,23 @@ pub(crate) mod __gated_printer { self.print_space(); } + if !self.options.minify_whitespace { + for comment in arg.leading_comments.slice() { + let text = comment.text.slice(); + if text.starts_with(b"//") { + let body = &text[2..]; + if !strings::contains(body, b"*/") { + self.print(b"/*"); + self.print(body); + self.print(b" */ "); + } + } else { + self.print(text); + self.print(b" "); + } + } + } + if has_rest_arg && i + 1 == args.len() { self.print(b"..."); } diff --git a/test/regression/issue/13451/13451.test.ts b/test/regression/issue/13451/13451.test.ts new file mode 100644 index 000000000000..7b2decf5df3c --- /dev/null +++ b/test/regression/issue/13451/13451.test.ts @@ -0,0 +1,175 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +// https://github.com/oven-sh/bun/issues/13451 +// +// Karma's `di` package (and AngularJS $injector) parse Function.prototype.toString() +// to extract DI tokens from /* comment */-annotated parameter names, e.g. +// +// exports.create = function (/* config */ config, /* config.proxies */ proxies) { ... } +// +// Bun's runtime transpiler was discarding those comments, so `di` saw `proxies` +// instead of `config.proxies` and failed with "No provider for "proxies"!". + +const FN_ARGS = /^function\s*[^\(]*\(\s*([^\)]*)\)/m; +const FN_ARG = /\/\*([^\*]*)\*\//m; + +function diParse(src: string): string[] { + const match = src.match(FN_ARGS); + if (!match || !match[1]) return []; + return match[1].split(",").map(arg => { + const m = arg.match(FN_ARG); + return m ? m[1].trim() : arg.trim(); + }); +} + +test.concurrent("inline /* */ comments in function parameter lists survive the runtime transpiler", async () => { + using dir = tempDir("issue-13451", { + "mod.cjs": ` +exports.anonExpr = function (/* a */ x, /* b.c */ y, /* d */ z) { + return [x, y, z]; +}; + +function decl(/* one */ p, /* two */ q) { + return [p, q]; +} +exports.decl = decl; + +exports.obj = { + method(/* inner */ v) { + return v; + }, +}; + +exports.karma = function (/* config */ config, /* config.proxies */ proxies, /* emitter */ emitter) { + return [config, proxies, emitter]; +}; + +exports.lineComment = function ( + // token + value +) { + return value; +}; + +exports.noMigrateDefault = function (a = /* x */ 1, b) { + return [a, b]; +}; + +exports.noMigrateTrailing = function (a /* trail */, b) { + return [a, b]; +}; + +exports.noLeakIntoBody = function (a = () => { return 1 /* mid */ + 2; }, b) { + return [a, b]; +}; +`, + "main.cjs": ` +const m = require("./mod.cjs"); +console.log(JSON.stringify({ + anonExpr: m.anonExpr.toString(), + decl: m.decl.toString(), + method: m.obj.method.toString(), + karma: m.karma.toString(), + lineComment: m.lineComment.toString(), + noMigrateDefault: m.noMigrateDefault.toString(), + noMigrateTrailing: m.noMigrateTrailing.toString(), + noLeakIntoBody: m.noLeakIntoBody.toString(), +})); +`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.cjs"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + + const results = JSON.parse(stdout); + + // anonymous function expression + expect(results.anonExpr).toContain("/* a */"); + expect(results.anonExpr).toContain("/* b.c */"); + expect(results.anonExpr).toContain("/* d */"); + expect(diParse(results.anonExpr)).toEqual(["a", "b.c", "d"]); + + // named function declaration + expect(results.decl).toContain("/* one */"); + expect(results.decl).toContain("/* two */"); + expect(diParse(results.decl)).toEqual(["one", "two"]); + + // object method shorthand + expect(results.method).toContain("/* inner */"); + + // karma's exact pattern from middleware/proxy.js + expect(results.karma).toContain("/* config */"); + expect(results.karma).toContain("/* config.proxies */"); + expect(results.karma).toContain("/* emitter */"); + expect(diParse(results.karma)).toEqual(["config", "config.proxies", "emitter"]); + + // line comment converted to block (otherwise it would swallow the arg list) + expect(results.lineComment).not.toMatch(/\/\/[^\n]*\)/); + expect(results.lineComment).toContain("token"); + + // Comments inside a default value / after a binding must not migrate to the + // next argument (that would change the DI token), nor surface as statements + // inside a nested body. + expect(results.noMigrateDefault).not.toContain("/* x */"); + expect(diParse(results.noMigrateDefault)).toEqual(["a = 1", "b"]); + expect(results.noMigrateTrailing).not.toContain("/* trail */"); + expect(diParse(results.noMigrateTrailing)).toEqual(["a", "b"]); + expect(results.noLeakIntoBody).not.toContain("/* mid */"); + + expect(exitCode).toBe(0); +}); + +test.concurrent("comments inside erased TS this-params/decorators are not relocated onto real args", async () => { + using dir = tempDir("issue-13451-ts", { + "in.ts": ` +export function f1(this: /* Self */ number, x: number) { return x; } +export function f2(/* ctx */ this: number, x: number) { return x; } +declare const dec: any; +export class C { + constructor(@dec(/* opts */) x: number) {} +} +`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "build", "--target=bun", "in.ts"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).not.toContain("/* Self */"); + expect(stdout).not.toContain("/* ctx */"); + expect(stdout).not.toContain("/* opts */"); + expect(exitCode).toBe(0); +}); + +test.concurrent("legal comments in parameter lists survive minification", async () => { + using dir = tempDir("issue-13451-legal", { + "in.js": ` +export const f = function(/*! @license MIT */ x) { return x; }; +export const g = function(/*! @preserve */) { return 1; }; +`, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "build", "--minify", "in.js"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout).toContain("/*! @license MIT */"); + expect(stdout).toContain("/*! @preserve */"); + expect(exitCode).toBe(0); +});