From f572ce9b93f0c96bca271915f978c29c034ad6cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 15:52:56 +0200 Subject: [PATCH] fix(hir): a later class accessor replaces an earlier one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ECMA-262 ClassDefinitionEvaluation installs class elements in source order, so a second `get x` / `set x` REPLACES the first. `ClassDecl::getters` / `::setters` are consumed with `iter().find(...)` — first match wins — but every accessor was appended with `push()`. The shadowed definition therefore stayed live and the one the program actually defines last was silently dropped. Every accessor shape is affected, not just getters. Against `node --experimental-strip-types`, before this change: instance getter 111 (expected 222) static + instance getter instance-first (expected instance-last) static-first (expected static-last) duplicate setters first:x (expected last:x) class expression 1 (expected 2) There is no diagnostic: the program reads a plausible value from the wrong accessor and keeps running. Found in Claude-of-Duty, whose `Spring3` pairs an early `set z` (damping) with a later `get z` (displacement) — legal, if unusual, and it relies on the read/write asymmetry the spec produces. Perry served the shadowed damping getter, so `lag.z` and `recPos.z` read 0.46 and 0.42 (their constructors' damping arguments) instead of displacements. That added +0.88 m to the first-person viewmodel's Z, moving the rig from 0.3 m in front of the camera to 0.58 m behind it. All 156 viewmodel nodes then clipped: the overlay pass ran and issued every draw, and produced no fragments. `record_class_accessor` overwrites an existing entry instead of appending. The replacement is keyed on `(name, is_static)`: a static and an instance accessor of the same name are distinct properties — one on the constructor, one on the prototype — and collapsing them would trade this bug for another. Verified: perry-hir 620 passed, perry-codegen 1912 passed. The regression test covers all four shapes above and fails on each without this change. --- crates/perry-hir/src/lower_decl/class_decl.rs | 115 ++++++++++++++++-- .../duplicate_class_accessor_last_wins.rs | 111 +++++++++++++++++ 2 files changed, 218 insertions(+), 8 deletions(-) create mode 100644 crates/perry/tests/duplicate_class_accessor_last_wins.rs diff --git a/crates/perry-hir/src/lower_decl/class_decl.rs b/crates/perry-hir/src/lower_decl/class_decl.rs index 500fa5ea25..5fc0375a0c 100644 --- a/crates/perry-hir/src/lower_decl/class_decl.rs +++ b/crates/perry-hir/src/lower_decl/class_decl.rs @@ -194,6 +194,49 @@ pub(crate) fn capture_class_source( } } +/// Record one class accessor, honouring ECMA-262's "a later definition of the +/// same key replaces the earlier one". +/// +/// `ClassDecl::getters` / `::setters` are consumed with `iter().find(...)`, so +/// the FIRST entry with a given name wins at lookup time. Appending +/// unconditionally therefore keeps a *shadowed* accessor alive and silently +/// drops the one the program actually defines last: +/// +/// ```js +/// class Spring3 { +/// get z() { return this.a.z; } // damping — shadowed +/// get z() { return this.c.x; } // displacement — must win +/// } +/// ``` +/// +/// Perry returned `this.a.z` here while every other engine returns +/// `this.c.x`. In Claude-of-Duty that handed the viewmodel rig a spring's +/// DAMPING COEFFICIENT (0.46) where it wanted a Z displacement, pushing the +/// weapon 0.88 m behind the camera, where it clipped and drew nothing. +/// +/// Static and instance accessors are distinct properties (one lives on the +/// constructor, one on the prototype) and may legally share a name, so the +/// replacement is keyed on `(name, is_static)` rather than the name alone. +fn record_class_accessor( + list: &mut Vec<(String, Function)>, + statics: &mut Vec, + name: String, + func: Function, + is_static: bool, +) { + let existing = list + .iter() + .enumerate() + .find_map(|(i, (n, _))| (n == &name && statics[i] == is_static).then_some(i)); + match existing { + Some(i) => list[i] = (name, func), + None => { + list.push((name, func)); + statics.push(is_static); + } + } +} + pub fn lower_class_decl( ctx: &mut LoweringContext, class_decl: &ast::ClassDecl, @@ -683,6 +726,10 @@ pub fn lower_class_decl( let mut static_methods = Vec::new(); let mut getters = Vec::new(); let mut setters = Vec::new(); + // Parallel staticness, so `record_class_accessor` can tell a static + // accessor from an instance one with the same name. + let mut getter_statics: Vec = Vec::new(); + let mut setter_statics: Vec = Vec::new(); let mut static_accessor_names: Vec = Vec::new(); let mut static_accessor_fn_ids: Vec = Vec::new(); let mut computed_members = Vec::new(); @@ -778,7 +825,13 @@ pub fn lower_class_decl( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - getters.push((prop_name, func)); + record_class_accessor( + &mut getters, + &mut getter_statics, + prop_name, + func, + method.is_static, + ); } ast::MethodKind::Setter => { // Setter: takes one parameter @@ -797,7 +850,13 @@ pub fn lower_class_decl( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - setters.push((prop_name, func)); + record_class_accessor( + &mut setters, + &mut setter_statics, + prop_name, + func, + method.is_static, + ); } ast::MethodKind::Method => { let mut func = with_static_member_context(ctx, method.is_static, |ctx| { @@ -915,7 +974,13 @@ pub fn lower_class_decl( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - getters.push((prop_name, func)); + record_class_accessor( + &mut getters, + &mut getter_statics, + prop_name, + func, + method.is_static, + ); } ast::MethodKind::Setter => { let prop_name = format!("#{}", method.key.name); @@ -924,7 +989,13 @@ pub fn lower_class_decl( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - setters.push((prop_name, func)); + record_class_accessor( + &mut setters, + &mut setter_statics, + prop_name, + func, + method.is_static, + ); } } } @@ -1577,6 +1648,10 @@ pub fn lower_class_from_ast( let mut static_methods = Vec::new(); let mut getters = Vec::new(); let mut setters = Vec::new(); + // Parallel staticness, so `record_class_accessor` can tell a static + // accessor from an instance one with the same name. + let mut getter_statics: Vec = Vec::new(); + let mut setter_statics: Vec = Vec::new(); let mut static_accessor_names: Vec = Vec::new(); let mut static_accessor_fn_ids: Vec = Vec::new(); let mut computed_members = Vec::new(); @@ -1663,7 +1738,13 @@ pub fn lower_class_from_ast( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - getters.push((prop_name, func)); + record_class_accessor( + &mut getters, + &mut getter_statics, + prop_name, + func, + method.is_static, + ); } ast::MethodKind::Setter => { let func = with_static_member_context(ctx, method.is_static, |ctx| { @@ -1681,7 +1762,13 @@ pub fn lower_class_from_ast( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - setters.push((prop_name, func)); + record_class_accessor( + &mut setters, + &mut setter_statics, + prop_name, + func, + method.is_static, + ); } ast::MethodKind::Method => { let mut func = with_static_member_context(ctx, method.is_static, |ctx| { @@ -1779,7 +1866,13 @@ pub fn lower_class_from_ast( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - getters.push((prop_name, func)); + record_class_accessor( + &mut getters, + &mut getter_statics, + prop_name, + func, + method.is_static, + ); } ast::MethodKind::Setter => { let prop_name = format!("#{}", method.key.name); @@ -1788,7 +1881,13 @@ pub fn lower_class_from_ast( static_accessor_names.push(prop_name.clone()); static_accessor_fn_ids.push(func.id); } - setters.push((prop_name, func)); + record_class_accessor( + &mut setters, + &mut setter_statics, + prop_name, + func, + method.is_static, + ); } } } diff --git a/crates/perry/tests/duplicate_class_accessor_last_wins.rs b/crates/perry/tests/duplicate_class_accessor_last_wins.rs new file mode 100644 index 0000000000..e3e28a0234 --- /dev/null +++ b/crates/perry/tests/duplicate_class_accessor_last_wins.rs @@ -0,0 +1,111 @@ +//! End-to-end regression coverage for duplicate class accessors. +//! +//! ECMA-262 ClassDefinitionEvaluation installs class elements in source order, +//! so a later accessor with the same key REPLACES an earlier one. Perry's HIR +//! appended every accessor to `ClassDecl::getters` / `::setters`, and those are +//! consumed with `iter().find(...)` — first match wins — so the SHADOWED +//! definition stayed live and the real one was dropped. +//! +//! The shape below is reduced from Claude-of-Duty's `Spring3`, which pairs an +//! early `set z` (damping) with a later `get z` (displacement). Perry returned +//! the damping coefficient from the shadowed getter, which put a first-person +//! weapon 0.88 m behind the camera, where it clipped and rendered nothing. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +#[test] +fn a_later_class_accessor_replaces_an_earlier_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let entry = dir.path().join("main.ts"); + let output = dir.path().join("main_bin"); + std::fs::write( + &entry, + r#" +class Spring3 { + a = 111; + c = 222; + damping = 0; + + // Reading `.z` must reach the LAST getter; writing `.z` must still reach + // this setter, which no later definition replaces. + set z(v: number) { this.damping = v; } + get z(): number { return this.a; } + get z(): number { return this.c; } +} + +// Static and instance accessors are distinct properties and may share a name: +// replacing on the key alone would collapse them. +class Split { + static _s = "static-first"; + _i = "instance-first"; + static get v(): string { return Split._s; } + get v(): string { return this._i; } + static get v(): string { return "static-last"; } + get v(): string { return "instance-last"; } +} + +// A later setter replaces an earlier setter too. +class Sink { + hits: string[] = []; + set s(v: string) { this.hits.push("first:" + v); } + set s(v: string) { this.hits.push("last:" + v); } +} + +const spring = new Spring3(); +spring.z = 7; +console.log("spring", spring.z, spring.damping); + +console.log("split", new Split().v, Split.v); + +const sink = new Sink(); +sink.s = "x"; +console.log("sink", sink.hits.join("|"), sink.hits.length); + +// Accessors defined on a class EXPRESSION follow the same rule. +const Expr = class { + p = 1; + q = 2; + get w(): number { return this.p; } + get w(): number { return this.q; } +}; +console.log("expr", new Expr().w); +"#, + ) + .expect("write fixture"); + + let compile = Command::new(perry_bin()) + .current_dir(dir.path()) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .arg("--no-cache") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + let run = Command::new(&output).output().expect("run compiled fixture"); + assert!( + run.status.success(), + "compiled fixture failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + + // Matches `node --experimental-strip-types` on the same source. + let expected = "spring 222 7\n\ + split instance-last static-last\n\ + sink last:x 1\n\ + expr 2\n"; + assert_eq!(String::from_utf8_lossy(&run.stdout), expected); +}