Bug Report
| Subject |
Details |
| Rector version |
v2.4.5 (also reproduced on dev-main via the demo) |
ReturnBinaryOrToEarlyReturnRector silently removes operands from a returned || chain when the chain contains a parenthesized && operand, changing the code's semantics.
Given a return of 4+ ||-ed operands where a parenthesized && appears before the last two and a method call follows it, the rule emits early returns only for the operands after the && — the operands before it are deleted:
public function hasStale(): bool
{
- return $this->donation === null
- || ($this->commission > 0 && $this->transaction === null)
- || $this->somethingElse()
- || $this->anotherThing();
+ if ($this->somethingElse()) {
+ return true;
+ }
+ return $this->anotherThing();
}Minimal PHP Code Causing Issue
https://getrector.com/demo/0f4aa7d0-6594-4513-93fe-e39fa41afc4a
Notes on the reproduction — every element is load-bearing:
- With only 3 operands (
a || (b && c) || $this->d()) the rule correctly skips the transform; the bug needs 4+ operands so the && is met inside a nested recursion.
- The operand after the
&& must be a method call, otherwise CallAnalyzer::doesIfHasObjectCall() bails before the broken output is emitted.
Suspected root cause: createMultipleIfs() returns [] when it encounters a BooleanAnd, intended as an abort signal that refactor() checks via $ifs === []. But when the && is reached inside the recursion through collectLeftBooleanOrToIfs(), the outer loop merges that empty array ($ifs = [...$ifs, ...$this->collectLeftBooleanOrToIfs(...)]) and continues collecting the remaining operands — the abort is swallowed, and everything before the && is lost. [] is ambiguous between "abort the whole transform" and "no ifs collected yet".
Expected Behaviour
Either a faithful transform with one early return per operand:
public function hasStale(): bool
{
if ($this->donation === null) {
return true;
}
if ($this->commission > 0 && $this->transaction === null) {
return true;
}
if ($this->somethingElse()) {
return true;
}
return (bool) $this->anotherThing();
}or skipping the node entirely, as already happens for the 3-operand case containing &&.
Bug Report
ReturnBinaryOrToEarlyReturnRectorsilently removes operands from a returned||chain when the chain contains a parenthesized&&operand, changing the code's semantics.Given a
returnof 4+||-ed operands where a parenthesized&&appears before the last two and a method call follows it, the rule emits early returns only for the operands after the&&— the operands before it are deleted:public function hasStale(): bool { - return $this->donation === null - || ($this->commission > 0 && $this->transaction === null) - || $this->somethingElse() - || $this->anotherThing(); + if ($this->somethingElse()) { + return true; + } + return $this->anotherThing(); }Minimal PHP Code Causing Issue
https://getrector.com/demo/0f4aa7d0-6594-4513-93fe-e39fa41afc4a
Notes on the reproduction — every element is load-bearing:
a || (b && c) || $this->d()) the rule correctly skips the transform; the bug needs 4+ operands so the&&is met inside a nested recursion.&&must be a method call, otherwiseCallAnalyzer::doesIfHasObjectCall()bails before the broken output is emitted.Suspected root cause:
createMultipleIfs()returns[]when it encounters aBooleanAnd, intended as an abort signal thatrefactor()checks via$ifs === []. But when the&&is reached inside the recursion throughcollectLeftBooleanOrToIfs(), the outer loop merges that empty array ($ifs = [...$ifs, ...$this->collectLeftBooleanOrToIfs(...)]) and continues collecting the remaining operands — the abort is swallowed, and everything before the&&is lost.[]is ambiguous between "abort the whole transform" and "no ifs collected yet".Expected Behaviour
Either a faithful transform with one early return per operand:
or skipping the node entirely, as already happens for the 3-operand case containing
&&.