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

[PHPStan 1.0] Remove node naming visitors and re-use PHPStan ones #1144

Merged
merged 7 commits into from
Nov 5, 2021
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
3 changes: 3 additions & 0 deletions packages/NodeTypeResolver/Node/AttributeKey.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

use Symplify\SmartFileSystem\SmartFileInfo;

/**
* @enum
*/
final class AttributeKey
{
/**
Expand Down
36 changes: 3 additions & 33 deletions packages/NodeTypeResolver/NodeScopeAndMetadataDecorator.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
use PhpParser\Node\Stmt;
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitor\CloningVisitor;
use PhpParser\NodeVisitor\NameResolver;
use PhpParser\NodeVisitor\NodeConnectingVisitor;
use Rector\Core\ValueObject\Application\File;
use Rector\NodeTypeResolver\NodeVisitor\FunctionLikeParamArgPositionNodeVisitor;
Expand All @@ -18,16 +17,6 @@

final class NodeScopeAndMetadataDecorator
{
/**
* @var string
*/
private const OPTION_PRESERVE_ORIGINAL_NAMES = 'preserveOriginalNames';

/**
* @var string
*/
private const OPTION_REPLACE_NODES = 'replaceNodes';

public function __construct(
private CloningVisitor $cloningVisitor,
private FunctionMethodAndClassNodeVisitor $functionMethodAndClassNodeVisitor,
Expand All @@ -45,35 +34,16 @@ public function __construct(
*/
public function decorateNodesFromFile(File $file, array $stmts): array
{
$nodeTraverser = new NodeTraverser();
$nodeTraverser->addVisitor(new NameResolver(null, [
self::OPTION_PRESERVE_ORIGINAL_NAMES => true,
// required by PHPStan
self::OPTION_REPLACE_NODES => true,
]));

/** @var Stmt[] $stmts */
$stmts = $nodeTraverser->traverse($stmts);

$smartFileInfo = $file->getSmartFileInfo();

$stmts = $this->phpStanNodeScopeResolver->processNodes($stmts, $smartFileInfo);

$nodeTraverserForPreservingName = new NodeTraverser();

$preservingNameResolver = new NameResolver(null, [
self::OPTION_PRESERVE_ORIGINAL_NAMES => true,
// this option would override old non-fqn-namespaced nodes otherwise, so it needs to be disabled
self::OPTION_REPLACE_NODES => false,
]);

$nodeTraverserForPreservingName->addVisitor($preservingNameResolver);
$stmts = $nodeTraverserForPreservingName->traverse($stmts);

Comment on lines 36 to -72
Copy link
Member Author

Choose a reason for hiding this comment

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

Main point of this PR is to remove these 2 node name traversers. Why?

Pros

Cons

It has downside in losing rules that rely deply on original node names without overriding, mainly with alias imports in union. Thus 2 rules had to removed:

Yet, I think it's worth it, as we cut 3 traverses to 1 and based on shared names with PHPStan traversing, making complexity lower in general.

$nodeTraverserForFormatPreservePrinting = new NodeTraverser();
// needed also for format preserving printing
$nodeTraverserForFormatPreservePrinting->addVisitor($this->cloningVisitor);

// this one has to be run again to re-connect nodes with new attributes
$nodeTraverserForFormatPreservePrinting->addVisitor($this->nodeConnectingVisitor);

$nodeTraverserForFormatPreservePrinting->addVisitor($this->functionMethodAndClassNodeVisitor);
$nodeTraverserForFormatPreservePrinting->addVisitor($this->namespaceNodeVisitor);
$nodeTraverserForFormatPreservePrinting->addVisitor($this->functionLikeParamArgPositionNodeVisitor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ declare(strict_types=1);

namespace Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Fixture;

use Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source\Contract\Token;
use Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source\Contract;

abstract class KeepStaticMethod
{
public static function decode(string $token): Contract\Token
Expand All @@ -22,7 +25,9 @@ declare(strict_types=1);

namespace Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Fixture;

use Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Fixture\Contract\Token;
use Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source\Contract\Token;
use Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source\Contract;

abstract class KeepStaticMethod
{
public static function decode(string $token): Token
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,3 @@ class SkipAliasedNames
$constraint = Assert\Blank::class;
}
}
?>
-----
<?php
namespace Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Fixture;

use Symfony\Component\Validator\Constraints\Blank;
use Symfony\Component\Validator\Constraints as Assert;

class SkipAliasedNames
{
public function __construct()
{
$constraint = Blank::class;
}
}
?>
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,3 @@ class SkipSameNamespacedUsedClass
{
}
}
-----
<?php

namespace Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Fixture;

use Rector\Tests\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector\Source\SharedShortName;
class SkipSameNamespacedUsedClass
{
/**
* @return SharedShortName
*/
public function run(): SharedShortName
{
}
}
Copy link
Member

Choose a reason for hiding this comment

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

this expected output seems previously correct, as the docblock @return changed to short name, the "skip" here means skipping from re-import

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ use PhpSpec\ObjectBehavior;
class RatesTest extends \PHPUnit\Framework\TestCase
{
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\Rates $rates;
private \PHPUnit\Framework\MockObject\MockObject|\spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\Provider $provider;
private \PHPUnit\Framework\MockObject\MockObject|\Provider $provider;
Copy link
Member

@samsonasik samsonasik Nov 6, 2021

Choose a reason for hiding this comment

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

this should be Provider

protected function setUp()
{
$this->provider = $this->createMock(Provider::class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class CreateMeTest extends \PHPUnit\Framework\TestCase

public function testCalled()
{
/** @var spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\Cart|\PHPUnit\Framework\MockObject\MockObject $cart */
/** @var Cart|\PHPUnit\Framework\MockObject\MockObject $cart */
Copy link
Member

Choose a reason for hiding this comment

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

this is incorrect, same with property to use namespaced name, the @var should use namespaced as well as namespace changed.

Copy link
Member

Choose a reason for hiding this comment

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

or possibly not, it depends on if actually the class used moved or not after namespace changed, as next this->createMock() uses Cart::class, it technically doesn't break as it doesn't add \ prefix.

it is ok then for now.

Copy link
Member

Choose a reason for hiding this comment

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

the outside class type actualy need to be moved under Source to avoid miss naming

$cart = $this->createMock(Cart::class);
$cart->expects($this->atLeastOnce())->method('price')->willReturn(5);
$cart->expects($this->atLeastOnce())->method('shippingAddress')->with($this->isInstanceOf(Address::class));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class BlablaTest extends \PHPUnit\Framework\TestCase
}
public function testMe()
{
/** @var spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\SomeType|\PHPUnit\Framework\MockObject\MockObject $someType */
/** @var SomeType|\PHPUnit\Framework\MockObject\MockObject $someType */
$someType = $this->createMock(SomeType::class);
$assignMe = $someType;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
class PageLinksGeneratorTest extends \PHPUnit\Framework\TestCase
{
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\PageLinksGenerator $pageLinksGenerator;
private \PHPUnit\Framework\MockObject\MockObject|\Symfony\Bundle\FrameworkBundle\Routing\Router $router;
private \PHPUnit\Framework\MockObject\MockObject|\Router $router;
Copy link
Member

@samsonasik samsonasik Nov 6, 2021

Choose a reason for hiding this comment

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

this is also incorrect for \Router as the namespace Symfony\Bundle\FrameworkBundle\Routing\Router imported in use statement.

public function testInitializable()
{
$this->assertInstanceOf(PageLinksGenerator::class, $this->pageLinksGenerator);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use PhpSpec\ObjectBehavior;
class CurrencyTest extends \PHPUnit\Framework\TestCase
{
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\Currency $currency;
private \PHPUnit\Framework\MockObject\MockObject|\spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\CurrencyData $data;
private \CurrencyData|\PHPUnit\Framework\MockObject\MockObject $data;
protected function setUp()
{
$this->data = $this->createMock(CurrencyData::class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ class DeliveryTest extends \PHPUnit\Framework\TestCase
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\Delivery $delivery;
protected function setUp()
{
/** @var Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Source\DeliveryFactory|\PHPUnit\Framework\MockObject\MockObject $factory */
/** @var DeliveryFactory|\PHPUnit\Framework\MockObject\MockObject $factory */
$factory = $this->createMock(DeliveryFactory::class);
/** @var Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Source\ShippingMethod|\PHPUnit\Framework\MockObject\MockObject $shippingMethod */
/** @var ShippingMethod|\PHPUnit\Framework\MockObject\MockObject $shippingMethod */
$shippingMethod = $this->createMock(ShippingMethod::class);
$factory->expects($this->atLeastOnce())
->method('createShippingMethodFor')->with($this->equalTo(5))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Source\
class MockPropertiesTest extends \PHPUnit\Framework\TestCase
{
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\MockProperties $mockProperties;
private \PHPUnit\Framework\MockObject\MockObject|\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Source\OrderFactory $factory;
private \OrderFactory|\PHPUnit\Framework\MockObject\MockObject $factory;
Copy link
Member

Choose a reason for hiding this comment

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

this is incorrect as OrderFactory is from Source namespace from use statement

protected function setUp()
{
$this->factory = $this->createMock(OrderFactory::class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,22 @@ class MockPropertiesNonLocalTest extends \PHPUnit\Framework\TestCase
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\MockPropertiesNonLocal $mockPropertiesNonLocal;
protected function setUp()
{
/** @var spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\OrderFactory|\PHPUnit\Framework\MockObject\MockObject $factory */
/** @var OrderFactory|\PHPUnit\Framework\MockObject\MockObject $factory */
$factory = $this->createMock(OrderFactory::class);
$this->mockPropertiesNonLocal = new \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\MockPropertiesNonLocal($factory);
}

public function testLetItGo()
{
/** @var Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Source\AnotherMock|\PHPUnit\Framework\MockObject\MockObject $anotherMock */
/** @var AnotherMock|\PHPUnit\Framework\MockObject\MockObject $anotherMock */
$anotherMock = $this->createMock(AnotherMock::class);
$anotherMock->setName('Nummy');
$this->mockPropertiesNonLocal->addAnotherMock($anotherMock);
}

public function testLetItGoAgain()
{
/** @var Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Source\AnotherMock|\PHPUnit\Framework\MockObject\MockObject $anotherMock */
/** @var AnotherMock|\PHPUnit\Framework\MockObject\MockObject $anotherMock */
$anotherMock = $this->createMock(AnotherMock::class);
$anotherMock->setName('Nummy2');
$this->mockPropertiesNonLocal->addAnotherMock($anotherMock);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ class OrderTest extends \PHPUnit\Framework\TestCase
private \Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\Order $order;
protected function setUp()
{
/** @var spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\OrderFactory|\PHPUnit\Framework\MockObject\MockObject $factory */
/** @var OrderFactory|\PHPUnit\Framework\MockObject\MockObject $factory */
$factory = $this->createMock(OrderFactory::class);
/** @var spec\Rector\Tests\PhpSpecToPHPUnit\Rector\Variable\PhpSpecToPHPUnitRector\Fixture\ShippingMethod|\PHPUnit\Framework\MockObject\MockObject $shippingMethod */
/** @var ShippingMethod|\PHPUnit\Framework\MockObject\MockObject $shippingMethod */
$shippingMethod = $this->createMock(ShippingMethod::class);
$factory->expects($this->atLeastOnce())->method('createShippingMethodFor')->willReturn($shippingMethod);
$factory->expects($this->atLeastOnce())->method('anotherMethod')->with($this->equalTo(25))->willReturn($shippingMethod);
Expand Down
22 changes: 2 additions & 20 deletions rules/DeadCode/NodeAnalyzer/ExprUsedInNextNodeAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@

use PhpParser\Node;
use PhpParser\Node\Expr;
use PhpParser\Node\Name;
use PHPStan\Analyser\Scope;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\NodeTypeResolver\Node\AttributeKey;

final class ExprUsedInNextNodeAnalyzer
{
Expand All @@ -19,26 +16,11 @@ public function __construct(
) {
}

/**
* $isCheckNameScope parameter is used to whether to check scope of Name that may be renamed
* @see https://github.com/rectorphp/rector/issues/6675
*/
public function isUsed(Expr $expr, bool $isCheckNameScope = false): bool
public function isUsed(Expr $expr): bool
{
return (bool) $this->betterNodeFinder->findFirstNext(
$expr,
function (Node $node) use ($expr, $isCheckNameScope): bool {
if ($isCheckNameScope && $node instanceof Name) {
$scope = $node->getAttribute(AttributeKey::SCOPE);
$resolvedName = $node->getAttribute(AttributeKey::RESOLVED_NAME);

if (! $scope instanceof Scope && ! $resolvedName instanceof Name) {
return true;
}
}

return $this->exprUsedInNodeAnalyzer->isUsed($node, $expr);
}
Copy link
Member

@samsonasik samsonasik Nov 5, 2021

Choose a reason for hiding this comment

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

This cause invalid result on mysql to mysqli on multiple rules applied like in my previous comment

fn (Node $node): bool => $this->exprUsedInNodeAnalyzer->isUsed($node, $expr)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public function refactor(Node $node): ?Node
return null;
}

if ($this->exprUsedInNextNodeAnalyzer->isUsed($node->var, true)) {
if ($this->exprUsedInNextNodeAnalyzer->isUsed($node->var)) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@

use PhpParser\Node;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\Interface_;
use PhpParser\Node\Stmt\Property;
use PhpParser\Node\Stmt\Trait_;
use Rector\Core\NodeManipulator\PropertyManipulator;
use Rector\Core\Rector\AbstractRector;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Removing\NodeManipulator\ComplexNodeRemover;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
Expand Down Expand Up @@ -83,8 +80,7 @@ private function shouldSkipProperty(Property $property): bool
return true;
}

/** @var Class_|Interface_|Trait_|null $classLike */
$classLike = $property->getAttribute(AttributeKey::CLASS_NODE);
return ! $classLike instanceof Class_;
$class = $this->betterNodeFinder->findParentType($property, Class_::class);
return ! $class instanceof Class_;
}
}
3 changes: 1 addition & 2 deletions src/NodeManipulator/PropertyManipulator.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,8 @@ public function isPropertyUsedInReadContext(Property | Param $propertyOrPromoted
return true;
}

// $propertyOrPromotedParam->attrGroups

$privatePropertyFetches = $this->propertyFetchFinder->findPrivatePropertyFetches($propertyOrPromotedParam);

foreach ($privatePropertyFetches as $privatePropertyFetch) {
if ($this->readWritePropertyAnalyzer->isRead($privatePropertyFetch)) {
return true;
Expand Down
Loading