Skip to content

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

Merged
samsonasik merged 2 commits intomainfrom
phpstan-config
Aug 24, 2023
Merged

[Traverser] Set explicitely nodeConnectingVisitorCompatibility: false config in config/phpstan/static-reflection.neon#4841
samsonasik merged 2 commits intomainfrom
phpstan-config

Conversation

@samsonasik
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@samsonasik samsonasik deleted the phpstan-config branch August 24, 2023 07:15
@staabm
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

staabm commented Aug 24, 2023

awesome 🚀

@TomasVotruba
Copy link
Copy Markdown
Member

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

@staabm
Copy link
Copy Markdown
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)

# see https://github.com/rectorphp/rector/issues/3490#issue-634342324
featureToggles:
disableRuntimeReflectionProvider: false
nodeConnectingVisitorCompatibility: false
Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Aug 27, 2023

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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

NodeConnectingVisitor enabled again after bleedingedge removed.

3 participants