Skip to content

[Renaming] Remove Scope filling from Name node on PHPStanNodeScopeResolver for RenameClassRector#4422

Merged
samsonasik merged 6 commits intomainfrom
remove-scope-fill
Jul 5, 2023
Merged

[Renaming] Remove Scope filling from Name node on PHPStanNodeScopeResolver for RenameClassRector#4422
samsonasik merged 6 commits intomainfrom
remove-scope-fill

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@staabm @TomasVotruba per #4400 (comment)

Here I remove Scope filling for Name node on RenameClassRector on PHPStanNodeScopeResolver

Comment on lines +52 to +54
if ($node->getAttribute(AttributeKey::EXPRESSION_DEPTH) >= 2) {
return $this->scopeFactory->createFromFile($filePath);
}
Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Jul 5, 2023

Choose a reason for hiding this comment

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

@TomasVotruba this is to avoid too deep scope filing assign -> assign -> expr -> method call -> property fetch , eg:

$$param = $$bar = self::decodeValue($result->getItem()->getTextContent());

that can goes out of control,.

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba @staabm I removed many scope filling 🎉

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba I am merging it to have faster feedback to test ;)

@samsonasik samsonasik merged commit 3fa4bec into main Jul 5, 2023
@samsonasik samsonasik deleted the remove-scope-fill branch July 5, 2023 12:14
{
$oldToNewClasses = $this->renamedClassesDataCollector->getOldToNewClasses();
if ($oldToNewClasses !== []) {
$scope = $node->getAttribute(AttributeKey::SCOPE);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't the scope passed in from rector core be the same as the one in the attribute?

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.

I still find why Scope for RenameClassRector after refactor require ORIGINAL_NODE instead of Node itself at

/** @var MutatingScope|null $currentScope */
$currentScope = $originalNode->getAttribute(AttributeKey::SCOPE);

that's possibly due to docblock is too late to get Scope, that's another PR to apply if possible ;)

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