Skip to content
Merged
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 Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

62 changes: 62 additions & 0 deletions changelog.d/9445-implicit-this-restore-sweep.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
**Every runtime callback site now restores the caller's `this` through a GC
root** (#9445) — the sweep PR #9444 asked for after fixing four accessor sites
for #9417.

The runtime binds a callback's receiver by writing the GC-rooted
`IMPLICIT_THIS` cell and keeping the previous occupant in a **bare Rust local**
for the duration of the callback:

```rust
let prev = js_implicit_this_set(receiver);
… user code, which allocates …
js_implicit_this_set(prev); // pre-collection address
```

That local is the caller's receiver, and the collector cannot see or rewrite
it. An evacuating young-gen minor inside the window — perry's default GC since
PR #7019 — relocates the caller's object, and the restore reinstalls a retired
from-space address as the caller's `this`. Nothing faults: the caller's next
`this.<field>` fails the object-type check on the recycled cell and answers
`undefined`, so the member access after it throws a TypeError naming a
property nowhere near the defect (#9417's `Cannot read properties of
undefined (reading 'def')`).

The issue counted ~20 sites; a grep of the whole runtime finds **122**
unrooted save/restores in 65 files (one of them landed with #9518 while this
sweep was in flight) (timers, node streams, dgram, cluster,
`fs.watch`, `EventTarget`, `Map`/`Set`/`URLSearchParams.forEach`, promisify,
JSON `toJSON`/replacer/reviver, ToPrimitive and ToPropertyKey, the iterator
protocol, Proxy traps and `Reflect`, bound functions, `super.x`, static
dispatch, …). Every one is now the idiom `prototype_chain.rs` and PR #9444
already use: root the saved value in a `RuntimeHandleScope` and re-read it at
the restore. Nine sites were already rooted; `dyn_eval/bridge.rs` roots
through its own stack. None of the 122 could be left alone — every one calls
user code (a closure, a class accessor or static method, a Proxy trap, a
`then`), which can allocate. Callback loops (`Map`/`Set`/`URLSearchParams.forEach`,
`EventTarget` dispatch, the emitters, watchers and timer batches) root the
displaced receiver **once per loop** rather than once per callback, and sites
that already own a `RuntimeHandleScope` reuse it, so the hot per-callback cost
is one handle read. Three sites also consumed their receiver again *after* the
call (`intl_subclass_super`, `temporal_subclass_super`, and the `process.stdin`
listener loops); those re-read it through a root too.

**Also fixed, same family, found by the fixture:** `JSON.stringify(value,
replacerFn)` handed the walk a **raw replacer closure pointer** after the
root-level replacer call (and the root `toJSON`) had run user code
(`json/replacer.rs`, both the pretty and the compact entry points). With an
allocating replacer this was a SIGSEGV in `js_closure_call2` — the walk called
a retired closure — and it survived the `prev` rooting alone. The closure and
the `""` key are now rooted across those calls.

**Test.** `test-files/test_gap_9445_implicit_this_restore_sweep.ts` — 34
cases, one per synchronously reachable site family, each a `function`-method
on a fresh young object that drives the site with an allocating callback and
then reads `this`. Deterministic, no GC env knobs; the PR description records
which cases print a non-zero `bad=` count on unfixed `main`. Event-loop-driven
sites (timers, `process.stdin`, dgram, cluster, `fs.watch`, pty, child
process) only see a heap `prev` from a nested pump and have no synchronous
reproduction; they carry the same mechanical fix. Two further candidate cases
(a `defineProperty` accessor on a typed array, a `toISOString` override on a
`Date`) diverge from node for a reason unrelated to rooting and are filed as
#9529; a `util.callbackify` case crashed the microtask pump at exit on every
build (a promise-side rooting bug, filed separately) and is not in the file.
15 changes: 15 additions & 0 deletions changelog.d/9451-9468-intl-method-source.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
### Fixed

- **Class methods and accessors now retain their original source text.**
`String(C.prototype.method)`, direct `.toString()`, template coercion, and
reflected getter/setter functions return the source MethodDefinition rather
than a synthesized native-function body. Object-literal accessors also
receive their specified `get name` / `set name` function names. CommonJS
class expressions keep assignment-inferred names without exposing Perry's
internal anonymous-default registration key. Fixes #9468.

- **Default `Intl.DateTimeFormat` dates now use the locale's CLDR numeric
pattern.** The implicit numeric year/month/day field set, its
`formatToParts()` output, and `Date.prototype.toLocaleDateString()` now agree
on locale order, separators, and padding instead of falling back to the
hard-coded US layout. Fixes #9451.
117 changes: 117 additions & 0 deletions crates/perry-codegen/src/codegen/artifact_source_text.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
//! Source-text registration for raw class method and accessor symbols.
//!
//! Class members are not ordinary closure wrappers, so their retained source
//! must be paired with the LLVM body symbol codegen actually emitted. Kept out
//! of `artifacts.rs` so that file remains below the repository's 2,000-line
//! limit.

use std::collections::HashSet;

use perry_hir::types::FuncId;
use perry_hir::Module as HirModule;

use crate::module::LlModule;

use super::helpers::{scoped_method_name, scoped_static_method_name};

pub(super) fn extend_class_method_source_text(
hir: &HirModule,
module_prefix: &str,
llmod: &LlModule,
user_fn_source: &mut Vec<(String, String)>,
) {
// An HIR registry entry is not proof that this module emitted the body: a
// cross-module or typed-only accessor can remain present without a local
// definition. Referencing such a symbol from module initialization makes
// LLVM reject the module, so `has_function` is the final authority.
let mut seen: HashSet<String> = user_fn_source
.iter()
.map(|(symbol, _)| symbol.clone())
.collect();
let mut push_defined = |func_id: FuncId, symbol: String| {
let Some(source) = hir.closure_source_text.get(&func_id) else {
return;
};
if symbol.is_empty() || !llmod.has_function(&symbol) || !seen.insert(symbol.clone()) {
return;
}
user_fn_source.push((symbol, source.clone()));
};

for class in &hir.classes {
if class.id == 0 {
continue;
}
for method in &class.methods {
push_defined(
method.id,
scoped_method_name(module_prefix, &class.name, &method.name),
);
}
for member in class
.computed_members
.iter()
.filter(|member| !member.is_static)
{
push_defined(
member.function.id,
scoped_method_name(module_prefix, &class.name, &member.function.name),
);
}
for (prop, getter) in &class.getters {
let symbol = if class.static_accessor_fn_ids.contains(&getter.id) {
scoped_static_method_name(
module_prefix,
class.id,
&class.name,
&format!("__get_{prop}"),
)
} else {
scoped_method_name(
module_prefix,
&class.name,
&format!("__get_{}", getter.name),
)
};
push_defined(getter.id, symbol);
}
for (prop, setter) in &class.setters {
let symbol = if class.static_accessor_fn_ids.contains(&setter.id) {
scoped_static_method_name(
module_prefix,
class.id,
&class.name,
&format!("__set_{prop}"),
)
} else {
scoped_method_name(
module_prefix,
&class.name,
&format!("__set_{}", setter.name),
)
};
push_defined(setter.id, symbol);
}
for method in &class.static_methods {
push_defined(
method.id,
scoped_static_method_name(module_prefix, class.id, &class.name, &method.name),
);
}
for member in class
.computed_members
.iter()
.filter(|member| member.is_static)
{
push_defined(
member.function.id,
scoped_static_method_name(
module_prefix,
class.id,
&class.name,
&member.function.name,
),
);
}
}
}
10 changes: 10 additions & 0 deletions crates/perry-codegen/src/codegen/artifacts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1854,6 +1854,16 @@ pub(super) fn emit_module_artifacts(c: ModuleArtifactsCtx<'_>) -> Result<()> {
user_fn_source.push((sym, src.clone()));
}

// #9468: method/accessor bodies are raw symbols rather than closure
// wrappers. Pair retained MethodDefinition text only with symbols this
// module actually emitted; the helper also preserves the file-size gate.
super::artifact_source_text::extend_class_method_source_text(
hir,
module_prefix,
llmod,
&mut user_fn_source,
);

// Wall 51: the standalone-ctor arity registered into CLASS_CONSTRUCTORS must
// match the arity of the ctor function actually emitted above (which, for a
// no-own-ctor class with heritage, is the synthesized `super(...args)`
Expand Down
1 change: 1 addition & 0 deletions crates/perry-codegen/src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ mod argument_shape_clone_tests;
pub(crate) mod arguments;
mod artifact_context;
mod artifact_display_names;
mod artifact_source_text;
mod artifacts;
mod boxed_locals;
#[cfg(test)]
Expand Down
20 changes: 19 additions & 1 deletion crates/perry-hir/src/destructuring/var_decl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,25 @@ pub(crate) fn lower_var_decl_with_destructuring(
if let Some(init_ast) = decl.init.as_ref() {
result.extend(predeclare_implicit_assignment_targets(ctx, init_ast));
}
let init = decl.init.as_ref().map(|e| lower_expr(ctx, e)).transpose()?;
// A simple binding performs NamedEvaluation for an anonymous
// function/class initializer (`const C = class {}`). Most class
// expressions take stmt.rs's direct class fast path, but a
// pre-existing class-registry entry can deliberately divert one
// through this generic path (notably the CommonJS factory's
// function-scope pre-registration). Preserve the binding name in
// that path too; the helper filters out named definitions and
// non-NamedEvaluation expressions before installing the context.
let init = decl
.init
.as_ref()
.map(|e| {
crate::lower::expr_assign::lower_rhs_with_assignment_name(
ctx,
e,
Some(name.clone()),
)
})
.transpose()?;
if matches!(ty, Type::Any) {
match &init {
Some(Expr::NativeMethodCall { module, method, .. }) => {
Expand Down
2 changes: 1 addition & 1 deletion crates/perry-hir/src/lower/expr_assign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub(crate) fn rhs_accepts_assignment_name(expr: &ast::Expr) -> bool {
}
}

fn lower_rhs_with_assignment_name(
pub(crate) fn lower_rhs_with_assignment_name(
ctx: &mut LoweringContext,
rhs: &ast::Expr,
name: Option<String>,
Expand Down
13 changes: 13 additions & 0 deletions crates/perry-hir/src/lower/expr_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,19 @@ fn lower_accessor_prop(
};

let func_id = ctx.fresh_func();
// #9468: accessor definitions run SetFunctionName with the `get`/`set`
// prefix. Class accessors already acquire this name when reflected from a
// descriptor; object-literal accessors are ordinary closure values and
// therefore need the same metadata recorded at lowering.
if let MethodKeyKind::Static(key) = &accessor_key {
let prefix = if setter_param.is_some() {
"set "
} else {
"get "
};
ctx.closure_display_names
.insert(func_id, format!("{prefix}{key}"));
}
let outer_locals: Vec<(String, LocalId)> = ctx
.locals
.iter()
Expand Down
22 changes: 22 additions & 0 deletions crates/perry-hir/src/lower_decl/class_decl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,28 @@ pub fn lower_class_decl(
.insert(class_id, class_decl.ident.sym.to_string());
}
capture_class_source(ctx, class_id, &class_decl.class);
// cjs_wrap rewrites a sole `module.exports = class { ... }` into a
// declaration under this reserved key so Perry can hoist and register the
// class. The key is compiler-only: the original class expression had no
// NamedEvaluation context, therefore its observable `.name` is empty and
// its retained source must not expose the injected identifier.
const CJS_ANONYMOUS_DEFAULT: &str = "__perry_cjs_default__";
if class_decl.ident.sym.as_ref() == CJS_ANONYMOUS_DEFAULT {
ctx.class_display_names.insert(class_id, String::new());
if let Some(source) = ctx.class_source_text.get_mut(&class_id) {
if let Some(after_class) = source.strip_prefix("class") {
let trimmed = after_class.trim_start();
if let Some(after_name) = trimmed.strip_prefix(CJS_ANONYMOUS_DEFAULT) {
let name_is_complete = after_name.as_bytes().first().is_none_or(|byte| {
!byte.is_ascii_alphanumeric() && *byte != b'_' && *byte != b'$'
});
if name_is_complete {
*source = format!("class{after_name}");
}
}
}
}
}
if let Some(ast::Expr::Ident(parent)) = class_decl.class.super_class.as_deref() {
if let Some(crate::lower::fn_ctor_env::FnCtorShape::DynCtor(kind)) =
ctx.fn_ctor_env.entries.get(parent.sym.as_ref()).cloned()
Expand Down
45 changes: 43 additions & 2 deletions crates/perry-hir/src/lower_decl/class_members.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,40 @@ use crate::lower_types::*;

use super::*;

/// #9468: retain a class member's MethodDefinition source under the FuncId of
/// its compiled method/accessor body. Unlike ordinary functions, class members
/// are emitted as raw `perry_method_*` / `perry_static_*` symbols, but the
/// source registry can still use the same FuncId-to-source handoff.
///
/// SWC's ClassMethod span includes the class-only `static` modifier. That
/// modifier is not part of the function object's [[SourceText]] (`String(C.m)`
/// starts at the method definition itself), so remove it while preserving the
/// rest byte-for-byte, including `get`/`set`, `async`, `*`, and comments.
fn capture_class_method_source(
ctx: &mut LoweringContext,
func_id: crate::types::FuncId,
method: &ast::ClassMethod,
) {
let Some(mut src) = crate::ir::current_module_source_slice(method.span.lo.0, method.span.hi.0)
else {
return;
};
if method.is_static {
let leading_ws = src.len() - src.trim_start().len();
let candidate = &src[leading_ws..];
if let Some(after_static) = candidate.strip_prefix("static") {
if after_static
.as_bytes()
.first()
.is_some_and(u8::is_ascii_whitespace)
{
src = after_static.trim_start().to_string();
}
}
}
ctx.closure_source_text.insert(func_id, src);
}

pub fn lower_constructor(
ctx: &mut LoweringContext,
class_name: &str,
Expand Down Expand Up @@ -670,6 +704,7 @@ pub fn lower_class_method_with_name(
ctx.exit_type_param_scope();

let func_id = ctx.fresh_func();
capture_class_method_source(ctx, func_id, method);
// Record the param-prologue length for generator methods so the generator
// transform runs param binding (default guards + destructuring) synchronously
// at call time per spec FunctionDeclarationInstantiation order. Without this,
Expand Down Expand Up @@ -799,8 +834,11 @@ pub fn lower_getter_method_with_name(
ctx.exit_scope(scope_mark);
ctx.in_nonarrow_fn = saved_in_nonarrow_fn;

let func_id = ctx.fresh_func();
capture_class_method_source(ctx, func_id, method);

Ok(Function {
id: ctx.fresh_func(),
id: func_id,
name,
type_params: Vec::new(),
params: Vec::new(),
Expand Down Expand Up @@ -967,8 +1005,11 @@ pub fn lower_setter_method_with_name(
ctx.exit_scope(scope_mark);
ctx.in_nonarrow_fn = saved_in_nonarrow_fn;

let func_id = ctx.fresh_func();
capture_class_method_source(ctx, func_id, method);

Ok(Function {
id: ctx.fresh_func(),
id: func_id,
name,
type_params: Vec::new(),
params,
Expand Down
Loading
Loading