Skip to content

Commit

Permalink
Refactor BetterNodeFinder::findFirstInFunctionLikeScoped() to work wi…
Browse files Browse the repository at this point in the history
…th SilentVoidResolver (#4931)

* Refactor findFirstInFunctionLikeScoped

* Fix
  • Loading branch information
samsonasik committed Sep 7, 2023
1 parent 54799fc commit fa28385
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 10 deletions.
13 changes: 5 additions & 8 deletions rules/TypeDeclaration/TypeInferer/SilentVoidResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,11 @@ public function hasExclusiveVoid(ClassMethod | Closure | Function_ $functionLike
return false;
}

$returns = $this->betterNodeFinder->findInstancesOfInFunctionLikeScoped($functionLike, Return_::class);
foreach ($returns as $return) {
if ($return->expr instanceof Expr) {
return false;
}
}

return true;
$return = $this->betterNodeFinder->findFirstInFunctionLikeScoped(
$functionLike,
static fn (Node $node): bool => $node instanceof Return_ && $node->expr instanceof Expr
);
return ! $return instanceof Return_;
}

public function hasSilentVoid(FunctionLike $functionLike): bool
Expand Down
11 changes: 9 additions & 2 deletions src/PhpParser/Node/BetterNodeFinder.php
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public function findFirstInFunctionLikeScoped(
$scopedNode = null;
$this->simpleCallableNodeTraverser->traverseNodesWithCallable(
$functionLike->stmts,
static function (Node $subNode) use (&$scopedNode, $foundNode): ?int {
function (Node $subNode) use (&$scopedNode, $foundNode, $filter): ?int {
if ($subNode instanceof Class_ || $subNode instanceof Function_ || $subNode instanceof Closure) {
if ($foundNode instanceof $subNode && $subNode === $foundNode) {
$scopedNode = $subNode;
Expand All @@ -256,7 +256,14 @@ static function (Node $subNode) use (&$scopedNode, $foundNode): ?int {
return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
}

if ($foundNode instanceof $subNode && $subNode === $foundNode) {
if (! $foundNode instanceof $subNode) {
return null;
}

// handle after Closure
// @see https://github.com/rectorphp/rector-src/pull/4931
$scopedFoundNode = $this->findFirst($subNode, $filter);

This comment has been minimized.

Copy link
@staabm

staabm Sep 7, 2023

Contributor

I am not sure this is a good idea. doesn't it mean on every findFirstInFunctionLikeScoped invocatin, we will traverse the AST twice now?

I think we should instead not do the "optimization" in SilentVoidResolver when it requires an additional AST traversal for all operations in the system and revert SilentVoidResolver to what we had before then

This comment has been minimized.

Copy link
@samsonasik

samsonasik Sep 7, 2023

Author Member

If it not found, it doesn't.

Most of the time, we use directly SimpleCallableNodeTraverser instead of BetterNodeFinder, this is bug fix for dynamic filter pass for syntax sugar findFirstInFunctionLikeScoped() instead of manual check inner class,function,closure.

if ($scopedFoundNode === $subNode) {
$scopedNode = $subNode;
return NodeTraverser::STOP_TRAVERSAL;
}
Expand Down

0 comments on commit fa28385

Please sign in to comment.