Skip to content

fix(hir): a later class accessor replaces an earlier one #9886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
proggeramlug wants to merge 1 commit into from
Closed

fix(hir): a later class accessor replaces an earlier one #9886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 107 additions & 8 deletions crates/perry-hir/src/lower_decl/class_decl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<bool>,
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,
Expand Down Expand Up @@ -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<bool> = Vec::new();
let mut setter_statics: Vec<bool> = Vec::new();
let mut static_accessor_names: Vec<String> = Vec::new();
let mut static_accessor_fn_ids: Vec<FuncId> = Vec::new();
let mut computed_members = Vec::new();
Expand Down Expand Up @@ -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
Expand All @@ -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| {
Expand Down Expand Up @@ -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);
Expand All @@ -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,
);
}
}
}
Expand Down Expand Up @@ -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<bool> = Vec::new();
let mut setter_statics: Vec<bool> = Vec::new();
let mut static_accessor_names: Vec<String> = Vec::new();
let mut static_accessor_fn_ids: Vec<FuncId> = Vec::new();
let mut computed_members = Vec::new();
Expand Down Expand Up @@ -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| {
Expand All @@ -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| {
Expand Down Expand Up @@ -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);
Expand All @@ -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,
);
}
}
}
Expand Down
111 changes: 111 additions & 0 deletions crates/perry/tests/duplicate_class_accessor_last_wins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
Loading