perf(codegen): interp 0.780 -> 0.675 s, iso_miss 1.061 -> 0.967 s — let a declared array type reach the guarded element read and the inline .length - #7890
Merged
Conversation
…et a declared array type reach the guarded element read and the inline .length Two halves of one mechanism: what a program may do with an array type that came from an annotation rather than from an initializer that proved an array. A. `e.vals[i]` / `p.toks[p.pos]`. #7854 recovered a receiver's declared property type for a LOCAL (`const names = e.names`), never for the read used directly as a receiver — the HIR types a PropertyGet off a UNION receiver as `Any`, so `index_get.rs` routed those to `js_dyn_index_get`. The tier this unlocks, `lower_guarded_array_index_get`, re-checks GC_TYPE_ARRAY, forwarding, descriptors, the prototype latch and the bounds on the receiver itself, so a violated claim costs a branch and returns the same answer. B. `.length` no longer refuses a declared-only array local. #7854 refused them because the arm's fallback was `js_value_length_f64`, which answered 0 where JS answers `undefined` and did not throw on a nullish receiver (#7853). #7862 replaced that fallback with `js_value_length_property_f64` and left the refusal standing. `declared_only_array_locals` and `refined_array_type_is_declared_only` are deleted with it; `declared_only_numeric_locals` (#7773) is untouched. Quiet M1 mini, best-of-5, exit-checked: interp 0.7796 -> 0.6748 (-13.4%), iso_miss 1.0607 -> 0.9670 (-8.8%). 17 of the 19 corpus programs compile byte-identically and the two that differ are exactly the two with a type-alias over an array; noise floor from those 17 is +-1.0%. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
📝 WalkthroughWalkthroughDeclared array and tuple property claims now enable guarded element reads and inline ChangesDeclared array claim handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant index_get
participant declared_array_property_claim
participant guarded_array_fast_path
index_get->>declared_array_property_claim: check receiver property claim
declared_array_property_claim-->>index_get: return array or tuple claim
index_get->>guarded_array_fast_path: lower guarded element read
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…ment-read receiver Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
changelog.d/7890-declared-array-claim-element-reads.md (1)
1-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrite one release-note entry for the shipped behavior.
Replace the “Round 7” framing and separate A/B development slices with one coherent summary of the guarded declared-array reads and corrected
.lengthfallback. Keep the validation details after that summary.Based on learnings: describe the final shipped behavior as one coherent release-note entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@changelog.d/7890-declared-array-claim-element-reads.md` around lines 1 - 60, Rewrite the changelog entry’s opening to remove the “Round 7” framing and merge sections A and B into one concise release-note summary covering guarded element reads from declared array properties and corrected .length semantics for declared-only array locals. Preserve the existing validation details and sabotage/test coverage after the unified summary, without retaining separate development-oriented sections.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/expr/index_get.rs`:
- Line 1178: Update the claimed_array handling around is_array_expr so
non-numeric and runtime-key accesses do not unbox an unchecked receiver before
js_array_get_index_or_string. Route claimed receivers through the boxed
fallback, or add an equivalent runtime array guard, while preserving the
existing direct path for proven arrays.
---
Nitpick comments:
In `@changelog.d/7890-declared-array-claim-element-reads.md`:
- Around line 1-60: Rewrite the changelog entry’s opening to remove the “Round
7” framing and merge sections A and B into one concise release-note summary
covering guarded element reads from declared array properties and corrected
.length semantics for declared-only array locals. Preserve the existing
validation details and sabotage/test coverage after the unified summary, without
retaining separate development-oriented sections.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec3599c5-08ff-46fa-a1e8-eaf7f57b6a5b
📒 Files selected for processing (11)
changelog.d/7890-declared-array-claim-element-reads.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/src/type_analysis/refine.rs
💤 Files with no reviewable changes (5)
- crates/perry-codegen/src/codegen/closure.rs
- crates/perry-codegen/src/codegen/entry.rs
- crates/perry-codegen/src/codegen/function.rs
- crates/perry-codegen/src/codegen/method.rs
- crates/perry-codegen/src/expr/mod.rs
| // 3. Anything else → fall back to dynamic object field | ||
| // access by stringifying the index at runtime | ||
| if is_array_expr(ctx, object) { | ||
| if is_array_expr(ctx, object) || claimed_array { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/perry-codegen/src/expr/index_get.rs --match lower --view expanded
rg -n -C 10 \
'\b(js_array_get_index_or_string|lower_array_index_get_via_runtime_key|js_typed_feedback_array_index_get_fallback_boxed)\b' \
crates
rg -n -C 6 \
'declared_array_property_claim|claimed_array|test_gap_declared_field_type_refine_guarded|test_gap_7853_declared_array_length_runtime_value' \
crates test-files changelog.d 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- index_get.rs relevant lowering ---'
sed -n '1080,1275p' crates/perry-codegen/src/expr/index_get.rs
printf '%s\n' '--- runtime boxed fallback ---'
sed -n '2088,2165p' crates/perry-runtime/src/typed_feedback.rs
printf '%s\n' '--- array helper implementation ---'
sed -n '1615,1665p' crates/perry-runtime/src/array/indexing.rs
printf '%s\n' '--- claim definitions and consumers ---'
rg -n -C 8 'declared_array_property_claim|claimed_array|array_property_claim' \
crates/perry-codegen/src crates/perry-codegen/testsRepository: PerryTS/perry
Length of output: 25147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remaining array lowering ---'
sed -n '1230,1325p' crates/perry-codegen/src/expr/index_get.rs
printf '%s\n' '--- guarded array getter ---'
rg -n -A 120 -B 10 'fn lower_guarded_array_index_get' crates/perry-codegen/src/expr/index_get.rs
printf '%s\n' '--- claim implementation ---'
sed -n '90,150p' crates/perry-codegen/src/type_analysis/refine.rs
printf '%s\n' '--- unbox_to_i64 implementation and uses ---'
rg -n -A 20 -B 8 'fn unbox_to_i64|unbox_to_i64\(' crates/perry-codegen/src | head -240Repository: PerryTS/perry
Length of output: 5375
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- lower_guarded_array_index_get references and definition ---'
rg -n -C 5 'lower_guarded_array_index_get' crates/perry-codegen/src crates/perry-codegen/tests || true
printf '%s\n' '--- unbox_to_i64 definition ---'
rg -n -C 12 'fn unbox_to_i64' crates/perry-codegen/src || true
printf '%s\n' '--- all unbox_to_i64 call sites in index_get.rs ---'
rg -n -C 3 'unbox_to_i64' crates/perry-codegen/src/expr/index_get.rs || true
printf '%s\n' '--- imported lowering helpers at file start ---'
sed -n '1,120p' crates/perry-codegen/src/expr/index_get.rsRepository: PerryTS/perry
Length of output: 18751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
index_get = Path("crates/perry-codegen/src/expr/index_get.rs").read_text()
refine = Path("crates/perry-codegen/src/type_analysis/refine.rs").read_text()
runtime = Path("crates/perry-runtime/src/array/indexing.rs").read_text()
checks = {
"claim_is_restricted_to_property_get": (
"let Expr::PropertyGet" in refine
and "declared_array_property_claim" in refine
),
"claim_enters_array_branch": "if is_array_expr(ctx, object) || claimed_array" in index_get,
"non_numeric_branch_unboxes_receiver": (
"if !is_numeric_expr(ctx, index)" in index_get
and "unbox_to_i64(blk, &arr_box)" in index_get
),
"runtime_key_branch_unboxes_receiver": (
"if numeric_index_needs_runtime_key" in index_get
and index_get.count("unbox_to_i64(blk, &arr_box)") >= 2
),
"raw_helper_only_null_checks_receiver": (
"pub extern \"C\" fn js_array_get_index_or_string" in runtime
and "if arr.is_null()" in runtime
and "is_valid_obj_ptr" not in runtime[
runtime.index("pub extern \"C\" fn js_array_get_index_or_string"):
runtime.index("pub extern \"C\" fn js_array_get_index_or_string") + 250
]
),
}
for name, result in checks.items():
print(f"{name}={result}")
assert all(checks.values()), checks
print("reachable_claim_violation_raw_handle_path=True")
PYRepository: PerryTS/perry
Length of output: 394
Keep claimed receivers on guarded or boxed paths.
When claimed_array is true, the non-numeric and runtime-key branches unbox the receiver and call js_array_get_index_or_string. A violated array claim can therefore pass a non-array value as an ArrayHeader; this helper only checks for null and can read an invalid layout. Route these branches through the boxed fallback or add an equivalent receiver guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/expr/index_get.rs` at line 1178, Update the
claimed_array handling around is_array_expr so non-numeric and runtime-key
accesses do not unbox an unchecked receiver before js_array_get_index_or_string.
Route claimed receivers through the boxed fallback, or add an equivalent runtime
array guard, while preserving the existing direct path for proven arrays.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-files/test_gap_7890_declared_array_receiver_element_read.ts (1)
16-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tuple-property receiver coverage.
declared_array_property_claimaccepts both arrays and tuples. This test only declaresstring[]properties. Add a tuple property case that readse.items[0]directly. Include a malformed runtime receiver to verify the boxed fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_gap_7890_declared_array_receiver_element_read.ts` around lines 16 - 30, Extend the receiver coverage in the declarations around Bag, IBag, and CBag with a tuple-typed property case that directly reads e.items[0]. Exercise the malformed runtime receiver as well, and assert that declared_array_property_claim uses the boxed fallback for that case while preserving the existing array-property coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test-files/test_gap_7890_declared_array_receiver_element_read.ts`:
- Around line 16-30: Extend the receiver coverage in the declarations around
Bag, IBag, and CBag with a tuple-typed property case that directly reads
e.items[0]. Exercise the malformed runtime receiver as well, and assert that
declared_array_property_claim uses the boxed fallback for that case while
preserving the existing array-property coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d405dd5c-c346-43a3-a490-0bed295c035b
📒 Files selected for processing (1)
test-files/test_gap_7890_declared_array_receiver_element_read.ts
proggeramlug
pushed a commit
that referenced
this pull request
Aug 11, 2026
This was referenced Aug 11, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 11, 2026
…claim (#7891) (#7893) The array arm's two key routes have different receiver-validation strength. Numeric goes through js_array_get_f64, which classifies the receiver (clean_arr_ptr / array_object_receiver) and answers correctly for a string, an array-like object, a typed array or a number. Static string/symbol goes through js_array_get_index_or_string -> array_get_property_by_key -> js_object_get_field_by_name, which has no string-receiver index arm and answers undefined for s["0"] where JS answers the character. So only the numeric route is claim-safe. #7890's claim now requires a non-string, non-symbol key; a string or symbol key keeps exactly the generic path it had before #7890. The undefined answer itself is pre-existing on main and reachable without any of this through a plain non-union declared receiver — tracked as #7891. interp and iso_miss read only numeric indexes, so the measured result is unchanged: all 19 corpus binaries are byte-identical to the ones timed for #7890. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 Co-authored-by: Ralph Küpper <ralph3@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 7 of the
interpcampaign (3.96 s → 1.893 → 1.499 → 1.237 → 0.844 → 0.784 → this).Two halves of one mechanism: what a program may do with an array type that came from an
annotation rather than from an initializer that proved an array.
A.
e.vals[i]/p.toks[p.pos]— a property read used directly as a receiver#7854 taught
refine_type_from_initto recover a receiver's declared property type for alocal (
const names = e.namesontype Env = { names: string[] }), which is whynames[i]is an inline element read today. It did nothing for the same read used directlyas the receiver —
e.vals[i],p.toks[p.pos]— because the HIR types aPropertyGetoffa UNION receiver as
Any(perry-hir/src/analysis/value_types.rs, theUnionarm), sostatic_type_ofanswersAnyandexpr/index_get.rsroutes the read to theunknown-receiver dispatcher
js_dyn_index_get.declared_array_property_claimanswers for that shape, andindex_get.rsconsumes it inexactly two places: it suppresses the
recv_unknownroute, and it admits the receiver to thearray arm.
It is a claim, not a proof, and the tier it unlocks is the one that tolerates a claim.
lower_guarded_array_index_getre-checksGC_TYPE_ARRAY, the forwarding flag, per-arraydescriptors, the prototype latch and the bounds on the receiver itself, and routes every
failure to
js_typed_feedback_array_index_get_fallback_boxed. A violated claim costs apredicted branch and returns the same answer — the deal #7854 records for element reads, and
the same guard #6132 relies on to make a typed-array-valued member receiver safe here.
B.
.lengthno longer refuses a declared-only array local#7854 recorded these locals in
FnCtx::declared_only_array_localsand had the inline.lengtharm refuse them. The reason was specific and correct at the time: the arm's inlinehalf was guarded, but its fallback was
js_value_length_f64, which answered 0 forevery value that carries no length where JS answers
undefined, and continued instead ofthrowing for a nullish receiver (#7853).
#7862 replaced that fallback with
js_value_length_property_f64— ordinary propertysemantics:
undefinedfor a missing property, the real value for a non-numeric one, normalobject / function / native / proxy dispatch, and a catchable
TypeErrorfor a nullishreceiver. It did not lift the refusal that existed only because of the old fallback.
This lifts it, and deletes the set and its classifier with it — a mode that no longer gates
anything is not a decision that has been made.
declared_only_numeric_locals(#7773) isuntouched and stays: its consumer is an arithmetic operator with no guarded fallback.
Measured — quiet M1 mini, best-of-5, exit-checked, under the token-guarded lock
Load 2.00 before / 1.81 after, zero foreign benchmark processes at both ends.
564bd997b)17 of the 19 corpus programs compile byte-identically (same basename, different
directories, both arms on the same
PERRY_RUNTIME_DIR— the change touches nothing underperry-runtime/perry-stdlib), and the two that differ are exactly the two containing atype-alias-over-array receiver. That is the no-regression evidence and the in-run noisefloor at once: ±1.0%.
vs node 26.5.1 (0.321 / 0.334):
interp2.63× → 2.10×,iso_miss3.18× → 2.90×.Verified live, not assumed
interp:js_dyn_index_getcall sites 1 → 0 (--trace llvm).test_gap_declared_field_type_refine_guarded.ts:plen.fastblocks 0 → 5.Sabotage — run, and RED
test-files/test_gap_declared_field_type_refine_guarded.tsandtest-files/test_gap_7853_declared_array_length_runtime_value.tsfeed astring[]-declaredlocal an array, a string, a number, an array-like object with a numeric
length, anarray-like object with a non-numeric
length, a function, a typed array,nullandundefined, through an alias, an interface and a class, and require node-identical output onevery row.
Planting
GC_TYPE_OBJECTinto the inline.lengtharm'shas_lengthpredicate turnsalias|len=7intoalias|len=1— the header'sobject_typeword read as a length —and both tests go red. They detect the exact hazard the refusal used to prevent.
(Worth recording:
test_gap_7853_…alone would have been vacuous for the refusal. Itsconst bag = makeBag(value)leavesbagatAny, soitemsnever entereddeclared_only_array_localsand that file already took the inline arm onmain. The_guardedfile is the one whoseplen.fastcount moves.)Other validation
node --experimental-strip-types+ exit 0, both arms.checksum 437840 misses 0on both arms, gating on the miss counter.PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=200 PERRY_GC_VERIFY_EVACUATION=1,with the instrument shown live — 50 retired sets on
iso_miss, 38 oninterp, 18 MB+protected per set.
re-compiled from branch HEAD after the sabotage was reverted and
cmps byte-identical.https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
Follow-up commit: coverage for the shape this PR actually adds
#7854's test always routes through an intermediate local (
const items = e.items),so it does not cover a
PropertyGetused directly as the receiver — which isexactly what half A adds.
test-files/test_gap_7890_declared_array_receiver_element_read.tsdoes:
e.items[i]/e.items.lengththrough atypealias, aninterface, a class,a nullable reassigned cursor and a nested chain, handed an array, a string, a number,
an array-like object with numeric and non-numeric
length, a typed array, a function,nullandundefined, plus negative / fractional / out-of-range indexes and a storethrough the same shape. Byte-identical to node on every row.
Live, not decorative: on that file the new arm's
arr.fastguarded-read blocks go11 → 15 and its
js_dyn_index_getcalls go 5 → 1.Summary by CodeRabbit
.lengthnow follows standard property behavior, with safe handling for non-arrays, missing properties, proxies, native objects, and nullish values.