[DeadCode] Avoid scan whole new stmts on RemoveAlwaysTrueIfConditionRector on dynamic variable check#8057
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 👍
There was a problem hiding this comment.
@TomasVotruba I've added fixture to proof that the getDefinedVariables() only lookup on scoped method ec3414d
Different method will use different scope so it proceed 👍
| 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.
|
@TomasVotruba I've added fixture to proof that the Different method will use different scope so it proceed 👍 Should be ready to merge 👍 |
samsonasik
force-pushed
the
clean-up-scan-whole-new-stmts
branch
from
ec3414d to
ff19731
Compare
|
Looks good now, thank you 👍 |
|
@TomasVotruba can we have new immediate release for it? Thank you. |
|
@samsonasik Just made one, so not yet. I want to add few fixes first |
2 participants
check directly on
$scope->getDefinedVariables()seems enough.Ref #8055 (review)