diff --git a/src/Parser/CleaningVisitor.php b/src/Parser/CleaningVisitor.php index 80f5b2f594..7e151fe30d 100644 --- a/src/Parser/CleaningVisitor.php +++ b/src/Parser/CleaningVisitor.php @@ -3,44 +3,84 @@ namespace PHPStan\Parser; use PhpParser\Node; -use PhpParser\NodeFinder; +use PhpParser\NodeTraverser; use PhpParser\NodeVisitorAbstract; use PHPStan\Reflection\ParametersAcceptor; +use PHPStan\ShouldNotHappenException; +use function array_filter; +use function array_map; use function in_array; use function is_array; final class CleaningVisitor extends NodeVisitorAbstract { - private NodeFinder $nodeFinder; + private const CONTEXT_DEFAULT = 0; - public function __construct() + private const CONTEXT_FUNCTION_OR_METHOD = 1; + + private const CONTEXT_PROPERTY_HOOK = 2; + + /** @var self::CONTEXT_* */ + private int $context = self::CONTEXT_DEFAULT; + + private string|null $propertyName = null; + + /** + * @return int|Node[]|null + */ + public function enterNode(Node $node): int|array|null { - $this->nodeFinder = new NodeFinder(); + switch ($this->context) { + case self::CONTEXT_DEFAULT: + return $this->clean($node); + case self::CONTEXT_FUNCTION_OR_METHOD: + return $this->cleanFunctionOrMethod($node); + case self::CONTEXT_PROPERTY_HOOK: + return $this->cleanPropertyHook($node); + } } - public function enterNode(Node $node): ?Node + private function clean(Node $node): int|null { - if ($node instanceof Node\Stmt\Function_) { - $node->stmts = $this->keepVariadicsAndYields($node->stmts, null); - return $node; - } + if (($node instanceof Node\Stmt\Function_ || $node instanceof Node\Stmt\ClassMethod) && $node->stmts !== null) { + $params = []; + foreach ($this->traverse($node->params, self::CONTEXT_DEFAULT) as $param) { + if (!($param instanceof Node\Param)) { + continue; + } - if ($node instanceof Node\Stmt\ClassMethod && $node->stmts !== null) { - $node->stmts = $this->keepVariadicsAndYields($node->stmts, null); - return $node; - } + $params[] = $param; + } + $node->params = $params; - if ($node instanceof Node\Expr\Closure) { - $node->stmts = $this->keepVariadicsAndYields($node->stmts, null); - return $node; + $stmts = []; + foreach ($this->traverse($node->stmts, self::CONTEXT_FUNCTION_OR_METHOD) as $stmt) { + if (!($stmt instanceof Node\Stmt)) { + continue; + } + + $stmts[] = $stmt; + } + $node->stmts = $stmts; + + return self::DONT_TRAVERSE_CHILDREN; } if ($node instanceof Node\PropertyHook && is_array($node->body)) { $propertyName = $node->getAttribute('propertyName'); if ($propertyName !== null) { - $node->body = $this->keepVariadicsAndYields($node->body, $propertyName); - return $node; + $body = []; + foreach ($this->traverse($node->body, self::CONTEXT_PROPERTY_HOOK, $propertyName) as $stmt) { + if (!($stmt instanceof Node\Stmt)) { + continue; + } + + $body[] = $stmt; + } + $node->body = $body; + + return self::DONT_TRAVERSE_CHILDREN; } } @@ -48,57 +88,87 @@ public function enterNode(Node $node): ?Node } /** - * @param Node\Stmt[] $stmts - * @return Node\Stmt[] + * @return int|Node[] */ - private function keepVariadicsAndYields(array $stmts, ?string $hookedPropertyName): array + private function cleanFunctionOrMethod(Node $node): int|array { - $results = $this->nodeFinder->find($stmts, static function (Node $node) use ($hookedPropertyName): bool { - if ($node instanceof Node\Expr\YieldFrom || $node instanceof Node\Expr\Yield_) { - return true; - } - if ($node instanceof Node\Expr\FuncCall && $node->name instanceof Node\Name) { - return in_array($node->name->toLowerString(), ParametersAcceptor::VARIADIC_FUNCTIONS, true); - } + if ($node instanceof Node\Expr\YieldFrom || $node instanceof Node\Expr\Yield_) { + return self::DONT_TRAVERSE_CHILDREN; + } - if ($node instanceof Node\Expr\Closure || $node instanceof Node\Expr\ArrowFunction) { - return true; - } + if ($node instanceof Node\Expr\FuncCall && $node->name instanceof Node\Name + && in_array($node->name->toLowerString(), ParametersAcceptor::VARIADIC_FUNCTIONS, true) + ) { + $node->name = new Node\Name\FullyQualified('func_get_args'); + return self::DONT_TRAVERSE_CHILDREN; + } - if ($hookedPropertyName !== null) { - if ( - $node instanceof Node\Expr\PropertyFetch - && $node->var instanceof Node\Expr\Variable - && $node->var->name === 'this' - && $node->name instanceof Node\Identifier - && $node->name->toString() === $hookedPropertyName - ) { - return true; - } - } + if ($node instanceof Node\Expr\Closure || $node instanceof Node\Expr\ArrowFunction) { + return self::REMOVE_NODE; + } - return false; - }); - $newStmts = []; - foreach ($results as $result) { - if ( - $result instanceof Node\Expr\Yield_ - || $result instanceof Node\Expr\YieldFrom - || $result instanceof Node\Expr\Closure - || $result instanceof Node\Expr\ArrowFunction - || $result instanceof Node\Expr\PropertyFetch - ) { - $newStmts[] = new Node\Stmt\Expression($result); - continue; - } - if (!$result instanceof Node\Expr\FuncCall) { - continue; - } + return $this->cleanSubnodes($node); + } + + /** + * @param Node[] $nodes + * @param self::CONTEXT_* $context + * @return Node[] + */ + private function traverse( + array $nodes, + int $context = self::CONTEXT_DEFAULT, + string|null $propertyName = null, + ): array + { + $visitor = new self(); + $visitor->context = $context; + $visitor->propertyName = $propertyName; + + return (new NodeTraverser($visitor))->traverse($nodes); + } - $newStmts[] = new Node\Stmt\Expression(new Node\Expr\FuncCall(new Node\Name\FullyQualified('func_get_args'))); + /** + * @return int|Node[] + */ + private function cleanPropertyHook(Node $node): int|array + { + if ( + $node instanceof Node\Expr\PropertyFetch + && $node->var instanceof Node\Expr\Variable + && $node->var->name === 'this' + && $node->name instanceof Node\Identifier + && $node->name->toString() === $this->propertyName + ) { + return self::DONT_TRAVERSE_CHILDREN; } - return $newStmts; + return $this->cleanSubnodes($node); + } + + /** + * @return Node[] + */ + private function cleanSubnodes(Node $node): array + { + $subnodes = []; + foreach ($node->getSubNodeNames() as $subnodeName) { + $subnodes = [...$subnodes, ...array_filter( + is_array($node->$subnodeName) ? $node->$subnodeName : [$node->$subnodeName], + static fn ($subnode) => $subnode instanceof Node, + )]; + } + + return array_map(static function ($node) { + switch (true) { + case $node instanceof Node\Stmt: + return $node; + case $node instanceof Node\Expr: + return new Node\Stmt\Expression($node); + default: + throw new ShouldNotHappenException(); + } + }, $this->traverse($subnodes, $this->context, $this->propertyName)); } } diff --git a/src/Reflection/Php/PhpFunctionFromParserNodeReflection.php b/src/Reflection/Php/PhpFunctionFromParserNodeReflection.php index 02a5b642af..c4a940d73c 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\Node\AnonymousClassNode; use PHPStan\Reflection\Assertions; use PHPStan\Reflection\AttributeReflection; use PHPStan\Reflection\ExtendedFunctionVariant; @@ -287,7 +288,11 @@ private function nodeIsOrContainsYield(Node $node): bool foreach ($node->getSubNodeNames() as $nodeName) { $nodeProperty = $node->$nodeName; - if ($nodeProperty instanceof Node && $this->nodeIsOrContainsYield($nodeProperty)) { + if ($nodeProperty instanceof Node && + !$nodeProperty instanceof FunctionLike && + !$nodeProperty instanceof AnonymousClassNode && + $this->nodeIsOrContainsYield($nodeProperty) + ) { return true; } diff --git a/tests/PHPStan/Parser/data/cleaning-1-after.php b/tests/PHPStan/Parser/data/cleaning-1-after.php index 1d22a6ac92..2b2bea4065 100644 --- a/tests/PHPStan/Parser/data/cleaning-1-after.php +++ b/tests/PHPStan/Parser/data/cleaning-1-after.php @@ -21,6 +21,10 @@ public function someGenerator2() { yield from [1, 2, 3]; } + public function someGenerator3() + { + yield; + } public function someVariadics() { \func_get_args(); @@ -43,9 +47,8 @@ class ContainsClosure { public function doFoo() { - static function () { - yield; - }; - yield; + } + public function doBar() + { } } diff --git a/tests/PHPStan/Parser/data/cleaning-1-before.php b/tests/PHPStan/Parser/data/cleaning-1-before.php index ae93a81b77..ef5c2abca1 100644 --- a/tests/PHPStan/Parser/data/cleaning-1-before.php +++ b/tests/PHPStan/Parser/data/cleaning-1-before.php @@ -36,6 +36,11 @@ public function someGenerator2() } } + public function someGenerator3() + { + echo yield; + } + public function someVariadics() { if (rand(0, 1)) { @@ -82,4 +87,9 @@ public function doFoo() }; } + public function doBar() + { + $fn = fn() => yield; + } + } diff --git a/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php b/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php index 2649996d3f..fd9ca158c1 100644 --- a/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php +++ b/tests/PHPStan/Rules/Functions/ReturnTypeRuleTest.php @@ -341,4 +341,24 @@ public function testBug11301(): void ]); } + public function testBug12462(): void + { + $this->checkExplicitMixed = true; + $this->checkNullables = true; + $this->analyse([__DIR__ . '/data/bug-12462.php'], [ + [ + 'Function Bug12462\functionReturningYieldingClosure() should return int but returns Closure.', + 7, + ], + [ + 'Function Bug12462\functionReturningYieldingArrowFunction() should return int but returns Closure.', + 12, + ], + [ + 'Function Bug12462\functionRetuningYieldingAnonymousClass() should return int but returns class@anonymous/tests/PHPStan/Rules/Functions/data/bug-12462.php:17.', + 17, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Functions/data/bug-12462.php b/tests/PHPStan/Rules/Functions/data/bug-12462.php new file mode 100644 index 0000000000..ca60dff02f --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/bug-12462.php @@ -0,0 +1,22 @@ + yield ''; +} + +function functionRetuningYieldingAnonymousClass (): int +{ + return new class () { + public function f(): \Generator { + yield ''; + } + }; +} \ No newline at end of file diff --git a/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php b/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php index 0c658ce936..7eff63627e 100644 --- a/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php +++ b/tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php @@ -1232,4 +1232,22 @@ public function testBug1O580(): void ]); } + public function testBug12462(): void + { + $this->analyse([__DIR__ . '/data/bug-12462.php'], [ + [ + 'Method Bug12462\A::methodReturningYieldingClosure() should return int but returns Closure.', + 8, + ], + [ + 'Method Bug12462\A::methodReturningYieldingArrowFunction() should return int but returns Closure.', + 13, + ], + [ + 'Method Bug12462\A::methodRetuningYieldingAnonymousClass() should return int but returns class@anonymous/tests/PHPStan/Rules/Methods/data/bug-12462.php:18.', + 18, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Methods/data/bug-12462.php b/tests/PHPStan/Rules/Methods/data/bug-12462.php new file mode 100644 index 0000000000..5dedf0836d --- /dev/null +++ b/tests/PHPStan/Rules/Methods/data/bug-12462.php @@ -0,0 +1,24 @@ + yield ''; + } + + function methodRetuningYieldingAnonymousClass (): int + { + return new class () { + public function f(): \Generator { + yield ''; + } + }; + } +} \ No newline at end of file