diff --git a/config/rector-rules.neon b/config/rector-rules.neon index e42b0127..a23d5867 100644 --- a/config/rector-rules.neon +++ b/config/rector-rules.neon @@ -11,6 +11,8 @@ rules: - Symplify\PHPStanRules\Rules\Rector\AvoidFeatureSetAttributeInRectorRule - Symplify\PHPStanRules\Rules\Rector\RectorCheaperGuardsFirstRule - Symplify\PHPStanRules\Rules\Rector\PreferGenericTemplateOnSameUnionTypeRule + - Symplify\PHPStanRules\Rules\Rector\NoScopeFetchBeforeCheapGuardRule + - Symplify\PHPStanRules\Rules\Rector\NarrowReturnToUnionParamRule - Symplify\PHPStanRules\Rules\ForbiddenTraitRule - Symplify\PHPStanRules\Rules\ForbiddenSwitchRule diff --git a/src/Rules/Rector/NarrowReturnToUnionParamRule.php b/src/Rules/Rector/NarrowReturnToUnionParamRule.php new file mode 100644 index 00000000..8b2ea456 --- /dev/null +++ b/src/Rules/Rector/NarrowReturnToUnionParamRule.php @@ -0,0 +1,204 @@ + + * @see \Symplify\PHPStanRules\Tests\Rules\Rector\NarrowReturnToUnionParamRule\NarrowReturnToUnionParamRuleTest + */ +final class NarrowReturnToUnionParamRule implements Rule +{ + public const ERROR_MESSAGE = 'Return type "%s" is too broad for a returned union param. Declare the explicit union return type "%s" instead.'; + + public function getNodeType(): string + { + return ClassMethod::class; + } + + /** + * @param ClassMethod $node + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + $returnName = $this->resolveSingleClassReturnName($node->returnType); + if ($returnName === null) { + return []; + } + + foreach ($node->params as $param) { + $paramNames = $this->resolveClassUnionNames($param->type); + if ($paramNames === null) { + continue; + } + + // return type is one of the union members, not a broad shared parent - already narrow enough + if (in_array($returnName, $paramNames, true)) { + continue; + } + + if (! $param->var instanceof Variable || ! is_string($param->var->name)) { + continue; + } + + if (! $this->methodReturnsVariable($node, $param->var->name)) { + continue; + } + + $shortNames = array_map(static function (string $name): string { + $parts = explode('\\', $name); + return end($parts); + }, $paramNames); + + if ($this->isNullableReturnType($node->returnType)) { + $shortNames[] = 'null'; + } + + return [ + RuleErrorBuilder::message(sprintf(self::ERROR_MESSAGE, $returnName, implode('|', $shortNames))) + ->identifier('rector.explicitUnionReturnType') + ->line($node->getStartLine()) + ->build(), + ]; + } + + return []; + } + + private function methodReturnsVariable(ClassMethod $classMethod, string $variableName): bool + { + if ($classMethod->stmts === null) { + return false; + } + + $nodeFinder = new NodeFinder(); + $foundReturn = $nodeFinder->findFirst( + $classMethod->stmts, + static fn (Node $subNode): bool => $subNode instanceof Return_ + && $subNode->expr instanceof Variable + && $subNode->expr->name === $variableName + ); + + return $foundReturn instanceof Return_; + } + + private function isNullableReturnType(?Node $node): bool + { + if ($node instanceof NullableType) { + return true; + } + + if (! $node instanceof UnionType) { + return false; + } + + foreach ($node->types as $memberType) { + if ($memberType instanceof Identifier && $memberType->toLowerString() === 'null') { + return true; + } + } + + return false; + } + + /** + * Returns the single class return type name (nullable allowed), or null when the return type is + * missing, scalar, or a union of 2+ class types - the latter is already an explicit union. + */ + private function resolveSingleClassReturnName(?Node $node): ?string + { + if ($node instanceof NullableType) { + $node = $node->type; + } + + if ($node instanceof Name) { + return $node->toString(); + } + + if (! $node instanceof UnionType) { + return null; + } + + $names = []; + foreach ($node->types as $memberType) { + if ($memberType instanceof Identifier) { + if ($memberType->toLowerString() === 'null') { + continue; + } + + return null; + } + + if (! $memberType instanceof Name) { + return null; + } + + $names[] = $memberType->toString(); + } + + if (count($names) !== 1) { + return null; + } + + return $names[0]; + } + + /** + * Returns sorted class-type member names (null stripped), or null when the type is not a + * union of 2+ class types - scalars, array and intersections disqualify it. + * + * @return list|null + */ + private function resolveClassUnionNames(?Node $node): ?array + { + if (! $node instanceof UnionType) { + return null; + } + + $names = []; + foreach ($node->types as $memberType) { + if ($memberType instanceof Identifier) { + if ($memberType->toLowerString() === 'null') { + continue; + } + + return null; + } + + if (! $memberType instanceof Name) { + return null; + } + + $names[] = $memberType->toString(); + } + + if (count($names) < 2) { + return null; + } + + $names = array_unique($names); + sort($names); + + return $names; + } +} diff --git a/src/Rules/Rector/NoScopeFetchBeforeCheapGuardRule.php b/src/Rules/Rector/NoScopeFetchBeforeCheapGuardRule.php new file mode 100644 index 00000000..4fdafca5 --- /dev/null +++ b/src/Rules/Rector/NoScopeFetchBeforeCheapGuardRule.php @@ -0,0 +1,152 @@ + + * @see \Symplify\PHPStanRules\Tests\Rules\Rector\NoScopeFetchBeforeCheapGuardRule\NoScopeFetchBeforeCheapGuardRuleTest + */ +final class NoScopeFetchBeforeCheapGuardRule implements Rule +{ + public const ERROR_MESSAGE = 'Move the "return null" guard above ScopeFetcher::fetch(), as it does not use the scope - most nodes then bail out before scope resolution.'; + + public function getNodeType(): string + { + return ClassMethod::class; + } + + /** + * @param ClassMethod $node + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + if ($node->name->toString() !== 'refactor') { + return []; + } + + $stmts = $node->stmts; + if ($stmts === null) { + return []; + } + + foreach ($stmts as $key => $stmt) { + $scopeVariableName = $this->matchScopeFetchVariableName($stmt); + if ($scopeVariableName === null) { + continue; + } + + $nextStmt = $stmts[$key + 1] ?? null; + if (! $nextStmt instanceof If_) { + continue; + } + + if (! $this->isReturnNullGuard($nextStmt)) { + continue; + } + + if ($this->usesVariable($nextStmt->cond, $scopeVariableName)) { + continue; + } + + return [ + RuleErrorBuilder::message(self::ERROR_MESSAGE) + ->identifier('rector.scopeFetchBeforeCheapGuard') + ->line($stmt->getStartLine()) + ->build(), + ]; + } + + return []; + } + + private function matchScopeFetchVariableName(Stmt $stmt): ?string + { + if (! $stmt instanceof Expression) { + return null; + } + + if (! $stmt->expr instanceof Assign) { + return null; + } + + $assign = $stmt->expr; + if (! $assign->var instanceof Variable || ! is_string($assign->var->name)) { + return null; + } + + if (! $assign->expr instanceof StaticCall) { + return null; + } + + $staticCall = $assign->expr; + if (! $staticCall->class instanceof Name) { + return null; + } + + if ($staticCall->class->toString() !== ScopeFetcher::class) { + return null; + } + + if (! $staticCall->name instanceof Identifier || $staticCall->name->toString() !== 'fetch') { + return null; + } + + return $assign->var->name; + } + + private function isReturnNullGuard(If_ $if): bool + { + if (count($if->stmts) !== 1) { + return false; + } + + $onlyStmt = $if->stmts[0]; + if (! $onlyStmt instanceof Return_) { + return false; + } + + return ! $onlyStmt->expr instanceof Expr || $this->isNullConstant($onlyStmt->expr); + } + + private function isNullConstant(Expr $expr): bool + { + return $expr instanceof ConstFetch && $expr->name->toLowerString() === 'null'; + } + + private function usesVariable(Expr $expr, string $variableName): bool + { + $nodeFinder = new NodeFinder(); + $foundVariable = $nodeFinder->findFirst( + [$expr], + static fn (Node $subNode): bool => $subNode instanceof Variable && $subNode->name === $variableName + ); + + return $foundVariable instanceof Variable; + } +} diff --git a/tests/Rules/Rector/NarrowReturnToUnionParamRule/Fixture/BroadReturnOfUnionParam.php b/tests/Rules/Rector/NarrowReturnToUnionParamRule/Fixture/BroadReturnOfUnionParam.php new file mode 100644 index 00000000..dbbd82ba --- /dev/null +++ b/tests/Rules/Rector/NarrowReturnToUnionParamRule/Fixture/BroadReturnOfUnionParam.php @@ -0,0 +1,21 @@ +var; + } + + if (! $expr instanceof Variable) { + return null; + } + + return $expr; + } +} diff --git a/tests/Rules/Rector/NarrowReturnToUnionParamRule/NarrowReturnToUnionParamRuleTest.php b/tests/Rules/Rector/NarrowReturnToUnionParamRule/NarrowReturnToUnionParamRuleTest.php new file mode 100644 index 00000000..e08c20d4 --- /dev/null +++ b/tests/Rules/Rector/NarrowReturnToUnionParamRule/NarrowReturnToUnionParamRuleTest.php @@ -0,0 +1,44 @@ +> $expectedErrorsWithLines + */ + #[DataProvider('provideData')] + public function testRule(string $filePath, array $expectedErrorsWithLines): void + { + $this->analyse([$filePath], $expectedErrorsWithLines); + } + + /** + * @return Iterator, mixed>> + */ + public static function provideData(): Iterator + { + yield [__DIR__ . '/Fixture/SkipBroadReturn.php', []]; + + yield [__DIR__ . '/Fixture/BroadReturnOfUnionParam.php', [ + [ + sprintf(NarrowReturnToUnionParamRule::ERROR_MESSAGE, Node::class, 'MethodCall|New_|null'), + 13, + ], + ]]; + } + + protected function getRule(): Rule + { + return new NarrowReturnToUnionParamRule(); + } +} diff --git a/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/Fixture/ScopeFetchBeforeGuard.php b/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/Fixture/ScopeFetchBeforeGuard.php new file mode 100644 index 00000000..49ff7683 --- /dev/null +++ b/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/Fixture/ScopeFetchBeforeGuard.php @@ -0,0 +1,22 @@ +returnType instanceof Node) { + return null; + } + + return $node; + } +} diff --git a/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/Fixture/SkipGuardBeforeScopeFetch.php b/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/Fixture/SkipGuardBeforeScopeFetch.php new file mode 100644 index 00000000..f336674a --- /dev/null +++ b/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/Fixture/SkipGuardBeforeScopeFetch.php @@ -0,0 +1,27 @@ +returnType instanceof Node) { + return null; + } + + $scope = ScopeFetcher::fetch($node); + if (! $scope->isInClass()) { + return null; + } + + return $node; + } +} diff --git a/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/NoScopeFetchBeforeCheapGuardRuleTest.php b/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/NoScopeFetchBeforeCheapGuardRuleTest.php new file mode 100644 index 00000000..c4426b9d --- /dev/null +++ b/tests/Rules/Rector/NoScopeFetchBeforeCheapGuardRule/NoScopeFetchBeforeCheapGuardRuleTest.php @@ -0,0 +1,43 @@ +> $expectedErrorsWithLines + */ + #[DataProvider('provideData')] + public function testRule(string $filePath, array $expectedErrorsWithLines): void + { + $this->analyse([$filePath], $expectedErrorsWithLines); + } + + /** + * @return Iterator, mixed>> + */ + public static function provideData(): Iterator + { + yield [__DIR__ . '/Fixture/SkipGuardBeforeScopeFetch.php', []]; + + yield [__DIR__ . '/Fixture/ScopeFetchBeforeGuard.php', [ + [ + NoScopeFetchBeforeCheapGuardRule::ERROR_MESSAGE, + 15, + ], + ]]; + } + + protected function getRule(): Rule + { + return new NoScopeFetchBeforeCheapGuardRule(); + } +}