diff --git a/phpstan.neon b/phpstan.neon index 5a90ac5203a..3114dd9d2a6 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -10,6 +10,11 @@ services: tags: - phpstan.rules.rule + - + class: Rector\Utils\PHPStan\Rule\CheaperGuardFirstRule + tags: + - phpstan.rules.rule + parameters: level: 8 diff --git a/rules/Php73/Rector/FuncCall/StringifyStrNeedlesRector.php b/rules/Php73/Rector/FuncCall/StringifyStrNeedlesRector.php index aa145b36db7..e4717ba1e3d 100644 --- a/rules/Php73/Rector/FuncCall/StringifyStrNeedlesRector.php +++ b/rules/Php73/Rector/FuncCall/StringifyStrNeedlesRector.php @@ -86,12 +86,12 @@ public function refactor(Node $node): ?Node // is argument string? $needleArgValue = $node->args[1]->value; - $needleType = $this->getType($needleArgValue); - if ($needleType->isString()->yes()) { + if ($needleArgValue instanceof InterpolatedString) { return null; } - if ($needleArgValue instanceof InterpolatedString) { + $needleType = $this->getType($needleArgValue); + if ($needleType->isString()->yes()) { return null; } diff --git a/rules/TypeDeclaration/Rector/ClassMethod/AddReturnDocblockForScalarArrayFromAssignsRector.php b/rules/TypeDeclaration/Rector/ClassMethod/AddReturnDocblockForScalarArrayFromAssignsRector.php index 3a965bff1ac..7bb7bf29d1a 100644 --- a/rules/TypeDeclaration/Rector/ClassMethod/AddReturnDocblockForScalarArrayFromAssignsRector.php +++ b/rules/TypeDeclaration/Rector/ClassMethod/AddReturnDocblockForScalarArrayFromAssignsRector.php @@ -110,6 +110,10 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { + if ($node->returnType instanceof Node && ! $this->isName($node->returnType, 'array')) { + return null; + } + $phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($node); $returnType = $phpDocInfo->getReturnType(); @@ -117,10 +121,6 @@ public function refactor(Node $node): ?Node return null; } - if ($node->returnType instanceof Node && ! $this->isName($node->returnType, 'array')) { - return null; - } - $returnsScoped = $this->betterNodeFinder->findReturnsScoped($node); if (! $this->returnAnalyzer->hasOnlyReturnWithExpr($node, $returnsScoped)) { diff --git a/rules/TypeDeclaration/Rector/FunctionLike/AddClosureParamTypeFromIterableMethodCallRector.php b/rules/TypeDeclaration/Rector/FunctionLike/AddClosureParamTypeFromIterableMethodCallRector.php index f014d56a3ae..9c944a80ac8 100644 --- a/rules/TypeDeclaration/Rector/FunctionLike/AddClosureParamTypeFromIterableMethodCallRector.php +++ b/rules/TypeDeclaration/Rector/FunctionLike/AddClosureParamTypeFromIterableMethodCallRector.php @@ -94,6 +94,10 @@ public function refactor(Node $node): ?Node return null; } + if (! $node->name instanceof Identifier) { + return null; + } + $varType = $this->getType($node->var); if (! $varType instanceof IntersectionType || ! $varType->isIterable()->yes()) { @@ -106,10 +110,6 @@ public function refactor(Node $node): ?Node return null; } - if (! $node->name instanceof Identifier) { - return null; - } - $methodReflection = $this->methodReflectionResolver->resolveMethodReflection( $className, $node->name->name, diff --git a/utils/phpstan/src/Rule/CheaperGuardFirstRule.php b/utils/phpstan/src/Rule/CheaperGuardFirstRule.php new file mode 100644 index 00000000000..85c6252b68e --- /dev/null +++ b/utils/phpstan/src/Rule/CheaperGuardFirstRule.php @@ -0,0 +1,274 @@ + + * @see \Rector\Utils\PHPStan\Tests\Rule\CheaperGuardFirstRule\CheaperGuardFirstRuleTest + */ +final class CheaperGuardFirstRule implements Rule +{ + private const string ERROR_MESSAGE = 'Cheap guard on line %d can run before the expensive call on line %d; move the early return up to bail before the costly analysis.'; + + /** + * Calls that trigger heavy analysis (type resolution, docblock parsing, file re-parsing). + * + * @var string[] + */ + private const array EXPENSIVE_CALLS = [ + 'getType', + 'getNativeType', + 'isObjectType', + 'isObjectTypes', + 'createFromNode', + 'createFromNodeOrEmpty', + ]; + + /** + * Calls cheap enough to evaluate as a pre-filter. + * + * @var string[] + */ + private const array CHEAP_CALLS = ['isName', 'isNames', 'isFirstClassCallable', 'in_array', 'count']; + + private const string ABSTRACT_RECTOR_CLASS = 'Rector\Rector\AbstractRector'; + + public function getNodeType(): string + { + return ClassMethod::class; + } + + /** + * @param ClassMethod $node + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + if ($node->stmts === null) { + return []; + } + + $classReflection = $scope->getClassReflection(); + if (! $classReflection instanceof ClassReflection) { + return []; + } + + if (! in_array(self::ABSTRACT_RECTOR_CLASS, $classReflection->getParentClassesNames(), true)) { + return []; + } + + $stmts = $node->stmts; + $anchorIndex = $this->findExpensiveAnchorIndex($stmts); + if ($anchorIndex === null) { + return []; + } + + $assignedVariableNames = $this->resolveAssignedVariableNames($stmts[$anchorIndex]); + $counter = count($stmts); + + for ($index = $anchorIndex + 1; $index < $counter; ++$index) { + $stmt = $stmts[$index]; + + if ($this->isPureBailGuard($stmt)) { + /** @var If_ $stmt */ + if ($this->isCheapCondition($stmt->cond) && $this->isIndependent($stmt->cond, $assignedVariableNames)) { + return [ + RuleErrorBuilder::message( + sprintf(self::ERROR_MESSAGE, $stmt->getStartLine(), $stmts[$anchorIndex]->getStartLine()) + ) + ->identifier('rector.cheaperGuardFirst') + ->line($stmt->getStartLine()) + ->build(), + ]; + } + + // a dependent or non-cheap bail guard is legitimately here; keep scanning + continue; + } + + if ($stmt instanceof Expression && $stmt->expr instanceof Assign) { + $assignedVariableNames = [...$assignedVariableNames, ...$this->resolveAssignedVariableNames($stmt)]; + continue; + } + + // any other statement (value return, transformation, loop) makes hoisting unsafe + return []; + } + + return []; + } + + /** + * @param Stmt[] $stmts + */ + private function findExpensiveAnchorIndex(array $stmts): ?int + { + foreach ($stmts as $index => $stmt) { + if (! $stmt instanceof Expression && ! $stmt instanceof If_) { + continue; + } + + if ($this->containsCall($stmt, self::EXPENSIVE_CALLS)) { + return $index; + } + } + + return null; + } + + private function isPureBailGuard(Stmt $stmt): bool + { + if (! $stmt instanceof If_) { + return false; + } + + if ($stmt->elseifs !== [] || $stmt->else instanceof Else_) { + return false; + } + + if (count($stmt->stmts) !== 1) { + return false; + } + + $onlyStmt = $stmt->stmts[0]; + if ($onlyStmt instanceof Continue_) { + return true; + } + + if (! $onlyStmt instanceof Return_) { + return false; + } + + // bare "return;" or "return null;" + if (! $onlyStmt->expr instanceof Node) { + return true; + } + + return $onlyStmt->expr instanceof ConstFetch && $onlyStmt->expr->name->toLowerString() === 'null'; + } + + private function isCheapCondition(Expr $expr): bool + { + $nodeFinder = new NodeFinder(); + $callLikes = $nodeFinder->findInstanceOf($expr, CallLike::class); + + foreach ($callLikes as $callLike) { + $name = $this->resolveCallName($callLike); + if ($name === null) { + return false; + } + + if (! in_array($name, self::CHEAP_CALLS, true)) { + return false; + } + } + + return true; + } + + /** + * @param string[] $assignedVariableNames + */ + private function isIndependent(Expr $expr, array $assignedVariableNames): bool + { + if ($assignedVariableNames === []) { + return true; + } + + $nodeFinder = new NodeFinder(); + $variables = $nodeFinder->findInstanceOf($expr, Variable::class); + + foreach ($variables as $variable) { + if (! is_string($variable->name)) { + continue; + } + + if (in_array($variable->name, $assignedVariableNames, true)) { + return false; + } + } + + return true; + } + + /** + * @param string[] $callNames + */ + private function containsCall(Node $node, array $callNames): bool + { + $nodeFinder = new NodeFinder(); + $callLikes = $nodeFinder->findInstanceOf($node, CallLike::class); + + foreach ($callLikes as $callLike) { + $name = $this->resolveCallName($callLike); + if ($name !== null && in_array($name, $callNames, true)) { + return true; + } + } + + return false; + } + + private function resolveCallName(CallLike $callLike): ?string + { + if ($callLike instanceof MethodCall || $callLike instanceof NullsafeMethodCall || $callLike instanceof StaticCall) { + return $callLike->name instanceof Identifier ? $callLike->name->toString() : null; + } + + if ($callLike instanceof FuncCall) { + return $callLike->name instanceof Name ? $callLike->name->toString() : null; + } + + return null; + } + + /** + * @return string[] + */ + private function resolveAssignedVariableNames(Stmt $stmt): array + { + if (! $stmt instanceof Expression || ! $stmt->expr instanceof Assign) { + return []; + } + + $assign = $stmt->expr; + if ($assign->var instanceof Variable && is_string($assign->var->name)) { + return [$assign->var->name]; + } + + return []; + } +} diff --git a/utils/phpstan/tests/Rule/CheaperGuardFirstRule/CheaperGuardFirstRuleTest.php b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/CheaperGuardFirstRuleTest.php new file mode 100644 index 00000000000..b369aef9321 --- /dev/null +++ b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/CheaperGuardFirstRuleTest.php @@ -0,0 +1,37 @@ + + */ +final class CheaperGuardFirstRuleTest extends RuleTestCase +{ + public function testExpensiveBeforeCheapGuard(): void + { + $expectedErrorMessage = 'Cheap guard on line 32 can run before the expensive call on line 26; move the early return up to bail before the costly analysis.'; + + $this->analyse([__DIR__ . '/Source/ExpensiveBeforeCheapGuardRector.php'], [[$expectedErrorMessage, 32]]); + } + + public function testCheapGuardFirst(): void + { + $this->analyse([__DIR__ . '/Source/CheapGuardFirstRector.php'], []); + } + + public function testDependentGuard(): void + { + $this->analyse([__DIR__ . '/Source/DependentGuardRector.php'], []); + } + + protected function getRule(): Rule + { + return new CheaperGuardFirstRule(); + } +} diff --git a/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/CheapGuardFirstRector.php b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/CheapGuardFirstRector.php new file mode 100644 index 00000000000..92d62231b5f --- /dev/null +++ b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/CheapGuardFirstRector.php @@ -0,0 +1,38 @@ +isName($node, 'array')) { + return null; + } + + $type = $this->getType($node); + if ($type->isString()->yes()) { + return null; + } + + return $node; + } +} diff --git a/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/DependentGuardRector.php b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/DependentGuardRector.php new file mode 100644 index 00000000000..181e66cb5a3 --- /dev/null +++ b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/DependentGuardRector.php @@ -0,0 +1,40 @@ +getType($node); + + // value-returning guard in between -> reordering is unsafe, must NOT report + if ($type->isString()->yes()) { + return $node; + } + + // depends on $type -> must NOT report + if ($type->isInteger()->yes()) { + return null; + } + + return $node; + } +} diff --git a/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/ExpensiveBeforeCheapGuardRector.php b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/ExpensiveBeforeCheapGuardRector.php new file mode 100644 index 00000000000..ad9af5a48b8 --- /dev/null +++ b/utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/ExpensiveBeforeCheapGuardRector.php @@ -0,0 +1,38 @@ +getType($node); + if ($type->isString()->yes()) { + return null; + } + + // cheap, independent of $type, but runs after the expensive getType() + if (! $this->isName($node, 'array')) { + return null; + } + + return $node; + } +}