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

[proggeramlug]

The bug

ECMA-262 ClassDefinitionEvaluation installs class elements in source order, so a second get x / set x replaces the first. Perry keeps the first.

ClassDecl::getters / ::setters are consumed with iter().find(...) — first match wins — but every accessor was appended with push(). The shadowed definition stays live and the one the program actually defines last is silently dropped.

Reproducer

class Vec {
  a = 111;
  c = 222;
  damped = 0;

  set z(v: number) { this.damped = v; }
  get z(): number { return this.a; }   // shadowed
  get z(): number { return this.c; }   // must win
}

const v = new Vec();
v.z = 7;
console.log(v.z, v.damped);

	output
node --experimental-strip-types	222 7
perry (before)	111 7
perry (after)	222 7

Note the setter still runs in every case: when a later definition supplies only a getter, the earlier setter stays in force. That asymmetry is the point — see below.

Every accessor shape is affected

Measured against Node on the fixture in the regression test:

shape	before	expected
instance getter	111	222
static getter	static-first	static-last
instance getter alongside a static one	instance-first	instance-last
duplicate setters	first:x	last:x
accessors on a class expression	1	2

There is no diagnostic. The program reads a plausible value from the wrong accessor and keeps running.

How it surfaced

Claude-of-Duty's Spring3 (src/weapons/mathx.js) pairs an early set z (damping) with a later get z (displacement):

export class Spring3 {
  constructor(f = 12, z = 1) { this.a = new Spring(f, z); /* … */ }
  set z(v) { this.a.z = this.b.z = this.c.z = v; }  // damping in
  get z()  { return this.a.z; }                     // shadowed
  // …
  get z()  { return this.c.x; }                     // displacement out — must win
}

Unusual, but legal, and it depends on exactly the read/write asymmetry the spec produces. Perry served the shadowed damping getter, so:

lag.z    = 0.4600   ← new Spring3(5.4, 0.46) damping
recPos.z = 0.4200   ← new Spring3(9,   0.42) damping

Those are added as displacements, contributing +0.880 m to the first-person viewmodel's Z. The rig moved from its authored hipPos.z = -0.300 (in front of the eye) to +0.580 (behind it):

rig.pos = 0.117, -0.186, +0.580
basePos = 0.118, -0.185, -0.300

All 156 viewmodel nodes then sat at camera-space Z ≥ 0 and clipped. The failure was invisible from the renderer's side: the overlay pass ran, the gate passed, all 156 draw_indexed calls were issued, and the pass's colour reached the screen — with zero fragments. lag.x and lag.y were correct throughout, because only .z collides with the damping accessor.

The change

record_class_accessor overwrites an existing entry instead of appending.

The replacement is keyed on (name, is_static), not on the name alone: a static and an instance accessor may legally share a name and are distinct properties — one on the constructor, one on the prototype. Deduping on the key alone would trade this bug for another, which is why the regression test includes a class carrying both.

Verification

• cargo test --release -p perry-hir — 620 passed, 0 failed
• cargo test --release -p perry-codegen — 1912 passed, 0 failed
• New test duplicate_class_accessor_last_wins covers all five shapes above, with expected output taken from Node. Reverting the one-line lowering change fails it on every shape:

left:  "spring 111 7\nsplit instance-first static-first\nsink first:x 1\nexpr 1\n"
right: "spring 222 7\nsplit instance-last static-last\nsink last:x 1\nexpr 2\n"

Summary by CodeRabbit

• Bug Fixes

  • Fixed duplicate class accessors so later getter or setter definitions correctly replace earlier ones.
  • Preserved separate behavior for static and instance accessors with the same name.
  • Applied consistent accessor handling to class declarations and class expressions.

• Tests

  • Added regression coverage verifying duplicate accessor resolution and runtime behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 92f13838-7b72-4544-b74b-e0fbdbe54a1f

📥 Commits

Reviewing files that changed from the base of the PR and between 890514a and f572ce9.

📒 Files selected for processing (2)

• crates/perry-hir/src/lower_decl/class_decl.rs
• crates/perry/tests/duplicate_class_accessor_last_wins.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.

---

📝 Walkthrough

Walkthrough

Class lowering now replaces duplicate getters and setters by name and staticness. The change applies to declarations and class expressions. A new end-to-end test verifies later-definition behavior for instance, static, private, and class-expression accessors.

Changes

Class accessor replacement

Layer / File(s)	Summary
Accessor registration crates/perry-hir/src/lower_decl/class_decl.rs	Adds record_class_accessor, tracks accessor staticness, and uses replacement logic in declaration and class-expression lowering paths.
Accessor regression coverage crates/perry/tests/duplicate_class_accessor_last_wins.rs	Adds a fixture and end-to-end assertions for duplicate accessors, static and instance accessors, and class expressions.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to f572c

This change makes later duplicate class accessors override earlier definitions while preserving distinct static and instance accessors. Regression coverage and successful test suites support merge readiness with no remaining actionable risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: later class accessors replace earlier definitions.
Description check	✅ Passed	The description provides a detailed bug explanation, reproducer, expected behavior, implementation details, regression coverage, and verification results. It does not use the template headings or incl…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Hold off merging — this regresses a real workload and I am still diagnosing.

Building Claude-of-Duty with this patch fails during world construction:

[boot] init failed TypeError: Cannot read properties of undefined (reading 'clone')
    at BufferGeometry.copy
    at PlaneGeometry.copy
    at BufferGeometry.clone
    at rollerShutter        (src/world/kit.js:661)

A/B against the same game source, same perry-three, same Bloom: the identical build without this patch boots past that point, and with it fails. So the regression is this change, not the workload.

It is not the fix behaving correctly on a duplicate accessor in three.js — I scanned three.core.js and only AnimationMixer declares duplicate accessors, nothing near BufferGeometry. So something else in the lowering is wrong; the parallel statics vector bookkeeping in record_class_accessor is my main suspect.

One more unexplained signal: the compiled binary grows from 106 MB to 151 MB with this patch. A change that only drops shadowed accessors should not add code, so lowering is shifting in some broader way I have not accounted for.

perry-hir (620) and perry-codegen (1912) are both green, so the unit suites do not cover whatever this is. I will either post a corrected patch or close this.

[proggeramlug]

Landed on main via merge train #9898. Validated as a tree: 64/64 lint gates, and perry-runtime/codegen/hir/stdlib all green (5,939 tests, 0 failures). Thanks!

[proggeramlug]

Correction — retracting my previous comment. This patch is not the cause of that regression, and the hold does not apply.

My earlier A/B was not controlled. The working build came from d36a1af0c plus local patches; the failing build came from current origin/main, 108 commits later. I attributed the failure to this patch without isolating that variable, which was wrong.

Properly controlled now — same base, same game source, only this patch differing:

build	binary	boot
base, patch reverted	151.0 MB	TypeError: Set.prototype.add is not a function
base + this patch	151.0 MB	gets further, then fails in rollerShutter

Both fail, and the binary is byte-identical in size, so neither the failure nor the 106 MB -> 151 MB growth I flagged comes from this change. Both belong to the 108 upstream commits. Notably this patch gets the game past the Set.prototype.add failure.

So the two concerns in my previous comment are both withdrawn:

• the rollerShutter failure is present without this patch
• the binary growth is present without this patch

The patch stands on its original evidence: perry-hir 620 passed, perry-codegen 1912 passed, and the regression test fails on all five accessor shapes without it.

Separately, and worth raising on its own: current main does not boot Claude-of-Duty at all. I will file that with a reproducer rather than leave it buried in this thread.