Skip to content

Commit 8cc6e7e

Browse files
ondrejmirtesondrejmirtes
committed
empty()/isset()/null-coalesce rules - report variables issues on level 1, the rest on level 4
1 parent 9689fbd commit 8cc6e7e

File tree

10 files changed

+184
-31
lines changed

10 files changed

+184
-31
lines changed

‎conf/config.level1.neon‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,27 @@ rules:
1212
- PHPStan\Rules\Constants\ConstantRule
1313
- PHPStan\Rules\Functions\UnusedClosureUsesRule
1414

15+
conditionalTags:
16+
PHPStan\Rules\Variables\EmptyRule:
17+
phpstan.rules.rule: %featureToggles.nullCoalesce%
18+
PHPStan\Rules\Variables\IssetRule:
19+
phpstan.rules.rule: %featureToggles.nullCoalesce%
20+
PHPStan\Rules\Variables\NullCoalesceRule:
21+
phpstan.rules.rule: %featureToggles.nullCoalesce%
22+
1523
services:
1624
-
1725
class: PHPStan\Rules\Variables\VariableCertaintyInIssetRule
1826
arguments:
1927
bleedingEdge: %featureToggles.bleedingEdge%
2028
tags:
2129
- phpstan.rules.rule
30+
31+
-
32+
class: PHPStan\Rules\Variables\EmptyRule
33+
34+
-
35+
class: PHPStan\Rules\Variables\IssetRule
36+
37+
-
38+
class: PHPStan\Rules\Variables\NullCoalesceRule

‎conf/config.level4.neon‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,9 @@ conditionalTags:
2828
phpstan.rules.rule: %featureToggles.unusedClassElements%
2929
PHPStan\Rules\DeadCode\UnusedPrivatePropertyRule:
3030
phpstan.rules.rule: %featureToggles.unusedClassElements%
31-
PHPStan\Rules\Variables\EmptyRule:
32-
phpstan.rules.rule: %featureToggles.nullCoalesce%
33-
PHPStan\Rules\Variables\IssetRule:
34-
phpstan.rules.rule: %featureToggles.nullCoalesce%
35-
PHPStan\Rules\Variables\NullCoalesceRule:
36-
phpstan.rules.rule: %featureToggles.nullCoalesce%
31+
32+
parameters:
33+
checkAdvancedIsset: true
3734

3835
services:
3936
-
@@ -165,12 +162,3 @@ services:
165162
checkProtectedAndPublicMethods: %checkTooWideReturnTypesInProtectedAndPublicMethods%
166163
tags:
167164
- phpstan.rules.rule
168-
169-
-
170-
class: PHPStan\Rules\Variables\EmptyRule
171-
172-
-
173-
class: PHPStan\Rules\Variables\IssetRule
174-
175-
-
176-
class: PHPStan\Rules\Variables\NullCoalesceRule

‎conf/config.neon‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ parameters:
5454
overridingProperty: false
5555
fileExtensions:
5656
- php
57+
checkAdvancedIsset: false
5758
checkAlwaysTrueCheckTypeFunctionCall: false
5859
checkAlwaysTrueInstanceof: false
5960
checkAlwaysTrueStrictComparison: false
@@ -239,6 +240,7 @@ parametersSchema:
239240
overridingProperty: bool()
240241
])
241242
fileExtensions: listOf(string())
243+
checkAdvancedIsset: bool()
242244
checkAlwaysTrueCheckTypeFunctionCall: bool()
243245
checkAlwaysTrueInstanceof: bool()
244246
checkAlwaysTrueStrictComparison: bool()
@@ -861,6 +863,7 @@ services:
861863
class: PHPStan\Rules\IssetCheck
862864
arguments:
863865
bleedingEdge: %featureToggles.bleedingEdge%
866+
checkAdvancedIsset: %checkAdvancedIsset%
864867

865868
-
866869
# checked as part of OverridingMethodRule

‎src/Rules/IssetCheck.php‎

Lines changed: 115 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,20 @@ class IssetCheck
1818

1919
private \PHPStan\Rules\Properties\PropertyReflectionFinder $propertyReflectionFinder;
2020

21+
private bool $checkAdvancedIsset;
22+
2123
private bool $bleedingEdge;
2224

2325
public function __construct(
2426
PropertyDescriptor $propertyDescriptor,
2527
PropertyReflectionFinder $propertyReflectionFinder,
26-
bool $bleedingEdge = false
28+
bool $checkAdvancedIsset,
29+
bool $bleedingEdge
2730
)
2831
{
2932
$this->propertyDescriptor = $propertyDescriptor;
3033
$this->propertyReflectionFinder = $propertyReflectionFinder;
34+
$this->checkAdvancedIsset = $checkAdvancedIsset;
3135
$this->bleedingEdge = $bleedingEdge;
3236
}
3337

@@ -68,11 +72,19 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, cal
6872
$dimType = $scope->getType($expr->dim);
6973
$hasOffsetValue = $type->hasOffsetValueType($dimType);
7074
if (!$type->isOffsetAccessible()->yes()) {
71-
return $error;
75+
return $error ?? $this->checkUndefined($expr->var, $scope, $operatorDescription);
7276
}
7377

7478
if ($hasOffsetValue->no()) {
75-
return $error ?? RuleErrorBuilder::message(
79+
if ($error !== null) {
80+
return $error;
81+
}
82+
83+
if (!$this->checkAdvancedIsset) {
84+
return null;
85+
}
86+
87+
return RuleErrorBuilder::message(
7688
sprintf(
7789
'Offset %s on %s %s does not exist.',
7890
$dimType->describe(VerbosityLevel::value()),
@@ -89,8 +101,15 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, cal
89101
// If offset is cannot be null, store this error message and see if one of the earlier offsets is.
90102
// E.g. $array['a']['b']['c'] ?? null; is a valid coalesce if a OR b or C might be null.
91103
if ($hasOffsetValue->yes()) {
104+
if ($error !== null) {
105+
return $error;
106+
}
92107

93-
$error = $error ?? $this->generateError($type->getOffsetValueType($dimType), sprintf(
108+
if (!$this->checkAdvancedIsset) {
109+
return null;
110+
}
111+
112+
$error = $this->generateError($type->getOffsetValueType($dimType), sprintf(
94113
'Offset %s on %s %s always exists and',
95114
$dimType->describe(VerbosityLevel::value()),
96115
$type->describe(VerbosityLevel::value()),
@@ -110,24 +129,62 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, cal
110129
$propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($expr, $scope);
111130

112131
if ($propertyReflection === null) {
132+
if ($expr instanceof Node\Expr\PropertyFetch) {
133+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
134+
}
135+
136+
if ($expr->class instanceof Expr) {
137+
return $this->checkUndefined($expr->class, $scope, $operatorDescription);
138+
}
139+
113140
return null;
114141
}
115142

116143
if (!$propertyReflection->isNative()) {
144+
if ($expr instanceof Node\Expr\PropertyFetch) {
145+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
146+
}
147+
148+
if ($expr->class instanceof Expr) {
149+
return $this->checkUndefined($expr->class, $scope, $operatorDescription);
150+
}
151+
117152
return null;
118153
}
119154

120155
$nativeType = $propertyReflection->getNativeType();
121156
if (!$nativeType instanceof MixedType) {
122157
if (!$scope->isSpecified($expr)) {
158+
if ($expr instanceof Node\Expr\PropertyFetch) {
159+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
160+
}
161+
162+
if ($expr->class instanceof Expr) {
163+
return $this->checkUndefined($expr->class, $scope, $operatorDescription);
164+
}
165+
123166
return null;
124167
}
125168
}
126169

127170
$propertyDescription = $this->propertyDescriptor->describeProperty($propertyReflection, $expr);
128171
$propertyType = $propertyReflection->getWritableType();
172+
if ($error !== null) {
173+
return $error;
174+
}
175+
if (!$this->checkAdvancedIsset) {
176+
if ($expr instanceof Node\Expr\PropertyFetch) {
177+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
178+
}
179+
180+
if ($expr->class instanceof Expr) {
181+
return $this->checkUndefined($expr->class, $scope, $operatorDescription);
182+
}
129183

130-
$error = $error ?? $this->generateError(
184+
return null;
185+
}
186+
187+
$error = $this->generateError(
131188
$propertyReflection->getWritableType(),
132189
sprintf('%s (%s) %s', $propertyDescription, $propertyType->describe(VerbosityLevel::typeOnly()), $operatorDescription),
133190
$typeMessageCallback
@@ -146,7 +203,59 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, cal
146203
return $error;
147204
}
148205

149-
return $error ?? $this->generateError($scope->getType($expr), sprintf('Expression %s', $operatorDescription), $typeMessageCallback);
206+
if ($error !== null) {
207+
return $error;
208+
}
209+
210+
if (!$this->checkAdvancedIsset) {
211+
return null;
212+
}
213+
214+
return $this->generateError($scope->getType($expr), sprintf('Expression %s', $operatorDescription), $typeMessageCallback);
215+
}
216+
217+
private function checkUndefined(Expr $expr, Scope $scope, string $operatorDescription): ?RuleError
218+
{
219+
if ($expr instanceof Node\Expr\Variable && is_string($expr->name)) {
220+
$hasVariable = $scope->hasVariableType($expr->name);
221+
if (!$hasVariable->no()) {
222+
return null;
223+
}
224+
225+
return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $expr->name, $operatorDescription))->build();
226+
}
227+
228+
if ($expr instanceof Node\Expr\ArrayDimFetch && $expr->dim !== null) {
229+
$type = $scope->getType($expr->var);
230+
$dimType = $scope->getType($expr->dim);
231+
$hasOffsetValue = $type->hasOffsetValueType($dimType);
232+
if (!$type->isOffsetAccessible()->yes()) {
233+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
234+
}
235+
236+
if (!$hasOffsetValue->no()) {
237+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
238+
}
239+
240+
return RuleErrorBuilder::message(
241+
sprintf(
242+
'Offset %s on %s %s does not exist.',
243+
$dimType->describe(VerbosityLevel::value()),
244+
$type->describe(VerbosityLevel::value()),
245+
$operatorDescription
246+
)
247+
)->build();
248+
}
249+
250+
if ($expr instanceof Expr\PropertyFetch) {
251+
return $this->checkUndefined($expr->var, $scope, $operatorDescription);
252+
}
253+
254+
if ($expr instanceof Expr\StaticPropertyFetch && $expr->class instanceof Expr) {
255+
return $this->checkUndefined($expr->class, $scope, $operatorDescription);
256+
}
257+
258+
return null;
150259
}
151260

152261
/**

‎tests/PHPStan/Levels/data/coalesce-1.json‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,5 +8,15 @@
88
"message": "Variable $bar on left side of ?? is never defined.",
99
"line": 6,
1010
"ignorable": true
11+
},
12+
{
13+
"message": "Variable $a on left side of ?? is never defined.",
14+
"line": 15,
15+
"ignorable": true
16+
},
17+
{
18+
"message": "Variable $s on left side of ?? always exists and is always null.",
19+
"line": 23,
20+
"ignorable": true
1121
}
12-
]
22+
]

‎tests/PHPStan/Rules/Variables/EmptyRuleTest.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ class EmptyRuleTest extends RuleTestCase
1515

1616
protected function getRule(): \PHPStan\Rules\Rule
1717
{
18-
return new EmptyRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder(), true));
18+
return new EmptyRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder(), true, true));
1919
}
2020

2121
public function testRule(): void
@@ -60,8 +60,8 @@ public function testBug970(): void
6060
{
6161
$this->analyse([__DIR__ . '/data/bug-970.php'], [
6262
[
63-
'aaa',
64-
10,
63+
'Variable $ar in empty() is never defined.',
64+
9,
6565
],
6666
]);
6767
}

‎tests/PHPStan/Rules/Variables/IssetRuleTest.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ class IssetRuleTest extends RuleTestCase
1616

1717
protected function getRule(): Rule
1818
{
19-
return new IssetRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder(), true));
19+
return new IssetRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder(), true, true));
2020
}
2121

2222
public function testRule(): void
@@ -46,6 +46,10 @@ public function testRule(): void
4646
'Offset \'dim\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) in isset() always exists and is not nullable.',
4747
67,
4848
],
49+
[
50+
'Offset \'dim-null-not-set\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) in isset() does not exist.',
51+
73,
52+
],
4953
[
5054
'Offset \'b\' on array() in isset() does not exist.',
5155
79,
@@ -166,7 +170,7 @@ public function testVariableCertaintyInIsset(): void
166170
116,
167171
],
168172
[
169-
'Variable $variableInSecondCase in isset() always exists and is not nullable.',
173+
'Variable $variableInSecondCase in isset() always exists and is always null.',
170174
117,
171175
],
172176
[

‎tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ class NullCoalesceRuleTest extends \PHPStan\Testing\RuleTestCase
1414

1515
protected function getRule(): \PHPStan\Rules\Rule
1616
{
17-
return new NullCoalesceRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder(), true));
17+
return new NullCoalesceRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder(), true, true));
1818
}
1919

2020
public function testCoalesceRule(): void
@@ -44,6 +44,10 @@ public function testCoalesceRule(): void
4444
'Offset \'dim\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) on left side of ?? always exists and is not nullable.',
4545
67,
4646
],
47+
[
48+
'Offset \'dim-null-not-set\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) on left side of ?? does not exist.',
49+
73,
50+
],
4751
[
4852
'Offset \'b\' on array() on left side of ?? does not exist.',
4953
79,
@@ -104,6 +108,14 @@ public function testCoalesceRule(): void
104108
'Property ReflectionClass<object>::$name (class-string<object>) on left side of ?? is not nullable.',
105109
136,
106110
],
111+
[
112+
'Variable $foo on left side of ?? is never defined.',
113+
141,
114+
],
115+
[
116+
'Variable $bar on left side of ?? is never defined.',
117+
143,
118+
],
107119
]);
108120
}
109121

@@ -138,6 +150,10 @@ public function testCoalesceAssignRule(): void
138150
'Offset \'dim\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) on left side of ??= always exists and is not nullable.',
139151
67,
140152
],
153+
[
154+
'Offset \'dim-null-not-set\' on array(\'dim\' => 1, \'dim-null\' => 0|1, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) on left side of ??= does not exist.',
155+
73,
156+
],
141157
[
142158
'Offset \'b\' on array() on left side of ??= does not exist.',
143159
79,

‎tests/PHPStan/Rules/Variables/data/null-coalesce.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,3 +136,9 @@ function (\ReflectionClass $ref): void {
136136
echo $ref->name ?? 'foo';
137137
echo $ref->nonexistent ?? 'bar';
138138
};
139+
140+
function (): void {
141+
echo $foo ?? 'foo';
142+
143+
echo $bar->bar ?? 'foo';
144+
};

0 commit comments

Comments
 (0)