Skip to content

test(hir): pin the for-of desugar counter's literal kind at both emission sites (#7766 follow-up) - #7790

Merged
proggeramlug merged 3 commits into
mainfrom
test/7766-forof-counter-verdict
Aug 10, 2026
Merged

test(hir): pin the for-of desugar counter's literal kind at both emission sites (#7766 follow-up)#7790
proggeramlug merged 3 commits into
mainfrom
test/7766-forof-counter-verdict

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #7778 (which closed #7766). Tests only — no behaviour change.

#7778 changed both for…of desugar sites to mint the synthetic __idx_* counter as Expr::Integer(0) instead of Expr::Number(0.0). That literal kind is what collectors/i32_locals.rs::collect_integer_let_ids seeds on — it does not reason about provable integrality — so it is the difference between a counter that gets a canonical i32 slot and one that is structurally invisible to every i32-counter loop optimization. #7771's element-binding element-shape clone is the case that made it visible: its matcher hard-requires ctx.i32_counter_slots, so before the fix it could not fire for for…of at all (measured: 0.83s → 0.09s on a 200k-element parameter-array loop).

Nothing pins it. The desugar is correct either way and prints identical output, so an edit back to Number(0.0) would silently un-optimize every for…of loop in the language with no test failing — CLAUDE.md's fourth way a gate can be unable to fail ("the gate runs but its subject never did").

These are verdict tests on the lowered HIR, one per emission site — lower/stmt_loops.rs (module init) and lower_decl/body_stmt.rs (function bodies), which have drifted independently before. Both are sabotage-verified: reverting either site to Number(0.0) reds them.

for-in is deliberately not covered: its counter indexes a keys array rather than the receiver, and #7778 left it alone.

Summary by CodeRabbit

  • Bug Fixes

    • Improved regression coverage for for…of loops, helping ensure loop counters are initialized consistently and retain optimized integer behavior.
  • Documentation

    • Added a changelog entry documenting the for…of counter behavior and its optimization expectations.
  • Chores

    • Incremented the workspace version to 0.5.1459.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7a8a1dfe-4749-4d22-ad8e-90c0844b387d

📥 Commits

Reviewing files that changed from the base of the PR and between 9137ec0 and 2f62c3e.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • CLAUDE.md
  • Cargo.toml
  • changelog.d/7790-forof-counter-verdict-tests.md

📝 Walkthrough

Walkthrough

Adds lowered-HIR tests for synthetic for…of counters in module-level and function-body contexts. The tests verify initialization with Expr::Integer(0), register the test module, update the workspace version, and add a changelog entry.

Changes

For-of counter validation

Layer / File(s) Summary
Lowering inspection infrastructure
crates/perry-hir/src/lower/for_of_counter_tests.rs
Adds test-only lowering helpers and recursive HIR traversal for synthetic counter initializers.
Counter verdict assertions and release metadata
crates/perry-hir/src/lower/for_of_counter_tests.rs, crates/perry-hir/src/lower/mod.rs, CLAUDE.md, Cargo.toml, changelog.d/7790-forof-counter-verdict-tests.md
Tests module-level and function-body for…of counters, registers the test module, updates the version to 0.5.1459, and documents the verdict tests.

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

Possibly related PRs

  • PerryTS/perry#6903: Directly tests the synthetic i32 counter initialization used by this regression coverage.
  • PerryTS/perry#7296: Concerns loop counter representation and range-based integer optimizations.
  • PerryTS/perry#7778: Modifies for…of counter initialization from Expr::Number(0.0) to Expr::Integer(0).

Suggested reviewers: jdalton

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds only counter-kind HIR tests; it does not deliver or verify #7766's optimization, soundness cases, corpus metrics, or protected benchmark floors. Link this follow-up to the issue that requires the regression tests, or provide evidence that #7766's acceptance criteria are satisfied.
Out of Scope Changes check ⚠️ Warning The version bump, CLAUDE.md edit, and changelog entry are release metadata changes outside the linked optimization and test objectives. Remove the Cargo.toml version bump, CLAUDE.md version edit, and changelog entry; maintainers add release metadata at merge.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the HIR regression tests and the specific for-of counter literal change.
Description check ✅ Passed The description clearly explains the test-only scope, rationale, affected emission sites, linked issues, and sabotage verification, despite not using every template heading.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/7766-forof-counter-verdict

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.

Ralph Küpper added 3 commits August 10, 2026 22:32
…sion sites (#7766 follow-up)

#7778 made both for-of desugar sites mint the synthetic __idx_* counter as
Expr::Integer(0) — the literal kind collect_integer_let_ids seeds on, and thus
the difference between a counter that gets a canonical i32 slot and one that is
invisible to every i32-counter loop optimization.

Nothing pinned it: the desugar is correct either way and prints identical
output, so a revert to Number(0.0) would silently un-optimize every for-of loop
with no test failing. Verdict tests on the lowered HIR, one per emission site
(module-init and function-body have drifted independently before), both
sabotage-verified.
@proggeramlug
proggeramlug force-pushed the test/7766-forof-counter-verdict branch from 9137ec0 to 2f62c3e Compare August 10, 2026 20:36
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Audit complete — merging.

  • Both verdict tests pass on the rebased tree (v0.5.1459); full perry-hir --lib 293 green.
  • Sabotage verified per site, applied-then-checked: reverting lower/stmt_loops.rs to Number(0.0) reds exactly the module-level test; reverting lower_decl/body_stmt.rs reds exactly the function-body test. The two-site split is real coverage — the sites have drifted independently before.
  • The walker asserts non-emptiness before asserting the kind, so a desugar rename that stops minting __idx_* counters cannot make these vacuously green.
  • One fix during audit: the changelog fragment was keyed 7783-* (another PR's number) — renamed to 7790-* per the PR-keyed convention.
  • Local gate: one perry-runtime --lib red at load average 26–37 that re-ran clean 2/2 (2,051 passed) — the known load transient; all other steps green.

This closes the coverage gap named in #7778's merge audit: the Integer(0) desugar change previously had only end-to-end coverage.

@proggeramlug
proggeramlug merged commit 4e7d245 into main Aug 10, 2026
1 of 18 checks passed
@proggeramlug
proggeramlug deleted the test/7766-forof-counter-verdict branch August 10, 2026 21:42
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.

repsel: rule-1 provenance does not survive a function boundary — a typed P[] parameter still reads fields by name (first-party half of #7152/#7170)

1 participant