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

Make ConsecutiveNullCompareReturnsToNullCoalesceQueueRector use of StmtsAwareInterface #3783

Merged
merged 3 commits into from
May 10, 2023

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented May 10, 2023

No description provided.

@TomasVotruba TomasVotruba changed the title tv less next prev Make ConsecutiveNullCompareReturnsToNullCoalesceQueueRector use of StmtsAwareInterface May 10, 2023
@@ -49,8 +49,6 @@
\Rector\NodeTypeResolver\Node\AttributeKey::SCOPE,
\Rector\NodeTypeResolver\Node\AttributeKey::REPRINT_RAW_VALUE,
\Rector\NodeTypeResolver\Node\AttributeKey::PARENT_NODE,
\Rector\NodeTypeResolver\Node\AttributeKey::NEXT_NODE,
\Rector\NodeTypeResolver\Node\AttributeKey::PREVIOUS_NODE,
Copy link
Member Author

Choose a reason for hiding this comment

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

@samsonasik Hi Abdul, I'm trying to remove all the next/previous node and replace with direct StmtsAwareInterface. That will help with improving memory load of the node tree as in PHPStan.

Could you help out? Take any rule and let's resolve them one by one :)

Copy link
Member

Choose a reason for hiding this comment

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

Using StmtsAwareInterface doesn't make memory lower, if we still need re-connect after refactor, see:

$this->nodeConnectingTraverser->traverse($nodes);

If we need to test it out, we can remove the above call, and run tests:

vendor/bin/phpunit tests/Issues

and many errors will happen.

There is a condition which next/prev is needed, eg: on next of Expr, eg:

$node = $concat->getAttribute(AttributeKey::NEXT_NODE);

Reduce is possible, but removing is possibly huge BC break on end user.

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment yes, we need traversing. The goal is to remove dependency on any connected nodes whatsoever.

It might take time, but it's worth it as we will release lot of memory connections.

@TomasVotruba TomasVotruba enabled auto-merge (squash) May 10, 2023 05:56
@TomasVotruba TomasVotruba merged commit 76da948 into main May 10, 2023
@TomasVotruba TomasVotruba deleted the tv-less-next-prev branch May 10, 2023 06:03
TomasVotruba added a commit that referenced this pull request May 10, 2023
TomasVotruba added a commit that referenced this pull request May 10, 2023
TomasVotruba added a commit that referenced this pull request May 10, 2023
…y complex and should be handled individually (#3784)

* fixup! Make ConsecutiveNullCompareReturnsToNullCoalesceQueueRector use of StmtsAwareInterface (#3783)

* fixup! fixup! Make ConsecutiveNullCompareReturnsToNullCoalesceQueueRector use of StmtsAwareInterface (#3783)

* misc
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.

2 participants