Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failing test case for SimplifyIfElseToTernaryRector and RemoveUnusedAssignVariableRector #788

Closed
wants to merge 1 commit into from

Conversation

Bl00D4NGEL
Copy link
Contributor

@Bl00D4NGEL Bl00D4NGEL commented Aug 30, 2021

refs: rectorphp/rector#6655
closes: rectorphp/rector#6655

The main reason why this seems to occur is because of an incosistent "statement tree" (not sure what the proper term for this would be). Let me try to explain:

If SimplifyIfElseToTernaryRector changes something it will take an PhpParser\Node\Stmt\If_ node and return a new PhpParser\Node\Stmt\Expression node. This works perfectly fine, however, the "parent statements" are not being updated properly.

So if I have a structure like so:

function getRes(): string
{
    if ('a' !== 'b') {
        $res = "a";
    } else {
        $res = "b";
    }

    return $res;
}

and the SimplifyIfElseToTernaryRector returns the new ternary version, the statements of the function are not updated.

So the PhpParser\Node\Stmt\If_ node that was replaced by an PhpParser\Node\Stmt\Expression node by the SimplifyIfElseToTernaryRector has a linked parent, this parent is the PhpParser\Node\Stmt\Function_. The PhpParser\Node\Stmt\Function_still has the statements [PhpParser\Node\Stmt\If_ , PhpParser\Node\Stmt\Return_] while in reality it is now [PhpParser\Node\Stmt\Expression, PhpParser\Node\Stmt\Return_]. This results in the "statement tree" (for lack of better words) to be inconsistent, thus resulting in differing results between rectors if they're used in conjunction with each other.

Given that information, in the RemoveUnusedAssignVariableRector the NextVariableUsageNodeFinder tries to look up the method / function statements to look for usages, which are now stale references to things that don't exist anymore (namely the PhpParser\Node\Stmt\If_ node) so it cannot skip the refactoring resulting in the behaviour that can be seen in the failing test case.

This is a rare condition though as usually nodes aren't entirely changed and/or two seperate rectors rarely need to touch the same type of statement constructs.

I'd hope someone with a deeper understanding of the rector core to take a look into this and potentially figure out if there's a decent solution for this.

I'd be happy to help implementing it if one was to point me in the right direction, however, currently I'm not sure where to go from here.

@samsonasik
Copy link
Member

@Bl00D4NGEL I cherry-picked your commit at PR #789

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.

Incorrect behavior of SimplifyIfElseToTernaryRector, RemoveUnusedAssignVariableRector
2 participants