Skip to content

fix(codegen): all-vouched guarded '+' tree lowers unguarded — un-ICEs the pi and cc builds - #9069

Merged
proggeramlug merged 3 commits into
PerryTS:mainfrom
proggeramlug:fix-guarded-add-all-vouched
Aug 29, 2026
Merged

fix(codegen): all-vouched guarded '+' tree lowers unguarded — un-ICEs the pi and cc builds#9069
proggeramlug merged 3 commits into
PerryTS:mainfrom
proggeramlug:fix-guarded-add-all-vouched

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Both application bundles fail to compile on current main with guarded + tree has no testable leaf (pi dies in graphemeWidth, the cc cli bundle in a closure). Minimized to 13 lines — the load-bearing shape is a compound add in a for-of loop where the RHS is a call:

function eaw(cp) { if (cp > 100) { return 2; } return 1; }
function gw(s) {
  let width = eaw(s.codePointAt(0));
  for (const ch of s) {
    const c = ch.codePointAt(0);
    if (c >= 65280) { width += eaw(c); }      // ← the ICE
    else if (c === 3635) { width += 1; }      // this alone compiles
  }
  return width;
}

Mechanism: the call leaf is flagged by numeric_proof_is_declared_only (declared numeric return ⇒ the tree routes into lower_guarded_numeric_add) while its integer-literal returns make the same leaf provenance-vouched by expr_produces_canonical_raw_f64 (⇒ no leaf needs a runtime test). The lowering's all-vouched corner was a deliberate bail! tripwire against predicate drift — this week's provenance widening made it reachable from ordinary code, and it fails whole builds.

Fix: all leaves proven ⇒ emit the fast tree unguarded. This is not new trust: the mixed case already fadds every vouched leaf untested, so an unsound vouching predicate ships wrong answers there regardless — the predicate's own tests are the guard, not the bail (rationale in the code comment).

Verification: the 13-line repro and the full extracted graphemeWidth+eastAsianWidth pair compile and print node-identical output; gap-suite fixture test_gap_9050_all_vouched_guarded_add.ts pins the shape (verified still-ICE on 5792671 server-side before fixing); perry-codegen lib 1344/0; fmt green. Un-blocks recompiling both pi and cc for the GC-share measurement.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed a compiler crash when compiling certain numeric addition expressions whose values are fully proven safe.
    • Improved compilation of integer-provenance calculations, including compound additions in loops.
    • Applications using these expressions now compile successfully instead of failing during compilation.
  • Tests

    • Added coverage for guarded additions involving character-code processing and conditional width calculations.

…d of ICEing

The guarded add lowering bailed when NO leaf needed a runtime test — a
drift tripwire from when the routing predicates and the vouching predicate
could not overlap. They can now: a call with a declared numeric return is
flagged declared-only by numeric_proof_is_declared_only (routing the tree
into the guarded lowering) while integer-literal returns make the same
leaf provenance-vouched by expr_produces_canonical_raw_f64 (nothing left
to test). The corner became reachable from ordinary code this week —
'width += eaw(c)' inside a for-of loop, i.e. pi's graphemeWidth and a
closure in the cc cli bundle — and the tripwire took both application
builds down with 'guarded + tree has no testable leaf'.

Emitting the unguarded fast tree is not new trust: the MIXED case already
fadds every vouched leaf without a runtime test, so an unsound vouching
predicate ships wrong answers there regardless; the predicate's own tests
are the guard, not this bail. Pinned by a gap-suite fixture with the
load-bearing shape (call-in-loop compound add; 'width += 1' alone does not
route into the guarded lowering), byte-identical to node.

Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP
@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 16e55ed8-4422-4517-aced-bd6f9bbab3ca

📥 Commits

Reviewing files that changed from the base of the PR and between 7cfea22 and 1f535db.

📒 Files selected for processing (1)
  • changelog.d/9069-all-vouched-guarded-add.md

📝 Walkthrough

Walkthrough

The guarded numeric addition lowering now compiles all-vouched addition trees by emitting an unguarded fadd tree. A regression test and changelog entry cover the previous compiler ICE.

Changes

Guarded numeric addition

Layer / File(s) Summary
Handle all-vouched addition trees
crates/perry-codegen/src/expr/binary.rs, test-files/test_gap_9050_all_vouched_guarded_add.ts, changelog.d/9069-all-vouched-guarded-add.md
lower_guarded_numeric_add now emits an unguarded fast tree when every leaf is canonical instead of raising an error. The regression test exercises the affected for...of and codePointAt pattern. The changelog records the fix.

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

Merge Risk: ⚪ Minimal · up to 7cfea

The change prevents a specific guarded numeric-add compilation failure by emitting the existing fast path when all leaves are proven safe, with a regression fixture covering the failing shape. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main code generation fix and its effect: all-vouched guarded '+' trees now lower through the unguarded path and no longer trigger the ICE.
Description check ✅ Passed The description explains the failure, reproducing code shape, mechanism, fix rationale, regression test, and verification results. It does not use the template headings or provide a related issue and …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the failure, reproducing code shape, mechanism, fix rationale, regression test, and verification results. It does not use the template headings or provide a related issue and checklist, but the required technical information is mostly complete.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Merged. Reproduced the ICE on main from your minimized shape before testing the fix:

Error compiling module 'probe.ts': lowering specialized entry for function 'gw':
  lowering body of 'gw': guarded `+` tree has no testable leaf

and it compiles and runs byte-identical to node afterwards, including the wide-character path (gw("01") → 6), a vouched-call leaf in a different loop shape, and a mixed numeric/string accumulator that must not take the unguarded path.

The diagnosis is satisfying because the two predicates are individually right and only wrong together: numeric_proof_is_declared_only routes the tree into the guarded lowering on the strength of a declared return type, while expr_produces_canonical_raw_f64 independently vouches the same leaf from its integer-literal returns — so the guarded path was entered with nothing left to guard. "Every leaf vouched" is precisely the case that needs no guard, so lowering unguarded is the right resolution rather than inventing a test.

This is the seam I flagged when auditing #9033, which extended expr_produces_canonical_raw_f64 to integer-provenance locals: I noted that predicate's inputs include declared-type-derived facts and said it was the first place to look if it were widened again. It didn't cause this one — the interaction predates it — but it's the same pairing, and worth remembering that these two proofs can disagree about whether a leaf is already known good.

Added the missing changelog.d/ fragment.

Validation: perry-codegen 1345/0, perry-runtime --lib 2812/0, perry --bins 1066/0, fmt --check, run_lint_gates.sh all 60 gates passed; 2 CI-only skipped.

@proggeramlug
proggeramlug merged commit 4bc5024 into PerryTS:main Aug 29, 2026
19 of 20 checks passed
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.

1 participant