From 6ffd43fea1e5c62dd782e8177a393c5533cb5b84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 18 Sep 2026 11:33:37 +0000 Subject: [PATCH 1/2] fix(hir): capture class member enclosing vars by reference, not snapshot Class members nested in a function or CommonJS module body captured enclosing var/let bindings by value snapshot instead of by reference: a method saw the value the var had at class-declaration time, and writes from constructors/static methods to a captured var were silently lost. The desugar_shared_mutable_captures pass (#5951's box-sharing machinery) decided whether a captured id was safe to box by counting Let declarations per LocalId and requiring exactly one; a var is declared twice in HIR (a body-entry predefine slot, then the declaration statement itself), so every var capture was rejected as ambiguous and fell back to the stale value-snapshot path. Two functions that each declared a same-named var plus a same-named class could also collide. Replace the declaration counter with a DeclCensus/CensusWalker that walks a region in execution order and asks whether an id denotes ONE binding (a single declaration, or several redeclarations in the same closure scope under the same name where the first dominates) rather than requiring literally one declaration. Redeclarations of an already-captured id are demoted so the box, not a fresh local, is written. Captured cells now propagate to nested classes explicitly. Fixes #10485 Fixes #10489 --- .../perry-hir/src/destructuring/var_decl.rs | 32 + crates/perry-hir/src/lower/context.rs | 2 +- crates/perry-hir/src/lower/expr_assign.rs | 7 +- crates/perry-hir/src/lower/expr_member.rs | 7 +- crates/perry-hir/src/lower/expr_new.rs | 24 +- .../src/lower/lower_expr/arm_class.rs | 1 + .../perry-hir/src/lower/lowering_context.rs | 61 +- .../src/lower/shared_mutable_capture.rs | 696 +++++++++++++++--- crates/perry-hir/src/lower/stmt.rs | 2 + crates/perry-hir/src/lower/tests.rs | 1 + .../lower/tests/class_member_var_captures.rs | 213 ++++++ .../class_member_var_captures_10485.cjs | 27 + ...est_gap_10485_class_member_var_captures.ts | 179 +++++ 13 files changed, 1148 insertions(+), 104 deletions(-) create mode 100644 crates/perry-hir/src/lower/tests/class_member_var_captures.rs create mode 100644 test-files/_helpers/class_member_var_captures_10485.cjs create mode 100644 test-files/test_gap_10485_class_member_var_captures.ts diff --git a/crates/perry-hir/src/destructuring/var_decl.rs b/crates/perry-hir/src/destructuring/var_decl.rs index 920a424882..21a52f018e 100644 --- a/crates/perry-hir/src/destructuring/var_decl.rs +++ b/crates/perry-hir/src/destructuring/var_decl.rs @@ -257,6 +257,19 @@ pub(crate) fn lower_var_decl_with_destructuring( // Alias / prototype / static-method tracking for the freshly- // bound identifier (extracted to `alias_tracking`). track_decl_aliases(ctx, decl, &name, id, &init); + // #10489: remember WHICH class this binding holds, so a `new ()` + // resolves to it even when another binding claimed the same name. + if let Some(key) = init.as_ref().and_then(class_expr_value_key) { + if decl + .init + .as_deref() + .is_some_and(crate::lower::expr_assign::rhs_accepts_assignment_name) + && ctx.inferred_class_bindings.contains(&name) + { + ctx.inferred_class_bindings + .record_binding(id, key.to_string()); + } + } // `with (o) { var foo = v; }` — the binding `foo` is hoisted to // the enclosing var scope, but the *initialisation* is a normal // PutValue under the with environment: when `o` has a `foo` @@ -528,3 +541,22 @@ pub(crate) fn lower_var_decl_with_destructuring( Ok(result) } + +/// The registration key of the class a lowered class-expression initializer +/// yields: a bare `ClassRef`/`ClassExprFresh`, or the tail of the sequence +/// `lower_class_expr` wraps it in (parent registration, computed names, the +/// #6654 capture-owner `LocalSet(owner, fresh), LocalGet(owner)` pair). +fn class_expr_value_key(expr: &Expr) -> Option<&str> { + match expr { + Expr::ClassRef(key) => Some(key), + Expr::ClassExprFresh { template, .. } => Some(template), + Expr::Sequence(items) => match items.as_slice() { + [.., Expr::LocalSet(owner, value), Expr::LocalGet(read)] if owner == read => { + class_expr_value_key(value) + } + [.., last] => class_expr_value_key(last), + [] => None, + }, + _ => None, + } +} diff --git a/crates/perry-hir/src/lower/context.rs b/crates/perry-hir/src/lower/context.rs index 3d4e00a380..6d1da1665b 100644 --- a/crates/perry-hir/src/lower/context.rs +++ b/crates/perry-hir/src/lower/context.rs @@ -127,7 +127,7 @@ impl LoweringContext { class_display_names: HashMap::new(), gen_param_prologue_len: HashMap::new(), assignment_inferred_name: None, - inferred_class_bindings: std::collections::HashSet::new(), + inferred_class_bindings: Default::default(), closure_source_text: HashMap::new(), class_source_text: HashMap::new(), func_return_native_instances: Vec::new(), diff --git a/crates/perry-hir/src/lower/expr_assign.rs b/crates/perry-hir/src/lower/expr_assign.rs index a947b6cdf9..83bda62ad3 100644 --- a/crates/perry-hir/src/lower/expr_assign.rs +++ b/crates/perry-hir/src/lower/expr_assign.rs @@ -829,9 +829,10 @@ fn lower_assignment_target( && ctx.lookup_func(&cls_name).is_none() { None - } else if ctx.lookup_local(&cls_name).is_some() - && !ctx.inferred_class_bindings.contains(cls_name.as_str()) - { + } else if ctx.lookup_local(&cls_name).is_some_and(|local| { + ctx.inferred_class_bindings.class_key_for(local, &cls_name) + != Some(cls_name.as_str()) + }) { // A lexical local shadows any same-named // module-scope class for this write too // (wall 7's disease, 4th surface): the diff --git a/crates/perry-hir/src/lower/expr_member.rs b/crates/perry-hir/src/lower/expr_member.rs index d6f5b3f391..08bc5cfc1b 100644 --- a/crates/perry-hir/src/lower/expr_member.rs +++ b/crates/perry-hir/src/lower/expr_member.rs @@ -1064,8 +1064,11 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re // so reading through the shared template's `StaticFieldGet` loses both // its value and its property-presence semantics. This mirrors the // static-call guard in `expr_call/static_and_instance.rs`. - let local_shadows_class = ctx.lookup_local(source_name).is_some() - && !ctx.inferred_class_bindings.contains(source_name); + let local_shadows_class = ctx.lookup_local(source_name).is_some_and(|local| { + ctx.inferred_class_bindings + .class_key_for(local, source_name) + != Some(source_name) + }); let obj_name = ctx.resolve_class_name(source_name); if !local_shadows_class && ctx.lookup_class(&obj_name).is_some() { if let ast::MemberProp::Ident(prop_ident) = &member.prop { diff --git a/crates/perry-hir/src/lower/expr_new.rs b/crates/perry-hir/src/lower/expr_new.rs index 573bcf30b4..0d1b42f541 100644 --- a/crates/perry-hir/src/lower/expr_new.rs +++ b/crates/perry-hir/src/lower/expr_new.rs @@ -1543,15 +1543,21 @@ pub(super) fn lower_new(ctx: &mut LoweringContext, new_expr: &ast::NewExpr) -> R // attach), which the generic dynamic construct does not. // A shadowing local over an UNRELATED same-named class // declaration has no alias entry, so wall 7's reroute keeps - // firing. - let local_is_class_alias = - ctx.inferred_class_bindings.contains(class_name.as_str()); - if !local_is_class_alias { - return Ok(Expr::NewDynamic { - callee: Box::new(Expr::LocalGet(local_id)), - args, - byte_offset: new_byte_offset, - }); + // firing. When two class expressions claimed this name, the + // binding's OWN class is used (#10489), never the first + // claimant's. + match ctx + .inferred_class_bindings + .class_key_for(local_id, &class_name) + { + Some(key) => class_name = key.to_string(), + None => { + return Ok(Expr::NewDynamic { + callee: Box::new(Expr::LocalGet(local_id)), + args, + byte_offset: new_byte_offset, + }); + } } } // Issue #838 followup (b): when `` is NOT a real diff --git a/crates/perry-hir/src/lower/lower_expr/arm_class.rs b/crates/perry-hir/src/lower/lower_expr/arm_class.rs index 273e1f1d7f..4bd9822d1d 100644 --- a/crates/perry-hir/src/lower/lower_expr/arm_class.rs +++ b/crates/perry-hir/src/lower/lower_expr/arm_class.rs @@ -92,6 +92,7 @@ pub(crate) fn lower_class_expr( // for this same expression. Record the BINDING name so // `new ()` keeps the static construct path. ctx.inferred_class_bindings.insert(name.clone()); + ctx.inferred_class_bindings.mark_contested(&name); display_override = Some(name.clone()); format!("{}__anon_dup_{}", name, ctx.fresh_class()) } diff --git a/crates/perry-hir/src/lower/lowering_context.rs b/crates/perry-hir/src/lower/lowering_context.rs index d00b76034f..aac869aceb 100644 --- a/crates/perry-hir/src/lower/lowering_context.rs +++ b/crates/perry-hir/src/lower/lowering_context.rs @@ -12,6 +12,61 @@ use std::collections::{HashMap, HashSet}; use crate::ir::*; use crate::ClassAccessorNames; +/// Binding names whose class registration was created by the binding's own +/// class-expression initializer (see `LoweringContext::inferred_class_bindings`). +/// +/// The name set alone cannot tell two same-named bindings apart. When a second +/// class expression infers an already-claimed name (the #5592 `__anon_dup_` +/// arm: `function a() { const K = class {…} }` next to `function b() { const +/// K = class {…} }`), `new K()` inside `b` resolved `K` to `a`'s class and +/// appended `a`'s capture ids, so `b`'s constructor ran against another +/// function's locals (#10489). Such CONTESTED names resolve per binding local: +/// a declaration records which class its local holds, and an unrecorded local +/// constructs its runtime value. +#[derive(Debug, Default)] +pub(crate) struct InferredClassBindings { + names: HashSet, + contested: HashSet, + by_local: HashMap, +} + +impl InferredClassBindings { + pub(crate) fn contains(&self, name: &str) -> bool { + self.names.contains(name) + } + + pub(crate) fn insert(&mut self, name: String) -> bool { + self.names.insert(name) + } + + pub(crate) fn remove(&mut self, name: &str) -> bool { + self.names.remove(name) + } + + /// A second class expression claimed `name` under a disambiguated key. + pub(crate) fn mark_contested(&mut self, name: &str) { + self.contested.insert(name.to_string()); + } + + /// The declaration of binding `local` evaluated the class registered as + /// `class_key`. + pub(crate) fn record_binding(&mut self, local: LocalId, class_key: String) { + self.by_local.insert(local, class_key); + } + + /// The registration key of the class the in-scope local `local` (a binding + /// named `name`) provably holds, or `None` when it must be treated as an + /// arbitrary runtime value. + pub(crate) fn class_key_for(&self, local: LocalId, name: &str) -> Option<&str> { + if let Some(key) = self.by_local.get(&local) { + return Some(key.as_str()); + } + (self.names.contains(name) && !self.contested.contains(name)) + .then(|| self.names.get(name).map(String::as_str)) + .flatten() + } +} + #[derive(Debug, Clone, Copy)] pub(crate) struct WithEnvFrame { pub(crate) local_id: LocalId, @@ -361,8 +416,10 @@ pub struct LoweringContext { /// bind name too). At a `new ()` site, such a name's local provably /// holds that same class, so the static construct path (with its exact /// builtin-parent handling) is correct; any OTHER in-scope local shadows - /// whatever same-named class exists and must construct dynamically. - pub(crate) inferred_class_bindings: std::collections::HashSet, + /// whatever same-named class exists and must construct dynamically. Names + /// claimed by more than one class expression resolve by binding identity + /// instead (see [`InferredClassBindings::class_key_for`]). + pub(crate) inferred_class_bindings: InferredClassBindings, /// #4101: original source text keyed by FuncId, captured by slicing the /// module source against each function's AST span at lowering time. /// Flushed into `Module.closure_source_text` alongside `pending_functions`. diff --git a/crates/perry-hir/src/lower/shared_mutable_capture.rs b/crates/perry-hir/src/lower/shared_mutable_capture.rs index 144cb4d05a..de84c599cd 100644 --- a/crates/perry-hir/src/lower/shared_mutable_capture.rs +++ b/crates/perry-hir/src/lower/shared_mutable_capture.rs @@ -75,6 +75,7 @@ fn is_cap_name_of(name: &str, ids: &HashSet) -> bool { struct BodySharedCaptures { ids: HashSet, by_class: HashMap>, + census: DeclCensus, } pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { @@ -112,27 +113,19 @@ pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { let fn_shared: Vec = module .functions .iter() - .map(|f| detect_shared_in_body(&f.body, &classes)) + .map(|f| detect_shared_in_body(&f.params, &f.body, &classes)) .collect(); - let init_shared = detect_shared_in_body(&module.init, &classes); + let init_shared = detect_shared_in_body(&[], &module.init, &classes); (fn_shared, init_shared) }; - // Keep only ids that are UNAMBIGUOUS within their body (declared exactly - // once across deep `Let`s + nested closure params — see - // `retain_unambiguous`). Nested closures restart their id spaces, so a - // numeric rewrite over the whole body is only sound for unique ids. - for (f, shared) in module.functions.iter().zip(fn_shared.iter_mut()) { + // Keep only ids that denote ONE binding within their body (see + // `DeclCensus::is_one_binding`). Nested closures restart their id spaces, + // so a numeric rewrite over the whole body is only sound for those. + for shared in fn_shared.iter_mut() { if shared.ids.is_empty() { continue; } - let mut counts: HashMap = HashMap::new(); - for p in &f.params { - *counts.entry(p.id).or_default() += 1; - } - for st in &f.body { - collect_declared_counts_stmt(st, &mut counts); - } - retain_unambiguous(&mut shared.ids, &counts); + retain_unambiguous(&mut shared.ids, &shared.census); let retained = &shared.ids; for ids in shared.by_class.values_mut() { ids.retain(|id| retained.contains(id)); @@ -140,11 +133,7 @@ pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { shared.by_class.retain(|_, ids| !ids.is_empty()); } if !init_shared.ids.is_empty() { - let mut counts: HashMap = HashMap::new(); - for st in &module.init { - collect_declared_counts_stmt(st, &mut counts); - } - retain_unambiguous(&mut init_shared.ids, &counts); + retain_unambiguous(&mut init_shared.ids, &init_shared.census); let retained = &init_shared.ids; for ids in init_shared.by_class.values_mut() { ids.retain(|id| retained.contains(id)); @@ -167,6 +156,7 @@ pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { .extend(ids.iter().copied()); } } + propagate_cells_to_nested_classes(module, &mut shared_by_class); // ---- declaring bodies: rewrite with ONLY the ids detected in them ------- for (f, shared) in module.functions.iter_mut().zip(fn_shared.iter()) { @@ -192,6 +182,7 @@ pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { } }) .collect(); + demote_var_redeclarations(&f.params, &mut f.body, ids); rewrite_stmts(&mut f.body, ids, ids); for id in shared_params.into_iter().rev() { f.body.insert( @@ -205,6 +196,7 @@ pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { } } if !init_shared.ids.is_empty() { + demote_var_redeclarations(&[], &mut module.init, &init_shared.ids); rewrite_stmts(&mut module.init, &init_shared.ids, &init_shared.ids); } @@ -257,24 +249,22 @@ pub(crate) fn desugar_shared_mutable_captures(module: &mut Module) { } if !ctor_ids.is_empty() { // Uniqueness over the whole ctor+fields region (one scope). - let mut counts: HashMap = HashMap::new(); + let mut census = DeclCensus::default(); + let mut walker = CensusWalker::new(&mut census); + let scope = walker.open_scope(); if let Some(ctor) = &c.constructor { - for p in &ctor.params { - *counts.entry(p.id).or_default() += 1; - } - for st in &ctor.body { - collect_declared_counts_stmt(st, &mut counts); - } + walker.params(&ctor.params, scope); + walker.stmts(&ctor.body, scope, 0, true); } for f in &c.fields { if let Some(init) = &f.init { - collect_declared_counts_expr(init, &mut counts); + walker.expr(init, scope, 0); } if let Some(key) = &f.key_expr { - collect_declared_counts_expr(key, &mut counts); + walker.expr(key, scope, 0); } } - retain_unambiguous(&mut ctor_ids, &counts); + retain_unambiguous(&mut ctor_ids, &census); } if !ctor_ids.is_empty() { if let Some(ctor) = &mut c.constructor { @@ -338,8 +328,8 @@ fn collect_fn_target_ids(f: &Function, targets: &HashSet, out: &mut Has } /// Rewrite one lifted member body with ONLY its own rebind ids — and only -/// those that are UNAMBIGUOUS within the member (declared exactly once across -/// its params and deep `Let`s/closure params; see `retain_unambiguous`). +/// those that are UNAMBIGUOUS within the member (one binding across its params +/// and deep `Let`s/closure params; see `retain_unambiguous`). fn rewrite_member_scoped( f: &mut Function, targets: &HashSet, @@ -350,20 +340,14 @@ fn rewrite_member_scoped( if ids.is_empty() { return; } - let mut counts: HashMap = HashMap::new(); - for p in &f.params { - *counts.entry(p.id).or_default() += 1; - } - for s in &f.body { - collect_declared_counts_stmt(s, &mut counts); - } - retain_unambiguous(&mut ids, &counts); + let census = DeclCensus::of_body(&f.params, &f.body); + retain_unambiguous(&mut ids, &census); if !ids.is_empty() { rewrite_stmts(&mut f.body, no_shared, &ids); } } -/// Drop every id that is declared more than once in the rewritten region. +/// Drop every id that does not denote exactly ONE binding in the region. /// /// LocalIds restart per closure scope (#5143 family): inside a CJS module /// wrapper the whole module body is ONE function whose nested closures reuse @@ -374,40 +358,366 @@ fn rewrite_member_scoped( /// every route of the Next.js standalone server (#6089). An ambiguous id is /// skipped: its capture stays a split cell (the lesser, pre-#6054 behavior) /// instead of corrupting unrelated code. -fn retain_unambiguous(ids: &mut HashSet, counts: &HashMap) { +fn retain_unambiguous(ids: &mut HashSet, census: &DeclCensus) { if std::env::var("PERRY_5951_TRACE").as_deref() == Ok("1") { let dropped: Vec = ids .iter() .copied() - .filter(|id| counts.get(id).copied().unwrap_or(0) != 1) + .filter(|id| !census.is_one_binding(*id)) .collect(); if !dropped.is_empty() { eprintln!("[5951] skipped ambiguous ids {dropped:?}"); } } - ids.retain(|id| counts.get(id).copied().unwrap_or(0) == 1); + ids.retain(|id| census.is_one_binding(*id)); } -/// Count declarations per id across a region: `Let`s plus nested closure -/// PARAMS (which `collect_let_names_*` ignores), descending into closures. -fn collect_declared_counts_stmt(stmt: &Stmt, out: &mut HashMap) { - if let Stmt::Let { id, .. } = stmt { - *out.entry(*id).or_default() += 1; - } - for_each_child_stmt(stmt, &mut |s| collect_declared_counts_stmt(s, out)); - for_each_top_expr(stmt, &mut |e| collect_declared_counts_expr(e, out)); +/// One declaration of a `LocalId` inside a detection region. +#[derive(Debug)] +struct DeclSite { + /// Closure scope the declaration lives in (0 = the region's own body). + scope: u32, + name: String, + /// A parameter, or a `Let` that is a direct statement of its scope's body + /// outside any loop: it runs once, before every later site of that scope. + dominating: bool, } -fn collect_declared_counts_expr(expr: &Expr, out: &mut HashMap) { - if let Expr::Closure { params, body, .. } = expr { +/// Every declaration of every id in a region (params, `Let`s, nested closure +/// params and bodies), in execution order, plus the `var` re-declarations that +/// are real writes to a class-captured binding. +#[derive(Default, Debug)] +struct DeclCensus { + sites: HashMap>, + /// Ids re-declared (`var x = v` after the body-entry `var` slot) at a point + /// where a class has ALREADY captured the binding, or inside a loop that + /// can re-run the declaration after a capture. Those declarations are + /// assignments to a captured binding, exactly like `x = v`. + late_redeclared: HashSet, +} + +impl DeclCensus { + fn of_body(params: &[Param], body: &[Stmt]) -> Self { + let mut census = DeclCensus::default(); + let mut walker = CensusWalker::new(&mut census); + let scope = walker.open_scope(); + walker.params(params, scope); + walker.stmts(body, scope, 0, true); + census + } + + /// Is `id` exactly one binding in this region? + /// + /// A single declaration trivially is. Several are one binding only when + /// they all sit in the SAME closure scope under the SAME name and the first + /// dominates the rest. That is the shape of a `var`: lowering declares it at + /// body entry (`predefine_var_bindings_in_function_body`) or reuses a + /// same-named parameter, and every `var x = v` statement re-declares the + /// same id (#10485/#10489). Treating those as distinct bindings left every + /// `var` captured by a class on the value-snapshot path, so class members + /// never saw later writes and their own writes were lost. Declarations of + /// one id in different closure scopes stay ambiguous (#6089). + fn is_one_binding(&self, id: LocalId) -> bool { + match self.sites.get(&id).map(Vec::as_slice) { + None | Some([]) => false, + Some([_]) => true, + Some([first, rest @ ..]) => { + first.dominating + && rest + .iter() + .all(|site| site.scope == first.scope && site.name == first.name) + } + } + } +} + +/// Execution-order walk that fills a [`DeclCensus`]. Closure bodies open a new +/// scope with a fresh loop depth: each closure invocation gets its own +/// bindings, so an enclosing loop does not re-run a closure-local declaration. +struct CensusWalker<'a> { + census: &'a mut DeclCensus, + next_scope: u32, + /// Ids captured by a class registration seen so far. + captured: HashSet, +} + +impl<'a> CensusWalker<'a> { + fn new(census: &'a mut DeclCensus) -> Self { + CensusWalker { + census, + next_scope: 0, + captured: HashSet::new(), + } + } + + fn open_scope(&mut self) -> u32 { + let scope = self.next_scope; + self.next_scope += 1; + scope + } + + fn declare(&mut self, id: LocalId, name: &str, scope: u32, dominating: bool) { + self.census.sites.entry(id).or_default().push(DeclSite { + scope, + name: name.to_string(), + dominating, + }); + } + + fn params(&mut self, params: &[Param], scope: u32) { for p in params { - *out.entry(p.id).or_default() += 1; + if let Some(default) = &p.default { + self.expr(default, scope, 0); + } + self.declare(p.id, &p.name, scope, true); } - for s in body { - collect_declared_counts_stmt(s, out); + } + + fn stmts(&mut self, stmts: &[Stmt], scope: u32, loop_depth: u32, top: bool) { + for s in stmts { + self.stmt(s, scope, loop_depth, top); + } + } + + fn stmt(&mut self, stmt: &Stmt, scope: u32, loop_depth: u32, top: bool) { + match stmt { + Stmt::Let { id, name, init, .. } => { + if let Some(e) = init { + self.expr(e, scope, loop_depth); + } + let redeclaration = self.census.sites.contains_key(id); + if redeclaration && init.is_some() && (loop_depth > 0 || self.captured.contains(id)) + { + self.census.late_redeclared.insert(*id); + } + self.declare(*id, name, scope, top && loop_depth == 0); + } + Stmt::Expr(e) | Stmt::Throw(e) | Stmt::Return(Some(e)) => { + self.expr(e, scope, loop_depth) + } + Stmt::If { + condition, + then_branch, + else_branch, + } => { + self.expr(condition, scope, loop_depth); + self.stmts(then_branch, scope, loop_depth, false); + if let Some(e) = else_branch { + self.stmts(e, scope, loop_depth, false); + } + } + Stmt::While { condition, body } | Stmt::DoWhile { body, condition } => { + self.expr(condition, scope, loop_depth + 1); + self.stmts(body, scope, loop_depth + 1, false); + } + Stmt::For { + init, + condition, + update, + body, + } => { + if let Some(i) = init { + self.stmt(i, scope, loop_depth, false); + } + if let Some(c) = condition { + self.expr(c, scope, loop_depth + 1); + } + self.stmts(body, scope, loop_depth + 1, false); + if let Some(u) = update { + self.expr(u, scope, loop_depth + 1); + } + } + Stmt::Labeled { body, .. } => self.stmt(body, scope, loop_depth, false), + Stmt::Try { + body, + catch, + finally, + } => { + self.stmts(body, scope, loop_depth, false); + if let Some(c) = catch { + self.stmts(&c.body, scope, loop_depth, false); + } + if let Some(fin) = finally { + self.stmts(fin, scope, loop_depth, false); + } + } + Stmt::Switch { + discriminant, + cases, + } => { + self.expr(discriminant, scope, loop_depth); + for case in cases { + if let Some(t) = &case.test { + self.expr(t, scope, loop_depth); + } + self.stmts(&case.body, scope, loop_depth, false); + } + } + Stmt::Return(None) + | Stmt::Break + | Stmt::Continue + | Stmt::LabeledBreak(_) + | Stmt::LabeledContinue(_) + | Stmt::PreallocateBoxes(_) + | Stmt::PreallocateTdzBoxes(_) + | Stmt::ReleaseBoxes(_) => {} + } + } + + fn expr(&mut self, expr: &Expr, scope: u32, loop_depth: u32) { + match expr { + Expr::Closure { params, body, .. } => { + let inner = self.open_scope(); + self.params(params, inner); + self.stmts(body, inner, 0, true); + return; + } + Expr::RegisterClassCaptures { captures, .. } + | Expr::ClassExprFresh { + captured_args: captures, + .. + } => { + for capture in captures { + if let Expr::LocalGet(id) = capture { + self.captured.insert(*id); + } + } + } + _ => {} } + walk_expr_children(expr, &mut |e| self.expr(e, scope, loop_depth)); + } +} + +/// Turn every re-declaration of a shared id into a plain assignment, so the +/// rewrite below writes the binding's EXISTING cell (`x[0] = v`) instead of +/// minting a new one that classes captured earlier would never see. A `var x;` +/// re-declaration without an initializer does not touch the binding at all. +/// Only ids `DeclCensus::is_one_binding` accepted reach here, so the first +/// site seen in execution order is the dominating declaration. +fn demote_var_redeclarations(params: &[Param], body: &mut [Stmt], ids: &HashSet) { + let mut seen: HashSet = params + .iter() + .map(|p| p.id) + .filter(|id| ids.contains(id)) + .collect(); + for s in body.iter_mut() { + demote_redeclarations_stmt(s, ids, &mut seen); } - walk_expr_children(expr, &mut |e| collect_declared_counts_expr(e, out)); +} + +fn demote_redeclarations_stmt( + stmt: &mut Stmt, + ids: &HashSet, + seen: &mut HashSet, +) { + if let Stmt::Let { id, init, .. } = stmt { + if let Some(e) = init { + demote_redeclarations_expr(e, ids, seen); + } + if ids.contains(id) && !seen.insert(*id) { + *stmt = Stmt::Expr(match init.take() { + Some(value) => Expr::LocalSet(*id, Box::new(value)), + None => Expr::Undefined, + }); + } + return; + } + let stmts = |body: &mut [Stmt], seen: &mut HashSet| { + for s in body.iter_mut() { + demote_redeclarations_stmt(s, ids, seen); + } + }; + match stmt { + Stmt::Expr(e) | Stmt::Throw(e) | Stmt::Return(Some(e)) => { + demote_redeclarations_expr(e, ids, seen) + } + Stmt::If { + condition, + then_branch, + else_branch, + } => { + demote_redeclarations_expr(condition, ids, seen); + stmts(then_branch, seen); + if let Some(e) = else_branch { + stmts(e, seen); + } + } + Stmt::While { condition, body } | Stmt::DoWhile { body, condition } => { + demote_redeclarations_expr(condition, ids, seen); + stmts(body, seen); + } + Stmt::For { + init, + condition, + update, + body, + } => { + if let Some(i) = init { + demote_redeclarations_stmt(i, ids, seen); + } + if let Some(c) = condition { + demote_redeclarations_expr(c, ids, seen); + } + stmts(body, seen); + if let Some(u) = update { + demote_redeclarations_expr(u, ids, seen); + } + } + Stmt::Labeled { body, .. } => demote_redeclarations_stmt(body, ids, seen), + Stmt::Try { + body, + catch, + finally, + } => { + stmts(body, seen); + if let Some(c) = catch { + stmts(&mut c.body, seen); + } + if let Some(fin) = finally { + stmts(fin, seen); + } + } + Stmt::Switch { + discriminant, + cases, + } => { + demote_redeclarations_expr(discriminant, ids, seen); + for case in cases { + if let Some(t) = &mut case.test { + demote_redeclarations_expr(t, ids, seen); + } + stmts(&mut case.body, seen); + } + } + Stmt::Let { .. } + | Stmt::Return(None) + | Stmt::Break + | Stmt::Continue + | Stmt::LabeledBreak(_) + | Stmt::LabeledContinue(_) + | Stmt::PreallocateBoxes(_) + | Stmt::PreallocateTdzBoxes(_) + | Stmt::ReleaseBoxes(_) => {} + } +} + +fn demote_redeclarations_expr( + expr: &mut Expr, + ids: &HashSet, + seen: &mut HashSet, +) { + if let Expr::Closure { params, body, .. } = expr { + for p in params.iter() { + if ids.contains(&p.id) { + seen.insert(p.id); + } + } + for s in body.iter_mut() { + demote_redeclarations_stmt(s, ids, seen); + } + } + walk_expr_children_mut(expr, &mut |e| demote_redeclarations_expr(e, ids, seen)); } fn retype_capture_holders( @@ -522,7 +832,11 @@ fn retype_lets_in_expr(expr: &mut Expr, targets: &HashSet) { /// Detect the shared-mutable capture ids declared in ONE body. The returned /// ids are meaningful only within that body's scope — callers must not apply /// them to other functions (LocalIds repeat across scopes; see #6089). -fn detect_shared_in_body(body: &[Stmt], classes: &HashMap<&str, &Class>) -> BodySharedCaptures { +fn detect_shared_in_body( + params: &[Param], + body: &[Stmt], + classes: &HashMap<&str, &Class>, +) -> BodySharedCaptures { let mut shared = BodySharedCaptures::default(); let mut regs = Vec::new(); for s in body { @@ -531,10 +845,14 @@ fn detect_shared_in_body(body: &[Stmt], classes: &HashMap<&str, &Class>) -> Body if regs.is_empty() { return shared; } + shared.census = DeclCensus::of_body(params, body); let mut assigned: HashSet = HashSet::new(); for s in body { collect_assigned_deep_stmt(s, &mut assigned); } + // A `var x = v` re-declaration that runs after a class captured `x` writes + // the captured binding just like `x = v` does. + assigned.extend(shared.census.late_redeclared.iter().copied()); for (class_name, ids) in ®s { for id in ids { // Declaring-function-side mutation (`c = 99` after `new T()`). @@ -544,7 +862,7 @@ fn detect_shared_in_body(body: &[Stmt], classes: &HashMap<&str, &Class>) -> Body } // Class-side mutation: a member assigns rebind local `__perry_cap_`. if let Some(c) = classes.get(class_name.as_str()) { - if class_mutates_capture(c, *id) { + if class_mutates_capture(classes, c, *id, 0) { shared.ids.insert(*id); } } @@ -566,16 +884,209 @@ fn detect_shared_in_body(body: &[Stmt], classes: &HashMap<&str, &Class>) -> Body shared } -fn class_mutates_capture(c: &Class, id: LocalId) -> bool { +/// Does class `c` (or a class NESTED in one of its member bodies) assign the +/// capture of `id`? +/// +/// A member's own write lands on its rebind local `__perry_cap_`. A class +/// declared inside that member body (`class Outer { make() { return class Inner +/// { constructor() { n++ } } } }`, the emscripten/`FS` shape) captures the +/// REBIND local instead, and its write lands one level deeper — invisible to a +/// walk of `c` alone, because a nested class's members live in their own +/// `module.classes` entry, not inside the method body (#10489). +fn class_mutates_capture( + classes: &HashMap<&str, &Class>, + c: &Class, + id: LocalId, + depth: u32, +) -> bool { let id_name = collect_class_names(c); let assigned = collect_class_assigned(c); - assigned.iter().any(|aid| { + let names_id = |aid: &LocalId| { id_name .get(aid) .is_some_and(|n| crate::cap_fields::cap_field_outer_id(n) == Some(id)) + }; + if assigned.iter().any(names_id) { + return true; + } + // Bounded: each level is one class nesting, and the chain is finite. + if depth >= MAX_NESTED_CLASS_DEPTH { + return false; + } + for_each_nested_capture(c, &HashSet::from([id]), |nested_name, outer_id| { + classes + .get(nested_name) + .is_some_and(|nested| class_mutates_capture(classes, nested, outer_id, depth + 1)) }) } +/// How far the nested-class walks descend (`class` inside a member body, whose +/// member body declares another class, …). Deep enough for real code, bounded +/// so a cyclic registration cannot loop. +const MAX_NESTED_CLASS_DEPTH: u32 = 8; + +/// Call `visit(nested_class_name, outer_id)` for every class registered inside +/// a member body of `c` that captures that member's rebind local for `outer_id` +/// (an id in `targets`). Returns true as soon as `visit` does. +/// +/// The nested class was lowered BEFORE `synthesize_class_captures` renamed the +/// enclosing member's references, so its own rebind holders are still named for +/// the ORIGINAL outer id — which is what the reported id must be for the +/// name-keyed matching in `rewrite_member_scoped` / `retype_capture_holders`. +fn for_each_nested_capture( + c: &Class, + targets: &HashSet, + mut visit: impl FnMut(&str, LocalId) -> bool, +) -> bool { + for f in class_member_fns(c) { + let mut rebinds = member_rebind_targets(f, targets); + let mut regs = Vec::new(); + for s in &f.body { + find_regs_stmt(s, &mut regs); + find_cap_arg_news_stmt(s, &mut regs); + } + // A field initializer shares the constructor's scope (it is lowered + // into the ctor body), so a class declared in one holds the ctor's + // rebind params. + if c.constructor.as_ref().is_some_and(|ctor| ctor.id == f.id) { + for field in &c.fields { + for expr in field.init.iter().chain(field.key_expr.iter()) { + let mut names: HashMap = HashMap::new(); + collect_let_names_expr(expr, &mut names); + for (id, name) in names { + if let Some(outer) = crate::cap_fields::cap_field_outer_id(&name) { + if targets.contains(&outer) { + rebinds.insert(id, outer); + } + } + } + find_regs_expr(expr, &mut regs); + find_cap_arg_news_expr(expr, &mut regs); + } + } + } + if rebinds.is_empty() { + continue; + } + for (nested_name, ids) in regs { + for id in ids { + if let Some(outer_id) = rebinds.get(&id) { + if visit(&nested_name, *outer_id) { + return true; + } + } + } + } + } + false +} + +/// Class constructions whose trailing `cap_args_appended` arguments forward +/// capture handles. An IMMEDIATELY constructed class expression (`new (class { +/// … })()`) has neither a `RegisterClassCaptures` nor a `ClassExprFresh` node — +/// `lower_new` lowers it straight to `Expr::New` — so `find_regs_stmt` alone +/// misses it, and its members kept reading the raw cell (#10485's nested-class +/// row printed `[[2],["set"]]` instead of the values). +fn find_cap_arg_news_stmt(stmt: &Stmt, out: &mut Vec<(String, Vec)>) { + for_each_child_stmt(stmt, &mut |s| find_cap_arg_news_stmt(s, out)); + for_each_top_expr(stmt, &mut |e| find_cap_arg_news_expr(e, out)); +} + +fn find_cap_arg_news_expr(expr: &Expr, out: &mut Vec<(String, Vec)>) { + if let Expr::New { + class_name, + args, + cap_args_appended, + .. + } = expr + { + let appended = *cap_args_appended as usize; + if appended > 0 && args.len() >= appended { + let ids: Vec = args[args.len() - appended..] + .iter() + .filter_map(|a| match a { + Expr::LocalGet(id) => Some(*id), + _ => None, + }) + .collect(); + if !ids.is_empty() { + out.push((class_name.clone(), ids)); + } + } + } + if let Expr::Closure { body, .. } = expr { + for s in body { + find_cap_arg_news_stmt(s, out); + } + } + walk_expr_children(expr, &mut |e| find_cap_arg_news_expr(e, out)); +} + +/// The member's capture holders (`__perry_cap_` params and `Let`s), +/// mapped back to the outer id each one rebinds. +fn member_rebind_targets(f: &Function, targets: &HashSet) -> HashMap { + let mut rebinds: HashMap = HashMap::new(); + let record = |id: LocalId, name: &str, out: &mut HashMap| { + if let Some(outer) = crate::cap_fields::cap_field_outer_id(name) { + if targets.contains(&outer) { + out.insert(id, outer); + } + } + }; + for p in &f.params { + record(p.id, &p.name, &mut rebinds); + } + let mut names: HashMap = HashMap::new(); + for s in &f.body { + collect_let_names_stmt(s, &mut names); + } + for (id, n) in names { + record(id, &n, &mut rebinds); + } + rebinds +} + +/// A class nested in a member body holds its captures through the member's +/// REBIND locals, which by then carry the shared cell — so ITS members must +/// index through `[0]` too. Walk the nesting chain and mark those classes. +fn propagate_cells_to_nested_classes( + module: &Module, + shared_by_class: &mut HashMap>, +) { + for _ in 0..MAX_NESTED_CLASS_DEPTH { + let mut discovered: Vec<(String, LocalId)> = Vec::new(); + for c in &module.classes { + let Some(targets) = shared_by_class.get(&c.name) else { + continue; + }; + for_each_nested_capture(c, targets, |nested_name, outer_id| { + discovered.push((nested_name.to_string(), outer_id)); + false + }); + } + let mut added = false; + for (class_name, id) in discovered { + added |= shared_by_class.entry(class_name).or_default().insert(id); + } + if !added { + break; + } + } +} + +/// Every member function of a class: methods, accessors, statics, computed +/// members and the constructor (whose scope the field initializers share). +fn class_member_fns(c: &Class) -> Vec<&Function> { + let mut v: Vec<&Function> = Vec::new(); + v.extend(c.methods.iter()); + v.extend(c.getters.iter().map(|(_, g)| g)); + v.extend(c.setters.iter().map(|(_, s)| s)); + v.extend(c.static_methods.iter()); + v.extend(c.computed_members.iter().map(|m| &m.function)); + v.extend(c.constructor.iter()); + v +} + /// id -> name across a class: every member function's PARAMS (a field-init /// closure captures the constructor param `__perry_cap_`, id-only in the /// closure) plus every `Let` name in member bodies and field initializers. @@ -622,8 +1133,8 @@ fn collect_class_names(c: &Class) -> HashMap { /// descending into closures). fn collect_class_assigned(c: &Class) -> HashSet { let mut assigned = HashSet::new(); - for body in class_member_bodies(c) { - for s in body { + for f in class_member_fns(c) { + for s in &f.body { collect_assigned_deep_stmt(s, &mut assigned); } } @@ -638,29 +1149,6 @@ fn collect_class_assigned(c: &Class) -> HashSet { assigned } -fn class_member_bodies(c: &Class) -> Vec<&Vec> { - let mut v: Vec<&Vec> = Vec::new(); - for m in &c.methods { - v.push(&m.body); - } - for (_, g) in &c.getters { - v.push(&g.body); - } - for (_, s) in &c.setters { - v.push(&s.body); - } - for sm in &c.static_methods { - v.push(&sm.body); - } - for member in &c.computed_members { - v.push(&member.function.body); - } - if let Some(ctor) = &c.constructor { - v.push(&ctor.body); - } - v -} - // ---- read-only walkers (exhaustive over Stmt; exprs recurse into closures) -- fn collect_let_names_stmt(stmt: &Stmt, out: &mut HashMap) { @@ -807,6 +1295,27 @@ fn for_each_top_expr(stmt: &Stmt, f: &mut dyn FnMut(&Expr)) { // Rewrite (mutable, exhaustive over Stmt) // --------------------------------------------------------------------------- +/// `Sequence([LocalSet(id, _) | Update { id }, this.__perry_cap_N = LocalGet(id)])` +/// for a cell id — see the `Expr::Sequence` arm of [`rewrite_expr`]. +fn is_redundant_cell_propagation(items: &[Expr], index_uses: &HashSet) -> bool { + let [write, Expr::PropertySet { + object, + property, + value, + }] = items + else { + return false; + }; + let written = match write { + Expr::LocalSet(id, _) | Expr::Update { id, .. } => *id, + _ => return false, + }; + index_uses.contains(&written) + && matches!(object.as_ref(), Expr::This) + && property.starts_with("__perry_cap_") + && matches!(value.as_ref(), Expr::LocalGet(id) if *id == written) +} + fn rewrite_stmts(stmts: &mut [Stmt], shared: &HashSet, index_uses: &HashSet) { for s in stmts.iter_mut() { rewrite_stmt(s, shared, index_uses); @@ -947,6 +1456,19 @@ fn rewrite_stmt(stmt: &mut Stmt, shared: &HashSet, index_uses: &HashSet fn rewrite_expr(expr: &mut Expr, shared: &HashSet, index_uses: &HashSet) { match expr { + // A member's write to a captured local arrives wrapped by the field + // propagation of `synthesize_class_captures`: + // `Sequence([write, this.__perry_cap_N = LocalGet(rebind)])`, which + // keeps a value SNAPSHOT field in step with the member's local. A + // shared cell needs no propagation — the field already holds the same + // cell — and keeping it makes the sequence yield the cell handle instead + // of the write's value (`return n++` returned `[3]`, not 2; #10489). + Expr::Sequence(items) if is_redundant_cell_propagation(items, index_uses) => { + let write = items.swap_remove(0); + *expr = write; + rewrite_expr(expr, shared, index_uses); + return; + } // A value read of a boxed id -> `id[0]`. The synthesized `LocalGet` is // the ARRAY handle and is not re-rewritten. Expr::LocalGet(id) if index_uses.contains(id) => { diff --git a/crates/perry-hir/src/lower/stmt.rs b/crates/perry-hir/src/lower/stmt.rs index 4e6bd026fa..bc94fe146c 100644 --- a/crates/perry-hir/src/lower/stmt.rs +++ b/crates/perry-hir/src/lower/stmt.rs @@ -100,6 +100,8 @@ fn emit_class_expression_value_binding( // The binding's local holds ITS OWN class — `new ()` must keep // the static construct path (see `inferred_class_bindings`). ctx.inferred_class_bindings.insert(bind_name.to_string()); + ctx.inferred_class_bindings + .record_binding(id, bind_name.to_string()); module.init.push(Stmt::Let { id, name: bind_name.to_string(), diff --git a/crates/perry-hir/src/lower/tests.rs b/crates/perry-hir/src/lower/tests.rs index f127f3eff9..0655a57fdc 100644 --- a/crates/perry-hir/src/lower/tests.rs +++ b/crates/perry-hir/src/lower/tests.rs @@ -1988,6 +1988,7 @@ mod unresolved_new_global; mod global_this_new_shadowed; mod capture_stash; +mod class_member_var_captures; mod function_ctor_runtime_routing; mod mixin_parent_chain; mod native_module_sync; diff --git a/crates/perry-hir/src/lower/tests/class_member_var_captures.rs b/crates/perry-hir/src/lower/tests/class_member_var_captures.rs new file mode 100644 index 0000000000..caf348f965 --- /dev/null +++ b/crates/perry-hir/src/lower/tests/class_member_var_captures.rs @@ -0,0 +1,213 @@ +//! #10485/#10489: class members share the enclosing bindings they capture. +//! +//! Two lowering properties are asserted here, both invisible to a runtime +//! probe until the miscompiled program prints a stale value: +//! +//! 1. a `var` captured and mutated across the class boundary becomes ONE +//! shared cell (`crate::lower::shared_mutable_capture`), even though a +//! `var` is declared twice in HIR (body-entry slot + declaration), and the +//! re-declaration writes that cell instead of minting a second one; +//! 2. a `new K()` resolves to the class the BINDING holds, not to whatever +//! class first claimed the name `K` in the module. + +use crate::ir::{Expr, Stmt}; + +fn lower(source: &str) -> crate::ir::Module { + let module = perry_parser::parse_typescript(source, "t.ts").expect("source parses"); + super::super::lower_module(&module, "t", "t.ts").expect("source lowers") +} + +fn function<'a>(module: &'a crate::ir::Module, name: &str) -> &'a crate::ir::Function { + module + .functions + .iter() + .find(|f| f.name == name) + .unwrap_or_else(|| panic!("fixture declares function {name}")) +} + +/// Statements of `body` (recursing into nested statement bodies) that declare +/// `name`, rendered compactly. +fn declarations_of<'a>(body: &'a [Stmt], name: &str) -> Vec<&'a Stmt> { + let mut out = Vec::new(); + for stmt in body { + if matches!(stmt, Stmt::Let { name: n, .. } if n == name) { + out.push(stmt); + } + if let Stmt::For { body, .. } | Stmt::While { body, .. } = stmt { + out.extend(declarations_of(body, name)); + } + } + out +} + +fn compact(value: &impl std::fmt::Debug) -> String { + format!("{value:?}") + .chars() + .filter(|ch| !ch.is_whitespace()) + .collect() +} + +/// A `var` mutated from a constructor is the same binding on both sides: it +/// lowers to a one-element cell, and every use — the declaring function's read +/// and the member's write — goes through it. Before #10489 the two HIR `Let`s a +/// `var` produces read as two bindings, so the desugar skipped the id and the +/// class kept a private copy (`declCtor` returned 0, not 3). +#[test] +fn var_mutated_from_a_constructor_becomes_one_shared_cell() { + let module = lower( + r#" + function declCtor() { + var a = 0; + class A { constructor() { a++; } } + new A(); new A(); + return a; + } + declCtor(); + "#, + ); + let decls = declarations_of(&function(&module, "declCtor").body, "a"); + assert_eq!( + decls.len(), + 1, + "the `var` re-declaration must write the cell, not re-declare it: {decls:#?}" + ); + assert!( + matches!(decls[0], Stmt::Let { init: Some(Expr::Array(items)), .. } if items.len() == 1), + "the captured `var` must lower to a one-element cell: {:#?}", + decls[0] + ); + let body = compact(&function(&module, "declCtor").body); + assert!( + body.contains("IndexSet{object:LocalGet"), + "the `var a = 0` re-declaration must write the existing cell: {body}" + ); + let class = module + .classes + .iter() + .find(|c| c.name == "A") + .expect("fixture declares class A"); + let ctor = compact(&class.constructor.as_ref().expect("class A has a ctor").body); + assert!( + ctor.contains("IndexUpdate{"), + "the constructor's `a++` must update the shared cell: {ctor}" + ); +} + +/// The control: a capture nobody mutates keeps the cheap value snapshot, so +/// the cell rewrite cannot quietly become universal (it costs an indirection +/// on every read). +#[test] +fn an_unmutated_var_capture_keeps_its_value_snapshot() { + let module = lower( + r#" + function readOnly() { + var a = 0; + class A { value() { return a; } } + return new A().value(); + } + readOnly(); + "#, + ); + let body = compact(&function(&module, "readOnly").body); + assert!( + !body.contains("Array([Undefined])") && !body.contains("IndexSet{object:LocalGet"), + "an unmutated capture must not be boxed into a cell: {body}" + ); +} + +/// A class nested in a member body captures the member's rebind local, which +/// holds the cell — so its own members must index through it too. The +/// immediately-constructed form has no `RegisterClassCaptures` node at all +/// (`lower_new` emits a bare `Expr::New`), which is how it was missed. +#[test] +fn a_class_nested_in_a_member_shares_the_same_cell() { + let module = lower( + r#" + function outerFn() { + var made = 0; + class Outer { + make() { return class Inner { constructor() { made++; } }; } + immediate() { return new (class { read() { return made; } })().read(); } + } + const Inner = new Outer().make(); + new Inner(); + return [made, new Outer().immediate()]; + } + outerFn(); + "#, + ); + // A named class EXPRESSION registers under a disambiguated key, so match + // the source name as a prefix. + let inner = module + .classes + .iter() + .find(|c| c.name.starts_with("Inner")) + .expect("fixture declares the nested class Inner"); + let inner_ctor = compact(&inner.constructor.as_ref().expect("Inner has a ctor").body); + assert!( + inner_ctor.contains("IndexUpdate{"), + "a nested class's write must reach the shared cell: {inner_ctor}" + ); + let anon = module + .classes + .iter() + .find(|c| c.name.starts_with("__anon_class_")) + .expect("fixture declares an immediately-constructed anonymous class"); + let read = compact( + &anon + .methods + .first() + .expect("the anon class has read()") + .body, + ); + assert!( + read.contains("IndexGet{"), + "an immediately-constructed nested class must read the cell's value: {read}" + ); +} + +/// Two sibling functions that each bind `const K = class {…}`: the second +/// binding holds its OWN class (registered under a disambiguated key), so +/// `new K()` inside it must not construct the first function's class with the +/// first function's capture ids (#10489's `twinTwo` returned 0). +#[test] +fn sibling_same_named_class_bindings_construct_their_own_class() { + let module = lower( + r#" + function twinOne() { var n = 0; const K = class { constructor() { n++; } }; new K(); return n; } + function twinTwo() { var n = 0; const K = class { constructor() { n += 10; } }; new K(); return n; } + twinOne(); twinTwo(); + "#, + ); + let one = compact(&function(&module, "twinOne").body); + let two = compact(&function(&module, "twinTwo").body); + let key_of = |body: &str| -> String { + let at = body + .find("New{class_name:\"") + .expect("the fixture constructs its class statically"); + let rest = &body[at + "New{class_name:\"".len()..]; + rest[..rest.find('"').expect("class name is terminated")].to_string() + }; + let (one_key, two_key) = (key_of(&one), key_of(&two)); + assert_ne!( + one_key, two_key, + "sibling functions must construct distinct classes, both built {one_key}" + ); + for (key, body) in [(&one_key, &one), (&two_key, &two)] { + let class = module + .classes + .iter() + .find(|c| &c.name == key) + .unwrap_or_else(|| panic!("the constructed class {key} exists")); + let ctor = compact(&class.constructor.as_ref().expect("has a ctor").body); + assert!( + ctor.contains("IndexUpdate{") || ctor.contains("IndexSet{object:LocalGet"), + "{key}'s constructor must write its own function's cell: {ctor}" + ); + assert!( + body.contains("init:Some(Array([Undefined]))") + && body.contains("IndexSet{object:LocalGet"), + "each function declares its own counter cell and initializes it: {body}" + ); + } +} diff --git a/test-files/_helpers/class_member_var_captures_10485.cjs b/test-files/_helpers/class_member_var_captures_10485.cjs new file mode 100644 index 0000000000..cb5e2c5627 --- /dev/null +++ b/test-files/_helpers/class_member_var_captures_10485.cjs @@ -0,0 +1,27 @@ +"use strict"; +// Helper for test_gap_10485_class_member_var_captures.ts: the shape TypeScript +// emits for a class whose static members refer to the class itself +// (`var _a; class X { … _a … } _a = X;`), as in @redis/client 6.1.0's +// dist/lib/client/index.js. A CommonJS module body is a function scope, so +// every read of `_a` below is a class-member capture of a module-body `var`. +var _a; +var hits = 0; +function attachConfig({ BaseClass, tag }) { + return class extends BaseClass { + tagged() { return tag + ":" + this.describe(); } + }; +} +class Client { + constructor(name) { this.name = name; hits++; } + static create(name) { return _a.factory(name); } + static factory(name) { + const Commanded = attachConfig({ BaseClass: _a, tag: "cmd" }); + return new Commanded(name); + } + static hits() { return hits; } + describe() { return "client(" + this.name + ") hits=" + _a.hits(); } +} +_a = Client; +exports.Client = Client; +exports.bumpHits = (n) => { hits += n; }; +exports.readHits = () => hits; diff --git a/test-files/test_gap_10485_class_member_var_captures.ts b/test-files/test_gap_10485_class_member_var_captures.ts new file mode 100644 index 0000000000..45cf4c4caf --- /dev/null +++ b/test-files/test_gap_10485_class_member_var_captures.ts @@ -0,0 +1,179 @@ +// #10485 / #10489: class members nested in a function scope (a function body, +// or a CommonJS module body) must share the enclosing bindings they capture, +// exactly like closures do: they see writes made after the class was declared, +// and their own writes are visible outside and to every other member/instance. +// +// Perry lifts class members out of their function and hands them captures as +// value snapshots; a mutable capture is shared through a one-element cell +// (#5951). A `var` is declared twice in HIR (the body-entry slot, then the +// declaration statement), which the cell rewrite read as two bindings and +// skipped, so every `var` capture stayed a stale copy. Separately, two +// functions that each bound `const K = class {…}` constructed the FIRST +// function's class from inside the second one. +// +// Validated byte-for-byte against `node --experimental-strip-types`. +import cjs from "./_helpers/class_member_var_captures_10485.cjs"; + +const show = (label: string, value: unknown) => console.log(label, JSON.stringify(value)); + +// ── 1. TypeScript `var _a; class X {…} _a = X;` emit in a CommonJS module ── +{ + const lib = cjs as any; + const client = lib.Client.create("a"); + show("cjs static via _a:", client.tagged()); + show("cjs extends _a:", client instanceof lib.Client); + lib.bumpHits(10); + show("cjs hits (member write + outer write):", [lib.Client.hits(), lib.readHits()]); +} + +// ── 2. reads after a later assignment, every member kind ── +function readsAfterAssignment(param: number) { + var withInit = 1; + var noInit: any; + let lexical = 1; + class K { + snapshot = [withInit, noInit, lexical, param]; + live = () => [withInit, noInit, lexical, param]; + static s() { return [withInit, noInit, lexical, param]; } + i() { return [withInit, noInit, lexical, param]; } + get g() { return [withInit, noInit, lexical, param]; } + static get sg() { return [withInit, noInit, lexical, param]; } + nested() { return new (class { r() { return [withInit, noInit, lexical, param]; } })().r(); } + } + withInit = 2; noInit = "set"; lexical = 2; param = 20; + const k = new K(); + show("after 1st: static", K.s()); + show("after 1st: static getter", K.sg); + show("after 1st: instance", k.i()); + show("after 1st: getter", k.g); + show("after 1st: field init", k.snapshot); + show("after 1st: field arrow", k.live()); + show("after 1st: nested class", k.nested()); + withInit = 3; noInit = "again"; lexical = 3; param = 30; + show("after 2nd: static", K.s()); + show("after 2nd: old instance", [k.i(), k.snapshot, k.live()]); + // a `var` re-declaration with an initializer writes the SAME binding + var withInit = 4; + show("after var redeclaration:", [K.s(), k.i()]); +} +readsAfterAssignment(10); + +// a `var` declared after the class (a `let`/`const` declared after a class that +// reads it is a separate, still-open gap — it stays undefined in Perry today) +function declaredAfterClass() { + class K { + static v() { return late; } + } + var late = "late var"; + return [K.v()]; +} +show("declared after class:", declaredAfterClass()); + +// ── 3. writes from class members ── +function ctorCounter() { + var count = 0; + class A { id: number; constructor() { this.id = ++count; } } + const ids = [new A().id, new A().id, new A().id]; + return [ids, count]; +} +show("ctor counter:", ctorCounter()); + +function staticAndInstanceWrites() { + var total = 0; + var log = ""; + class B { + static add(n: number) { total += n; } + push(s: string) { log = log + s; return log.length; } + set value(v: number) { total = v; } + get value() { return total; } + } + B.add(1); B.add(2); + const b1 = new B(), b2 = new B(); + b1.push("x"); b2.push("y"); + const afterStatic = total; + b1.value = 100; + return [afterStatic, total, b2.value, log]; +} +show("static/instance/setter writes:", staticAndInstanceWrites()); + +function fieldInitWrite() { + var next = 1; + class F { id = next++; } + new F(); new F(); + return [new F().id, next]; +} +show("field initializer write:", fieldInitWrite()); + +function postfixValue() { + var n = 0; + let m = 0; + class P { a() { return n++; } b() { return m++; } } + const p = new P(); + p.a(); p.a(); p.b(); p.b(); + return [p.a(), n, p.b(), m]; +} +show("postfix result:", postfixValue()); + +function nestedClassWrites() { + var made = 0; + class Outer { + make() { return class Inner { constructor() { made++; } }; } + } + const Inner = new Outer().make(); + new Inner(); new Inner(); + return made; +} +show("nested class write:", nestedClassWrites()); + +function fieldInitNestedClass() { + var seen = 0; + class Holder { + Tracked = class { constructor() { seen++; } }; + count() { return seen; } + } + const h = new Holder(); + new h.Tracked(); new h.Tracked(); + return [h.count(), seen]; +} +show("field-init nested class:", fieldInitNestedClass()); + +// a `var` in a loop body is ONE function-scoped binding +function loopVar() { + const classes: any[] = []; + for (let i = 0; i < 3; i++) { + var shared = i * 10; + class C { static get() { return shared; } } + classes.push(C); + } + return classes.map((c) => c.get()); +} +show("var in loop body:", loopVar()); + +// ── 4. emscripten FS shape: hoisted function constructs a later class expression ── +function hoistedFactory() { + function createNode() { return new FSNode(); } + var nextInode = 1, FSNode = class { id: number; constructor() { this.id = nextInode++; } }; + createNode(); createNode(); + return createNode().id + "/" + nextInode; +} +show("hoisted factory:", hoistedFactory()); + +// ── 5. same-named bindings and classes in sibling functions ── +function twinOne() { var n = 0; const K = class { static tag = "one"; constructor() { n++; } }; new K(); new K(); new K(); return [n, K.tag, new K() instanceof K]; } +function twinTwo() { var n = 0; const K = class { static tag = "two"; constructor() { n += 10; } }; new K(); new K(); new K(); return [n, K.tag, new K() instanceof K]; } +function twinDeclOne() { var n = 0; class D { constructor() { n++; } } new D(); new D(); return n; } +function twinDeclTwo() { var n = 0; class D { constructor() { n += 100; } } new D(); new D(); return n; } +function notAClass() { + var K: any = function (this: any) { this.kind = "function"; }; + return new K().kind; +} +show("twin expr one:", twinOne()); +show("twin expr two:", twinTwo()); +show("twin decl:", [twinDeclOne(), twinDeclTwo()]); +show("same-named non-class local:", notAClass()); + +// ── 6. module top level (always worked; must keep working) ── +var _top: any; +class Top { static viaAlias() { return _top === Top; } } +_top = Top; +show("esm top-level alias:", Top.viaAlias()); From ada8c4b2b6d456f15162ee446cc2824a83454129 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 18 Sep 2026 11:40:57 +0000 Subject: [PATCH 2/2] docs(changelog): add fragment for #10615 --- changelog.d/10615-class-member-var-captures.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog.d/10615-class-member-var-captures.md diff --git a/changelog.d/10615-class-member-var-captures.md b/changelog.d/10615-class-member-var-captures.md new file mode 100644 index 0000000000..ec85da4424 --- /dev/null +++ b/changelog.d/10615-class-member-var-captures.md @@ -0,0 +1,5 @@ +Fix class members nested in a function or CommonJS module body capturing enclosing `var`/`let` bindings by value snapshot instead of by reference: a method saw the value the binding had at class-declaration time, and writes from constructors/static methods to a captured `var` were silently lost. This blocked `@redis/client` (TypeScript's `var _a; class X {…} _a = X;` static-self-reference emit) and `sql.js`/emscripten (the `FS.nextInode` counter that never advanced, corrupting every `FSNode.id`). + +Root cause: `desugar_shared_mutable_captures` decides whether a captured local is safe to promote to a shared reference cell by counting how many times its `LocalId` is declared and requiring exactly one — but a `var` is declared twice in HIR (a body-entry predefine slot, then the declaration statement), so every `var` capture failed that check and fell back to the stale value-snapshot path. The counter is replaced with a `DeclCensus`/`CensusWalker` that walks a region in execution order and accepts an id as one binding when either it has a single declaration, or all its declarations sit in the same closure scope under the same name with the first dominating (the shape of a `var`) — declarations in different closure scopes stay ambiguous, preserving the earlier #6089 fix. A `var` redeclaration that runs after the class has already captured the id, or inside a loop, is demoted into a write to the shared cell instead of being treated as a fresh binding. + +Added `test-files/test_gap_10485_class_member_var_captures.ts` (byte-for-byte against Node 26.5.1) covering every member kind, constructor/static/setter writes, `var` redeclaration, the emscripten hoisted-factory shape, and same-named `var`+class pairs in sibling functions, plus 4 new HIR-level unit tests in `crates/perry-hir/src/lower/tests/class_member_var_captures.rs`. Proven to fail on a pristine pre-fix tree and pass on this change.