Skip to content

[NodeAnalyzer] Remove parent attribute on ScopeAnalyzer#4400

Merged
samsonasik merged 10 commits intomainfrom
remove-parent-scope
Jul 1, 2023
Merged

[NodeAnalyzer] Remove parent attribute on ScopeAnalyzer#4400
samsonasik merged 10 commits intomainfrom
remove-parent-scope

Conversation

@samsonasik
Copy link
Copy Markdown
Member

No description provided.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner July 1, 2023 20:52
@samsonasik samsonasik marked this pull request as draft July 1, 2023 21:42
@samsonasik samsonasik marked this pull request as ready for review July 1, 2023 22:53
@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it to have faster feedback to test ;)

@samsonasik samsonasik merged commit 222569c into main Jul 1, 2023
@samsonasik samsonasik deleted the remove-parent-scope branch July 1, 2023 22:58
@samsonasik
Copy link
Copy Markdown
Member Author

Tested on CodeIgniter4 project, now got error:

➜  CodeIgniter4 git:(develop) ✗ time vendor/bin/rector 
 603/790 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓░░░░░░░]  76%
                                                                                
 [ERROR] Could not process                                                      
         "/home/abdul/www/CodeIgniter4/system/CLI/Commands.php" file, due to:   
         "System error: "Scope not available on                                 
         "PhpParser\Node\Name\FullyQualified" node with parent node of          
         "PhpParser\Node\Stmt\Catch_", but is required by a refactorWithScope() 
         method of "Rector\Renaming\Rector\Name\RenameClassRector" rule. Fix    
         scope refresh on changed nodes first"                                  
         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On line:
         53                                                                     

I will check check it.

@samsonasik
Copy link
Copy Markdown
Member Author

#4401

@staabm
Copy link
Copy Markdown
Contributor

staabm commented Jul 3, 2023

My unverified guess:

I think Scope instances are way heavier in memory than Ast-Nodes. This would mean that this patch might even worse memory use, as we are trading references to Ast Nodes with references to Scope objects.

After getting rid of the parent-AST-attribute we need to check these Scope attribute, as it is used so often. Maybe we can make use of WeakReference

@samsonasik
Copy link
Copy Markdown
Member Author

@staabm it maybe due to stack nodes on ParentConnectingVisitor, I create an RFC for that, see rectorphp/rector#8035

@samsonasik
Copy link
Copy Markdown
Member Author

@staabm I will check if this can be reduced by using $mutatingScope->enterXYZ() method to fill inside instead of manually fill it, as it may goes out of control.

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba @staabm I am checking PHPStan source, and it seems NodeScopeResolver->processNodes() is called internally at FileAnalyser::analyseFile()

https://github.com/phpstan/phpstan-src/blob/762c8148ac9a6a66d3a513cbc52e5be1c2d75331/src/Analyser/FileAnalyser.php#L176

I am wondering if we can just use that method instead of manually fill the Scope in every use finding cases, I will lookup if that possible.

@staabm
Copy link
Copy Markdown
Contributor

staabm commented Jul 3, 2023

I don't know enough about the inner workings of rector to answer that question.

@samsonasik
Copy link
Copy Markdown
Member Author

@staabm if we can get rid of "scope" attribute, and get the scope from its PHPStan node scope context, that probably will be a real solution.

PHPStan has "Virtual" node, that probably a way to resolve Scope from that.

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba @staabm the most scope filling here is for RenameClassRector which tries to get scope from Name node, I will try if there is a way to refactor RenameClassRector to use its ClassLike or TraitUse or Use_ instead of direct Name node, so Scope filling will be unneeded for name, as Name scope itself is "unrefreshable", see

if ($node instanceof Stmt) {
return [$node];
}
if ($node instanceof Expr) {
return [new Expression($node)];
}
$errorMessage = sprintf('Complete parent node of "%s" be a stmt.', $node::class);
throw new ShouldNotHappenException($errorMessage);

@samsonasik
Copy link
Copy Markdown
Member Author

@staabm @TomasVotruba I created PR #4422 for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants