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

TomasVotruba · 2021-11-04T17:21:50Z

No description provided.

…of use import

TomasVotruba · 2021-11-05T22:44:40Z

packages/NodeTypeResolver/NodeScopeAndMetadataDecorator.php

    {
-        $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);
-


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

Pros

They already happen in PHPStan parser that we now use since PHPStan 1.0

/~https://github.com/phpstan/phpstan-src/blob/c471c7b050e0929daf432288770de673b394a983/src/Parser/RichParser.php

/~https://github.com/phpstan/phpstan-src/blob/c471c7b050e0929daf432288770de673b394a983/src/Parser/SimpleParser.php

Here are used to work around PHPStan changing names without keeping the originals, without those, Rector would print nodes in FQN forms everywhere

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:

[CodeQuality] Remove FixClassCaseSensitivityNameRector, conflicting with name node removal and edge case already handled by PHPStan reports #1160

[CodingStyle] Remove RemoveUnusedAliasRector, job rather for coding standard tool and opinonated #1157

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.

samsonasik · 2021-11-05T22:47:56Z

tests/Issues/Issue6675/Fixture/fixture.php.inc

@@ -12,6 +12,6 @@ echo mysql_error();
 namespace Rector\Core\Tests\Issues\Issue6675\Fixture;

 $db = mysqli_connect("server","user","password");
-echo mysqli_error($db);
+echo mysql_error();


previous code is already correct as it migrate mysql to mysqli

samsonasik · 2021-11-05T22:50:02Z

rules/DeadCode/NodeAnalyzer/ExprUsedInNextNodeAnalyzer.php

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


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

samsonasik · 2021-11-05T23:00:19Z

...r/Namespace_/ImportFullyQualifiedNamesRector/Fixture/skip_same_namespaced_used_class.php.inc

-    public function run(): SharedShortName
-    {
-    }
-}


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

TomasVotruba · 2021-11-05T23:06:12Z

Thanks for feedback. Could you try restore behavior of those fixture after I merge this PR? I failed to do so due to changes caused by switching to PHPStan's NameResolver. I've put in last 2 days and I'm bit frustrated with it.

samsonasik · 2021-11-05T23:08:39Z

...ests/PhpSpecToPHPUnit/Rector/Variable/PhpSpecToPHPUnitRector/Fixture/mock_properties.php.inc

@@ -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;


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

samsonasik · 2021-11-05T23:11:17Z

@TomasVotruba I will try.

samsonasik · 2021-11-06T00:02:30Z

rules-tests/PhpSpecToPHPUnit/Rector/Variable/PhpSpecToPHPUnitRector/Fixture/issue_3931.php.inc

@@ -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;


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

samsonasik · 2021-11-06T00:46:12Z

...tests/PhpSpecToPHPUnit/Rector/Variable/PhpSpecToPHPUnitRector/Fixture/custom_matcher.php.inc

@@ -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;


this should be Provider

samsonasik · 2021-11-06T08:07:29Z

.../Property/RemoveUnusedPrivatePropertyRector/Fixture/no_usage_outside_anonymous_class.php.inc

-        };
-    }
-}
-?>


this is incorrect and the behaviour need to be kept

samsonasik · 2021-11-06T08:50:49Z

rules-tests/PhpSpecToPHPUnit/Rector/Variable/PhpSpecToPHPUnitRector/Fixture/fixture.php.inc

@@ -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 */


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

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.

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

TomasVotruba changed the title ~~remove node naming visitors~~ [PHPStan 1.0] Remove node naming visitors and re-use PHPStan ones Nov 4, 2021

TomasVotruba force-pushed the tv-traversing-less branch 20 times, most recently from ecda4e3 to ab88e1f Compare November 5, 2021 13:42

TomasVotruba mentioned this pull request Nov 5, 2021

[CodingStyle] Remove RemoveUnusedAliasRector, job rather for coding standard tool and opinonated #1157

Merged

TomasVotruba force-pushed the tv-traversing-less branch 8 times, most recently from 07295c0 to e4597a6 Compare November 5, 2021 14:49

TomasVotruba force-pushed the tv-traversing-less branch 3 times, most recently from 93ae985 to fb751c3 Compare November 5, 2021 20:42

TomasVotruba added 6 commits November 5, 2021 23:36

remove node naming visitors

059f54e

skip FixClassCaseSensitivityNameRector for unknown orig name in case …

f40a02b

…of use import

restore state before PHPStan 1.0

a8044ed

skip dead property in unused private property

939846c

imports

695764e

enum

830e7f2

TomasVotruba force-pushed the tv-traversing-less branch 5 times, most recently from 345aaa1 to 1f3994e Compare November 5, 2021 22:38

TomasVotruba commented Nov 5, 2021

View reviewed changes

skip rename for now, till re-traversing

063002e

TomasVotruba force-pushed the tv-traversing-less branch from 1f3994e to 063002e Compare November 5, 2021 22:45

samsonasik reviewed Nov 5, 2021

View reviewed changes

TomasVotruba merged commit 2304666 into main Nov 5, 2021

TomasVotruba deleted the tv-traversing-less branch November 5, 2021 23:06

samsonasik reviewed Nov 5, 2021

View reviewed changes

samsonasik mentioned this pull request Nov 6, 2021

[MYSQL_TO_MYSQLI] Restore Mysql to Mysqli Behaviour when used with Dead Code SetList #1161

Merged

samsonasik reviewed Nov 6, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TomasVotruba commented Nov 4, 2021

TomasVotruba Nov 5, 2021

samsonasik Nov 5, 2021

samsonasik Nov 5, 2021 •

edited

Loading

samsonasik Nov 5, 2021

TomasVotruba commented Nov 5, 2021 •

edited

Loading

samsonasik Nov 5, 2021

samsonasik commented Nov 5, 2021

samsonasik Nov 6, 2021 •

edited

Loading

samsonasik Nov 6, 2021 •

edited

Loading

samsonasik Nov 6, 2021

samsonasik Nov 6, 2021

samsonasik Nov 6, 2021

samsonasik Nov 6, 2021

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

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

Conversation

TomasVotruba commented Nov 4, 2021

Choose a reason for hiding this comment

Pros

Cons

Choose a reason for hiding this comment

samsonasik Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

samsonasik commented Nov 5, 2021

samsonasik Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

samsonasik Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik Nov 5, 2021 •

edited

Loading

TomasVotruba commented Nov 5, 2021 •

edited

Loading

samsonasik Nov 6, 2021 •

edited

Loading

samsonasik Nov 6, 2021 •

edited

Loading