Skip to content

Commit b31f6ae

Browse files
TomasVotrubaTomasVotruba
authored
[perf] Add NoScopeFetchBeforeCheapGuardRule to spot scope fetched before a scope-free return-null guard (#8584)
1 parent e9eee8d commit b31f6ae

7 files changed

Lines changed: 237 additions & 4 deletions

File tree

‎phpstan.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ includes:
33

44
rules:
55
- Rector\Utils\PHPStan\Rule\RegisterRelatedPolyfillRectorRule
6+
- Rector\Utils\PHPStan\Rule\NoScopeFetchBeforeCheapGuardRule
67

78
parameters:
89
level: 8

‎rules/DeadCode/Rector/ClassMethod/RemoveUnusedPrivateMethodRector.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public function refactor(Node $node): ?Node
147147

148148
private function hasPrivateMethod(Class_ $class): bool
149149
{
150-
return array_any($class->getMethods(), fn(ClassMethod $classMethod): bool => $classMethod->isPrivate());
150+
return array_any($class->getMethods(), fn (ClassMethod $classMethod): bool => $classMethod->isPrivate());
151151
}
152152

153153
private function shouldSkip(ClassMethod $classMethod, ClassReflection $classReflection): bool

‎rules/TypeDeclaration/Rector/BooleanAnd/BinaryOpNullableToInstanceofRector.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,11 @@ public function refactor(Node $node): ?Node
8282
return $this->processNullableInstance($node);
8383
}
8484

85+
/**
86+
* @template TBinaryOp of BooleanAnd|BooleanOr
87+
* @param TBinaryOp $node
88+
* @return TBinaryOp|null
89+
*/
8590
private function processNullableInstance(BooleanAnd|BooleanOr $node): null|BooleanAnd|BooleanOr
8691
{
8792
$nullableObjectType = $this->nullableTypeAnalyzer->resolveNullableObjectType($node->left);
@@ -134,9 +139,7 @@ private function processNegationBooleanOr(BooleanOr $booleanOr): ?BooleanOr
134139
return $booleanOr;
135140
}
136141

137-
/** @var BooleanOr|null $result */
138-
$result = $this->processNullableInstance($booleanOr);
139-
return $result;
142+
return $this->processNullableInstance($booleanOr);
140143
}
141144

142145
private function createExprInstanceof(Expr $expr, ObjectType $objectType): Instanceof_

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

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Utils\PHPStan\Rule;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Expr;
9+
use PhpParser\Node\Expr\Assign;
10+
use PhpParser\Node\Expr\StaticCall;
11+
use PhpParser\Node\Expr\Variable;
12+
use PhpParser\Node\Stmt;
13+
use PhpParser\Node\Stmt\ClassMethod;
14+
use PhpParser\Node\Stmt\Expression;
15+
use PhpParser\Node\Stmt\If_;
16+
use PhpParser\Node\Stmt\Return_;
17+
use PhpParser\NodeFinder;
18+
use PHPStan\Analyser\Scope;
19+
use PHPStan\Rules\IdentifierRuleError;
20+
use PHPStan\Rules\Rule;
21+
use PHPStan\Rules\RuleErrorBuilder;
22+
use Rector\PHPStan\ScopeFetcher;
23+
24+
/**
25+
* Flags ScopeFetcher::fetch() called right before a cheap guard that does not use the scope.
26+
* The guard should run first, so most nodes bail out before scope resolution.
27+
*
28+
* @implements Rule<ClassMethod>
29+
* @see \Rector\Utils\PHPStan\Tests\Rule\NoScopeFetchBeforeCheapGuardRule\NoScopeFetchBeforeCheapGuardRuleTest
30+
*/
31+
final class NoScopeFetchBeforeCheapGuardRule implements Rule
32+
{
33+
private const string 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.';
34+
35+
public function getNodeType(): string
36+
{
37+
return ClassMethod::class;
38+
}
39+
40+
/**
41+
* @param ClassMethod $node
42+
* @return list<IdentifierRuleError>
43+
*/
44+
public function processNode(Node $node, Scope $scope): array
45+
{
46+
if ($node->name->toString() !== 'refactor') {
47+
return [];
48+
}
49+
50+
$stmts = $node->stmts;
51+
if ($stmts === null) {
52+
return [];
53+
}
54+
55+
foreach ($stmts as $key => $stmt) {
56+
$scopeVariableName = $this->matchScopeFetchVariableName($stmt);
57+
if ($scopeVariableName === null) {
58+
continue;
59+
}
60+
61+
$nextStmt = $stmts[$key + 1] ?? null;
62+
if (! $nextStmt instanceof If_) {
63+
continue;
64+
}
65+
66+
if (! $this->isReturnNullGuard($nextStmt)) {
67+
continue;
68+
}
69+
70+
if ($this->usesVariable($nextStmt->cond, $scopeVariableName)) {
71+
continue;
72+
}
73+
74+
return [
75+
RuleErrorBuilder::message(self::ERROR_MESSAGE)
76+
->identifier('rector.scopeFetchBeforeCheapGuard')
77+
->line($stmt->getStartLine())
78+
->build(),
79+
];
80+
}
81+
82+
return [];
83+
}
84+
85+
private function matchScopeFetchVariableName(Stmt $stmt): ?string
86+
{
87+
if (! $stmt instanceof Expression) {
88+
return null;
89+
}
90+
91+
if (! $stmt->expr instanceof Assign) {
92+
return null;
93+
}
94+
95+
$assign = $stmt->expr;
96+
if (! $assign->var instanceof Variable || ! is_string($assign->var->name)) {
97+
return null;
98+
}
99+
100+
if (! $assign->expr instanceof StaticCall) {
101+
return null;
102+
}
103+
104+
$staticCall = $assign->expr;
105+
if (! $staticCall->class instanceof Node\Name) {
106+
return null;
107+
}
108+
109+
if ($staticCall->class->toString() !== ScopeFetcher::class) {
110+
return null;
111+
}
112+
113+
if (! $staticCall->name instanceof Node\Identifier || $staticCall->name->toString() !== 'fetch') {
114+
return null;
115+
}
116+
117+
return $assign->var->name;
118+
}
119+
120+
private function isReturnNullGuard(If_ $if): bool
121+
{
122+
if (count($if->stmts) !== 1) {
123+
return false;
124+
}
125+
126+
$onlyStmt = $if->stmts[0];
127+
if (! $onlyStmt instanceof Return_) {
128+
return false;
129+
}
130+
131+
return $onlyStmt->expr === null || $this->isNullConstant($onlyStmt->expr);
132+
}
133+
134+
private function isNullConstant(Expr $expr): bool
135+
{
136+
return $expr instanceof Node\Expr\ConstFetch && $expr->name->toLowerString() === 'null';
137+
}
138+
139+
private function usesVariable(Expr $expr, string $variableName): bool
140+
{
141+
$foundVariable = (new NodeFinder())->findFirst(
142+
[$expr],
143+
static fn (Node $subNode): bool => $subNode instanceof Variable && $subNode->name === $variableName
144+
);
145+
146+
return $foundVariable instanceof Variable;
147+
}
148+
}

‎utils/phpstan/tests/Rule/NoScopeFetchBeforeCheapGuardRule/NoScopeFetchBeforeCheapGuardRuleTest.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Utils\PHPStan\Tests\Rule\NoScopeFetchBeforeCheapGuardRule;
6+
7+
use PHPStan\Rules\Rule;
8+
use PHPStan\Testing\RuleTestCase;
9+
use Rector\Utils\PHPStan\Rule\NoScopeFetchBeforeCheapGuardRule;
10+
11+
/**
12+
* @extends RuleTestCase<NoScopeFetchBeforeCheapGuardRule>
13+
*/
14+
final class NoScopeFetchBeforeCheapGuardRuleTest extends RuleTestCase
15+
{
16+
public function testScopeFetchBeforeGuard(): void
17+
{
18+
$expectedErrorMessage = 'Move the "return null" guard above ScopeFetcher::fetch(), as it does not use the scope - most nodes then bail out before scope resolution.';
19+
20+
$this->analyse([__DIR__ . '/Source/ScopeFetchBeforeGuardRector.php'], [[$expectedErrorMessage, 15]]);
21+
}
22+
23+
public function testSkip(): void
24+
{
25+
$this->analyse([__DIR__ . '/Source/SkipScopeFetchRector.php'], []);
26+
}
27+
28+
protected function getRule(): Rule
29+
{
30+
return new NoScopeFetchBeforeCheapGuardRule();
31+
}
32+
}

‎utils/phpstan/tests/Rule/NoScopeFetchBeforeCheapGuardRule/Source/ScopeFetchBeforeGuardRector.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Utils\PHPStan\Tests\Rule\NoScopeFetchBeforeCheapGuardRule\Source;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Stmt\ClassMethod;
9+
use Rector\PHPStan\ScopeFetcher;
10+
11+
final class ScopeFetchBeforeGuardRector
12+
{
13+
public function refactor(ClassMethod $node): ?Node
14+
{
15+
$scope = ScopeFetcher::fetch($node);
16+
if ($node->returnType instanceof Node) {
17+
return null;
18+
}
19+
20+
return $node;
21+
}
22+
}

‎utils/phpstan/tests/Rule/NoScopeFetchBeforeCheapGuardRule/Source/SkipScopeFetchRector.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Rector\Utils\PHPStan\Tests\Rule\NoScopeFetchBeforeCheapGuardRule\Source;
6+
7+
use PhpParser\Node;
8+
use PhpParser\Node\Stmt\ClassMethod;
9+
use Rector\PHPStan\ScopeFetcher;
10+
11+
final class SkipScopeFetchRector
12+
{
13+
public function refactor(ClassMethod $node): ?Node
14+
{
15+
// cheap guard already runs first
16+
if ($node->returnType instanceof Node) {
17+
return null;
18+
}
19+
20+
$scope = ScopeFetcher::fetch($node);
21+
if (! $scope->isInClass()) {
22+
return null;
23+
}
24+
25+
return $node;
26+
}
27+
}

0 commit comments

Comments
 (0)