fix(codegen): keep a string or symbol key off #7890's declared-array claim (#7891) - #7893
Merged
Merged
Conversation
…claim (#7891) 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
📝 WalkthroughWalkthroughThe change classifies static string and symbol keys before declared-array claims. String and symbol reads use generic property access, while numeric reads retain the guarded array path. Tests cover multiple receiver values, and the changelog records the updated coverage. ChangesDeclared-array key handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🤖 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`:
- Around line 1136-1139: Update the key classification around
index_is_static_string_or_symbol so every symbol-valued key, including
Expr::SymbolNew(_) and Type::Symbol LocalGet expressions, is excluded from
claimed_array before declared_array_property_claim. Ensure such keys use
js_object_get_symbol_property rather than js_array_get_index_or_string, and add
a regression covering const sym = Symbol(); arr[sym].
🪄 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: 6b5b9969-cc26-45fb-9604-a2237b7ef2fc
📒 Files selected for processing (3)
changelog.d/7891-declared-array-claim-string-key.mdcrates/perry-codegen/src/expr/index_get.rstest-files/test_gap_7890_declared_array_receiver_element_read.ts
Comment on lines
+1136
to
+1139
| let index_is_static_string_or_symbol = matches!( | ||
| index.as_ref(), | ||
| Expr::String(_) | Expr::WtfString(_) | Expr::SymbolFor(_) | ||
| ) || is_string_expr(ctx, index); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: every symbol expression form is excluded before the declared-array
# claim or reaches the runtime symbol-property path.
rg -n -C 8 \
'Expr::SymbolFor|js_is_symbol|js_object_get_symbol_property|js_array_get_index_or_string|declared_array_property_claim|is_numeric_expr' \
--glob '*.rs' .Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="crates/perry-codegen/src/expr/index_get.rs"
printf '%s\n' '--- target implementation ---'
sed -n '1110,1215p' "$target"
sed -n '1410,1485p' "$target"
printf '%s\n' '--- symbol-related HIR forms and constructors ---'
rg -n -C 3 \
'enum Expr|SymbolFor|SymbolKeyFor|SymbolDescription|SymbolToString|well_known_symbol|js_symbol' \
crates/perry-hir crates/perry-codegen/src --glob '*.rs' \
| head -n 500
printf '%s\n' '--- declared-array claim references ---'
rg -n -C 6 \
'declared_array_property_claim|index_is_static_string_or_symbol|claimed_array' \
"$target"Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="crates/perry-codegen/src/expr/index_get.rs"
printf '%s\n' '--- complete claimed-array lowering ---'
sed -n '1190,1265p' "$target"
printf '%s\n' '--- runtime fallback dispatch ---'
sed -n '1440,1535p' "$target"
printf '%s\n' '--- predicate definitions and numeric analysis ---'
rg -n -C 12 \
'fn is_string_expr|pub.*is_string_expr|is_numeric_expr|fn declared_array_property_claim|declared_array_property_claim' \
crates/perry-codegen/src --glob '*.rs' \
| head -n 700
printf '%s\n' '--- symbol-producing expressions in member/index lowering ---'
rg -n -C 8 \
'Expr::SymbolNew|Expr::SymbolFor|Type::Symbol|local_type.*Symbol|Symbol\\.iterator|Symbol\\[|computed.*Symbol' \
crates/perry-hir/src/lower crates/perry-hir/src/analysis crates/perry-codegen/src/expr \
--glob '*.rs' \
| head -n 1000Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact predicate implementations ---'
rg -n -C 25 \
'fn is_string_expr|fn is_numeric_expr|fn declared_array_property_claim' \
crates/perry-codegen/src/type_analysis --glob '*.rs'
printf '%s\n' '--- relevant type cases ---'
rg -n -C 18 \
'Type::Symbol|Expr::SymbolNew|Expr::SymbolFor|Expr::LocalGet' \
crates/perry-codegen/src/type_analysis/numeric \
crates/perry-codegen/src/type_analysis/strings \
crates/perry-codegen/src/type_analysis/refine \
--glob '*.rs' | head -n 900
printf '%s\n' '--- declared-array claim callers and tests ---'
rg -n -C 15 \
'declared_array_property_claim|`#7854`|claimed_array' \
crates/perry-codegen --glob '*.rs' | head -n 900Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
target = Path("crates/perry-codegen/src/expr/index_get.rs").read_text()
numeric = Path("crates/perry-codegen/src/type_analysis/numeric.rs").read_text()
strings = Path("crates/perry-codegen/src/type_analysis/strings.rs").read_text()
hir_types = Path("crates/perry-hir/src/analysis/value_types.rs").read_text()
claim = "let claimed_array = recv_unknown"
claim_pos = target.index(claim)
claim_block = target[claim_pos:target.index("// Three cases:", claim_pos)]
checks = {
"claim excludes only the static SymbolFor form": (
"Expr::String(_) | Expr::WtfString(_) | Expr::SymbolFor(_)" in claim_block
and "Expr::SymbolNew" not in claim_block
),
"claimed array branch handles only SymbolFor specially": (
"if matches!(index.as_ref(), Expr::SymbolFor(_))" in claim_block
and '"js_object_get_symbol_property"' in claim_block
),
"non-numeric claimed-array keys use array index-or-string helper": (
"if !is_numeric_expr(ctx, index)" in claim_block
and '"js_array_get_index_or_string"' in claim_block
),
"numeric predicate does not classify SymbolNew as numeric": (
"Expr::Integer(_)" in numeric
and "Expr::LocalGet(id)" in numeric
and "Some(HirType::Number) | Some(HirType::Int32)" in numeric
),
"string predicate does not classify Symbol locals as strings": (
"Some(HirType::String | HirType::StringLiteral(_))" in strings
and "Expr::SymbolNew" not in strings[strings.index("pub(crate) fn is_string_expr"):strings.index("pub(crate) fn is_string_expr") + 5000]
),
"later fallback performs runtime symbol dispatch": (
'"js_is_symbol"' in target
and '"js_object_get_symbol_property"' in target
),
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
print("Conclusion: a SymbolNew or Symbol-typed LocalGet can satisfy the claimed-array path, skip its SymbolFor arm, and reach js_array_get_index_or_string.")
PYRepository: PerryTS/perry
Length of output: 516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
target = Path("crates/perry-codegen/src/expr/index_get.rs").read_text()
numeric = Path("crates/perry-codegen/src/type_analysis/numeric.rs").read_text()
strings = Path("crates/perry-codegen/src/type_analysis/strings.rs").read_text()
value_types = Path("crates/perry-hir/src/analysis/value_types.rs").read_text()
claim_start = target.index("let claimed_array = recv_unknown")
array_start = target.index("if is_array_expr(ctx, object) || claimed_array", claim_start)
fallback_start = target.index("// Last-resort fallback with runtime tag checks", array_start)
claim_region = target[claim_start:fallback_start]
checks = {
"claim predicate has no SymbolNew or general symbol-type check": (
"Expr::SymbolFor(_)" in claim_region
and "Expr::SymbolNew" not in claim_region
and "Type::Symbol" not in claim_region
),
"array branch special-cases only SymbolFor": (
"if matches!(index.as_ref(), Expr::SymbolFor(_))" in claim_region
and '"js_object_get_symbol_property"' in claim_region
),
"other non-numeric claimed-array keys use array index-or-string helper": (
"if !is_numeric_expr(ctx, index)" in claim_region
and '"js_array_get_index_or_string"' in claim_region
),
"SymbolNew is a symbol-valued HIR expression": (
"Expr::SymbolNew(_) | Expr::SymbolFor(_) => Type::Symbol" in value_types
),
"Symbol locals are not numeric or string locals": (
"Some(HirType::Number) | Some(HirType::Int32)" in numeric
and "Some(HirType::String | HirType::StringLiteral(_))" in strings
),
"later fallback performs runtime symbol dispatch": (
'"js_is_symbol"' in target
and target.index('"js_object_get_symbol_property"') < target.rindex('"js_object_get_symbol_property"')
),
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
print("Conclusion: a SymbolNew or Symbol-typed LocalGet can satisfy claimed_array, skip the SymbolFor arm, and reach js_array_get_index_or_string.")
PYRepository: PerryTS/perry
Length of output: 640
Exclude all symbol-valued keys from claimed_array.
index_is_static_string_or_symbol excludes only Expr::SymbolFor(_). Expr::SymbolNew(_) and a LocalGet with Type::Symbol can therefore reach js_array_get_index_or_string instead of js_object_get_symbol_property. Use a complete symbol predicate before declared_array_property_claim, or restrict the claim to proven numeric keys. Add a regression for const sym = Symbol(); arr[sym].
🤖 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` around lines 1136 - 1139, Update
the key classification around index_is_static_string_or_symbol so every
symbol-valued key, including Expr::SymbolNew(_) and Type::Symbol LocalGet
expressions, is excluded from claimed_array before
declared_array_property_claim. Ensure such keys use
js_object_get_symbol_property rather than js_array_get_index_or_string, and add
a regression covering const sym = Symbol(); arr[sym].
Source: Learnings
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.
Follow-up to #7890 (merged as
c25ee9db7). Both halves came out of writing the coverage#7890 was missing.
A. A string or symbol key must not ride the declared-array claim
#7890 lets a property read whose receiver's declared property type is an array
(
e.vals[i],p.toks[p.pos]) reachexpr/index_get.rs's array arm. That is a CLAIM, nota proof, and it was admitted on the grounds that the array arm re-checks
GC_TYPE_ARRAYonthe receiver and falls back.
That is true of the arm as a whole and false of one route inside it. The two key routes
have different receiver-validation strength:
js_array_get_f64clean_arr_ptr/array_object_receiver— correct for a string, an array-like object, a typed array, a numberjs_array_get_index_or_string→array_get_property_by_key→js_object_get_field_by_names["0"]answersundefinedwhere JS answers the characterSo only the numeric route is claim-safe. The claim now requires a non-string, non-symbol
key; a string or symbol key keeps exactly the generic path it had before #7890.
The
undefinedanswer itself is pre-existing onmainand reachable without any ofthis, through a plain non-union declared receiver:
The same read on a bare
anyis correct — the wrong answer is selected by the ANNOTATION,not by the value. Filed as #7891. Deliberately not checked in as a gap test: it
would be red by construction, and
test-parity/gap_snapshot.jsonis generated on Linux andmust not be hand-edited.
B. Coverage for the shape this claim reaches
test-files/test_gap_7890_declared_array_receiver_element_read.tsgains the static-stringkey rows that A leaves on the generic path, so the refusal is pinned rather than assumed.
Validation
interp0.7796 → 0.6748 (−13.4%) andiso_miss1.0607 → 0.9670 (−8.8%) stand unchanged—
interpandiso_missread only numeric indexes. No re-measurement needed, and noneclaimed.
interpandiso_miss.node --experimental-strip-types+ exit 0.test_gap_7890_…,test_gap_declared_field_type_refine_guarded,test_gap_7853_declared_array_length_runtime_valueall byte-identical to node.checksum 437840 misses 0; whole corpus byte-exact underPERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=200 PERRY_GC_VERIFY_EVACUATION=1with the instrument shown live (50 retired sets).
cargo test --release -p perry-codegen: 26 suites, 1337 passed, 0 failed.The general lesson, recorded because it nearly shipped
When you widen a type claim, enumerate the consumers the claim newly reaches and check
each one's guard separately. "Element reads are guarded" was a true statement about the
arm and a false statement about one route inside it, and the only reason it did not ship is
that the coverage was written after the optimization rather than instead of it.
https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
Summary by CodeRabbit
Bug Fixes
undefinedresults for string receivers.Tests
length,constructor, and unknown properties across arrays and other runtime values.