Skip to content

Commit 2d0e0f1

Browse files
TomasVotrubaTomasVotruba
authored
[utils] Add CheaperGuardFirstRule PHPStan rule to enforce early bail before expensive analysis (#8105)
1 parent 449db3f commit 2d0e0f1

9 files changed

Lines changed: 443 additions & 11 deletions

File tree

‎phpstan.neon‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ services:
1010
tags:
1111
- phpstan.rules.rule
1212

13+
-
14+
class: Rector\Utils\PHPStan\Rule\CheaperGuardFirstRule
15+
tags:
16+
- phpstan.rules.rule
17+
1318
parameters:
1419
level: 8
1520

‎rules/Php73/Rector/FuncCall/StringifyStrNeedlesRector.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,12 @@ public function refactor(Node $node): ?Node
8686
// is argument string?
8787
$needleArgValue = $node->args[1]->value;
8888

89-
$needleType = $this->getType($needleArgValue);
90-
if ($needleType->isString()->yes()) {
89+
if ($needleArgValue instanceof InterpolatedString) {
9190
return null;
9291
}
9392

94-
if ($needleArgValue instanceof InterpolatedString) {
93+
$needleType = $this->getType($needleArgValue);
94+
if ($needleType->isString()->yes()) {
9595
return null;
9696
}
9797

‎rules/TypeDeclaration/Rector/ClassMethod/AddReturnDocblockForScalarArrayFromAssignsRector.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,17 +110,17 @@ public function getNodeTypes(): array
110110
*/
111111
public function refactor(Node $node): ?Node
112112
{
113+
if ($node->returnType instanceof Node && ! $this->isName($node->returnType, 'array')) {
114+
return null;
115+
}
116+
113117
$phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($node);
114118
$returnType = $phpDocInfo->getReturnType();
115119

116120
if (! $returnType instanceof MixedType || $returnType->isExplicitMixed()) {
117121
return null;
118122
}
119123

120-
if ($node->returnType instanceof Node && ! $this->isName($node->returnType, 'array')) {
121-
return null;
122-
}
123-
124124
$returnsScoped = $this->betterNodeFinder->findReturnsScoped($node);
125125

126126
if (! $this->returnAnalyzer->hasOnlyReturnWithExpr($node, $returnsScoped)) {

‎rules/TypeDeclaration/Rector/FunctionLike/AddClosureParamTypeFromIterableMethodCallRector.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,10 @@ public function refactor(Node $node): ?Node
9494
return null;
9595
}
9696

97+
if (! $node->name instanceof Identifier) {
98+
return null;
99+
}
100+
97101
$varType = $this->getType($node->var);
98102

99103
if (! $varType instanceof IntersectionType || ! $varType->isIterable()->yes()) {
@@ -106,10 +110,6 @@ public function refactor(Node $node): ?Node
106110
return null;
107111
}
108112

109-
if (! $node->name instanceof Identifier) {
110-
return null;
111-
}
112-
113113
$methodReflection = $this->methodReflectionResolver->resolveMethodReflection(
114114
$className,
115115
$node->name->name,

‎utils/phpstan/src/Rule/CheaperGuardFirstRule.php‎

Lines changed: 274 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,274 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Utils\PHPStan\Rule;
6+
7+
use PhpParser\Node\Stmt\Else_;
8+
use PhpParser\Node\Expr\MethodCall;
9+
use PhpParser\Node\Expr\NullsafeMethodCall;
10+
use PhpParser\Node\Expr\StaticCall;
11+
use PhpParser\Node\Expr\FuncCall;
12+
use PhpParser\Node\Name;
13+
use PhpParser\Node;
14+
use PhpParser\Node\Expr;
15+
use PhpParser\Node\Expr\Assign;
16+
use PhpParser\Node\Expr\CallLike;
17+
use PhpParser\Node\Expr\ConstFetch;
18+
use PhpParser\Node\Expr\Variable;
19+
use PhpParser\Node\Identifier;
20+
use PhpParser\Node\Stmt;
21+
use PhpParser\Node\Stmt\ClassMethod;
22+
use PhpParser\Node\Stmt\Continue_;
23+
use PhpParser\Node\Stmt\Expression;
24+
use PhpParser\Node\Stmt\If_;
25+
use PhpParser\Node\Stmt\Return_;
26+
use PhpParser\NodeFinder;
27+
use PHPStan\Analyser\Scope;
28+
use PHPStan\Reflection\ClassReflection;
29+
use PHPStan\Rules\IdentifierRuleError;
30+
use PHPStan\Rules\Rule;
31+
use PHPStan\Rules\RuleErrorBuilder;
32+
33+
/**
34+
* Inside a Rector rule, a cheap early-return guard (isName(), instanceof, isset(), arg count)
35+
* that does not depend on an expensive analysis call (getType(), isObjectType(),
36+
* createFromNodeOrEmpty(), ...) should run BEFORE that call, so non-matching nodes bail out
37+
* before paying for the costly analysis.
38+
*
39+
* @implements Rule<ClassMethod>
40+
* @see \Rector\Utils\PHPStan\Tests\Rule\CheaperGuardFirstRule\CheaperGuardFirstRuleTest
41+
*/
42+
final class CheaperGuardFirstRule implements Rule
43+
{
44+
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.';
45+
46+
/**
47+
* Calls that trigger heavy analysis (type resolution, docblock parsing, file re-parsing).
48+
*
49+
* @var string[]
50+
*/
51+
private const array EXPENSIVE_CALLS = [
52+
'getType',
53+
'getNativeType',
54+
'isObjectType',
55+
'isObjectTypes',
56+
'createFromNode',
57+
'createFromNodeOrEmpty',
58+
];
59+
60+
/**
61+
* Calls cheap enough to evaluate as a pre-filter.
62+
*
63+
* @var string[]
64+
*/
65+
private const array CHEAP_CALLS = ['isName', 'isNames', 'isFirstClassCallable', 'in_array', 'count'];
66+
67+
private const string ABSTRACT_RECTOR_CLASS = 'Rector\Rector\AbstractRector';
68+
69+
public function getNodeType(): string
70+
{
71+
return ClassMethod::class;
72+
}
73+
74+
/**
75+
* @param ClassMethod $node
76+
* @return list<IdentifierRuleError>
77+
*/
78+
public function processNode(Node $node, Scope $scope): array
79+
{
80+
if ($node->stmts === null) {
81+
return [];
82+
}
83+
84+
$classReflection = $scope->getClassReflection();
85+
if (! $classReflection instanceof ClassReflection) {
86+
return [];
87+
}
88+
89+
if (! in_array(self::ABSTRACT_RECTOR_CLASS, $classReflection->getParentClassesNames(), true)) {
90+
return [];
91+
}
92+
93+
$stmts = $node->stmts;
94+
$anchorIndex = $this->findExpensiveAnchorIndex($stmts);
95+
if ($anchorIndex === null) {
96+
return [];
97+
}
98+
99+
$assignedVariableNames = $this->resolveAssignedVariableNames($stmts[$anchorIndex]);
100+
$counter = count($stmts);
101+
102+
for ($index = $anchorIndex + 1; $index < $counter; ++$index) {
103+
$stmt = $stmts[$index];
104+
105+
if ($this->isPureBailGuard($stmt)) {
106+
/** @var If_ $stmt */
107+
if ($this->isCheapCondition($stmt->cond) && $this->isIndependent($stmt->cond, $assignedVariableNames)) {
108+
return [
109+
RuleErrorBuilder::message(
110+
sprintf(self::ERROR_MESSAGE, $stmt->getStartLine(), $stmts[$anchorIndex]->getStartLine())
111+
)
112+
->identifier('rector.cheaperGuardFirst')
113+
->line($stmt->getStartLine())
114+
->build(),
115+
];
116+
}
117+
118+
// a dependent or non-cheap bail guard is legitimately here; keep scanning
119+
continue;
120+
}
121+
122+
if ($stmt instanceof Expression && $stmt->expr instanceof Assign) {
123+
$assignedVariableNames = [...$assignedVariableNames, ...$this->resolveAssignedVariableNames($stmt)];
124+
continue;
125+
}
126+
127+
// any other statement (value return, transformation, loop) makes hoisting unsafe
128+
return [];
129+
}
130+
131+
return [];
132+
}
133+
134+
/**
135+
* @param Stmt[] $stmts
136+
*/
137+
private function findExpensiveAnchorIndex(array $stmts): ?int
138+
{
139+
foreach ($stmts as $index => $stmt) {
140+
if (! $stmt instanceof Expression && ! $stmt instanceof If_) {
141+
continue;
142+
}
143+
144+
if ($this->containsCall($stmt, self::EXPENSIVE_CALLS)) {
145+
return $index;
146+
}
147+
}
148+
149+
return null;
150+
}
151+
152+
private function isPureBailGuard(Stmt $stmt): bool
153+
{
154+
if (! $stmt instanceof If_) {
155+
return false;
156+
}
157+
158+
if ($stmt->elseifs !== [] || $stmt->else instanceof Else_) {
159+
return false;
160+
}
161+
162+
if (count($stmt->stmts) !== 1) {
163+
return false;
164+
}
165+
166+
$onlyStmt = $stmt->stmts[0];
167+
if ($onlyStmt instanceof Continue_) {
168+
return true;
169+
}
170+
171+
if (! $onlyStmt instanceof Return_) {
172+
return false;
173+
}
174+
175+
// bare "return;" or "return null;"
176+
if (! $onlyStmt->expr instanceof Node) {
177+
return true;
178+
}
179+
180+
return $onlyStmt->expr instanceof ConstFetch && $onlyStmt->expr->name->toLowerString() === 'null';
181+
}
182+
183+
private function isCheapCondition(Expr $expr): bool
184+
{
185+
$nodeFinder = new NodeFinder();
186+
$callLikes = $nodeFinder->findInstanceOf($expr, CallLike::class);
187+
188+
foreach ($callLikes as $callLike) {
189+
$name = $this->resolveCallName($callLike);
190+
if ($name === null) {
191+
return false;
192+
}
193+
194+
if (! in_array($name, self::CHEAP_CALLS, true)) {
195+
return false;
196+
}
197+
}
198+
199+
return true;
200+
}
201+
202+
/**
203+
* @param string[] $assignedVariableNames
204+
*/
205+
private function isIndependent(Expr $expr, array $assignedVariableNames): bool
206+
{
207+
if ($assignedVariableNames === []) {
208+
return true;
209+
}
210+
211+
$nodeFinder = new NodeFinder();
212+
$variables = $nodeFinder->findInstanceOf($expr, Variable::class);
213+
214+
foreach ($variables as $variable) {
215+
if (! is_string($variable->name)) {
216+
continue;
217+
}
218+
219+
if (in_array($variable->name, $assignedVariableNames, true)) {
220+
return false;
221+
}
222+
}
223+
224+
return true;
225+
}
226+
227+
/**
228+
* @param string[] $callNames
229+
*/
230+
private function containsCall(Node $node, array $callNames): bool
231+
{
232+
$nodeFinder = new NodeFinder();
233+
$callLikes = $nodeFinder->findInstanceOf($node, CallLike::class);
234+
235+
foreach ($callLikes as $callLike) {
236+
$name = $this->resolveCallName($callLike);
237+
if ($name !== null && in_array($name, $callNames, true)) {
238+
return true;
239+
}
240+
}
241+
242+
return false;
243+
}
244+
245+
private function resolveCallName(CallLike $callLike): ?string
246+
{
247+
if ($callLike instanceof MethodCall || $callLike instanceof NullsafeMethodCall || $callLike instanceof StaticCall) {
248+
return $callLike->name instanceof Identifier ? $callLike->name->toString() : null;
249+
}
250+
251+
if ($callLike instanceof FuncCall) {
252+
return $callLike->name instanceof Name ? $callLike->name->toString() : null;
253+
}
254+
255+
return null;
256+
}
257+
258+
/**
259+
* @return string[]
260+
*/
261+
private function resolveAssignedVariableNames(Stmt $stmt): array
262+
{
263+
if (! $stmt instanceof Expression || ! $stmt->expr instanceof Assign) {
264+
return [];
265+
}
266+
267+
$assign = $stmt->expr;
268+
if ($assign->var instanceof Variable && is_string($assign->var->name)) {
269+
return [$assign->var->name];
270+
}
271+
272+
return [];
273+
}
274+
}

‎utils/phpstan/tests/Rule/CheaperGuardFirstRule/CheaperGuardFirstRuleTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Utils\PHPStan\Tests\Rule\CheaperGuardFirstRule;
6+
7+
use PHPStan\Rules\Rule;
8+
use PHPStan\Testing\RuleTestCase;
9+
use Rector\Utils\PHPStan\Rule\CheaperGuardFirstRule;
10+
11+
/**
12+
* @extends RuleTestCase<CheaperGuardFirstRule>
13+
*/
14+
final class CheaperGuardFirstRuleTest extends RuleTestCase
15+
{
16+
public function testExpensiveBeforeCheapGuard(): void
17+
{
18+
$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.';
19+
20+
$this->analyse([__DIR__ . '/Source/ExpensiveBeforeCheapGuardRector.php'], [[$expectedErrorMessage, 32]]);
21+
}
22+
23+
public function testCheapGuardFirst(): void
24+
{
25+
$this->analyse([__DIR__ . '/Source/CheapGuardFirstRector.php'], []);
26+
}
27+
28+
public function testDependentGuard(): void
29+
{
30+
$this->analyse([__DIR__ . '/Source/DependentGuardRector.php'], []);
31+
}
32+
33+
protected function getRule(): Rule
34+
{
35+
return new CheaperGuardFirstRule();
36+
}
37+
}

0 commit comments

Comments
 (0)