Skip to content

fix(runtime,codegen): counted loops over an Array subclass read element 0 as the object's meta word - #8976

Merged
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/elements-loop-guard-kind
Aug 28, 2026
Merged

fix(runtime,codegen): counted loops over an Array subclass read element 0 as the object's meta word#8976
proggeramlug merged 2 commits into
PerryTS:mainfrom
proggeramlug:fix/elements-loop-guard-kind

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

The bug (live on main since #8974)

class Query extends Array {}
class Archetype extends Array {}
function make(source) {
  const query = source;
  return () => {
    let text = "";
    for (let i = 0, length = query.length; i < length; i++) {
      const current = query[i];
      for (let j = 0, length = current.length; j < length; j++) text += current[j] + ",";
    }
    return text;
  };
}
const dense = new Query(); const row = new Archetype();
row.push(1); row.push(2); row.push(3); dense.push(row);
console.log("loop=" + make(dense)());
node:        loop=1,2,3,
merged main: loop=1.271897499676e-311,2,3,
this PR:     loop=1,2,3,

1.27e-311 is a bare heap pointer read as a double — the object's meta word.

Cause

The loop guard resolved an elements-backed receiver (#8966) to its elements store and published it as kind 1. Kind 1 means "the receiver IS the ArrayHeader": the generated loop computes receiver + header + i*8 itself, and on the ordinary (non-capture) path fast_raw is the receiver's own address — the object, not the payload. So element 0 read object + 8 (the meta pointer) while later elements happened to land on real data. It was invisible while the store was gated off; #8974 made it the default representation, and issue_8773_closure_capture_packed_loops's dense case catches it with the gate on.

Fix

Elements-backed receivers get their own kind (3):

  • every proof is the store's — no descriptors, prototype latch clear, bound <= length <= capacity, and the whole-array raw-f64 bit when a numeric mode is requested;
  • the live address stays the receiver, so the capture-safe caller keeps reloading the binding it owns;
  • the payload address is published in descriptor word 3 (unused by the other kinds), and codegen's two plain-payload sites take their base from it (plain_payload_base) — this is what makes both the capture path and the ordinary path read the payload rather than the object;
  • revalidation re-resolves the store from the receiver, refreshes word 3 after an evacuation (contents unchanged, address moved), and side-exits when an append re-allocates it — exactly as a grown plain Array does;
  • mode 2 (the fused ECS entity-id clone) declines, as it already does for plain Arrays.

Verification

  • The reproducer above matches node exactly, with the loops admitted (not declined).
  • Runtime unit test the_counted_loop_guard_admits_an_elements_backed_receiver_as_kind_three: kind 3, live address == receiver, word 3 == store, bound == length, revalidation stable, then a re-allocating append → side exit → fresh admission tracks the new store.
  • Integration: issue_8773_closure_capture_packed_loops 4/4, issue_8690_loop_versioned_arraylike 3/3, issue_8655_array_subclass_indexing 2/2, issue_8772_short_packed_spread 5/5.
  • cargo test -p perry-runtime --lib 2779/0; RUSTFLAGS=-D warnings cargo check --workspace --all-targets clean. (temp_root_operand_temporaries::string_literal_concat_operand_is_re_derived_below_the_allocating_sibling fails on plain origin/main too — unrelated.)
  • Mac mini, 11 alternating pairs vs merged main: add/remove −0.14% (2 s, 11/11) / −0.19% (50 ms), entity cycle ±0.06% — i.e. perf-neutral on these benchmarks; the point is the correctness of loops over subclass instances, which now stay on the versioned path instead of falling back.

https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ

Summary by CodeRabbit

  • Bug Fixes
    • Fixed counted loops over Array subclasses so elements-backed arrays read the correct values, including element 0.
    • Improved loop handling when array storage moves or is reallocated, keeping payload access accurate and reliable.

…ops as their own kind

The loop guard resolved an elements-backed receiver to its store and
published it as kind 1. Kind 1 means "the receiver IS the ArrayHeader",
and the generated loop computes `receiver + header + i*8` itself — on the
ordinary (non-capture) path `fast_raw` is the receiver's own address — so
element 0 read the object's `meta` word as a double (`1.8e-311`) while
later elements happened to land on real data.
`issue_8773_closure_capture_packed_loops`'s dense case caught it once the
store became the default representation (PerryTS#8974).

Elements-backed receivers are now admitted as **kind 3**: every proof is
the store's (no descriptors, prototype latch clear,
`bound <= length <= capacity`, and the whole-array raw-f64 bit for a
numeric mode), the live address stays the RECEIVER so the capture-safe
caller keeps reloading the binding it owns, and the payload address is
published in descriptor word 3. Codegen's two plain-payload sites take
their base from that word (`plain_payload_base`), so both the capture path
and the ordinary path read the payload. Revalidation re-resolves the store
from the receiver, refreshes word 3 after an evacuation, and side-exits
when an append re-allocates it — exactly as a grown plain Array does.
Mode 2 (the fused ECS entity-id clone) declines, as it does for plain
Arrays.

Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fd03f1a7-4cd6-44ad-9902-6042e004b4b2

📥 Commits

Reviewing files that changed from the base of the PR and between 685da8d and 50d1a8c.

📒 Files selected for processing (1)
  • changelog.d/8976-elements-loop-guard-kind.md

📝 Walkthrough

Walkthrough

Elements-backed Array subclasses now use loop-guard kind 3. The receiver remains the live address, while descriptor word 3 stores the elements payload address. Code generation and revalidation use this address and refresh it after storage relocation.

Changes

Elements-backed counted loops

Layer / File(s) Summary
Kind-3 admission and revalidation
crates/perry-runtime/src/array/subclass_loop_guard.rs
The loop guard admits elements-backed receivers as kind 3. It publishes the elements store metadata and payload address, returns the receiver as the live address, and refreshes the payload during revalidation.
Payload selection in loop access
crates/perry-codegen/src/stmt/stable_packed_loop.rs
Numeric and fallback indexed access treats kinds 1 and 3 as plain payloads. Kind 3 access uses descriptor word 3 as the payload base.
Admission and relocation coverage
crates/perry-runtime/src/array/subclass_elements_tests.rs, changelog.d/8976-elements-loop-guard-kind.md
Tests verify kind-3 admission, receiver-based liveness, payload tracking, append reallocation, and re-admission. The changelog records the fix.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 685da

The change fixes counted loops over Array subclasses so they read actual element values instead of object metadata, while preserving safe fallback behavior when storage changes. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. (1 skipped: … 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 bug fix for counted loops over Array subclasses.
Description check ✅ Passed The description is detailed and covers the bug, cause, fix, and verification results. It does not use the repository template headings or include an explicit checklist, related-issue section, or scree…
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.
Full details: Description check

Explanation

The description is detailed and covers the bug, cause, fix, and verification results. It does not use the repository template headings or include an explicit checklist, related-issue section, or screenshots section, but the substantive information is mostly complete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. (1 skipped: 1 unsupported.)

  • 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

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

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Merged. I reproduced this end to end rather than trusting the test, because a silent wrong-value bug deserves to be seen:

output
node 26.5.1 loop=1,2,3,
pristine main loop=1.0864632502273e-311,2,3,
with this PR loop=1,2,3,

That 1.08e-311 is exactly what you describe — the bare heap pointer read as a double, element 0 reading object + 8.

The fix is the right shape. Giving elements-backed receivers their own kind rather than overloading kind 1 is what makes the two paths agree: keeping the live address as the receiver preserves the capture-safe reload, while publishing the payload in descriptor word 3 gives both the capture path and the ordinary path the same base. Refreshing word 3 after an evacuation (contents unchanged, address moved) and side-exiting when an append re-allocates are the two cases that would otherwise reintroduce a stale base.

Worth recording how this got in, since I merged it. I landed #8974 an hour ago on the strength of all three knob states at 2779/0 plus its whole-corpus differential — 1386 files, 9 differences, none attributable. Both were true and neither caught this. The shape needs a nested counted loop over a subclass instance reached through a closure capture, and it corrupts only element 0, so a corpus diff over fixtures that mostly do not nest this way stays clean. #8966 hid it behind the gate; #8974 made it the default. A green corpus differential is evidence about the fixtures in it, not about the representation.

Validation — runtime 2779/0 and codegen 1341/0 (RUST_TEST_THREADS=1); PERRY_ARRAY_SUBCLASS_ELEMENTS=0 also 2779/0, so the kill switch still bisects; issue_8773_closure_capture_packed_loops 4 passed on the integration tier, built with the coherent set (-p perry -p perry-runtime-static -p perry-stdlib-static -p perry-ext-net); scripts/run_lint_gates.sh 57 of 58 with the compile tier green — the exception is the pre-existing Actions-expression artifact (#8929).

One fix pushed: the fragment used the 0000 placeholder; renamed to 8976-. That is the third today, after #8973 cleared three off main — the convention is easy to miss, and the collision is silent until release time.

@proggeramlug
proggeramlug merged commit 3fca591 into PerryTS:main Aug 28, 2026
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant