fix(ir_lower): release owned condition operands after truthiness coercion#593
Open
mirchaemanuel wants to merge 1 commit into
Open
fix(ir_lower): release owned condition operands after truthiness coercion#593mirchaemanuel wants to merge 1 commit into
mirchaemanuel wants to merge 1 commit into
Conversation
…cion Reading a value through `?? default` and feeding the result directly into a condition leaked the boxed coalesce temporary on every evaluation (issue illegalstudio#586). `IsTruthy` reads its operand and yields a fresh, non-aliasing boolean, but the owned `Mixed` box produced by the coalesce was never released once its truthiness had been tested, so a loop like `($r[0] ?? 0 ? 1 : 0)` leaked one block per iteration. The leak is not specific to `match`/ternary merges or to element-type widening: any owned temporary consumed purely as a branch condition (`if`, ternary, `while`/`do-while`/`for`, short-circuit `&&`/`||`, negation `!`, magic `__isset`) was orphaned. Add `LoweringContext::truthy_consuming`, which computes truthiness and releases the input when this path owns it (type-gated through `value_is_owning_temporary` + `release_if_owned`, the same contract the scalar-coercion casts use). Route every condition site that discards its operand through it; `lower_short_ternary` keeps using `truthy` because PHP `?:` forwards the original value. Regression tests cover the issue repro plus ternary-merge, plain-array, `if`, `&&`, and `!` siblings, with controls (store-then-test, direct read) guarding against double frees. Claude-Session: https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L
Contributor
Author
|
While bisecting this I found two adjacent pre-existing bugs (both reverified on current main, independent of this fix — no |
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
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.
Summary
Reading a value through the null-coalescing form (
$x[0] ?? default) and using the result directly as a condition leaked the boxedMixedcoalesce temporary on every evaluation (issue #586). The issue's loop repro leaks one heap block per iteration; stdout is correct.Bisection shows the leak is not tied to the
matchmerge, to ternary merges, or to element-type widening (issue #549): a plain indexed-array local read through?? defaultleaks just the same, and the trigger is that the coalesce result is consumed as a condition. Storing it in a local first is already heap-clean, and varying the array size keeps the leaked block at the fixed box size — so the leaked allocation is the coalesce box, not the container.Root cause
LoweringContext::truthyemitsOp::IsTruthy, which reads its operand and yields a fresh, non-aliasing boolean, but it never releases the operand. When the operand is an owned temporary — most visibly the boxedMixedproduced bylower_null_coalesce'stake_owned_temp— nothing releases it after the truthiness test, so it leaks. Every condition site that fully consumes its operand was affected:if, ternary,while/do-while/for, short-circuit&&/||, negation!, and magic__isset.Fix
Add
LoweringContext::truthy_consuming, which computes truthiness and, when this path owns the input (value_is_owning_temporary), releases it through the type-gatedrelease_if_owned— the same ownership contract the scalar-coercion casts already use for(bool)/(int)/(float). Route every condition site that discards its operand through it.lower_short_ternarykeeps usingtruthybecause PHP?:forwards the original value when truthy, so releasing there would double-free.Test plan
New heap-debug regression tests in
tests/codegen/runtime_gc/regressions.rs($i/$argcto defeat constant folding):matcharms + ternary), plus ternary-merge and plain-array siblings;if,&&, and!condition siblings;6 leaking shapes were red before the fix and the 2 controls were already green; all 8 pass after. Focused slices green: coalesce/ternary/logical/truthy/isset (154), match/heap/leak/mixed/ownership/nulls (546), ir_lower lib (28), loop/control-flow (445), negation/switch/magic_isset/bool (114).
Fixes #586
https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L