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

[CodeQuality] Handle crash on yield from on OptionalParametersAfterRequiredRector #6545

Merged
merged 6 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

declare(strict_types=1);

namespace Rector\Tests\CodeQuality\Rector\ClassMethod\OptionalParametersAfterRequiredRector\Fixture;

final class SkipFirstClassCallableInYield2
{
public function getSubscribedEvents(string $nodePath): iterable
{
yield from $this->textElement(...);
}

public function textElement() { return []; }
}
15 changes: 7 additions & 8 deletions src/Configuration/ConfigurationRuleFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,33 +20,32 @@ public function setConfiguration(Configuration $configuration): void
}

/**
* @param array<RectorInterface> $rectors
* @return array<RectorInterface>
* @param list<RectorInterface> $rectors
* @return list<RectorInterface>
*/
public function filter(array $rectors): array
{
if ($this->configuration === null) {
if (!$this->configuration instanceof Configuration) {
return $rectors;
}

$onlyRule = $this->configuration->getOnlyRule();
if ($onlyRule !== null) {
$rectors = $this->filterOnlyRule($rectors, $onlyRule);
return $rectors;
return $this->filterOnlyRule($rectors, $onlyRule);
}

return $rectors;
}

/**
* @param array<RectorInterface> $rectors
* @return array<RectorInterface>
* @param list<RectorInterface> $rectors
* @return list<RectorInterface>
*/
public function filterOnlyRule(array $rectors, string $onlyRule): array
{
$activeRectors = [];
foreach ($rectors as $rector) {
if (is_a($rector, $onlyRule)) {
if ($rector instanceof $onlyRule) {
$activeRectors[] = $rector;
}
}
Expand Down
13 changes: 8 additions & 5 deletions src/Configuration/OnlyRuleResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@
/**
* @see \Rector\Tests\Configuration\OnlyRuleResolverTest
*/
final class OnlyRuleResolver
final readonly class OnlyRuleResolver
{
/**
* @param RectorInterface[] $rectors
*/
public function __construct(
private readonly array $rectors
private array $rectors
) {
}

Expand Down Expand Up @@ -46,11 +46,13 @@ public function resolve(string $rule): string
$matching[] = $rector::class;
}
}
$matching = array_unique($matching);

$matching = array_unique($matching);
if (count($matching) == 1) {
return $matching[0];
} elseif (count($matching) > 1) {
}

if (count($matching) > 1) {
sort($matching);
$message = sprintf(
'Short rule name "%s" is ambiguous. Specify the full rule name:' . PHP_EOL
Expand All @@ -60,7 +62,7 @@ public function resolve(string $rule): string
throw new RectorRuleNameAmbigiousException($message);
}

if (strpos($rule, '\\') === false) {
if (in_array(str_contains($rule, '\\'), [0, false], true)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

rectify seems should be only str_contains($rule, '\\') === false, as str_contains never return 0, only bool. I will check.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems due 2 rules, with union type, that buggy due to usage of:

if ($exprType->isBoolean()->yes()) {
 0/1 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░]   0%
PHPStan\Type\UnionType #22767
   types: array (2)
   |  0 => PHPStan\Type\IntegerRangeType #20331
   |  |  min: 0
   |  |  max: null
   |  1 => PHPStan\Type\Constant\ConstantBooleanType #22757
   |  |  value: false
   normalized: true
   sortedTypes: true
   cachedDescriptions: array (1)
   |  4 => 'int<0, max>|false'
➜  rector-src git:(main) ✗ bin/rector process src/Configuration/OnlyRuleResolver.php --dry-run
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) src/Configuration/OnlyRuleResolver.php:61

    ---------- begin diff ----------
@@ @@
             throw new RectorRuleNameAmbigiousException($message);
         }

-        if (strpos($rule, '\\') === false) {
+        if (in_array(str_contains($rule, '\\'), [0, false], true)) {
             $message = sprintf(
                 'Rule "%s" was not found.%sThe rule has no namespace. Make sure to escape the backslashes, and add quotes around the rule name: --only="My\\Rector\\Rule"',
                 $rule,
    ----------- end diff -----------

Applied rules:
 * StrContainsRector
 * BooleanInBooleanNotRuleFixerRector

Copy link
Member Author

Choose a reason for hiding this comment

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

That's seems issue in phpstan side for detecting it as union, I just directly patch it on the code for now:

$message = sprintf(
'Rule "%s" was not found.%sThe rule has no namespace. Make sure to escape the backslashes, and add quotes around the rule name: --only="My\\Rector\\Rule"',
$rule,
Expand All @@ -73,6 +75,7 @@ public function resolve(string $rule): string
PHP_EOL
);
}

throw new RectorRuleNotFoundException($message);
}
}
1 change: 1 addition & 0 deletions src/Console/Command/ListRulesCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
if ($onlyRule !== null) {
$onlyRule = $this->onlyRuleResolver->resolve($onlyRule);
}

$rectorClasses = $this->resolveRectorClasses($onlyRule);

$skippedClasses = $this->getSkippedCheckers();
Expand Down
2 changes: 1 addition & 1 deletion src/Console/Command/ProcessCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function __construct(
private readonly ConfigurationFactory $configurationFactory,
private readonly DeprecatedRulesReporter $deprecatedRulesReporter,
private readonly MissConfigurationReporter $missConfigurationReporter,
private ConfigurationRuleFilter $configurationRuleFilter,
private readonly ConfigurationRuleFilter $configurationRuleFilter,
) {
parent::__construct();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use PhpParser\Node\Expr\Ternary;
use PhpParser\Node\Expr\Variable;
use PhpParser\Node\Expr\Yield_;
use PhpParser\Node\Expr\YieldFrom;
use PhpParser\Node\Identifier;
use PhpParser\Node\IntersectionType;
use PhpParser\Node\Name;
Expand Down Expand Up @@ -178,7 +179,8 @@ public function processNodes(
$node instanceof Expression ||
$node instanceof Return_ ||
$node instanceof EnumCase ||
$node instanceof Cast
$node instanceof Cast ||
$node instanceof YieldFrom
) && $node->expr instanceof Expr) {
$node->expr->setAttribute(AttributeKey::SCOPE, $mutatingScope);
return;
Expand Down Expand Up @@ -306,6 +308,7 @@ public function processNodes(

if ($node instanceof Yield_) {
$this->processYield($node, $mutatingScope);
return;
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/Parallel/Command/WorkerCommandLineFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public function create(

if ($input->getOption(Option::ONLY) !== null) {
$workerCommandArray[] = self::OPTION_DASHES . Option::ONLY;
$workerCommandArray[] = escapeshellarg($input->getOption(Option::ONLY));
$workerCommandArray[] = escapeshellarg((string) $input->getOption(Option::ONLY));
}

return implode(' ', $workerCommandArray);
Expand Down
48 changes: 25 additions & 23 deletions tests/Configuration/OnlyRuleResolverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

namespace Rector\Tests\Configuration;

use Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector;
use Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPrivateMethodRector;
use Rector\Configuration\OnlyRuleResolver;
use Rector\Contract\Rector\RectorInterface;
use Rector\Exception\Configuration\RectorRuleNameAmbigiousException;
Expand All @@ -12,46 +14,46 @@

final class OnlyRuleResolverTest extends AbstractLazyTestCase
{
protected OnlyRuleResolver $resolver;
private OnlyRuleResolver $onlyRuleResolver;

protected function setUp(): void
{
$this->bootFromConfigFiles([__DIR__ . '/config/only_rule_resolver_config.php']);
$rectorConfig = self::getContainer();

$this->resolver = new OnlyRuleResolver(iterator_to_array($rectorConfig->tagged(RectorInterface::class)));
$this->onlyRuleResolver = new OnlyRuleResolver(iterator_to_array($rectorConfig->tagged(RectorInterface::class)));
}

public function testResolveOk(): void
{
$this->assertEquals(
\Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector::class,
$this->resolver->resolve('Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector')
$this->assertSame(
RemoveDoubleAssignRector::class,
$this->onlyRuleResolver->resolve('Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector')
);
}

public function testResolveOkLeadingBackslash(): void
{
$this->assertEquals(
\Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector::class,
$this->resolver->resolve('\\Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector')
$this->assertSame(
RemoveDoubleAssignRector::class,
$this->onlyRuleResolver->resolve('\\Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector')
);
}

public function testResolveOkDoubleBackslashes(): void
{
$this->assertEquals(
\Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector::class,
$this->resolver->resolve('\\\\Rector\\\\DeadCode\\\\Rector\\\\Assign\\\\RemoveDoubleAssignRector'),
$this->assertSame(
RemoveDoubleAssignRector::class,
$this->onlyRuleResolver->resolve('\\\\Rector\\\\DeadCode\\\\Rector\\\\Assign\\\\RemoveDoubleAssignRector'),
'We want to fix wrongly double-quoted backslashes automatically'
);
}

public function testResolveOkSingleQuotes(): void
{
$this->assertEquals(
\Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector::class,
$this->resolver->resolve("'Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector'"),
$this->assertSame(
RemoveDoubleAssignRector::class,
$this->onlyRuleResolver->resolve("'Rector\\DeadCode\\Rector\\Assign\\RemoveDoubleAssignRector'"),
'Remove stray single quotes on Windows systems'
);
}
Expand All @@ -64,7 +66,7 @@ public function testResolveMissingBackslash(): void
);
$this->expectException(RectorRuleNotFoundException::class);

$this->resolver->resolve('RectorDeadCodeRectorAssignRemoveDoubleAssignRector');
$this->onlyRuleResolver->resolve('RectorDeadCodeRectorAssignRemoveDoubleAssignRector');
}

public function testResolveNotFound(): void
Expand All @@ -75,22 +77,22 @@ public function testResolveNotFound(): void
);
$this->expectException(RectorRuleNotFoundException::class);

$this->resolver->resolve('This\\Rule\\Does\\Not\\Exist');
$this->onlyRuleResolver->resolve('This\\Rule\\Does\\Not\\Exist');
}

public function testResolveShortOk(): void
{
$this->assertEquals(
\Rector\DeadCode\Rector\ClassMethod\RemoveUnusedPrivateMethodRector::class,
$this->resolver->resolve('RemoveUnusedPrivateMethodRector'),
$this->assertSame(
RemoveUnusedPrivateMethodRector::class,
$this->onlyRuleResolver->resolve('RemoveUnusedPrivateMethodRector'),
);
}

public function testResolveShortOkTwoLevels(): void
{
$this->assertEquals(
\Rector\DeadCode\Rector\Assign\RemoveDoubleAssignRector::class,
$this->resolver->resolve('Assign\\RemoveDoubleAssignRector'),
$this->assertSame(
RemoveDoubleAssignRector::class,
$this->onlyRuleResolver->resolve('Assign\\RemoveDoubleAssignRector'),
);
}

Expand All @@ -103,6 +105,6 @@ public function testResolveShortAmbiguous(): void
);
$this->expectException(RectorRuleNameAmbigiousException::class);

$this->resolver->resolve('RemoveDoubleAssignRector');
$this->onlyRuleResolver->resolve('RemoveDoubleAssignRector');
}
}
Loading