Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/aot/lower/transpiled.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ impl RuntimeTranspiler<'_> {
options.tree_shaking = true;
options.warn_about_unbundled_modules = false;
options.use_define_for_class_fields = !request.assigns_class_fields;
options.keep_parameter_lists_as_written = true;
let features = &mut options.features;
features.standard_decorators = !request.has_legacy_decorators;
features.allow_runtime = true;
Expand Down
12 changes: 10 additions & 2 deletions src/ast/ast_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use bun_collections::{ArrayHashMap, StringArrayHashMap, StringHashMap};

use crate::runtime;
use crate::{
CharFreq, ExportsKind, Expr, InlinedEnumValue, LocRef, NamedExport, NamedImport, Part, Range,
Ref, Scope, SlotCounts, StoreStr, Target,
CharFreq, ExportsKind, Expr, InlinedEnumValue, LocRef, NamedExport, NamedImport,
ParameterListsAsWritten, Part, Range, Ref, Scope, SlotCounts, StoreStr, Target,
};

use crate::part::List as PartList;
Expand Down Expand Up @@ -104,6 +104,8 @@ pub struct Ast<'a> {
pub import_meta_ref: Ref,
/// Output depends on more than the source text (macros, `import.meta.glob`), so don't cache it.
pub depends_on_more_than_source: bool,
/// Filled for a module the runtime loads.
pub parameter_lists_as_written: ParameterListsAsWritten,
}

// `parts`/`symbols`/`import_records` are now `ArenaVec`s and need an allocator,
Expand Down Expand Up @@ -150,8 +152,14 @@ impl<'a> Ast<'a> {
has_import_meta: false,
import_meta_ref: Ref::NONE,
depends_on_more_than_source: false,
parameter_lists_as_written: Default::default(),
}
}

/// What the printer takes: `None` when no list is kept, so that it tests one pointer for each function.
pub fn kept_parameter_lists(&self) -> Option<&ParameterListsAsWritten> {
(!self.parameter_lists_as_written.is_empty()).then_some(&self.parameter_lists_as_written)
}
}

pub struct CommonJSNamedExport {
Expand Down
2 changes: 2 additions & 0 deletions src/ast/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2623,6 +2623,7 @@ pub mod runtime;
pub mod ast_result;
pub mod import_record;
pub mod loader;
pub mod parameter_list_as_written;
pub mod target;

pub use ast_result::{
Expand All @@ -2631,6 +2632,7 @@ pub use ast_result::{
};
pub use import_record::{Flags as ImportRecordFlags, ImportRecord, Tag as ImportRecordTag};
pub use loader::{Loader, LoaderHashTable, SideEffects};
pub use parameter_list_as_written::ParameterListsAsWritten;
pub use target::Target;
pub mod transpiler_cache;
// Glob re-export: `link_interface!` emits `#[doc(hidden)]` type aliases that
Expand Down
282 changes: 282 additions & 0 deletions src/ast/parameter_list_as_written.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,282 @@
//! The runtime prints a parameter list that holds a comment as it is written: an injector reads `/* token */ name` from `fn.toString()`.

use bun_alloc::AstAlloc;
use bun_collections::ArrayHashMap;
use bun_collections::array_hash_map::AutoContext;
use bun_core::strings;

use crate::StoreStr;

/// The key is where the printer finds the list again: the `(` of a function, the start of an arrow function.
pub type ParameterListsAsWritten = ArrayHashMap<i32, StoreStr, AutoContext, AstAlloc>;

/// `mentionsOriginalModule` (src/jsc/bindings/BunPlugin.cpp) takes each of these, anywhere in the text of a `mock.module` factory, as a mention of the original module.
const READ_BY_MOCK_MODULE: [&[u8]; 3] = [b"importOriginal", b"importActual", b"requireActual"];

/// The text up to the `)` of the list that starts at `after_open_paren`, if the printer is to write it: plain names and a comment.
pub fn text_to_keep(after_open_paren: &[u8]) -> Option<&[u8]> {
let mut names = Names::new(after_open_paren);
while names.next_name().ok()?.is_some() {}
let (text, rest) = after_open_paren.split_at(names.position);
let is_kept = names.has_comment
&& rest.first() == Some(&b')')
&& !READ_BY_MOCK_MODULE
.iter()
.any(|word| strings::contains(text, word));
is_kept.then_some(text)
}

pub struct Name {
pub start: usize,
pub end: usize,
/// `...` stands before it.
pub is_rest: bool,
}

/// The text is not a list of names with a comma between them, `...` before the last at most, ASCII whitespace and ASCII comments.
#[derive(Debug)]
pub struct NotPlainNames;

/// The names in the text between the parentheses of a parameter list.
pub struct Names<'a> {
text: &'a [u8],
position: usize,
has_comment: bool,
/// A name was passed, and no comma after it.
is_after_name: bool,
/// That name has `...` before it: nothing but whitespace and comments follows it.
is_after_rest: bool,
}

impl<'a> Names<'a> {
pub fn new(text: &'a [u8]) -> Self {
Names {
text,
position: 0,
has_comment: false,
is_after_name: false,
is_after_rest: false,
}
}

/// `Ok(None)` at the end of the list: the end of the text, or a `)`.
pub fn next_name(&mut self) -> Result<Option<Name>, NotPlainNames> {
let text = self.text;
let mut at = self.position;
let mut is_rest = false;
let found = loop {
match text.get(at).copied() {
Some(b' ' | b'\t' | b'\n' | b'\r' | 0x0B | 0x0C) => at += 1,
Some(b'/') => match end_of_comment(text, at) {
Some(end) => {
at = end;
self.has_comment = true;
}
None => break Err(NotPlainNames),
},
None | Some(b')') if !is_rest => break Ok(None),
Some(b',') if self.is_after_name && !self.is_after_rest => {
self.is_after_name = false;
at += 1;
}
Some(b'.') if !self.is_after_name && !is_rest && text[at..].starts_with(b"...") => {
is_rest = true;
at += 3;
}
Some(b'a'..=b'z' | b'A'..=b'Z' | b'_' | b'$') if !self.is_after_name => {
let start = at;
while text.get(at).is_some_and(|&byte| is_name_byte(byte)) {
at += 1;
}
self.is_after_name = true;
self.is_after_rest = is_rest;
break Ok(Some(Name {
start,
end: at,
is_rest,
}));
}
_ => break Err(NotPlainNames),
}
};
self.position = at;
found
}
}

fn is_name_byte(byte: u8) -> bool {
byte.is_ascii_alphanumeric() || byte == b'_' || byte == b'$'
}

/// `text[at]` is `/`. The end of the ASCII comment that starts there, if it ends inside the text: a line comment ends before its line terminator.
fn end_of_comment(text: &[u8], at: usize) -> Option<usize> {
let body = text.get(at + 2..)?;
let length = match (text[at + 1], body.first()) {
// The printer writes a comment that starts with `!` by itself, into the body of the function.
(_, Some(b'!')) => return None,
(b'*', _) => strings::index_of(body, b"*/")? + 2,
// JavaScriptCore reads `sourceURL=` and `sourceMappingURL=` from a comment that starts with `//#` or `//@`.
(b'/', Some(b'#' | b'@')) => return None,
(b'/', _) => strings::index_of_any(body, b"\r\n")?,
_ => return None,
};
body[..length].is_ascii().then_some(at + 2 + length)
}

#[cfg(test)]
mod tests {
use super::*;

/// `None`: not a list of plain names.
fn names_of(text: &str) -> Option<(Vec<String>, bool)> {
let mut names = Names::new(text.as_bytes());
let mut found = Vec::new();
while let Some(name) = names.next_name().ok()? {
let spelling = &text[name.start..name.end];
found.push(if name.is_rest {
format!("...{spelling}")
} else {
spelling.to_owned()
});
}
Some((found, names.has_comment))
}

fn commented(names: &[&str]) -> Option<(Vec<String>, bool)> {
Some((names.iter().map(|&name| name.to_owned()).collect(), true))
}

fn kept(after_open_paren: &str) -> Option<&str> {
text_to_keep(after_open_paren.as_bytes()).map(|text| core::str::from_utf8(text).unwrap())
}

#[test]
fn names_and_comments() {
assert_eq!(
names_of("/* config */config, /* config.proxies */proxies"),
commented(&["config", "proxies"])
);
assert_eq!(
names_of("a /* after */, $b_1 // line\n"),
commented(&["a", "$b_1"])
);
assert_eq!(names_of("/* a, b) */"), commented(&[]));
assert_eq!(
names_of(" a,\r\n\tb, "),
Some((vec!["a".into(), "b".into()], false))
);
assert_eq!(names_of(""), Some((vec![], false)));
}

#[test]
fn rest_parameter() {
assert_eq!(
names_of("a, /* all */ ...rest"),
commented(&["a", "...rest"])
);
assert_eq!(names_of("... /* all */ rest"), commented(&["...rest"]));
assert_eq!(names_of("a, ..."), None);
assert_eq!(names_of("......a"), None);
assert_eq!(names_of("..a"), None);
assert_eq!(names_of("...,a"), None);
}

#[test]
fn what_is_not_a_plain_name() {
for text in [
"a = 1",
"{ a }",
"[a]",
"a: number",
"a?",
"@inject a",
"a /* \u{e9} */",
"a // \u{e9}\n",
"\u{e9}",
"a\\u0062",
"a <!-- b\n",
"a / b",
"1",
"a b",
",a",
"a,,b",
"a ...b",
"...a, b",
"...a,",
] {
assert_eq!(names_of(text), None, "{text}");
}
}

#[test]
fn a_comment_ends_inside_the_text() {
assert_eq!(names_of("a /* open"), None);
assert_eq!(names_of("a // no line terminator"), None);
assert_eq!(names_of("a /"), None);
assert_eq!(names_of("a // ends\r"), commented(&["a"]));
assert_eq!(names_of("/**/"), commented(&[]));
assert_eq!(names_of("/*/ a */ b"), commented(&["b"]));
}

#[test]
fn a_line_comment_that_can_hold_a_directive_is_not_passed() {
assert_eq!(names_of("a, //# sourceURL=x.js\n b"), None);
assert_eq!(names_of("a, //@ sourceMappingURL=x\n b"), None);
assert_eq!(names_of("a, //#region\n b"), None);
assert_eq!(
names_of("a, // # sourceURL=x.js\n b"),
commented(&["a", "b"])
);
assert_eq!(names_of("/*# sourceURL=x.js */ a"), commented(&["a"]));
}

#[test]
fn a_comment_that_the_printer_writes_by_itself_is_not_passed() {
assert_eq!(names_of("/*! legal */ a"), None);
assert_eq!(names_of("a, //! legal\n b"), None);
assert_eq!(names_of("/* ! */ a"), commented(&["a"]));
}

#[test]
fn a_keyword_is_a_name_here() {
// The parser leaves a TypeScript `this` parameter out of the arguments, so the printer finds another name.
assert_eq!(names_of("this, /* c */ a"), commented(&["this", "a"]));
assert_eq!(names_of("public /* c */ a"), None);
}

#[test]
fn the_list_ends_at_a_close_paren() {
assert_eq!(names_of("a) { b }"), Some((vec!["a".into()], false)));
assert_eq!(names_of("a, /* c */ b,) => a"), commented(&["a", "b"]));
assert_eq!(names_of("a, ...) {}"), None);
}

#[test]
fn the_text_to_keep_ends_before_the_close_paren() {
assert_eq!(kept("a, /* c */ b) { return a; }"), Some("a, /* c */ b"));
assert_eq!(kept("/* none */) => 1"), Some("/* none */"));
assert_eq!(kept("a, // c\n b,\n) {}"), Some("a, // c\n b,\n"));
}

#[test]
fn no_text_is_kept() {
for after_open_paren in [
"a, b) /* c */ => a",
"/* c */ a = 1) {}",
"/* c */ a: string) {}",
"/* c */ a, { b }) {}",
"/* c */ a",
"/* c */",
"/* c */ ...) {}",
"/* c */ a b) {}",
"/* c */ ,a) {}",
"/* c */ ...a, b) {}",
"/*! legal */ a) {}",
"/* importOriginal */ load) => load()",
"load /* see vi.importActual */) => load()",
"/* c */ requireActual) => 1",
] {
assert_eq!(kept(after_open_paren), None, "{after_open_paren}");
}
}
}
1 change: 1 addition & 0 deletions src/bundler/aot/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ impl SourceProvider for BundlerServices {
ExportsKind::Cjs => crate::options::Format::Cjs,
_ => crate::options::Format::Esm,
},
parameter_lists_as_written: tree.kept_parameter_lists(),
..Default::default()
};
let printed = js_printer::print::<false>(
Expand Down
3 changes: 3 additions & 0 deletions src/bundler/transpiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,7 @@ impl<'a> Transpiler<'a> {
output_format: p_opts::Format::Esm,
transform_only: self.options.transform_only,
import_meta_glob: None,
keep_parameter_lists_as_written: false,
Comment thread
robobun marked this conversation as resolved.
Resolved
import_meta_main_value: None,
lower_import_meta_main_for_node_js: false,
framework: None,
Expand Down Expand Up @@ -1630,6 +1631,7 @@ impl<'a> Transpiler<'a> {
// SAFETY: the resolver outlives the parse, which only calls it from this thread.
opts.import_meta_glob =
Some(unsafe { Resolver::import_meta_glob_host(&raw mut self.resolver) });
opts.keep_parameter_lists_as_written = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Elysia 1.4.x apps that write a comment inside a handler's parameter list can answer 200 where a guard set 401 after this merges, which the base does not do. Once the runtime enables the kept lists at src/bundler/transpiler.rs:1634, fn.toString() of (/* c */ ctx) => ... carries the comment and Elysia's source analysis loses the context, so set.status writes are dropped; the PR text lists this as a downside. This follows from the stated purpose (matching Node). Fix: ship a general safeguard with the feature rather than nothing: an opt-out (env feature flag or runtime option) that restores the base printing and a release note naming the affected Elysia versions, so an auth guard does not silently stop working. [also at: src/js_printer/lib.rs:2528 - Elysia 1.4.x users (npm latest) whose route handler has a comment in a plain parameter list, e.g. (/* c */ ctx) => ..., get a 200 where the handler set 401 after merging; on the base branch the handler works.]

Why this was flagged

An Elysia 1.4.30 app (npm latest per the PR description) defines a handler or guard as (/* c */ ctx) => { ctx.set.status = 401 } and loads it through bun run, which sets keep_parameter_lists_as_written at src/bundler/transpiler.rs:1634. The printer then emits the comment inside the parentheses (src/js_printer/lib.rs:2523-2528 and print_parameter_list_as_written), so String(handler) holds /* c */. Elysia's sucrose reads toString() to decide whether a handler touches the context; with the comment it no longer finds ctx, and the response is 200 instead of the 401 the guard set. On the base branch the comment is dropped (src/js_parser/lexer.rs:1917) and the same app answers 401. The PR description itself records '200 where it set 401' for elysia 1.4.30 and says 2.0.0-rc.1 is fine; no opt-out, flag or note is added by the diff, so an operator upgrading Bun gets the auth change silently.

Verification: normal — acknowledged in diff: the PR description's "Downsides" section states exactly this ("elysia 1.4.30 ... loses the context of a handler (/* c */ ctx) => ...: 200 where it set 401"); 2.0.0-rc.1 is unaffected. conflicts with stated purpose: the only complete fix (not printing the comment) undoes the Node-parity the PR exists for, so this is a trade-off, not an oversight.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The measurement is right, and the description states it. The PR does not choose for the maintainers, so I leave this thread open for them.

Facts, from runs on builds of main and of this branch:

  • elysia 1.4.30 (npm latest): a handler or hook written (/* c */ ctx) => ... loses its inferred context. The table is in the description (Notes, "Who sees another toString() text").
  • Node 26.3 gives the same result as this branch in every row of that table. Main differs from Node because it drops the comment.
  • elysia 2.0.0-rc.1 is right on main, on this branch and on Node.
  • On 1.4.30 these keep the result of main: a typed or destructured parameter, a comment before the parentheses, new Elysia({ aot: false }).

Two rules of .claude/docs/landing-prs.md point in different directions here: "Never silently break existing users" (a changed default goes behind a flag) and "Ship no speculative surface: no options nobody asked for". The choices I see:

  1. Ship it as it is, with a release note that names elysia 1.4.x.
  2. Add an opt-out. That is an environment flag, the flag in the cache key of the runtime transpiler (the cache holds the printed text), a test and a line in the docs.
  3. Hold the PR until elysia 1.4.x has a release that reads the parameter name past a comment.

I added no flag without a maintainer's word on it.

}

// spec calls `transpiler.resolver.caches.js.parse`.
Expand Down Expand Up @@ -2483,6 +2485,7 @@ impl<'a> Transpiler<'a> {
module_info,
hmr_ref: ast.wrapper_ref,
mangled_props: None,
parameter_lists_as_written: ast.kept_parameter_lists(),
// The printer reads `opts.target` at
// js_printer/lib.rs:6872 to gate the `var {require}=import.meta;`
// hoist on `Target::Bun` — defaulting to `Browser` here regressed
Expand Down
Loading
Loading