From cc76394bf59965cf4253789b9c70c539db4fb898 Mon Sep 17 00:00:00 2001 From: staabm <120441+staabm@users.noreply.github.com> Date: Sun, 21 Jun 2026 16:05:57 +0000 Subject: [PATCH] Detect generators by scanning the body for `yield`, including unreachable code and excluding nested functions - Add `YieldFindingVisitor` that collects every `yield`/`yield from` in a function-like body, regardless of reachability, while not descending into nested function-likes (a `yield` in a nested closure makes that closure a generator, not the outer scope). - `NodeScopeResolver`: gather the `*ReturnStatementsNode` yield statements with the visitor instead of accumulating only the reachable yields visited by the node callback, so `isGenerator()` is correct for functions, methods and closures even when the only `yield` is unreachable. - `NodeScopeResolver`: compute a syntactic `bodyHasYield` flag for the `ExecutionEndNode` so `MissingReturnRule` no longer reports a missing return for generators whose `yield` sits behind a terminating statement (e.g. `while (false) { return; yield; }` or `throw; yield;`). The flow-based `hasYield` propagated in `StatementResult` is left untouched so loop-condition rules keep using reachable yields only. - `PhpFunctionFromParserNodeReflection::isGenerator()`: use the visitor instead of a recursive walk that descended into nested function-likes. Previously a function/method containing a nested generator closure was wrongly treated as a generator, which suppressed its return-type check. --- src/Analyser/NodeScopeResolver.php | 31 +++++------ src/Parser/YieldFindingVisitor.php | 52 +++++++++++++++++++ .../PhpFunctionFromParserNodeReflection.php | 40 +++----------- .../Rules/Functions/ReturnTypeRuleTest.php | 12 +++++ .../PHPStan/Rules/Functions/data/bug-4100.php | 13 +++++ .../Rules/Missing/MissingReturnRuleTest.php | 6 +++ tests/PHPStan/Rules/Missing/data/bug-4100.php | 45 ++++++++++++++++ 7 files changed, 149 insertions(+), 50 deletions(-) create mode 100644 src/Parser/YieldFindingVisitor.php create mode 100644 tests/PHPStan/Rules/Functions/data/bug-4100.php create mode 100644 tests/PHPStan/Rules/Missing/data/bug-4100.php diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 3f63434d03f..1aacc6d3987 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -115,6 +115,7 @@ use PHPStan\Parser\ImmediatelyInvokedClosureVisitor; use PHPStan\Parser\LineAttributesVisitor; use PHPStan\Parser\Parser; +use PHPStan\Parser\YieldFindingVisitor; use PHPStan\PhpDoc\PhpDocInheritanceResolver; use PHPStan\PhpDoc\ResolvedPhpDocBlock; use PHPStan\PhpDoc\Tag\VarTag; @@ -551,6 +552,11 @@ private function processStmtNodesInternalWithoutFlushingPendingFibers( || $parentNode instanceof PropertyHookStatementNode || $parentNode instanceof Expr\Closure; + // A `yield` anywhere in the body makes the function a generator, even when the + // `yield` is unreachable. $hasYield only tracks reachable yields, so it cannot be + // used to decide whether a missing return is acceptable for a generator. + $bodyHasYield = $shouldCheckLastStatement && YieldFindingVisitor::findInNodes($stmts) !== []; + foreach ($stmts as $i => $stmt) { if ($alreadyTerminated && !($stmt instanceof Node\Stmt\Function_ || $stmt instanceof Node\Stmt\ClassLike || $stmt instanceof Node\Stmt\Label)) { continue; @@ -617,7 +623,7 @@ private function processStmtNodesInternalWithoutFlushingPendingFibers( $endStatement->getStatement(), (new InternalStatementResult( $endStatementResult->getScope(), - $hasYield, + $hasYield || $bodyHasYield, $endStatementResult->isAlwaysTerminating(), $endStatementResult->getExitPoints(), $endStatementResult->getThrowPoints(), @@ -631,7 +637,7 @@ private function processStmtNodesInternalWithoutFlushingPendingFibers( $stmt, (new InternalStatementResult( $scope, - $hasYield, + $hasYield || $bodyHasYield, $statementResult->isAlwaysTerminating(), $statementResult->getExitPoints(), $statementResult->getThrowPoints(), @@ -805,10 +811,10 @@ public function processStmtNode( $this->callNodeCallback($nodeCallback, new InFunctionNode($functionReflection, $stmt), $functionScope, $storage); $gatheredReturnStatements = []; - $gatheredYieldStatements = []; + $gatheredYieldStatements = YieldFindingVisitor::findInNodes($stmt->stmts); $executionEnds = []; $functionImpurePoints = []; - $statementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $functionScope, $storage, static function (Node $node, Scope $scope) use ($nodeCallback, $functionScope, &$gatheredReturnStatements, &$gatheredYieldStatements, &$executionEnds, &$functionImpurePoints): void { + $statementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $functionScope, $storage, static function (Node $node, Scope $scope) use ($nodeCallback, $functionScope, &$gatheredReturnStatements, &$executionEnds, &$functionImpurePoints): void { $nodeCallback($node, $scope); if ($scope->getFunction() !== $functionScope->getFunction()) { return; @@ -830,9 +836,6 @@ public function processStmtNode( $executionEnds[] = $node; return; } - if ($node instanceof Expr\Yield_ || $node instanceof Expr\YieldFrom) { - $gatheredYieldStatements[] = $node; - } if (!$node instanceof Return_) { return; } @@ -954,10 +957,10 @@ public function processStmtNode( if ($stmt->stmts !== null) { $gatheredReturnStatements = []; - $gatheredYieldStatements = []; + $gatheredYieldStatements = YieldFindingVisitor::findInNodes($stmt->stmts); $executionEnds = []; $methodImpurePoints = []; - $statementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $methodScope, $storage, static function (Node $node, Scope $scope) use ($nodeCallback, $methodScope, &$gatheredReturnStatements, &$gatheredYieldStatements, &$executionEnds, &$methodImpurePoints): void { + $statementResult = $this->processStmtNodesInternal($stmt, $stmt->stmts, $methodScope, $storage, static function (Node $node, Scope $scope) use ($nodeCallback, $methodScope, &$gatheredReturnStatements, &$executionEnds, &$methodImpurePoints): void { $nodeCallback($node, $scope); if ($scope->getFunction() !== $methodScope->getFunction()) { return; @@ -988,9 +991,6 @@ public function processStmtNode( $executionEnds[] = $node; return; } - if ($node instanceof Expr\Yield_ || $node instanceof Expr\YieldFrom) { - $gatheredYieldStatements[] = $node; - } if (!$node instanceof Return_) { return; } @@ -2993,10 +2993,10 @@ public function processClosureNode( $executionEnds = []; $gatheredReturnStatements = []; - $gatheredYieldStatements = []; + $gatheredYieldStatements = YieldFindingVisitor::findInNodes($expr->stmts); $closureImpurePoints = []; $invalidateExpressions = []; - $closureStmtsCallback = static function (Node $node, Scope $scope) use ($nodeCallback, &$executionEnds, &$gatheredReturnStatements, &$gatheredYieldStatements, &$closureScope, &$closureImpurePoints, &$invalidateExpressions): void { + $closureStmtsCallback = static function (Node $node, Scope $scope) use ($nodeCallback, &$executionEnds, &$gatheredReturnStatements, &$closureScope, &$closureImpurePoints, &$invalidateExpressions): void { $nodeCallback($node, $scope); if ($scope->getAnonymousFunctionReflection() !== $closureScope->getAnonymousFunctionReflection()) { return; @@ -3020,9 +3020,6 @@ public function processClosureNode( $invalidateExpressions[] = $node; return; } - if ($node instanceof Expr\Yield_ || $node instanceof Expr\YieldFrom) { - $gatheredYieldStatements[] = $node; - } if (!$node instanceof Return_) { return; } diff --git a/src/Parser/YieldFindingVisitor.php b/src/Parser/YieldFindingVisitor.php new file mode 100644 index 00000000000..d221248cd9a --- /dev/null +++ b/src/Parser/YieldFindingVisitor.php @@ -0,0 +1,52 @@ + */ + private array $yieldNodes = []; + + #[Override] + public function enterNode(Node $node): ?int + { + if ($node instanceof Node\FunctionLike) { + return NodeVisitor::DONT_TRAVERSE_CHILDREN; + } + + if ($node instanceof Yield_ || $node instanceof YieldFrom) { + $this->yieldNodes[] = $node; + } + + return null; + } + + /** + * @param Node[] $nodes + * @return list + */ + public static function findInNodes(array $nodes): array + { + $visitor = new self(); + $traverser = new NodeTraverser($visitor); + $traverser->traverse($nodes); + + return $visitor->yieldNodes; + } + +} diff --git a/src/Reflection/Php/PhpFunctionFromParserNodeReflection.php b/src/Reflection/Php/PhpFunctionFromParserNodeReflection.php index ba4e66fa1b9..ed7a96db9ce 100644 --- a/src/Reflection/Php/PhpFunctionFromParserNodeReflection.php +++ b/src/Reflection/Php/PhpFunctionFromParserNodeReflection.php @@ -7,6 +7,7 @@ use PhpParser\Node\FunctionLike; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Function_; +use PHPStan\Parser\YieldFindingVisitor; use PHPStan\Reflection\Assertions; use PHPStan\Reflection\AttributeReflection; use PHPStan\Reflection\ExtendedFunctionVariant; @@ -22,7 +23,6 @@ use PHPStan\Type\Type; use PHPStan\Type\TypehintHelper; use function array_reverse; -use function is_array; use function is_string; use function strtolower; @@ -271,7 +271,12 @@ public function isBuiltin(): bool public function isGenerator(): bool { - return $this->nodeIsOrContainsYield($this->functionLike); + $stmts = $this->functionLike->getStmts(); + if ($stmts === null) { + return false; + } + + return YieldFindingVisitor::findInNodes($stmts) !== []; } public function acceptsNamedArguments(): TrinaryLogic @@ -279,37 +284,6 @@ public function acceptsNamedArguments(): TrinaryLogic return TrinaryLogic::createFromBoolean($this->acceptsNamedArguments); } - private function nodeIsOrContainsYield(Node $node): bool - { - if ($node instanceof Node\Expr\Yield_) { - return true; - } - - if ($node instanceof Node\Expr\YieldFrom) { - return true; - } - - foreach ($node->getSubNodeNames() as $nodeName) { - $nodeProperty = $node->$nodeName; - - if ($nodeProperty instanceof Node && $this->nodeIsOrContainsYield($nodeProperty)) { - return true; - } - - if (!is_array($nodeProperty)) { - continue; - } - - foreach ($nodeProperty as $nodePropertyArrayItem) { - if ($nodePropertyArrayItem instanceof Node && $this->nodeIsOrContainsYield($nodePropertyArrayItem)) { - return true; - } - } - } - - return false; - } - public function getAsserts(): Assertions { return $this->assertions; diff --git a/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php b/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php index da3ba5d6d56..942f4bddc07 100644 --- a/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php +++ b/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php @@ -451,4 +451,16 @@ public function testBug13565(): void ]); } + public function testBug4100(): void + { + $this->checkNullables = true; + $this->checkExplicitMixed = true; + $this->analyse([__DIR__ . '/data/bug-4100.php'], [ + [ + 'Function Bug4100ReturnType\returnsStringWithNestedGenerator() should return int but returns string.', + 12, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Functions/data/bug-4100.php b/tests/PHPStan/Rules/Functions/data/bug-4100.php new file mode 100644 index 00000000000..31f4c2a4a8f --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/bug-4100.php @@ -0,0 +1,13 @@ +analyse([__DIR__ . '/data/bug-5681.php'], []); } + public function testBug4100(): void + { + $this->checkExplicitMixedMissingReturn = true; + $this->analyse([__DIR__ . '/data/bug-4100.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Missing/data/bug-4100.php b/tests/PHPStan/Rules/Missing/data/bug-4100.php new file mode 100644 index 00000000000..778e73d6d82 --- /dev/null +++ b/tests/PHPStan/Rules/Missing/data/bug-4100.php @@ -0,0 +1,45 @@ +