Skip to content

Commit

Permalink
[DeadCode] Fix binary different nesting in RemoveOverriddenValuesRect…
Browse files Browse the repository at this point in the history
…or (#4422)

* decopule NodeByTypeAndPositionCollector

* decouple VariableUseFinder
  • Loading branch information
TomasVotruba authored Oct 15, 2020
1 parent 09dc6ce commit 16031da
Show file tree
Hide file tree
Showing 7 changed files with 260 additions and 120 deletions.
44 changes: 37 additions & 7 deletions packages/node-nesting-scope/src/FlowOfControlLocator.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Rector\NodeNestingScope;

use PhpParser\Node;
use PhpParser\Node\Expr\BinaryOp;
use PhpParser\Node\FunctionLike;
use PhpParser\Node\Stmt\Expression;
use Rector\NodeTypeResolver\Node\AttributeKey;
Expand All @@ -13,20 +14,49 @@ final class FlowOfControlLocator
{
public function resolveNestingHashFromFunctionLike(FunctionLike $functionLike, Node $checkedNode): string
{
$nestingHash = '_';
$nestingHash = spl_object_hash($functionLike) . '__';

$parentNode = $checkedNode;
while ($parentNode = $parentNode->getAttribute(AttributeKey::PARENT_NODE)) {
if ($parentNode instanceof Expression) {
$currentNode = $checkedNode;
$previous = $currentNode;
while ($currentNode = $currentNode->getAttribute(AttributeKey::PARENT_NODE)) {
if ($currentNode instanceof Expression) {
continue;
}

$nestingHash .= spl_object_hash($parentNode);
if ($functionLike === $parentNode) {
return $nestingHash;
if (! $currentNode instanceof Node) {
continue;
}

if ($functionLike === $currentNode) {
// to high
break;
}

$nestingHash .= $this->resolveBinaryOpNestingHash($currentNode, $previous);

$nestingHash .= spl_object_hash($currentNode);

$previous = $currentNode;
}

return $nestingHash;
}

private function resolveBinaryOpNestingHash(Node $currentNode, Node $previous): string
{
if (! $currentNode instanceof BinaryOp) {
return '';
}

// left && right have differnt nesting
if ($currentNode->left === $previous) {
return 'binary_left__';
}

if ($currentNode->right === $previous) {
return 'binary_right__';
}

return '';
}
}
67 changes: 67 additions & 0 deletions rules/dead-code/src/FlowControl/VariableUseFinder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php

declare(strict_types=1);

namespace Rector\DeadCode\FlowControl;

use PhpParser\Node;
use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\Variable;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\Core\PhpParser\Printer\BetterStandardPrinter;
use Rector\NodeNameResolver\NodeNameResolver;
use Rector\NodeTypeResolver\Node\AttributeKey;

final class VariableUseFinder
{
/**
* @var BetterNodeFinder
*/
private $betterNodeFinder;

/**
* @var NodeNameResolver
*/
private $nodeNameResolver;

/**
* @var BetterStandardPrinter
*/
private $betterStandardPrinter;

public function __construct(
BetterNodeFinder $betterNodeFinder,
NodeNameResolver $nodeNameResolver,
BetterStandardPrinter $betterStandardPrinter
) {
$this->betterNodeFinder = $betterNodeFinder;
$this->nodeNameResolver = $nodeNameResolver;
$this->betterStandardPrinter = $betterStandardPrinter;
}

/**
* @param Variable[] $assignedVariables
* @return Variable[]
*/
public function resolveUsedVariables(Node $node, array $assignedVariables): array
{
return $this->betterNodeFinder->find($node, function (Node $node) use ($assignedVariables): bool {
if (! $node instanceof Variable) {
return false;
}

$parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);
// is the left assign - not use of one
if ($parentNode instanceof Assign && ($parentNode->var instanceof Variable && $parentNode->var === $node)) {
return false;
}

// simple variable only
if ($this->nodeNameResolver->getName($node) === null) {
return false;
}

return $this->betterStandardPrinter->isNodeEqual($node, $assignedVariables);
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php

declare(strict_types=1);

namespace Rector\DeadCode\NodeCollector;

use PhpParser\Node\Expr\Assign;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\FunctionLike;
use Rector\DeadCode\ValueObject\VariableNodeUse;
use Rector\NodeNameResolver\NodeNameResolver;
use Rector\NodeNestingScope\FlowOfControlLocator;
use Rector\NodeTypeResolver\Node\AttributeKey;

final class NodeByTypeAndPositionCollector
{
/**
* @var FlowOfControlLocator
*/
private $flowOfControlLocator;

/**
* @var NodeNameResolver
*/
private $nodeNameResolver;

public function __construct(FlowOfControlLocator $flowOfControlLocator, NodeNameResolver $nodeNameResolver)
{
$this->flowOfControlLocator = $flowOfControlLocator;
$this->nodeNameResolver = $nodeNameResolver;
}

/**
* @param Variable[] $assignedVariables
* @param Variable[] $assignedVariablesUse
* @return VariableNodeUse[]
*/
public function collectNodesByTypeAndPosition(
array $assignedVariables,
array $assignedVariablesUse,
FunctionLike $functionLike
): array {
$nodesByTypeAndPosition = [];

foreach ($assignedVariables as $assignedVariable) {
/** @var int $startTokenPos */
$startTokenPos = $assignedVariable->getAttribute(AttributeKey::START_TOKEN_POSITION);

// not in different scope, than previous one - e.g. if/while/else...
// get nesting level to $classMethodNode
/** @var Assign $assign */
$assign = $assignedVariable->getAttribute(AttributeKey::PARENT_NODE);
$nestingHash = $this->flowOfControlLocator->resolveNestingHashFromFunctionLike($functionLike, $assign);

/** @var string $variableName */
$variableName = $this->nodeNameResolver->getName($assignedVariable);

$nodesByTypeAndPosition[] = new VariableNodeUse(
$startTokenPos,
$variableName,
VariableNodeUse::TYPE_ASSIGN,
$assignedVariable,
$nestingHash
);
}

foreach ($assignedVariablesUse as $assignedVariableUse) {
/** @var int $startTokenPos */
$startTokenPos = $assignedVariableUse->getAttribute(AttributeKey::START_TOKEN_POSITION);

/** @var string $variableName */
$variableName = $this->nodeNameResolver->getName($assignedVariableUse);

$nodesByTypeAndPosition[] = new VariableNodeUse(
$startTokenPos,
$variableName,
VariableNodeUse::TYPE_USE,
$assignedVariableUse
);
}

return $this->sortByStart($nodesByTypeAndPosition);
}

/**
* @param VariableNodeUse[] $nodesByTypeAndPosition
* @return VariableNodeUse[]
*/
private function sortByStart(array $nodesByTypeAndPosition): array
{
usort(
$nodesByTypeAndPosition,
function (VariableNodeUse $firstVariableNodeUse, VariableNodeUse $secondVariableNodeUse): int {
return $firstVariableNodeUse->getStartTokenPosition() <=> $secondVariableNodeUse->getStartTokenPosition();
}
);
return $nodesByTypeAndPosition;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
use Rector\Core\Rector\AbstractRector;
use Rector\Core\RectorDefinition\CodeSample;
use Rector\Core\RectorDefinition\RectorDefinition;
use Rector\DeadCode\FlowControl\VariableUseFinder;
use Rector\DeadCode\NodeCollector\NodeByTypeAndPositionCollector;
use Rector\DeadCode\ValueObject\VariableNodeUse;
use Rector\NodeNestingScope\FlowOfControlLocator;
use Rector\NodeTypeResolver\Node\AttributeKey;

/**
Expand All @@ -27,14 +28,23 @@ final class RemoveOverriddenValuesRector extends AbstractRector
private $contextAnalyzer;

/**
* @var FlowOfControlLocator
* @var NodeByTypeAndPositionCollector
*/
private $flowOfControlLocator;
private $nodeByTypeAndPositionCollector;

public function __construct(ContextAnalyzer $contextAnalyzer, FlowOfControlLocator $flowOfControlLocator)
{
/**
* @var VariableUseFinder
*/
private $variableUseFinder;

public function __construct(
ContextAnalyzer $contextAnalyzer,
NodeByTypeAndPositionCollector $nodeByTypeAndPositionCollector,
VariableUseFinder $variableUseFinder
) {
$this->contextAnalyzer = $contextAnalyzer;
$this->flowOfControlLocator = $flowOfControlLocator;
$this->nodeByTypeAndPositionCollector = $nodeByTypeAndPositionCollector;
$this->variableUseFinder = $variableUseFinder;
}

public function getDefinition(): RectorDefinition
Expand Down Expand Up @@ -85,9 +95,9 @@ public function refactor(Node $node): ?Node
$assignedVariableNames = $this->getNodeNames($assignedVariables);

// 2. collect use of those variables
$assignedVariablesUse = $this->resolveUsedVariables($node, $assignedVariables);
$assignedVariablesUse = $this->variableUseFinder->resolveUsedVariables($node, $assignedVariables);

$nodesByTypeAndPosition = $this->collectNodesByTypeAndPosition(
$nodesByTypeAndPosition = $this->nodeByTypeAndPositionCollector->collectNodesByTypeAndPosition(
$assignedVariables,
$assignedVariablesUse,
$node
Expand Down Expand Up @@ -151,92 +161,6 @@ private function getNodeNames(array $nodes): array
return array_unique($nodeNames);
}

/**
* @param Variable[] $assignedVariables
* @return Variable[]
*/
private function resolveUsedVariables(Node $node, array $assignedVariables): array
{
return $this->betterNodeFinder->find($node, function (Node $node) use ($assignedVariables): bool {
if (! $node instanceof Variable) {
return false;
}

$parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);
// is the left assign - not use of one
if ($parentNode instanceof Assign && ($parentNode->var instanceof Variable && $parentNode->var === $node)) {
return false;
}

// simple variable only
if ($this->getName($node) === null) {
return false;
}

return $this->isNodeEqual($node, $assignedVariables);
});
}

/**
* @param Variable[] $assignedVariables
* @param Variable[] $assignedVariablesUse
* @return VariableNodeUse[]
*/
private function collectNodesByTypeAndPosition(
array $assignedVariables,
array $assignedVariablesUse,
FunctionLike $functionLike
): array {
$nodesByTypeAndPosition = [];

foreach ($assignedVariables as $assignedVariable) {
/** @var int $startTokenPos */
$startTokenPos = $assignedVariable->getAttribute(AttributeKey::START_TOKEN_POSITION);

// not in different scope, than previous one - e.g. if/while/else...
// get nesting level to $classMethodNode
/** @var Assign $assignNode */
$assignNode = $assignedVariable->getAttribute(AttributeKey::PARENT_NODE);
$nestingHash = $this->flowOfControlLocator->resolveNestingHashFromFunctionLike($functionLike, $assignNode);

/** @var string $variableName */
$variableName = $this->getName($assignedVariable);

$nodesByTypeAndPosition[] = new VariableNodeUse(
$startTokenPos,
$variableName,
VariableNodeUse::TYPE_ASSIGN,
$assignedVariable,
$nestingHash
);
}

foreach ($assignedVariablesUse as $assignedVariableUse) {
/** @var int $startTokenPos */
$startTokenPos = $assignedVariableUse->getAttribute(AttributeKey::START_TOKEN_POSITION);

/** @var string $variableName */
$variableName = $this->getName($assignedVariableUse);

$nodesByTypeAndPosition[] = new VariableNodeUse(
$startTokenPos,
$variableName,
VariableNodeUse::TYPE_USE,
$assignedVariableUse
);
}

// sort
usort(
$nodesByTypeAndPosition,
function (VariableNodeUse $firstVariableNodeUse, VariableNodeUse $secondVariableNodeUse): int {
return $firstVariableNodeUse->getStartTokenPosition() <=> $secondVariableNodeUse->getStartTokenPosition();
}
);

return $nodesByTypeAndPosition;
}

/**
* @param string[] $assignedVariableNames
* @param VariableNodeUse[] $nodesByTypeAndPosition
Expand Down Expand Up @@ -295,9 +219,11 @@ private function shouldRemoveAssignNode(
return false;
}

if (! $previousNode->isType(VariableNodeUse::TYPE_ASSIGN) || ! $nodeByTypeAndPosition->isType(
VariableNodeUse::TYPE_ASSIGN
)) {
if (! $previousNode->isType(VariableNodeUse::TYPE_ASSIGN)) {
return false;
}

if (! $nodeByTypeAndPosition->isType(VariableNodeUse::TYPE_ASSIGN)) {
return false;
}

Expand Down
Loading

0 comments on commit 16031da

Please sign in to comment.