Skip to content

fix(codegen): invalidate Array.isArray folds on reassignment - #7855

Merged
proggeramlug merged 2 commits into
mainfrom
fix/7844-array-isarray-reassignment
Aug 11, 2026
Merged

fix(codegen): invalidate Array.isArray folds on reassignment#7855
proggeramlug merged 2 commits into
mainfrom
fix/7844-array-isarray-reassignment

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop Array.isArray from folding a reassigned local from its initializer-refined type
  • route those locals through js_array_is_array, so the predicate checks the current runtime value
  • retain the existing compile-time fold for literals and locals that were never reassigned

Closes #7844

Reproduction

On origin/main (079e646dd), the same six-case program produced stale answers in both directions:

number-to-array false true object
string-to-array false true object
null-to-array false true object
array-to-number true false number
array-to-string true false string
unchanged-array true true object

The second boolean is instanceof Array, showing that the runtime value itself was correct. Node and the fixed compiler both produce:

number-to-array true true object
string-to-array true true object
null-to-array true true object
array-to-number false false number
array-to-string false false string
unchanged-array true true object

The A/B used two compilers against the same prebuilt runtime.

Validation

Summary by CodeRabbit

  • Bug Fixes
    • Fixed Array.isArray to correctly evaluate local values at runtime after reassignment.
    • Prevented incorrect array classifications when values change between arrays and non-arrays.
    • Preserved optimized handling for unchanged array values.
  • Tests
    • Added coverage for reassigned values, instanceof Array, null, primitives, and unchanged arrays.
  • Documentation
    • Added a changelog entry describing the corrected behavior.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Array.isArray now evaluates reassigned locals at runtime instead of relying on stale initializer types. Regression tests cover transitions between arrays and non-arrays, while unchanged arrays retain compile-time folding.

Changes

Array.isArray reassignment handling

Layer / File(s) Summary
Runtime predicate for reassigned locals
crates/perry-codegen/src/expr/array_methods.rs, crates/perry-codegen/tests/native_proof_regressions.rs
Reassigned locals bypass static array classification and use runtime evaluation. Regression coverage verifies mutable locals and unchanged immutable arrays.
Reassignment behavior coverage
test-files/test_gap_7844_array_isarray_reassigned_local.ts, changelog.d/7855-array-isarray-reassignment.md
Tests cover array and non-array reassignment states. The changelog records the corrected behavior.

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

Possibly related PRs

  • PerryTS/perry#7052: Addresses stale compile-time assumptions for reassigned locals.
  • PerryTS/perry#7188: Adjusts compile-time array-related folding when static type evidence is insufficient.
  • PerryTS/perry#7831: Updates codegen type analysis to avoid unsafe constant folding.

Suggested labels: bug

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main fix: invalidating Array.isArray constant folding after reassignment.
Description check ✅ Passed The description explains the fix, reproduction, linked issue, and validation, although it uses Validation instead of the template's Test plan heading.
Linked Issues check ✅ Passed The changes address issue #7844 by routing reassigned locals through runtime Array.isArray checks while preserving valid compile-time folding.
Out of Scope Changes check ✅ Passed The code, regression tests, changelog entry, and validation changes are directly related to the linked issue and PR objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/7844-array-isarray-reassignment

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

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
crates/perry-codegen/src/expr/array_methods.rs (1)

97-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Make the static-type analysis lazy.

bool::then_some evaluates its argument before the method runs. Therefore, crate::type_analysis::static_type_of(ctx, o) still runs for reassigned locals. Replace it with then(|| ...) so reassigned locals skip this analysis.

🤖 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/array_methods.rs` around lines 97 - 99, Update
the conditional static-type lookup around static_type_is_still_valid to use lazy
evaluation, replacing then_some with then so
crate::type_analysis::static_type_of(ctx, o) runs only when the validity flag is
true and is skipped for reassigned locals.

Source: Coding guidelines

test-files/test_gap_7844_array_isarray_reassigned_local.ts (1)

1-2: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a loop reassignment regression case.

The test covers only straight-line assignments. Add one loop case to verify that reassigned_locals invalidates the Array.isArray static fold for loop writes. Run ./scripts/run_gap_tests.sh with Node 26.5.1 from .node-version.

🤖 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_7844_array_isarray_reassigned_local.ts` around lines 1 -
2, Extend the regression test around numberToArray with a loop that reassigns
the local, verifying reassigned_locals prevents Array.isArray static folding for
loop writes. Keep the existing straight-line assignment case, and run
./scripts/run_gap_tests.sh using the Node version specified in .node-version.

Source: Coding guidelines

🤖 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 `@crates/perry-codegen/src/expr/array_methods.rs`:
- Around line 97-99: Update the conditional static-type lookup around
static_type_is_still_valid to use lazy evaluation, replacing then_some with then
so crate::type_analysis::static_type_of(ctx, o) runs only when the validity flag
is true and is skipped for reassigned locals.

In `@test-files/test_gap_7844_array_isarray_reassigned_local.ts`:
- Around line 1-2: Extend the regression test around numberToArray with a loop
that reassigns the local, verifying reassigned_locals prevents Array.isArray
static folding for loop writes. Keep the existing straight-line assignment case,
and run ./scripts/run_gap_tests.sh using the Node version specified in
.node-version.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 63d339df-704e-4ae7-bfa9-910811af4cf8

📥 Commits

Reviewing files that changed from the base of the PR and between 079e646 and 7506179.

📒 Files selected for processing (4)
  • changelog.d/7855-array-isarray-reassignment.md
  • crates/perry-codegen/src/expr/array_methods.rs
  • crates/perry-codegen/tests/native_proof_regressions.rs
  • test-files/test_gap_7844_array_isarray_reassigned_local.ts

@proggeramlug
proggeramlug merged commit 6b00795 into main Aug 11, 2026
1 of 19 checks passed
@proggeramlug
proggeramlug deleted the fix/7844-array-isarray-reassignment branch August 11, 2026 15:21
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.

Array.isArray answers from the binding's initializer, not the value — wrong in BOTH directions, so a guard admits a number

1 participant