Repository navigation
Conversation
The aggregation php::Array holds a second reference to every caller slot, but the ArrayCleanupGuard was emitted after all per-argument RefWrap::commit() lines, so committing saw refcount 2 and raised 'A temporary typed reference cannot escape a dynamic call' for plain local-variable arguments. Emit the guard when the aggregation array is created so cleanup() is queued before the commit lines.
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.
Body
Fixes a false-positive runtime error when calling a by-ref variadic function/method with plain local-variable arguments. Found while testing the follow-up to #153; master-only repro.
Symptom
Compiles on master, then fails at runtime:
Write-back should simply succeed (
int(11)). The error is a false positive: the caller's slot is a stable local, not a temporary.Root cause
By-ref variadic arguments are aggregated into a temporary
php::Arraywhose slots hold references back to the caller. The aggregation array therefore owns a second reference to each caller slot, so at commit time the refcount is 2 and the typed-ref escape check intypephp_helper.hrejects it.The
ArrayCleanupGuardthat drops the aggregation array's references before the per-argumentRefWrap::commit()lines was emitted too late: itscleanup()was appended after all commit lines, so every commit still observed refcount 2.Scope matrix (all verified): fails for local-variable arguments (loop / non-loop, function / method, typed / untyped, named arguments); unaffected are property and array-element arguments (different wrapping paths) and non-variadic by-ref; literal arguments are a compile error, same as PHP.
Fix
Emit the guard when the aggregation array is created (inside the
if ($variadicVar === null)block, after the per-statementunset()), so itscleanup()is queued inafterStmtLinesbefore anyRefWrap::commit()line, and drop the late emission at the statement tail. The guard stays per-call-site by design:ArrayCleanupGuard::cleanup()is one-shot (it clears its internal pointer; the destructor is only the exception-safety fallback).Tests
New
tests/compiler/variadic/by-reference-variadic-local.phptcovering, in one binary: typed local argument (non-loop and loop), named argument, untyped variadic, property argument, and array-element argument — with the expected output generated as ground truth against stock PHP 8.4.Verification
tests/compiler/variadic: 14/14 pass (including fix(codegen): rebuild variadic aggregation array per statement #153'sloop-accumulation.phptandvariadic-goto-skip.phpt)tests/compiler: 1272 pass / 16 fail — the 16 failures are identical on master (pre-existing, unrelated areas: static binding, heredoc, enum, foreach, bigfloat, …)Out of scope (pre-existing, separate)
A forward
gotothat skips a by-ref call fails C++ compilation on master today (crosses initialization of 'php::RefWrap<…>'/'php::ArrayCleanupGuard'— call-site declarations live in the function body scope). This PR does not change that behavior (same state before and after); a follow-up will be tracked separately.