diff --git a/config/rector-rules.neon b/config/rector-rules.neon index b3b601d7..b4a235b8 100644 --- a/config/rector-rules.neon +++ b/config/rector-rules.neon @@ -9,6 +9,7 @@ rules: - Symplify\PHPStanRules\Rules\Rector\NoOnlyNullReturnInRefactorRule - Symplify\PHPStanRules\Rules\Rector\NoIntegerRefactorReturnRule - Symplify\PHPStanRules\Rules\Rector\AvoidFeatureSetAttributeInRectorRule + - Symplify\PHPStanRules\Rules\Rector\RectorCheaperGuardsFirstRule services: # $node->getAttribute($1) => Type|null by $1 diff --git a/src/Enum/RuleIdentifier/RectorRuleIdentifier.php b/src/Enum/RuleIdentifier/RectorRuleIdentifier.php index db6bfc50..70465dda 100644 --- a/src/Enum/RuleIdentifier/RectorRuleIdentifier.php +++ b/src/Enum/RuleIdentifier/RectorRuleIdentifier.php @@ -23,4 +23,6 @@ final class RectorRuleIdentifier public const string NO_INTEGER_REFACTOR_RETURN = 'rector.noIntegerRefactorReturn'; public const string AVOID_FEATURE_SET_ATTRIBUTE_IN_RECTOR = 'rector.avoidFeatureSetAttributeInRector'; + + public const string RECTOR_CHEAPER_GUARDS_FIRST = 'rector.rectorCheaperGuardsFirst'; } diff --git a/src/Rules/Rector/RectorCheaperGuardsFirstRule.php b/src/Rules/Rector/RectorCheaperGuardsFirstRule.php new file mode 100644 index 00000000..77197765 --- /dev/null +++ b/src/Rules/Rector/RectorCheaperGuardsFirstRule.php @@ -0,0 +1,276 @@ + + * @see \Symplify\PHPStanRules\Tests\Rules\Rector\RectorCheaperGuardsFirstRule\RectorCheaperGuardsFirstRuleTest + */ +final class RectorCheaperGuardsFirstRule implements Rule +{ + public 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 = AbstractRector::class; + + 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(RectorRuleIdentifier::RECTOR_CHEAPER_GUARDS_FIRST) + ->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/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/ExpensiveBeforeCheapGuard.php b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/ExpensiveBeforeCheapGuard.php new file mode 100644 index 00000000..c4d0de2a --- /dev/null +++ b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/ExpensiveBeforeCheapGuard.php @@ -0,0 +1,32 @@ +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; + } +} diff --git a/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/SkipCheapGuardFirst.php b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/SkipCheapGuardFirst.php new file mode 100644 index 00000000..720bed11 --- /dev/null +++ b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/SkipCheapGuardFirst.php @@ -0,0 +1,32 @@ +isName($node, 'array')) { + return null; + } + + $type = $this->getType($node); + if ($type->isString()->yes()) { + return null; + } + + return $node; + } +} diff --git a/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/SkipDependentGuard.php b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/SkipDependentGuard.php new file mode 100644 index 00000000..568ba5d3 --- /dev/null +++ b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/Fixture/SkipDependentGuard.php @@ -0,0 +1,34 @@ +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/tests/Rules/Rector/RectorCheaperGuardsFirstRule/RectorCheaperGuardsFirstRuleTest.php b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/RectorCheaperGuardsFirstRuleTest.php new file mode 100644 index 00000000..eb1a1852 --- /dev/null +++ b/tests/Rules/Rector/RectorCheaperGuardsFirstRule/RectorCheaperGuardsFirstRuleTest.php @@ -0,0 +1,45 @@ +> $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/SkipCheapGuardFirst.php', []]; + + yield [__DIR__ . '/Fixture/SkipDependentGuard.php', []]; + + yield [__DIR__ . '/Fixture/ExpensiveBeforeCheapGuard.php', [ + [ + sprintf(RectorCheaperGuardsFirstRule::ERROR_MESSAGE, 26, 20), + 26, + ], + ]]; + } + + protected function getRule(): Rule + { + return new RectorCheaperGuardsFirstRule(); + } +}