[DeadCode] Avoid scan whole new stmts on RemoveAlwaysTrueIfConditionRector on dynamic variable check#8057
[DeadCode] Avoid scan whole new stmts on RemoveAlwaysTrueIfConditionRector on dynamic variable check#8057samsonasik wants to merge 6 commits into
Conversation
samsonasik
force-pushed
the
clean-up-scan-whole-new-stmts
branch
from
d66415f to
663cb42
Compare
|
Fixed 🎉 /cc @TomasVotruba Handled with check from |
samsonasik
changed the title
|
@TomasVotruba ready 👍 |
| continue; | ||
| } | ||
|
|
||
| $variableType = $scope->getVariableType($definedVariable); |
There was a problem hiding this comment.
This call would be more expensive than simple $node->name instanceof Expr
We need a better solution without type resolution.
There was a problem hiding this comment.
this only check defined variables inside the current scope, eg: method, so should be cheap process as scoped.
There was a problem hiding this comment.
I've cleaned up check hasVariableType() in loop as always defined on getDefinedVariables() usage d32d28d
There was a problem hiding this comment.
I've moved check on defined variables on last resort after other check d40ba5d
should be cheaper now as last resort, and this is already only lookup variables on this scope 👍
| private function hasDynamicVariable(): bool | ||
| { | ||
| return (bool) $this->betterNodeFinder->findFirst( | ||
| $this->getFile() | ||
| ->getNewStmts(), | ||
| static fn (Node $node): bool => $node instanceof Variable && $node->name instanceof Expr | ||
| ); |
There was a problem hiding this comment.
This structure is indeed weird, should be either cached per file or replaced by check inside aprent node, e.g. ClassMethod, Function_ or FileNode
There was a problem hiding this comment.
that's seems will make too much clutter imo, as the flag need to be reset on enter another scope.
2 participants
check directly on
$scope->getDefinedVariables()seems enough.Ref #8055 (review)