[NodeVisitor] Drop ByRefReturnNodeVisitor, check early in only used once SimplifyUselessVariableRector

[samsonasik]

This PR apply:

Drop ByRefReturnNodeVisitor that set is_byref_return attribute on return when it is inside FunctionLike, this can be handled by return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN on refactor() method when node is instanceof FunctionLike and it by ref return via:

if ($node instanceof FunctionLike && $node->returnsByRef()) {
    return NodeVisitor::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
}

This require change on AbstractRector::decorateCurrentAndChildren() method, as getNodeTypes() only define single StmtsAwareInterface:

rector-src/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php

Line 109 in fa54d46

return [StmtsAwareInterface::class];

which mean no other types, so it require sub type check of it to skip on current rule passed.

If there are other types defined after it and subnode is equal then other type defined in getNodeTypes() method, set it to skip, otherwise, verify if getNodeTypes() is [StmtsAwareInterface::class] and current node is FunctionLike, then to not apply as requires realidated and no need to apply skip rule flag, then otherwise, apply any child of node of it to skip on current rule only via:

$subNode->setAttribute(AttributeKey::SKIPPED_BY_RECTOR_RULE, static::class);

[samsonasik]

composer dependency analyser notice is unrelated:

https://github.com/rectorphp/rector-src/actions/runs/19675350225/job/56354827988?pr=7667#step:5:16

seems due to latest release of shipmonk-rnd/composer-dependency-analyser version 1.8.4, I will temporary pin to 1.8.3 for now.

I created issue for it at:

• version 1.8.4 cause Found 1 unknown class error on single class definition and use shipmonk-rnd/composer-dependency-analyser#247

[samsonasik]

I temporary pin shipmonk-rnd/composer-dependency-analyser to 1.8.3 89ff96e

[samsonasik]

All checks have passed 🎉 @TomasVotruba it is ready for review.

[samsonasik]

I add outher ref fixture test for it that inner should keep applied 73448a4

[samsonasik]

All checks have passed 🎉 @TomasVotruba I think it is ready.

[samsonasik]

Rebased. All checks have passed 🎉 @TomasVotruba I think it is ready.

[TomasVotruba]

I like removal of the node traverser above, as we can use the rule directly.
But I'm bit confused by this part. AbstractRector should get slimmer, not bigger.

How can we remove the node traverser and keep this class as small as possible at the same time?

[samsonasik]

That removal of visitor, not traverser.

It cost of return NodeVisitor::DONT_TRAVERSE_CHILDREN, the original functionality is "dont traverse ANY visitor" to visit below node.

On single visitor, eg: callable traverser, it is ok, on rector rules, the tweak is needed as it needs to only skip below node on "current rule", other rules are allowed to visit and apply change, that's why the flag is needed.

I can move the functionality to separate service tho, but flagging node to skip below node still needed, since other still allowed to visit and apply change.

[samsonasik]

Closing for now.

[github-actions]

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.