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.

Sign up for GitHub

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

Jump to bottom

Change RemovePhpVersionIdCheckRector to work with direct If_ #2259

Merged
merged 2 commits into from
May 7, 2022
Merged

Change RemovePhpVersionIdCheckRector to work with direct If_ #2259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45f65c3
Change RemovePhpVersionIdCheckRector to work with direct If_
TomasVotruba May 7, 2022
a3b7575
[ci-review] Rector Rectify
actions-user May 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ecs.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use PhpCsFixer\Fixer\FunctionNotation\FunctionTypehintSpaceFixer;
use PhpCsFixer\Fixer\Phpdoc\GeneralPhpdocAnnotationRemoveFixer;
use PhpCsFixer\Fixer\Phpdoc\NoSuperfluousPhpdocTagsFixer;
use PhpCsFixer\Fixer\Phpdoc\PhpdocNoEmptyReturnFixer;
use PhpCsFixer\Fixer\Phpdoc\PhpdocTypesFixer;
use PhpCsFixer\Fixer\PhpUnit\PhpUnitStrictFixer;
use Symplify\CodingStandard\Fixer\LineLength\DocBlockLineLengthFixer;
Expand Down Expand Up @@ -71,5 +72,10 @@
],

AssignmentInConditionSniff::class . '.FoundInWhileCondition',

// null on purpose as no change
PhpdocNoEmptyReturnFixer::class => [
__DIR__ . '/rules/DeadCode/Rector/ConstFetch/RemovePhpVersionIdCheckRector.php',
],
]);
};
4 changes: 4 additions & 0 deletions phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,9 +585,13 @@ parameters:

# avoid moving rule around, while re-targeting to stmt
- '#Class "Rector\\(.*?)\\Rector\\(.*?) has invalid namespace category "(.*?)"\. Pick one of\: "(Expression|Class_|NodeTypeGroup)"#'
- '#Class "Rector\\DeadCode\\Rector\\ConstFetch\\RemovePhpVersionIdCheckRector" has invalid namespace category "ConstFetch"\. Pick one of\: "If_"#'

- '#Cognitive complexity for "Rector\\CodingStyle\\Rector\\ClassMethod\\MakeInheritedMethodVisibilitySameAsParentRector\:\:refactorWithScope\(\)" is 11, keep it under 10#'
-
message: '#Cognitive complexity for "Rector\\EarlyReturn\\Rector\\If_\\ChangeAndIfToEarlyReturnRector\:\:refactor\(\)" is \d+, keep it under 10#'
paths:
- rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php

# known value
- '#Method Rector\\Core\\Php\\PhpVersionProvider\:\:provide\(\) should return 50200\|50300\|50400\|50500\|50600\|70000\|70100\|70200\|70300\|70400\|80000\|80100\|80200\|100000 but returns int#'
200 changes: 113 additions & 87 deletions rules/DeadCode/Rector/ConstFetch/RemovePhpVersionIdCheckRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,27 @@
use PhpParser\Node\Expr\BinaryOp\Smaller;
use PhpParser\Node\Expr\ConstFetch;
use PhpParser\Node\Scalar\LNumber;
use PhpParser\Node\Stmt;
use PhpParser\Node\Stmt\If_;
use Rector\Core\Contract\Rector\ConfigurableRectorInterface;
use Rector\Core\Php\PhpVersionProvider;
use Rector\Core\Rector\AbstractRector;
use Rector\Core\Util\PhpVersionFactory;
use Rector\Core\ValueObject\PhpVersion;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
use Webmozart\Assert\Assert;

/**
* @see \Rector\Tests\DeadCode\Rector\ConstFetch\RemovePhpVersionIdCheckRector\RemovePhpVersionIdCheckRectorTest
*/
final class RemovePhpVersionIdCheckRector extends AbstractRector implements ConfigurableRectorInterface
{
private string | int | null $phpVersionConstraint = null;
/**
* @var PhpVersion::*|null
*/
private int | null $phpVersion = null;

public function __construct(
private readonly PhpVersionFactory $phpVersionFactory,
private readonly PhpVersionProvider $phpVersionProvider,
) {
}
Expand All @@ -39,13 +41,19 @@ public function __construct(
*/
public function configure(array $configuration): void
{
$this->phpVersionConstraint = array_pop($configuration);
$phpVersion = $configuration[0];
Assert::integer($phpVersion);
PhpVersion::assertValidValue($phpVersion);

// ensure cast to (string) first to allow string like "8.0" value to be converted to the int value
/** @var PhpVersion::* $phpVersion */
$this->phpVersion = $phpVersion;
}

public function getRuleDefinition(): RuleDefinition
{
return new RuleDefinition(
'Remove unneeded PHP_VERSION_ID check',
'Remove unneeded PHP_VERSION_ID conditional checks',
[
new ConfiguredCodeSample(
<<<'CODE_SAMPLE'
Expand All @@ -56,6 +64,7 @@ public function run()
if (PHP_VERSION_ID < 80000) {
return;
}

echo 'do something';
}
}
Expand All @@ -82,195 +91,212 @@ public function run()
*/
public function getNodeTypes(): array
{
return [ConstFetch::class];
return [If_::class];
}

/**
* @param ConstFetch $node
* @param If_ $node
* @return Stmt[]|null
*/
public function refactor(Node $node): ?Node
public function refactor(Node $node): ?array
{
if (! $this->isName($node, 'PHP_VERSION_ID')) {
return null;
}

/**
* $this->phpVersionProvider->provide() fallback is here as $currentFileProvider must be accessed after initialization
*/
$phpVersionConstraint = $this->phpVersionConstraint ?? $this->phpVersionProvider->provide();

// ensure cast to (string) first to allow string like "8.0" value to be converted to the int value
$this->phpVersionConstraint = $this->phpVersionFactory->createIntVersion((string) $phpVersionConstraint);

$if = $this->betterNodeFinder->findParentType($node, If_::class);
if (! $if instanceof If_) {
return null;
if ($this->phpVersion === null) {
$this->phpVersion = $this->phpVersionProvider->provide();
}

$parent = $node->getAttribute(AttributeKey::PARENT_NODE);
if ($this->shouldSkip($node, $if, $parent)) {
if (! $node->cond instanceof BinaryOp) {
return null;
}

if ($parent instanceof Smaller) {
return $this->processSmaller($node, $parent, $if);
}

if ($parent instanceof GreaterOrEqual) {
return $this->processGreaterOrEqual($node, $parent, $if);
$binaryOp = $node->cond;
if ($binaryOp->left instanceof ConstFetch && $this->isName($binaryOp->left->name, 'PHP_VERSION_ID')) {
return $this->refactorConstFetch($binaryOp->left, $node, $binaryOp);
}

if ($parent instanceof Greater) {
return $this->processGreater($node, $parent, $if);
}

return null;
}

private function shouldSkip(ConstFetch $constFetch, If_ $if, ?Node $node): bool
{
$if = $this->betterNodeFinder->findParentType($constFetch, If_::class);
if (! $if instanceof If_) {
return true;
if (! $binaryOp->right instanceof ConstFetch) {
return null;
}

$node = $constFetch->getAttribute(AttributeKey::PARENT_NODE);
if (! $node instanceof BinaryOp) {
return true;
if (! $this->isName($binaryOp->right->name, 'PHP_VERSION_ID')) {
return null;
}

return $if->cond !== $node;
return $this->refactorConstFetch($binaryOp->right, $node, $binaryOp);
}

private function processSmaller(ConstFetch $constFetch, Smaller $smaller, If_ $if): ?ConstFetch
/**
* @return Stmt[]|null
*/
private function processSmaller(ConstFetch $constFetch, Smaller $smaller, If_ $if): ?array
{
if ($smaller->left === $constFetch) {
return $this->processSmallerLeft($constFetch, $smaller, $if);
return $this->processSmallerLeft($smaller, $if);
}

if ($smaller->right === $constFetch) {
return $this->processSmallerRight($constFetch, $smaller, $if);
return $this->processSmallerRight($smaller, $if);
}

return null;
}

private function processGreaterOrEqual(ConstFetch $constFetch, GreaterOrEqual $greaterOrEqual, If_ $if): ?ConstFetch
{
/**
* @return Stmt[]|null
*/
private function processGreaterOrEqual(
ConstFetch $constFetch,
GreaterOrEqual $greaterOrEqual,
If_ $if,
): ?array {
if ($greaterOrEqual->left === $constFetch) {
return $this->processGreaterOrEqualLeft($constFetch, $greaterOrEqual, $if);
return $this->processGreaterOrEqualLeft($greaterOrEqual, $if);
}

if ($greaterOrEqual->right === $constFetch) {
return $this->processGreaterOrEqualRight($constFetch, $greaterOrEqual, $if);
return $this->processGreaterOrEqualRight($greaterOrEqual, $if);
}

return null;
}

private function processSmallerLeft(ConstFetch $constFetch, Smaller $smaller, If_ $if): ?ConstFetch
/**
* @return null
*/
private function processSmallerLeft(Smaller $smaller, If_ $if)
{
$value = $smaller->right;
if (! $value instanceof LNumber) {
return null;
}

if ($this->phpVersionConstraint >= $value->value) {
if ($this->phpVersion >= $value->value) {
$this->removeNode($if);
}

return $constFetch;
return null;
}

private function processSmallerRight(ConstFetch $constFetch, Smaller $smaller, If_ $if): ?ConstFetch
/**
* @return Stmt[]|null
*/
private function processSmallerRight(Smaller $smaller, If_ $if): ?array
{
$value = $smaller->left;
if (! $value instanceof LNumber) {
return null;
}

if ($this->phpVersionConstraint >= $value->value) {
$this->nodesToAddCollector->addNodesBeforeNode($if->stmts, $if);
$this->removeNode($if);
if ($this->phpVersion >= $value->value) {
return $if->stmts;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomasVotruba it needs ensure no stmts check or it can cause error:

There was 1 error:

1) Rector\Tests\DeadCode\Rector\ConstFetch\RemovePhpVersionIdCheckRector\RemovePhpVersionIdCheckRectorTest::test with data set #7 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
Rector\Core\Exception\ShouldNotHappenException: Array of nodes cannot be empty. Ensure "Rector\DeadCode\Rector\ConstFetch\RemovePhpVersionIdCheckRector->refactor()" returns non-empty array for Nodes.

A) Return null for no change:

    return null;

B) Remove the Node:

    $this->removeNode($node);
    return $node;

I created PR #2260 for it.

}

return $constFetch;
return null;
}

private function processGreaterOrEqualLeft(
ConstFetch $constFetch,
GreaterOrEqual $greaterOrEqual,
If_ $if
): ?ConstFetch {
/**
* @return Stmt[]|null
*/
private function processGreaterOrEqualLeft(GreaterOrEqual $greaterOrEqual, If_ $if): array|null
{
$value = $greaterOrEqual->right;
if (! $value instanceof LNumber) {
return null;
}

if ($this->phpVersionConstraint >= $value->value) {
$this->nodesToAddCollector->addNodesBeforeNode($if->stmts, $if);
$this->removeNode($if);
if ($this->phpVersion >= $value->value) {
return $if->stmts;
}

return $constFetch;
return null;
}

private function processGreaterOrEqualRight(
ConstFetch $constFetch,
GreaterOrEqual $greaterOrEqual,
If_ $if
): ?ConstFetch {
/**
* @return null
*/
private function processGreaterOrEqualRight(GreaterOrEqual $greaterOrEqual, If_ $if)
{
$value = $greaterOrEqual->left;
if (! $value instanceof LNumber) {
return null;
}

if ($this->phpVersionConstraint >= $value->value) {
if ($this->phpVersion >= $value->value) {
$this->removeNode($if);
}

return $constFetch;
return null;
}

private function processGreater(ConstFetch $constFetch, Greater $greater, If_ $if): ?ConstFetch
/**
* @return Stmt[]|null
*/
private function processGreater(ConstFetch $constFetch, Greater $greater, If_ $if): ?array
{
if ($greater->left === $constFetch) {
return $this->processGreaterLeft($constFetch, $greater, $if);
return $this->processGreaterLeft($greater, $if);
}

if ($greater->right === $constFetch) {
return $this->processGreaterRight($constFetch, $greater, $if);
return $this->processGreaterRight($greater, $if);
}

return null;
}

private function processGreaterLeft(ConstFetch $constFetch, Greater $greater, If_ $if): ?ConstFetch
/**
* @return Stmt[]|null
*/
private function processGreaterLeft(Greater $greater, If_ $if): ?array
{
$value = $greater->right;
if (! $value instanceof LNumber) {
return null;
}

if ($this->phpVersionConstraint >= $value->value) {
$this->nodesToAddCollector->addNodesBeforeNode($if->stmts, $if);
$this->removeNode($if);
if ($this->phpVersion >= $value->value) {
return $if->stmts;
}

return $constFetch;
return null;
}

private function processGreaterRight(ConstFetch $constFetch, Greater $greater, If_ $if): ?ConstFetch
/**
* @return null
*/
private function processGreaterRight(Greater $greater, If_ $if)
{
$value = $greater->left;
if (! $value instanceof LNumber) {
return null;
}

if ($this->phpVersionConstraint >= $value->value) {
if ($this->phpVersion >= $value->value) {
$this->removeNode($if);
}

return $constFetch;
return null;
}

/**
* @return Stmt[]|null
*/
private function refactorConstFetch(ConstFetch $constFetch, If_ $if, BinaryOp $binaryOp): ?array
{
if ($binaryOp instanceof Smaller) {
return $this->processSmaller($constFetch, $binaryOp, $if);
}

if ($binaryOp instanceof GreaterOrEqual) {
return $this->processGreaterOrEqual($constFetch, $binaryOp, $if);
}

if ($binaryOp instanceof Greater) {
return $this->processGreater($constFetch, $binaryOp, $if);
}

return null;
}
}
3 changes: 3 additions & 0 deletions src/Php/PhpVersionProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ public function __construct(
) {
}

/**
* @return PhpVersion::*
*/
public function provide(): int
{
$phpVersionFeatures = $this->parameterProvider->provideParameter(Option::PHP_VERSION_FEATURES);
Expand Down