-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Description
the current Node->getSubNodeNames()
api leads to consuming code with dynamic property fetches like:
foreach ($node->getSubNodeNames() as $subNodeName) {
if ($node instanceof Node\Expr\Closure && $subNodeName !== 'uses') {
continue;
}
$subNode = $node->{$subNodeName};
$variableNames = array_merge($variableNames, $this->getUsedVariables($scope, $subNode));
}
In PHPStan-src alone this pattern repeats 8 times.
Its also visibile in the php-parser codebase 2 times:
PHP-Parser/lib/PhpParser/PrettyPrinterAbstract.php
Lines 643 to 651 in 3abf742
$result = ''; | |
$pos = $startPos; | |
foreach ($node->getSubNodeNames() as $subNodeName) { | |
$subNode = $node->$subNodeName; | |
$origSubNode = $origNode->$subNodeName; | |
if ((!$subNode instanceof Node && $subNode !== null) | |
|| (!$origSubNode instanceof Node && $origSubNode !== null) | |
) { |
PHP-Parser/lib/PhpParser/NodeTraverser.php
Lines 93 to 98 in 3abf742
protected function traverseNode(Node $node): void { | |
foreach ($node->getSubNodeNames() as $name) { | |
$subNode = $node->$name; | |
if (\is_array($subNode)) { | |
$node->$name = $this->traverseArray($subNode); |
this dynamic property fetches lead to badly static analysable code
as soon as a class contains a single dynamic property lookup, phpstan is no longer able to tell you whether one of the declared properties is unused (analog for methods).
what do you think about adding a new Node->getSubNodes()
method, so the dynamic property lookup is only available in one place insider php-parser and caller code would be more static analysis friendly?
Activity
staabm commentedon Oct 7, 2024
I have something prepared which can make the code static analysis friendly and does not require a new method. stay tuned.