Skip to content

Commit e86fd6a

Browse files
zonuexezonuexe
committed
Check the purity declarations of methods without a body
1 parent b0e7a99 commit e86fd6a

8 files changed

Lines changed: 309 additions & 75 deletions

File tree

‎conf/bleedingEdge.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,4 @@ parameters:
3232
staticVariablesFromUsages: true
3333
unusedVariable: true
3434
unusedParameters: true
35+
reportAbstractMethodPurity: true

‎conf/config.level2.neon‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ conditionalTags:
1919
phpstan.rules.rule: %featureToggles.checkDynamicConstantNameValues%
2020
PHPStan\Rules\Constants\ValueAssignedToGlobalConstantRule:
2121
phpstan.rules.rule: %featureToggles.checkDynamicConstantNameValues%
22+
PHPStan\Rules\Pure\PureAbstractMethodRule:
23+
phpstan.rules.rule: %featureToggles.reportAbstractMethodPurity%
2224

2325
services:
2426
-
@@ -32,3 +34,6 @@ services:
3234

3335
-
3436
class: PHPStan\Rules\Constants\ValueAssignedToGlobalConstantRule
37+
38+
-
39+
class: PHPStan\Rules\Pure\PureAbstractMethodRule

‎conf/config.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ parameters:
6464
staticVariablesFromUsages: false
6565
unusedVariable: false
6666
unusedParameters: false
67+
reportAbstractMethodPurity: false
6768
fileExtensions:
6869
- php
6970
checkAdvancedIsset: false

‎conf/parametersSchema.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ parametersSchema:
6161
staticVariablesFromUsages: bool()
6262
unusedVariable: bool()
6363
unusedParameters: bool()
64+
reportAbstractMethodPurity: bool()
6465
])
6566
fileExtensions: listOf(string())
6667
checkAdvancedIsset: bool()

‎src/Rules/Pure/FunctionPurityCheck.php‎

Lines changed: 116 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use PHPStan\Rules\Functions\CallToFunctionStatementWithoutSideEffectsRule;
1717
use PHPStan\Rules\IdentifierRuleError;
1818
use PHPStan\Rules\RuleErrorBuilder;
19+
use PHPStan\TrinaryLogic;
1920
use PHPStan\Type\Type;
2021
use function array_filter;
2122
use function array_key_exists;
@@ -51,18 +52,111 @@ public function check(
5152
bool $isConstructor,
5253
): array
5354
{
54-
$errors = [];
55+
$errors = $this->checkSignature($scope, $functionDescription, $identifier, $functionReflection, $parameters, $returnType, $isConstructor);
5556
$isPure = $functionReflection->isPure();
57+
$pureUnlessCallableParamNames = $this->collectFlaggedParameterNames($parameters, $functionReflection->getPureUnlessCallableIsImpureParameters());
58+
$pureUnlessParameterPassedParamNames = $this->collectFlaggedParameterNames($parameters, $functionReflection->getPureUnlessParameterPassedParameters());
59+
60+
if ($isPure->yes()) {
61+
$errors = array_merge($errors, $this->reportImpurePoints($impurePoints, $pureUnlessCallableParamNames, $functionDescription));
62+
} elseif ($pureUnlessCallableParamNames !== [] || $pureUnlessParameterPassedParamNames !== []) {
63+
// A function declared @pure-unless-callable-is-impure is pure except
64+
// for the flagged callables, so its body is checked for purity while
65+
// the flagged callables' own invocations are exempt. The same holds for
66+
// @pure-unless-parameter-passed: writing through the flagged by-ref
67+
// parameter is not an impure point, so the rest of the body still has
68+
// to be pure for the conditional verdict to hold.
69+
$errors = array_merge($errors, $this->reportImpurePoints($impurePoints, $pureUnlessCallableParamNames, $functionDescription));
70+
} elseif ($isPure->no()) {
71+
if (
72+
count($throwPoints) === 0
73+
&& count($impurePoints) === 0
74+
&& count($functionReflection->getAsserts()->getAll()) === 0
75+
&& (
76+
!$functionReflection instanceof ExtendedMethodReflection
77+
|| $functionReflection->isFinal()->yes()
78+
|| $functionReflection->getDeclaringClass()->isFinal()
79+
)
80+
) {
81+
$errors[] = RuleErrorBuilder::message(sprintf(
82+
'%s is marked as impure but does not have any side effects.',
83+
$functionDescription,
84+
))->identifier(sprintf('impure%s.pure', $identifier))->build();
85+
}
86+
} elseif ($returnType->isVoid()->yes()) {
87+
if (
88+
count($throwPoints) === 0
89+
&& count($impurePoints) === 0
90+
&& !$isConstructor
91+
&& (!$functionReflection instanceof ExtendedMethodReflection || $functionReflection->isPrivate())
92+
&& count($functionReflection->getAsserts()->getAll()) === 0
93+
) {
94+
$hasByRef = false;
95+
foreach ($parameters as $parameter) {
96+
if (!$parameter->passedByReference()->createsNewVariable()) {
97+
continue;
98+
}
99+
100+
$hasByRef = true;
101+
break;
102+
}
103+
104+
$statements = array_filter($statements, static function (Stmt $stmt): bool {
105+
if ($stmt instanceof Stmt\Nop) {
106+
return false;
107+
}
108+
109+
if (!$stmt instanceof Stmt\Expression) {
110+
return true;
111+
}
112+
if (!$stmt->expr instanceof FuncCall) {
113+
return true;
114+
}
115+
if (!$stmt->expr->name instanceof Name) {
116+
return true;
117+
}
118+
119+
return !in_array($stmt->expr->name->toString(), CallToFunctionStatementWithoutSideEffectsRule::PHPSTAN_TESTING_FUNCTIONS, true);
120+
});
121+
122+
if (!$hasByRef && count($statements) > 0) {
123+
$errors[] = RuleErrorBuilder::message(sprintf(
124+
'%s returns void but does not have any side effects.',
125+
$functionDescription,
126+
))->identifier('void.pure')->build();
127+
}
128+
}
129+
}
130+
131+
return $errors;
132+
}
133+
134+
/**
135+
* The checks of a purity declaration that need only the signature, so they
136+
* also apply to methods without a body.
137+
*
138+
* @param 'Function'|'Method'|'PropertyHook' $identifier
139+
* @param ExtendedParameterReflection[] $parameters
140+
* @return list<IdentifierRuleError>
141+
*/
142+
public function checkSignature(
143+
Scope $scope,
144+
string $functionDescription,
145+
string $identifier,
146+
FunctionReflection|ExtendedMethodReflection $functionReflection,
147+
array $parameters,
148+
Type $returnType,
149+
bool $isConstructor,
150+
): array
151+
{
152+
$errors = [];
56153

57154
$pureUnlessCallableParameters = $functionReflection->getPureUnlessCallableIsImpureParameters();
58-
$pureUnlessCallableParamNames = [];
59155
foreach ($parameters as $parameter) {
60156
if (!array_key_exists($parameter->getName(), $pureUnlessCallableParameters)) {
61157
continue;
62158
}
63159

64-
$pureUnlessCallableParamNames[$parameter->getName()] = true;
65-
66160
$acceptors = $parameter->getType()->getCallableParametersAcceptors($scope);
67161
if (count($acceptors) === 0) {
68162
continue;
@@ -92,14 +186,11 @@ public function check(
92186
}
93187

94188
$pureUnlessParameterPassedParameters = $functionReflection->getPureUnlessParameterPassedParameters();
95-
$pureUnlessParameterPassedParamNames = [];
96189
foreach ($parameters as $parameter) {
97190
if (!array_key_exists($parameter->getName(), $pureUnlessParameterPassedParameters)) {
98191
continue;
99192
}
100193

101-
$pureUnlessParameterPassedParamNames[$parameter->getName()] = true;
102-
103194
if (!$parameter->isOptional()) {
104195
$errors[] = RuleErrorBuilder::message(sprintf(
105196
'%s is marked @pure-unless-parameter-passed for parameter $%s, but $%s is not optional, so %s is never pure.',
@@ -127,7 +218,7 @@ public function check(
127218
))->identifier(sprintf('pure%s.parameterPassedNotByRef', $identifier))->build();
128219
}
129220

130-
if ($isPure->yes()) {
221+
if ($functionReflection->isPure()->yes()) {
131222
foreach ($parameters as $parameter) {
132223
if (!$parameter->passedByReference()->createsNewVariable()) {
133224
continue;
@@ -152,78 +243,28 @@ public function check(
152243
$functionDescription,
153244
))->identifier(sprintf('pure%s.void', $identifier))->build();
154245
}
246+
}
155247

156-
$errors = array_merge($errors, $this->reportImpurePoints($impurePoints, $pureUnlessCallableParamNames, $functionDescription));
157-
} elseif ($pureUnlessCallableParamNames !== [] || $pureUnlessParameterPassedParamNames !== []) {
158-
// A function declared @pure-unless-callable-is-impure is pure except
159-
// for the flagged callables, so its body is checked for purity while
160-
// the flagged callables' own invocations are exempt. The same holds for
161-
// @pure-unless-parameter-passed: writing through the flagged by-ref
162-
// parameter is not an impure point, so the rest of the body still has
163-
// to be pure for the conditional verdict to hold.
164-
$errors = array_merge($errors, $this->reportImpurePoints($impurePoints, $pureUnlessCallableParamNames, $functionDescription));
165-
} elseif ($isPure->no()) {
166-
if (
167-
count($throwPoints) === 0
168-
&& count($impurePoints) === 0
169-
&& count($functionReflection->getAsserts()->getAll()) === 0
170-
&& (
171-
!$functionReflection instanceof ExtendedMethodReflection
172-
|| $functionReflection->isFinal()->yes()
173-
|| $functionReflection->getDeclaringClass()->isFinal()
174-
)
175-
) {
176-
$errors[] = RuleErrorBuilder::message(sprintf(
177-
'%s is marked as impure but does not have any side effects.',
178-
$functionDescription,
179-
))->identifier(sprintf('impure%s.pure', $identifier))->build();
180-
}
181-
} elseif ($returnType->isVoid()->yes()) {
182-
if (
183-
count($throwPoints) === 0
184-
&& count($impurePoints) === 0
185-
&& !$isConstructor
186-
&& (!$functionReflection instanceof ExtendedMethodReflection || $functionReflection->isPrivate())
187-
&& count($functionReflection->getAsserts()->getAll()) === 0
188-
) {
189-
$hasByRef = false;
190-
foreach ($parameters as $parameter) {
191-
if (!$parameter->passedByReference()->createsNewVariable()) {
192-
continue;
193-
}
194-
195-
$hasByRef = true;
196-
break;
197-
}
198-
199-
$statements = array_filter($statements, static function (Stmt $stmt): bool {
200-
if ($stmt instanceof Stmt\Nop) {
201-
return false;
202-
}
203-
204-
if (!$stmt instanceof Stmt\Expression) {
205-
return true;
206-
}
207-
if (!$stmt->expr instanceof FuncCall) {
208-
return true;
209-
}
210-
if (!$stmt->expr->name instanceof Name) {
211-
return true;
212-
}
213-
214-
return !in_array($stmt->expr->name->toString(), CallToFunctionStatementWithoutSideEffectsRule::PHPSTAN_TESTING_FUNCTIONS, true);
215-
});
248+
return $errors;
249+
}
216250

217-
if (!$hasByRef && count($statements) > 0) {
218-
$errors[] = RuleErrorBuilder::message(sprintf(
219-
'%s returns void but does not have any side effects.',
220-
$functionDescription,
221-
))->identifier('void.pure')->build();
222-
}
251+
/**
252+
* @param ExtendedParameterReflection[] $parameters
253+
* @param array<string, TrinaryLogic> $flaggedParameters
254+
* @return array<string, true>
255+
*/
256+
private function collectFlaggedParameterNames(array $parameters, array $flaggedParameters): array
257+
{
258+
$names = [];
259+
foreach ($parameters as $parameter) {
260+
if (!array_key_exists($parameter->getName(), $flaggedParameters)) {
261+
continue;
223262
}
263+
264+
$names[$parameter->getName()] = true;
224265
}
225266

226-
return $errors;
267+
return $names;
227268
}
228269

229270
/**

‎src/Rules/Pure/PureAbstractMethodRule.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Rules\Pure;
4+
5+
use PhpParser\Node;
6+
use PHPStan\Analyser\Scope;
7+
use PHPStan\Node\InClassMethodNode;
8+
use PHPStan\Rules\Rule;
9+
use function sprintf;
10+
11+
/**
12+
* Applies the signature checks of a purity declaration to methods without a
13+
* body (abstract and interface methods), which PureMethodRule never sees.
14+
*
15+
* @implements Rule<InClassMethodNode>
16+
*/
17+
final class PureAbstractMethodRule implements Rule
18+
{
19+
20+
public function __construct(private FunctionPurityCheck $check)
21+
{
22+
}
23+
24+
public function getNodeType(): string
25+
{
26+
return InClassMethodNode::class;
27+
}
28+
29+
public function processNode(Node $node, Scope $scope): array
30+
{
31+
if ($node->getOriginalNode()->stmts !== null) {
32+
return [];
33+
}
34+
35+
$method = $node->getMethodReflection();
36+
37+
return $this->check->checkSignature(
38+
$scope,
39+
sprintf('Method %s::%s()', $method->getDeclaringClass()->getDisplayName(), $method->getName()),
40+
'Method',
41+
$method,
42+
$method->getParameters(),
43+
$method->getReturnType(),
44+
$method->isConstructor(),
45+
);
46+
}
47+
48+
}

‎tests/PHPStan/Rules/Pure/PureAbstractMethodRuleTest.php‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Rules\Pure;
4+
5+
use PHPStan\Rules\Rule;
6+
use PHPStan\Testing\RuleTestCase;
7+
8+
/**
9+
* @extends RuleTestCase<PureAbstractMethodRule>
10+
*/
11+
class PureAbstractMethodRuleTest extends RuleTestCase
12+
{
13+
14+
public function getRule(): Rule
15+
{
16+
return new PureAbstractMethodRule(new FunctionPurityCheck());
17+
}
18+
19+
public function testRule(): void
20+
{
21+
$this->analyse([__DIR__ . '/data/pure-abstract-method.php'], [
22+
[
23+
'Method PureAbstractMethod\Foo::pureByRef() is marked as pure but parameter $p is passed by reference.',
24+
11,
25+
],
26+
[
27+
'Method PureAbstractMethod\Foo::pureVoid() is marked as pure but returns void.',
28+
16,
29+
],
30+
[
31+
'Method PureAbstractMethod\Foo::requiredCount() is marked @pure-unless-parameter-passed for parameter $count, but $count is not optional, so method PureAbstractMethod\Foo::requiredCount() is never pure.',
32+
33,
33+
],
34+
[
35+
'Method PureAbstractMethod\Foo::byValueFlag() is marked @pure-unless-parameter-passed for parameter $flag, but $flag is not passed by reference.',
36+
38,
37+
],
38+
[
39+
'Method PureAbstractMethod\Foo::alreadyPureCallback() is marked @pure-unless-callable-is-impure for parameter $cb, but $cb is already a pure callable, so method PureAbstractMethod\Foo::alreadyPureCallback() can be marked @phpstan-pure instead.',
40+
50,
41+
],
42+
[
43+
'Method PureAbstractMethod\Bar::pureByRef() is marked as pure but parameter $p is passed by reference.',
44+
66,
45+
],
46+
[
47+
'Method PureAbstractMethod\Bar::requiredCount() is marked @pure-unless-parameter-passed for parameter $count, but $count is not optional, so method PureAbstractMethod\Bar::requiredCount() is never pure.',
48+
72,
49+
],
50+
]);
51+
}
52+
53+
}

0 commit comments

Comments
 (0)