Skip to content

fix(codegen): preserve observable nested array rows (#10733) - #11019

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/10733-nested-array-alias
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/10733-nested-array-alias

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Summary

Prevent the flat constant 2D array optimization from replacing observable JavaScript row objects. Candidates now remain on the normal heap path when a row escapes, is used as a value, or is written through, while direct element reads and read-only row aliases keep the flat table fast path.

Changes

  • add a context-aware use analysis for flat constant array candidates
  • reject returned rows, value-position aliases, nested writes, and compiler-lowered compound writes
  • preserve direct array[row][column] and const row = array[index]; row[column] optimization
  • add a regression fixture for row identity, returned-row mutation, stale reads, and compound assignment

Related issue

Fixes #10733

Test plan

  • cargo test -p perry-codegen --lib (1674 passed, 1 ignored)

  • cargo test -p perry-codegen collectors::mutation::tests --lib (8 passed after rebase)

  • cargo fmt --all -- --check

  • python3 scripts/check_test_registration.py

  • scripts/check_file_size.sh

  • release build of perry and perry-runtime-static on perrymaster.skelpo.net

  • Node/Perry output parity for test_issue_10733_nested_array_alias.ts

  • LLVM trace: regression uses the heap representation; image convolution still emits @perry_flat_image_conv_ts__4 and direct GEP loads

  • cargo build --release clean

  • cargo test --workspace --exclude perry-ui-ios --exclude perry-ui-tvos --exclude perry-ui-watchos --exclude perry-ui-gtk4 --exclude perry-ui-android --exclude perry-ui-windows passes

  • (if user-facing) Added or updated a test under test-files/ or a #[test] in the affected crate

  • (if CLI / stdlib / runtime API changed) Updated docs/src/

  • (if touching a platform UI backend) Built -p perry-ui-<backend> locally on that platform

Screenshots / output

value [1,2,3] true true
returned-write 99 [[1,99,3]]
compound 12 [[1,12,3]]

Checklist

  • I have NOT bumped the workspace version or edited CLAUDE.md / CHANGELOG.md (maintainer handles these at merge)
  • My commits follow the loose feat: / fix: / docs: / chore: prefix convention used in the log
  • I've read CONTRIBUTING.md and agree to the Code of Conduct

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 384ced14-9f62-4d19-9740-3cf1475148d2

📥 Commits

Reviewing files that changed from the base of the PR and between 57a6d60 and f891a81.

📒 Files selected for processing (5)
  • changelog.d/11019-nested-array-row-alias.md
  • crates/perry-codegen/src/codegen/mod.rs
  • crates/perry-codegen/src/collectors/mod.rs
  • crates/perry-codegen/src/collectors/mutation.rs
  • test-files/test_issue_10733_nested_array_alias.ts

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

Pulled from merge train 257. Three gap tests regress pass -> parity_fail with this in the train, and this is the only PR in 35 that touches the codegen mutation/collector path the three share:

test_gap_numeric_push_guarded            node exit 0, perry exit 1 (first line matches, throws later)
test_gap_rest_bundle_and_map_fill        node exit 0, perry exit 1 (first line matches, throws later)
test_gap_7541_array_subclass_inherited_statics   node "[ 1, 2, 3 ]"  vs  perry "TypeError: value is not a function"

Run 35787084787, shards 2/3/4, fast mode, against the committed Linux snapshot.

Attribution is by file ownership, not bisect, so treat it as strong rather than proven: crates/perry-codegen/src/collectors/mutation.rs (new, 263 lines), collectors/mod.rs and codegen/mod.rs are touched by this PR and by nothing else in the train.

TypeError: value is not a function is worth your attention specifically. In this codebase that string is the classic symptom of a GC-rooting defect — a rooted slot holding a pointer the collector already moved — and it surfaces cycles later in unrelated code, which is exactly the shape here (correct output first, throw afterwards). See CLAUDE.md's "Root-store dominance in codegen": a GC-managed value's root store must dominate every later site that can collect. If the new mutation collector changes which array rows are materialised or aliased, it can move a value's live range past its root store without any local error.

Suggested checks, cheapest first:

  1. Run those three fixtures against this branch and against main. All three pass on main.
  2. scripts/gc_root_dominance_check.py over --trace llvm output for the nested-array fixture — the static checker for exactly this class.
  3. If it reproduces, --trace hir --focus on the function that throws will show whether the row survived lowering.

The underlying fix (#10733, preserving observable nested array rows) is wanted — this is about the three regressions, not the goal. Ping me when they're green and it rides the next train.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main in merge train 265 (#11108), released as v0.5.1648 at 9d26936298.

Cherry-picked from this PR's head f891a813ad and validated as one tree with 15 other PRs — CI 22/22 green, all 6 gap-suite shards. A train rebase gives the commits new SHAs, so GitHub cannot auto-close the source PR; closing by hand.

Nothing needed from you. Thanks.

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.

correctness: a const-bound array literal containing an inline array literal reads undefined, serves stale data, and silently drops compound assignments

1 participant