Skip to content
Closed
5 changes: 5 additions & 0 deletions src/ast/g.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Comment>,
}

impl Default for Arg {
Expand All @@ -347,6 +350,7 @@ impl Default for Arg {
default: None,
is_typescript_ctor_field: false,
ts_metadata: TypeScript::Metadata::MNone,
leading_comments: StoreSlice::EMPTY,
}
}
}
Expand All @@ -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,
})
}
}
62 changes: 62 additions & 0 deletions src/js_parser/parse/parse_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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<js_ast::LocRef>,
Expand All @@ -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
Expand Down Expand Up @@ -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()?;
Expand All @@ -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::<G::Comment>::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;
}
Comment thread
robobun marked this conversation as resolved.
Resolved
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -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());
}
Expand Down
17 changes: 17 additions & 0 deletions src/js_printer/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2108,6 +2108,23 @@ pub(crate) mod __gated_printer {
self.print_space();
}

if !self.options.minify_whitespace {
Comment thread
robobun marked this conversation as resolved.
Resolved
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"...");
}
Expand Down
175 changes: 175 additions & 0 deletions test/regression/issue/13451/13451.test.ts
Original file line number Diff line number Diff line change
@@ -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("inline /* */ comments in function parameter lists survive the runtime transpiler", async () => {
Comment thread
robobun marked this conversation as resolved.
Resolved
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("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("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);
});
Loading