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. 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());