fix(codegen): a non-numeric key on a Uint8Array/Buffer local is a property read, not a byte (#7700) - #7746
Conversation
📝 WalkthroughWalkthroughThe codegen now requires proven numeric keys before treating ChangesUint8Array numeric-key handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
5b4a5ed to
43f869d
Compare
…perty read, not a byte (#7700)
43f869d to
fe1ba7d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Cargo.toml`:
- Line 318: Remove the contributor-managed version updates from Cargo.toml lines
318-318 and CLAUDE.md lines 11-11. Leave both files’ existing version metadata
unchanged, since the PR-keyed changelog fragment already records the change and
maintainers manage release version updates.
In `@changelog.d/7746-uint8array-nonnumeric-key.md`:
- Line 7: Correct the affected-site count in the changelog paragraph to match
the listed locations: `integer_locals` ×2, `i32_locals`, `int_valued_ta_locals`
×3, and `not_bigint_locals`, which total seven sites.
In `@crates/perry-codegen/src/type_analysis/numeric.rs`:
- Around line 134-142: Update the Uint8ArrayGet branch in the numeric expression
analysis to use the shared byte-read key predicate instead of is_numeric_expr.
Preserve the existing BufferIndexGet, Uint8ArrayLength, and BufferLength
handling, while ensuring BigInt-preserving unary BitNot keys are treated as
property access rather than byte reads.
In `@test-files/test_gap_uint8array_nonnumeric_key_7700.ts`:
- Around line 49-53: Add a regression case alongside the existing numeric-key
read in the test, accessing the Uint8Array with the canonical numeric-string key
"1" (directly or through an any-typed key), while preserving the current
i32-index loop coverage.
🪄 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: e7f07f2a-47ab-44f7-b91b-dbb7c31352cd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
CLAUDE.mdCargo.tomlchangelog.d/7746-uint8array-nonnumeric-key.mdcrates/perry-codegen/src/collectors/byte_read_key.rscrates/perry-codegen/src/collectors/byte_read_key_tests.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/collectors/i32_locals.rscrates/perry-codegen/src/collectors/int_valued_ta_locals.rscrates/perry-codegen/src/collectors/integer_locals.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/collectors/not_bigint_locals.rscrates/perry-codegen/src/stmt/masked_window_region.rscrates/perry-codegen/src/type_analysis/numeric.rstest-files/test_gap_uint8array_nonnumeric_key_7700.ts
|
|
||
| [workspace.package] | ||
| version = "0.5.1435" | ||
| version = "0.5.1436" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove contributor-managed release-version edits.
The PR-keyed changelog fragment already records this change. Leave version metadata to the maintainer merge or release process.
Cargo.toml#L318-L318: remove the workspace version update.CLAUDE.md#L11-L11: remove the Current Version update.
Based on learnings, contributor PRs must leave Cargo.toml and CLAUDE.md version metadata to maintainers when a PR-keyed changelog.d/ fragment exists.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 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 `@Cargo.toml` at line 318, Remove the contributor-managed version updates from
Cargo.toml lines 318-318 and CLAUDE.md lines 11-11. Leave both files’ existing
version metadata unchanged, since the PR-keyed changelog fragment already
records the change and maintainers manage release version updates.
Source: Learnings
|
|
||
| `lower/expr_member/member_tail.rs` folds every non-STRING key on such a local onto `Expr::Uint8ArrayGet`, and six codegen collectors read that node as "a byte, hence a number" **with no regard for the key kind** — so the destination local was classified integer-valued, took an i32 slot, and `i32_from_indexed_get_lowered` applied `ToInt32(ToNumber(v))` to a function pointer. Codegen's own byte-path gate was already correct: `arrays_finds::lower_uint8array_get_i32` routes an unproven key to `js_object_get_index_polymorphic` and hands back a boxed JS value. The representation decision one level up is what discarded it, which is why only the *stored* form was wrong — `typeof u8[Symbol.iterator]` consumed directly was always right. | ||
|
|
||
| The key-kind condition now lives in one place, `collectors/byte_read_key.rs`, and is an **allowlist**: the key must be provably a number. The "not a string" blocklist it replaces is precisely what shipped this bug, by putting every key kind nobody enumerated — symbols first — on the byte-read side. Applied at all six sites (`integer_locals` ×2, `i32_locals`, `int_valued_ta_locals` ×3, `not_bigint_locals`) plus the three `type_analysis/numeric.rs` predicates that also mean "a raw double" (`is_numeric_expr`, `is_provably_not_bigint`, `integer_magnitude_bits`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the affected-site count.
Line 7 says “six sites.” The parenthetical lists seven sites: 2 + 1 + 3 + 1. Update the count or the list so the release note is accurate.
🤖 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/7746-uint8array-nonnumeric-key.md` at line 7, Correct the
affected-site count in the changelog paragraph to match the listed locations:
`integer_locals` ×2, `i32_locals`, `int_valued_ta_locals` ×3, and
`not_bigint_locals`, which total seven sites.
| // #7700: a `Uint8ArrayGet` is a BYTE read only when its key is numeric. | ||
| // This is the very test `arrays_finds::lower_uint8array_get_i32` applies | ||
| // to choose between the byte accessor and | ||
| // `js_object_get_index_polymorphic`, so the two cannot disagree about | ||
| // whether `u8[Symbol.iterator]` is a number — which matters wherever a | ||
| // `true` here means "a raw double": `fcmp`-based truthiness, `fadd` | ||
| // operands, the non-BigInt bitwise fast path. | ||
| Expr::Uint8ArrayGet { index, .. } => is_numeric_expr(ctx, index), | ||
| Expr::BufferIndexGet { .. } | Expr::Uint8ArrayLength(_) | Expr::BufferLength(_) => true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Under ECMAScript, does unary ~preserve a BigInt value, and what doesnew Uint8Array([1])[~1n] evaluate to?
💡 Result:
Under ECMAScript, the unary bitwise NOT operator (~) does not preserve a BigInt value in the sense that it returns the same value; rather, it performs a bitwise operation on the BigInt and returns a new BigInt [1][2]. Specifically, for a BigInt operand x, ~x returns the one's complement of x, which is mathematically equivalent to -x - 1n [2][3]. Regarding the expression new Uint8Array([4])[~1n]: 1. Evaluation of the index: The expression ~1n evaluates to -2n [1]. 2. Array access: In JavaScript, when you attempt to access an index of an array (including TypedArrays like Uint8Array) using a BigInt, the engine will attempt to convert that index to a number. However, negative indices are not valid for accessing elements within a TypedArray; they are treated as non-existent properties [5][6]. 3. Result: Accessing an out-of-bounds or invalid index on a TypedArray returns undefined [5]. Therefore, new Uint8Array([4])[~1n] evaluates to undefined [5][6].
Citations:
- 1: https://github.com/mdn/content/blob/main/files/en-us/web/javascript/reference/operators/bitwise_not/index.md
- 2: https://tc39.es/ecma262/2026/multipage/ecmascript-data-types-and-values.html
- 3: https://tc39.es/proposal-bigint/
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
- 6: https://mdn2.netlify.app/en-us/docs/web/javascript/reference/global_objects/uint8array/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'crates/perry-codegen/src/(type_analysis/numeric|collectors/(byte_read_key|uint8array_get_reads_byte|uint8array_get_reads_a_byte)|collectors\.rs|.*\.rs)$' | sed -n '1,120p'
echo "== numeric.rs outline =="
ast-grep outline crates/perry-codegen/src/type_analysis/numeric.rs --match is_numeric_expr --view expanded || true
ast-grep outline crates/perry-codegen/src/type_analysis/numeric.rs --match is_bigint_expr --view expanded || true
echo "== numeric.rs relevant sections =="
cat -n crates/perry-codegen/src/type_analysis/numeric.rs | sed -n '1,240p'
echo "== collector searches =="
rg -n "uint8array_get_reads_a_byte|byte_read_key|is_numeric_expr|is_bigint_expr|integer_locals|unsigned_i32_locals" crates/perry-codegen/src -S | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 47576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic semantic probe of shared classifier candidates =="
python3 - <<'PY'
from pathlib import Path
p = Path('crates/perry-codegen/src')
for f in sorted(p.rglob('*.rs')):
s = f.read_text(errors='ignore')
if 'uint8array_get_reads_a_byte' in s or 'byte_read_key' in s:
print(f'--- {f} ---')
for i, line in enumerate(s.splitlines(), 1):
if 'uint8array_get_reads_a_byte' in line or 'byte_read_key' in line or 'is_numeric_expr' in line or 'matches!' in line and ('local' in line or 'binary' in line or 'unary' in line):
lo=max(1,i-8); hi=min(len(s.splitlines()),i+14)
lines=s.splitlines()
for j in range(lo, hi+1):
print(f'{j:4}: {lines[j-1]}')
print()
PY
echo "== AST structural search for classifier == "
if command -v ast-grep >/dev/null 2>&1; then
ast-grep run --pattern '$$funcname($$expr, $$...)' --lang rust crates/perry-codegen/src/type_analysis/numeric.rs --view compact || true
else
echo "ast-grep not available"
fiRepository: PerryTS/perry
Length of output: 27285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe: model the deterministic logic described for is_numeric_expr
# vs Uint8ArrayGet key classifier inputs. No repository code is executed.
python3 - <<'PY'
def is_numeric_expr_local_any_or_int_any(k_type):
"""
Reflect the observed shared numeric classifier behavior for identifiers:
- Any unproven local is accepted as numeric here because the BigInt predicate cannot prove Any is BigInt.
- Integer proven locals are accepted.
"""
return k_type in {"Any", "Int32", "i32_literal", "usize_literal", "u64_literal"}
def is_uint8array_get_reads_byte_local_any_or_int_local(k_type):
"""
Shared classifier candidate: rejects keys whose identity is only Any unless other numeric proof exists.
For this probe we model Any local as the only uncertain shape and integer locals as accepted.
"""
return k_type in {"Int32", "i32_literal", "usize_literal", "u64_literal"}
cases = [
"local_any",
"local_int32",
"local_usize_literal",
]
for k_type in cases:
print(f"{k_type}: is_numeric_expr={is_numeric_expr_local_any_or_int_any(k_type)}, shared_byte_predicate={is_uint8array_get_reads_byte_local_any_or_int_local(k_type)}")
PYRepository: PerryTS/perry
Length of output: 349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== continue numeric.rs from 230 =="
cat -n crates/perry-codegen/src/type_analysis/numeric.rs | sed -n '230,370p'
echo "== continue byte_read_key.rs from 130 =="
cat -n crates/perry-codegen/src/collectors/byte_read_key.rs | sed -n '130,195p'
echo "== byte_read_key_tests =="
cat -n crates/perry-codegen/src/collectors/byte_read_key_tests.rs | sed -n '1,180p'
echo "== local handling in type_analysis/numeric.rs =="
cat -n crates/perry-codegen/src/type_analysis/numeric.rs | sed -n '520,670p'Repository: PerryTS/perry
Length of output: 27850
Do not route Uint8ArrayGet through is_numeric_expr for numeric-key checks.
is_numeric_expr treats ~k as numeric unless k is provably a BigInt, so u8[~k] can be admitted when k: any is a BigInt. ECMAScript unary BitNot preserves BigInt values, so ~k is not a number-valued key in that case and the read is property access, not a byte read. Use the shared byte-read key predicate instead.
🤖 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/type_analysis/numeric.rs` around lines 134 - 142,
Update the Uint8ArrayGet branch in the numeric expression analysis to use the
shared byte-read key predicate instead of is_numeric_expr. Preserve the existing
BufferIndexGet, Uint8ArrayLength, and BufferLength handling, while ensuring
BigInt-preserving unary BitNot keys are treated as property access rather than
byte reads.
| // The numeric-key byte read is unchanged — including the loop shape whose i32 | ||
| // representation this fix must not cost. | ||
| const i = 2; | ||
| const b = u8[i]; | ||
| console.log("byte:", b); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add canonical numeric-string coverage.
Line 51 covers a numeric local key. It does not cover a canonical numeric string key. Add a case such as u8["1"] or an any key holding "1". The PR objective requires numeric-string behavior, and changelog.d/7746-uint8array-nonnumeric-key.md identifies this shape as regression-sensitive.
🤖 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_uint8array_nonnumeric_key_7700.ts` around lines 49 - 53,
Add a regression case alongside the existing numeric-key read in the test,
accessing the Uint8Array with the canonical numeric-string key "1" (directly or
through an any-typed key), while preserving the current i32-index loop coverage.
Merging as v0.5.1436 — I verified both load-bearing claims independentlyThe behaviour fix
The hot path — measured, not taken on trustThis is the claim that mattered, because these collectors decide the Why the diagnosis is right, and the fix shape betterThe The lie is one level up: Replacing a blocklist with an allowlist is the right correction, not just a fix. "Not a string" put every key kind nobody enumerated — symbols first — on the byte-read side. "Provably a number" fails safe instead. Verified applied at all six sites ( Two things I want to keep from the writeupThe rejected alternative is documented with its regressions. Refusing the fold in The Unit tests rather than gap-only is also the right call, and for the stated reason: the gap suite is tag-gated, so a regression there sits red for days (#5960).
|
Gap suite: 515 tests, no regression from this changeRan the full gap suite ( Comparing to
Several of these are already known and tracked: A further 10 tests move Caveat stated plainly: this run used the |
1 participant
Fixes #7700.
The bug
A non-numeric key on a
Uint8Array/Buffer-typed local read a byte instead of a property — but only when the result was stored in a local:const it = u8[Symbol.iterator]; typeof itfunctionnumberfunctionconst k: any = "subarray"; typeof u8[k](stored)functionnumberfunctionconst k: any = "byteLength"; u8[k](stored)404u8.tag = {…}; const k: any = "tag"; u8[k](stored){"kind":"buffer"}0{"kind":"buffer"}u8.n = 1n; const k: any = "n"; typeof u8[k]bigintnumberbiginttypeof u8[Symbol.iterator](consumed directly)functionfunctionfunctionThat last row is the tell, and it is what narrows the diagnosis: codegen's byte-path gate was already correct.
arrays_finds::lower_uint8array_get_i32routes an unproven key tojs_object_get_index_polymorphic— which dispatches numeric keys to the byte read and everything else to the property path — and hands back a boxed JS value.The lie is one level up.
lower/expr_member/member_tail.rsfolds every non-STRING key on such a local ontoExpr::Uint8ArrayGet, and six codegen collectors read that node as "a byte, hence a number" with no regard for the key kind. So the destination local was classified integer-valued, took an i32 slot, andi32_from_indexed_get_loweredappliedToInt32(ToNumber(v))to a function pointer. Consumed directly, there is no local, no i32 slot, and nothing to coerce.The fix
The key-kind condition now lives in one place —
collectors/byte_read_key.rs— and it is an allowlist: the key must be provably a number. The "not a string" blocklist it replaces is exactly what shipped this bug, by putting every key kind nobody enumerated — symbols first — on the byte-read side.Applied at all six sites the issue names (
integer_locals×2,i32_locals,int_valued_ta_locals×3,not_bigint_locals) plus the threetype_analysis/numeric.rspredicates that also mean "this is a raw double":is_numeric_expr(drivesfcmptruthiness andfaddoperands),is_provably_not_bigint, andinteger_magnitude_bits.The hot path is unchanged — measured, not asserted
The issue flags this as "a measured change, not a soundness patch", because these collectors decide the
buf[i]i32 fast path, the hottest buffer code in the compiler. It is: gating on declared types alone demotes the loop.A
for (let i = …) sum += buf[i]counter is a bodylet, and thebinding_typesmap the collectors are handed covers only params and module globals. Keying the predicate on that map produced a real regression —js_uint8array_get/i32-slot →js_uint8array_index_get_value/double-slot, visible in the emitted IR. Socollect_numeric_typed_localswalks the body for declared numeric types (includingfor-init counters), and that set is the evidence.With it, the emitted LLVM IR for a four-loop buffer fixture (FNV-1a hash, byte sum, masked mix,
Buffer→Buffercopy) is byte-identical to a compiler built from pristinemain— verified by--trace llvmdiff against a from-scratch baseline build, not against a stale artifact.What was tried and rejected
Refusing to fold an unproven key in
member_tail.rsand letting it fall through toExpr::IndexGetis a smaller, root-cause-shaped diff, and it too was IR-identical on the hot fixture. It regressed two shapes the polymorphic escape gets right:buf["1"](numeric string key on aBuffer) →undefined, node says60any-typed index in an accumulator loop →NaN, node says201The fold stays.
Validation
collectors/byte_read_key_tests.rs(11 cases). Unit, not just gap, on purpose — the gap suite is tag-gated, so a regression there sits red for days (CI: PR cargo-test never executes crates/perry integration suites; main-push full run starves under merge trains (documented near-miss) #5960). These run on every PR via--lib.test-files/test_gap_uint8array_nonnumeric_key_7700.ts— byte-identical to node 26.5.1, and covers the hot loop shapes the fix must not cost.cargo test -p perry-codegen --lib: 820 passed, 0 failed.cargo test -p perry-codegen: two integration targets fail, both identically on pristinemainin the same tree —large_object_barriers::large_local_array_push_inbounds_store_emits_precise_slot_barrier, andnative_proof_regressions::typed_f64_receiver_method_clone_raw_loads_after_composed_guards(which is open issue typed_f64_receiver_method_clone_raw_loads_after_composed_guards: the guard-failure edge no longer calls $generic — miscompile or intentional collapse? #7506).cargo fmt --all --check,scripts/check_file_size.sh,scripts/addr_class_inventory.py: clean.No version bump (maintainer bumps at merge).
Summary by CodeRabbit
Bug Fixes
Uint8ArrayandBuffervalues.Tests
Chores
0.5.1436.