[CodeQuality] LogicalToBooleanRector fix left or right argument causing recursion error #786
Conversation
|
All checks have passed 🎉 Please review @TomasVotruba |
Co-authored-by: GitHub Action <action@github.com>
| if ($this->shouldRecursivelyCall($right)) { | ||
| /** @var BooleanAnd|BooleanOr $right */ | ||
| $right = $this->refactor($right); | ||
| } |
There was a problem hiding this comment.
Recurse call should not be needed, as Rector rule works untill there is a code to be fixed. It's recursive by nature.
There was a problem hiding this comment.
The original problem stems from the "infinite loop detection" in \Rector\Core\Validation\InfiniteLoopValidator::process.
The test fixtures multi_or and multi_and will run into an InfiniteLoopTraversingException.
If I use print_node on the offending node(s) this is the output:
'(1 === $val1 or 1 === $val2) || 1 === $val3'
and
'(1 === $val1 and 1 === $val2) && 1 === $val3'
Not sure if this is now a bug in the InfiniteLoopValidator or if it is intended and my recursion is approach is the correct one in this case
…UnusedAssignVariableRector (rectorphp#789) Co-authored-by: Bl00D4NGEL <kuhlesdominik@gmx.de>
…ctor (rectorphp#791) * failing fixture null item on ManualJsonStringToJsonEncodeArrayRector * fix null item on ManualJsonStringToJsonEncodeArrayRector * phpstan
…ctorphp#792) * [CodingStyle] Add more statements for NewlineAfterStatementRector * cs fix
…ttps://github.com/Bl00D4NGEL/rector-src into prevent-recursion-error-in-LogicalToBooleanRector
…or' of https://github.com/Bl00D4NGEL/rector-src into prevent-recursion-error-in-LogicalToBooleanRector" This reverts commit 35d2f8e, reversing changes made to 15cce15.
| } | ||
|
|
||
| return new BooleanAnd($node->left, $node->right); | ||
| return new BooleanAnd($left, $right); |
There was a problem hiding this comment.
this whole code inside refactor can be moved to separate method, eg: processLogicalToBoolean(), so the recursive method is in the new method.
For clean rebase, I created a blogpost about it: https://samsonasik.wordpress.com/2015/09/16/practical-git-4-rebasing-conflicted-task-branch-against-primary-branch/
There was a problem hiding this comment.
Moved the code, is this what you wanted?
…n NewlineAfterStatementRector (rectorphp#793) * [CodingStyle] Failing fixture complex if on NewlineAfterStatementRector * [CodingStyle] Handle complex If_ ElseIf_ Else_ on on NewlineAfterStatementRector * more example * refactor * naming * fix
|
@Bl00D4NGEL there is conflict now, please clean rebase to avoid conflict with unrelated files. |
… + ChangeAndIfToEarlyReturnRector + DateTimeToDateTimeInterfaceRector (rectorphp#794)
…EarlyReturnRector + ChangeAndIfToEarlyReturnRector + AddArrayReturnDocTypeRector + NarrowUnionTypeDocRector (rectorphp#795)
…s null or return void (rectorphp#796)
…ector (rectorphp#797) * [EarlyReturn] Skip same return expr on ChangeOrIfReturnToEarlyReturnRector * fixture update * phpstan * rollback handling tweak on NarrowUnionTypeDocRector * tweak
…arlyReturnRector (rectorphp#799) * [EarlyReturn] Handle left or right is BooleanAnd on ChangeOrIfReturnToEarlyReturnRector * skip * remove tweak in DateTimeToDateTimeInterfaceRector * naming * remove tweak in RemoveAlwaysElseRector * rename fixture
…rectorphp#800) * [EarlyReturn] Clean up ChangeOrIfReturnToEarlyReturnRector skip check * more cleaning up
…or' of https://github.com/Bl00D4NGEL/rector-src into prevent-recursion-error-in-LogicalToBooleanRector" This reverts commit 35d2f8e, reversing changes made to 15cce15.
…ogicalToBooleanRector' into prevent-recursion-error-in-LogicalToBooleanRector # Conflicts: # rules-tests/CodingStyle/Rector/Stmt/NewlineAfterStatementRector/Fixture/no_new_line_switch.php.inc # rules/CodeQuality/Rector/LogicalAnd/LogicalToBooleanRector.php # rules/CodingStyle/Rector/Stmt/NewlineAfterStatementRector.php # tests/Issues/IssueEarlyReturnAndOrDatetime/IssueEarlyReturnAndOrDatetimeTest.php # tests/Issues/IssueEarlyReturnAndOrNarrow/IssueEarlyReturnAndOrNarrowTest.php
|
I seem to be struggling way too much with git.. I hope I did it correctly now? |
|
Thank you, looks very clean now 👍 |
|
@Bl00D4NGEL I tried to rebase locally, but failed after 30 mins :D could you re-open new PR with single commit? It would save us some rebase hell |
|
@Bl00D4NGEL I rebased your commits at PR #808 |
Bl00D4NGEL
deleted the
prevent-recursion-error-in-LogicalToBooleanRector
branch
refs rectorphp/rector#6667
closes rectorphp/rector#6667
The problem originated from
$node->leftand/or$node->rightbeing an instance ofPhpParser\Node\Expr\BinaryOp\LogicalAndorPhpParser\Node\Expr\BinaryOp\LogicalOrwhich made rector think it detected a recursion.Fixing this by recursively calling the refactor method on the left and right properties of the operator until it is not an instance of
LogicalOrorLogicalAndanymore