From e2f4f5b7825694fbac7100e612b905b68a9e0caf Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 29 Jun 2026 10:18:12 +0200 Subject: [PATCH 1/2] [utils] Add CheaperGuardFirstRule PHPStan rule to enforce early bail before expensive analysis --- phpstan.neon | 5 + .../FuncCall/StringifyStrNeedlesRector.php | 6 +- ...ocblockForScalarArrayFromAssignsRector.php | 8 +- ...eParamTypeFromIterableMethodCallRector.php | 8 +- .../src/Rule/CheaperGuardFirstRule.php | 273 ++++++++++++++++++ .../CheaperGuardFirstRuleTest.php | 37 +++ .../Source/CheapGuardFirstRector.php | 38 +++ .../Source/DependentGuardRector.php | 40 +++ .../ExpensiveBeforeCheapGuardRector.php | 38 +++ 9 files changed, 442 insertions(+), 11 deletions(-) create mode 100644 utils/phpstan/src/Rule/CheaperGuardFirstRule.php create mode 100644 utils/phpstan/tests/Rule/CheaperGuardFirstRule/CheaperGuardFirstRuleTest.php create mode 100644 utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/CheapGuardFirstRector.php create mode 100644 utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/DependentGuardRector.php create mode 100644 utils/phpstan/tests/Rule/CheaperGuardFirstRule/Source/ExpensiveBeforeCheapGuardRector.php 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..9779dfbfc6a --- /dev/null +++ b/utils/phpstan/src/Rule/CheaperGuardFirstRule.php @@ -0,0 +1,273 @@ + + * @see \Rector\Utils\PHPStan\Tests\Rule\CheaperGuardFirstRule\CheaperGuardFirstRuleTest + */ +final class CheaperGuardFirstRule implements Rule +{ + /** + * @var string + */ + private const 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 EXPENSIVE_CALLS = [ + 'getType', + 'getNativeType', + 'isObjectType', + 'isObjectTypes', + 'createFromNode', + 'createFromNodeOrEmpty', + ]; + + /** + * Calls cheap enough to evaluate as a pre-filter. + * + * @var string[] + */ + private const CHEAP_CALLS = ['isName', 'isNames', 'isFirstClassCallable', 'in_array', 'count']; + + /** + * @var string + */ + private const 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]); + + for ($index = $anchorIndex + 1; $index < count($stmts); ++$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 !== null) { + 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 $cond): bool + { + $nodeFinder = new NodeFinder(); + $callLikes = $nodeFinder->findInstanceOf($cond, 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 $cond, array $assignedVariableNames): bool + { + if ($assignedVariableNames === []) { + return true; + } + + $nodeFinder = new NodeFinder(); + $variables = $nodeFinder->findInstanceOf($cond, 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 Node\Expr\MethodCall || $callLike instanceof Node\Expr\NullsafeMethodCall || $callLike instanceof Node\Expr\StaticCall) { + return $callLike->name instanceof Identifier ? $callLike->name->toString() : null; + } + + if ($callLike instanceof Node\Expr\FuncCall) { + return $callLike->name instanceof Node\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; + } +} From 90d2e2003ee5ff82421782d480128a9b7d563ea7 Mon Sep 17 00:00:00 2001 From: GitHub Action Date: Mon, 29 Jun 2026 08:30:53 +0000 Subject: [PATCH 2/2] [ci-review] Rector Rectify --- .../src/Rule/CheaperGuardFirstRule.php | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/utils/phpstan/src/Rule/CheaperGuardFirstRule.php b/utils/phpstan/src/Rule/CheaperGuardFirstRule.php index 9779dfbfc6a..85c6252b68e 100644 --- a/utils/phpstan/src/Rule/CheaperGuardFirstRule.php +++ b/utils/phpstan/src/Rule/CheaperGuardFirstRule.php @@ -4,6 +4,12 @@ namespace Rector\Utils\PHPStan\Rule; +use PhpParser\Node\Stmt\Else_; +use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\NullsafeMethodCall; +use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\Expr\FuncCall; +use PhpParser\Node\Name; use PhpParser\Node; use PhpParser\Node\Expr; use PhpParser\Node\Expr\Assign; @@ -35,17 +41,14 @@ */ final class CheaperGuardFirstRule implements Rule { - /** - * @var string - */ - private const 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.'; + 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 EXPENSIVE_CALLS = [ + private const array EXPENSIVE_CALLS = [ 'getType', 'getNativeType', 'isObjectType', @@ -59,12 +62,9 @@ final class CheaperGuardFirstRule implements Rule * * @var string[] */ - private const CHEAP_CALLS = ['isName', 'isNames', 'isFirstClassCallable', 'in_array', 'count']; + private const array CHEAP_CALLS = ['isName', 'isNames', 'isFirstClassCallable', 'in_array', 'count']; - /** - * @var string - */ - private const ABSTRACT_RECTOR_CLASS = 'Rector\Rector\AbstractRector'; + private const string ABSTRACT_RECTOR_CLASS = 'Rector\Rector\AbstractRector'; public function getNodeType(): string { @@ -97,8 +97,9 @@ public function processNode(Node $node, Scope $scope): array } $assignedVariableNames = $this->resolveAssignedVariableNames($stmts[$anchorIndex]); + $counter = count($stmts); - for ($index = $anchorIndex + 1; $index < count($stmts); ++$index) { + for ($index = $anchorIndex + 1; $index < $counter; ++$index) { $stmt = $stmts[$index]; if ($this->isPureBailGuard($stmt)) { @@ -154,7 +155,7 @@ private function isPureBailGuard(Stmt $stmt): bool return false; } - if ($stmt->elseifs !== [] || $stmt->else !== null) { + if ($stmt->elseifs !== [] || $stmt->else instanceof Else_) { return false; } @@ -179,10 +180,10 @@ private function isPureBailGuard(Stmt $stmt): bool return $onlyStmt->expr instanceof ConstFetch && $onlyStmt->expr->name->toLowerString() === 'null'; } - private function isCheapCondition(Expr $cond): bool + private function isCheapCondition(Expr $expr): bool { $nodeFinder = new NodeFinder(); - $callLikes = $nodeFinder->findInstanceOf($cond, CallLike::class); + $callLikes = $nodeFinder->findInstanceOf($expr, CallLike::class); foreach ($callLikes as $callLike) { $name = $this->resolveCallName($callLike); @@ -201,14 +202,14 @@ private function isCheapCondition(Expr $cond): bool /** * @param string[] $assignedVariableNames */ - private function isIndependent(Expr $cond, array $assignedVariableNames): bool + private function isIndependent(Expr $expr, array $assignedVariableNames): bool { if ($assignedVariableNames === []) { return true; } $nodeFinder = new NodeFinder(); - $variables = $nodeFinder->findInstanceOf($cond, Variable::class); + $variables = $nodeFinder->findInstanceOf($expr, Variable::class); foreach ($variables as $variable) { if (! is_string($variable->name)) { @@ -243,12 +244,12 @@ private function containsCall(Node $node, array $callNames): bool private function resolveCallName(CallLike $callLike): ?string { - if ($callLike instanceof Node\Expr\MethodCall || $callLike instanceof Node\Expr\NullsafeMethodCall || $callLike instanceof Node\Expr\StaticCall) { + if ($callLike instanceof MethodCall || $callLike instanceof NullsafeMethodCall || $callLike instanceof StaticCall) { return $callLike->name instanceof Identifier ? $callLike->name->toString() : null; } - if ($callLike instanceof Node\Expr\FuncCall) { - return $callLike->name instanceof Node\Name ? $callLike->name->toString() : null; + if ($callLike instanceof FuncCall) { + return $callLike->name instanceof Name ? $callLike->name->toString() : null; } return null;