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

[Traverser] Set explicitely nodeConnectingVisitorCompatibility: false config in config/phpstan/static-reflection.neon #4841

Merged
merged 2 commits into from Aug 24, 2023

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Aug 24, 2023

@TomasVotruba the parent, prev, and next attribute seems shown after bleeding edge disabled, because of this disabled by bleedingEdge.neon config:

https://github.com/phpstan/phpstan-src/blob/9a014a17000af1e1c99783f48aa3d61699406c3f/conf/bleedingEdge.neon#L10

Before set nodeConnectingVisitorCompatibility: false

.PhpParser\Node\Stmt\ClassMethod #42281
   attributes: array (13)
   |  'parent' => PhpParser\Node\Stmt\Enum_ #42282 ...
   |  'previous' => PhpParser\Node\Stmt\ClassMethod #42274 ...

After set nodeConnectingVisitorCompatibility: false

null

This config set is to disable NodeConnectingVisitor by default that caused by PR:

make enabled again.

Fixes rectorphp/rector#8155

@samsonasik
Copy link
Member Author

It seems cause error on rector-downgrade-php:

There was 1 failure:

1) Rector\Tests\DowngradePhp81\Rector\FuncCall\DowngradeFirstClassCallableSyntaxRector\DowngradeFirstClassCallableSyntaxRectorTest::test with data set #5
Failed on fixture file "static_call.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 {
     public function run(): \Closure
     {
-        return \Closure::fromCallable([SomeClass::class, 'dump']);
+        return \Closure::fromCallable([\Closure::fromCallable('strlen'), 'dump']);

I will check.

@samsonasik samsonasik changed the title [Traverser] Set nodeConnectingVisitorCompatibility: false config in config/phpstan/static-reflection.neon [Traverser] Set explicitely nodeConnectingVisitorCompatibility: false config in config/phpstan/static-reflection.neon Aug 24, 2023
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba let's merge it to have speed again 👍

@samsonasik samsonasik merged commit 295156c into main Aug 24, 2023
39 checks passed
@samsonasik samsonasik deleted the phpstan-config branch August 24, 2023 07:15
@staabm
Copy link
Contributor

staabm commented Aug 24, 2023

did you check whether we see a reduction in memory usage / improvement in performance with this change?

@samsonasik
Copy link
Member Author

@staabm Yes, on downgrade, it 30 seconds faster:

Before

Screen Shot 2023-08-24 at 14 19 38

Ref https://github.com/rectorphp/rector-src/actions/runs/5953405843/job/16147568686#step:10:1

After

Screen Shot 2023-08-24 at 14 19 49

Ref https://github.com/rectorphp/rector-src/actions/runs/5960656514/job/16168370106#step:10:1

@staabm
Copy link
Contributor

staabm commented Aug 24, 2023

awesome 🚀

@TomasVotruba
Copy link
Member

Thanks for handling this. Explicit configuration is makes it more clear 🙏

@staabm
Copy link
Contributor

staabm commented Aug 24, 2023

cool. thank you guys.

any blockers for a new release?

AFAIR there is a bug in 0.18.0 which prevents updating to 0.18.x for us (which is fixed in the main branch already)

@@ -2,6 +2,7 @@ parameters:
# see https://github.com/rectorphp/rector/issues/3490#issue-634342324
featureToggles:
disableRuntimeReflectionProvider: false
nodeConnectingVisitorCompatibility: false
Copy link
Member Author

Choose a reason for hiding this comment

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

@TomasVotruba after various try, this disable seems the issue that create error in vendor/rector/rector-symfony/config that temporary not downgraded at PR:

Step to reproduce:

  • comment 'vendor/rector/rector-symfony/config' on build/config/config-downgrade.php
-        'vendor/rector/rector-symfony/config',
+       // 'vendor/rector/rector-symfony/config',
  • Run command:
bin/rector process vendor/rector/rector-symfony/config/sets --config build/config/config-downgrade.php --ansi --dry-run --clear-cache --no-diffs

and it will got error:

 [ERROR] Could not process                                                                                              
         "/Users/samsonasik/www/rector-src/vendor/rector/rector-symfony/config/sets/twig/level/up-to-twig-127.php" file,
         due to:                                                                                                        
         "System error: "Trying to replace statement (Stmt_Return) with expression (Expr_MethodCall). Are you missing a 
         Stmt_Expression wrapper?"                                                                                      
         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On line: 277

Then, try comment this nodeConnectingVisitorCompatibility: false flag:

-        nodeConnectingVisitorCompatibility: false
+#        nodeConnectingVisitorCompatibility: false

and it working ok:

➜  rector-src git:(main) ✗ bin/rector process vendor/rector/rector-symfony/config/sets --config build/config/config-downgrade.php --ansi --dry-run --clear-cache --no-diffs
 69/69 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [OK] 6 files would have changed (dry-run) by Rector                                                                    

so the problem "parent" attribute seems somehow used in PHPStan internally probably on connecting scope or printing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it seems can be tweaked by early skip when origNode class is not equal with target node on RectifiedAnalyzer:

     private function isJustReprintedOverlappedTokenStart(Node $node, ?Node $originalNode): bool
     {
         if ($originalNode instanceof Node) {
-            return false;
+            return $originalNode::class === $node::class;
         }

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants