Skip to content

fix: preserve fractional keys in dynamic array reads - #10229

Closed
proggeramlug wants to merge 2 commits into
mainfrom
codex/10190-fractional-array-index
Closed

proggeramlug wants to merge 2 commits into
mainfrom
codex/10190-fractional-array-index

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

JSON.parse('[{"id":1},{"id":2},{"id":3}]')[0.5] returned the first element because the dynamic runtime fallback truncated its numeric key to i32. It now returns undefined, or resolves an own/inherited "0.5" property when one exists.

Changes

  • Validate that the key is a finite, non-negative integer within the fallback's i32 range before narrowing it.
  • Route fractional, negative, non-finite, and large keys through the existing polymorphic property lookup with their original value. Preserve boxed integer decoding and valid array indices above i32::MAX.
  • Add runtime coverage for ordinary, lazy, and materialized arrays, named properties, and boxed integer keys. The same tests exercise the packed array-like fallback used by generated code.
  • Add a Node parity fixture for the reported JSON case, missing properties, own/inherited properties, typed and erased receiver paths, and index-range boundaries. Compiler inline tiers remain unchanged.

Related issue

Fixes #10190

Test plan

  • Both initial runtime regressions failed before the fix, and a freshly built base compiler reproduced the wrong JSON element and named-property reads.
  • RUST_TEST_THREADS=1 cargo test -p perry-runtime --lib dynamic_numeric_key_tests passes (3 tests).
  • RUST_TEST_THREADS=1 cargo test -p perry-runtime --lib index passes (168 tests).
  • Built the compiler and matching runtime/stdlib archives together using cargo build -p perry -p perry-runtime-static -p perry-stdlib-static in the default dev profile; verified all three artifact timestamps follow the runtime edit.
  • Compiled test_gap_10190_fractional_array_index.ts and test_gap_array_index_range_proof.ts with those archives and --no-auto-optimize; both outputs match Node 26.5.1 byte-for-byte.
  • Formatting and git diff --check pass. scripts/pre-tag-check.sh --quick passes all checks except the existing public benchmark artifact freshness failure; benchmark fingerprint inputs are unchanged from base 6000a00dfe.

Checklist

  • No workspace version bump or changes to Cargo.lock, CLAUDE.md, or CHANGELOG.md.
  • Added a changelog fragment and registered the new Rust test module.
  • Commits use the repository's conventional prefixes.
  • Read the contribution guide and Code of Conduct.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed array lookups for fractional, negative, non-finite, and oversized numeric keys.
    • Numeric keys that are not valid array indices now correctly access named properties instead of being truncated to an element index.
    • Preserved support for valid array indices, including large unsigned indices and boxed integer keys.
    • Corrected behavior across regular, lazy, materialized, and JSON-backed arrays.

CI follow-up

CI is red. The logs show the runtime custom-thread-stack test failure, generated API-doc drift, benchmark freshness failure, existing warnings, and the separate TLS-checker failure. The independent branches #10229, #10230, and #10232 report the same eight gap failures: iterator_prototype_next_patch (Node oracle exit 1), 2899_2779_2777_static_helpers, disposablestack_2875, gc_http2_pending_event_callback_rooting (port 443 permission denial), 2159_defineproperty_class_prototype (Perry exit 13), json_lazy_defineproperty_index (descriptor-read assertion), perfhooks_3088_3008_3010_3011, and v8_2_3680plus (Perry exit 1). The HTTP/2 failure also appears in all seven GC-stress arms. This concurrence supports a shared base/environment cause; it is not an exact-main A/B rerun. The PR does not have green CI.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 63845927-8016-42d8-96d6-ba6031c12871

📥 Commits

Reviewing files that changed from the base of the PR and between 6000a00 and bb667c5.

📒 Files selected for processing (5)
  • changelog.d/10229-fractional-array-index.md
  • crates/perry-runtime/src/array/dynamic_numeric_key_tests.rs
  • crates/perry-runtime/src/array/mod.rs
  • crates/perry-runtime/src/value/dyn_index.rs
  • test-files/test_gap_10190_fractional_array_index.ts

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


📝 Walkthrough

Walkthrough

The dynamic numeric-index fallback now preserves fractional, negative, non-finite, and oversized keys as property lookups. New runtime and regression tests cover regular, lazy, materialized, and JSON arrays.

Changes

Fractional array index fix

Layer / File(s) Summary
Runtime index validation
crates/perry-runtime/src/value/dyn_index.rs, crates/perry-runtime/src/array/*
The numeric-key guard routes fractional, negative, non-finite, and oversized keys through polymorphic property lookup before narrowing. Tests cover array states, named numeric properties, valid elements, and boxed integer keys.
Regression coverage and changelog
test-files/test_gap_10190_fractional_array_index.ts, changelog.d/10229-fractional-array-index.md
The regression test covers absent, own, inherited, and non-index numeric properties, plus the expected TypeError. The changelog documents the corrected lookup behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to bb667

The fractional array-key fix has no confirmed merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #10190 requires fractional keys to use property lookup. js_dyn_index_get now rejects fractional, negative, non-finite, and out-of-range values before the i32 cast. It passes the original key…
Out of Scope Changes check ✅ Passed The changed runtime logic directly fixes the linked indexed-read bug. The runtime tests, parity fixture, and changelog document and verify this fix. The additional handling for negative, non-finite, a…
Title check ✅ Passed The title clearly and concisely describes the main change: preserving fractional keys during dynamic array reads.
Description check ✅ Passed The description includes the required Summary, Changes, Related issue, Test plan, and Checklist sections. It explains the fix, test coverage, verification commands, and known CI failures. The optional…
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/10190-fractional-array-index

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

Landed via merge train 185r (#10242) at 9fda98d on main.

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.

rows[0.5] reads rows[0] instead of undefined (fractional index treated as an integer index)

1 participant