Skip to content

Commit 8ace7ea

Browse files
janedbaljanedbal
authored
ForbidUnsafeArrayKeyRule (#254)
1 parent e517483 commit 8ace7ea

File tree

8 files changed

+316
-2
lines changed

8 files changed

+316
-2
lines changed

‎README.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ parameters:
8686
forbidReturnValueInYieldingMethod:
8787
enabled: true
8888
reportRegardlessOfReturnType: true
89+
forbidUnsafeArrayKey:
90+
enabled: true
91+
reportMixed: true
92+
reportInsideIsset: true
8993
forbidVariableTypeOverwriting:
9094
enabled: true
9195
forbidUnsetClassField:
@@ -610,6 +614,31 @@ parameters:
610614
reportRegardlessOfReturnType: true # optional stricter mode, defaults to false
611615
```
612616

617+
618+
### forbidUnsafeArrayKey
619+
- Denies non-int non-string array keys
620+
- PHP casts `null`, `float` and `bool` to some nearest int/string
621+
- You should rather do that intentionally and explicitly
622+
- Those types are the main difference to default PHPStan behaviour which allows using them as array keys
623+
- You can exclude reporting `mixed` keys via `reportMixed` configuration
624+
- You can exclude reporting `isset($array[$invalid])` and `$array[$invalid] ?? null` via `reportInsideIsset` configuration
625+
626+
```php
627+
$taxRates = [ // denied, float key gets casted to int (causing $taxRates to contain one item)
628+
1.15 => 'reduced',
629+
1.21 => 'standard',
630+
];
631+
```
632+
633+
```neon
634+
parameters:
635+
shipmonkRules:
636+
forbidUnsafeArrayKey:
637+
reportMixed: false # defaults to true
638+
reportInsideIsset: false # defaults to true
639+
```
640+
641+
613642
### forbidVariableTypeOverwriting
614643
- Restricts variable assignment to those that does not change its type
615644
- Array append `$array[] = 1;` not yet supported

‎rules.neon

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,10 @@ parameters:
6565
forbidReturnValueInYieldingMethod:
6666
enabled: true
6767
reportRegardlessOfReturnType: true
68+
forbidUnsafeArrayKey:
69+
enabled: true
70+
reportMixed: true
71+
reportInsideIsset: true
6872
forbidVariableTypeOverwriting:
6973
enabled: true
7074
forbidUnsetClassField:
@@ -177,6 +181,11 @@ parametersSchema:
177181
enabled: bool()
178182
reportRegardlessOfReturnType: bool()
179183
])
184+
forbidUnsafeArrayKey: structure([
185+
enabled: bool()
186+
reportMixed: bool()
187+
reportInsideIsset: bool()
188+
])
180189
forbidVariableTypeOverwriting: structure([
181190
enabled: bool()
182191
])
@@ -259,6 +268,8 @@ conditionalTags:
259268
phpstan.rules.rule: %shipmonkRules.forbidProtectedEnumMethod.enabled%
260269
ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule:
261270
phpstan.rules.rule: %shipmonkRules.forbidReturnValueInYieldingMethod.enabled%
271+
ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule:
272+
phpstan.rules.rule: %shipmonkRules.forbidUnsafeArrayKey.enabled%
262273
ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule:
263274
phpstan.rules.rule: %shipmonkRules.forbidVariableTypeOverwriting.enabled%
264275
ShipMonk\PHPStan\Rule\ForbidUnsetClassFieldRule:
@@ -369,6 +380,11 @@ services:
369380
class: ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule
370381
arguments:
371382
reportRegardlessOfReturnType: %shipmonkRules.forbidReturnValueInYieldingMethod.reportRegardlessOfReturnType%
383+
-
384+
class: ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule
385+
arguments:
386+
reportMixed: %shipmonkRules.forbidUnsafeArrayKey.reportMixed%
387+
reportInsideIsset: %shipmonkRules.forbidUnsafeArrayKey.reportInsideIsset%
372388
-
373389
class: ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule
374390
-

‎src/Rule/ForbidCheckedExceptionInCallableRule.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -469,7 +469,7 @@ private function whitelistAllowedCallables(CallLike $node, Scope $scope): void
469469
$parameters = $parametersAcceptor->getParameters();
470470

471471
foreach ($args as $index => $arg) {
472-
$parameterIndex = $this->getParameterIndex($arg, $index, $parameters);
472+
$parameterIndex = $this->getParameterIndex($arg, $index, $parameters) ?? -1;
473473
$parameter = $parameters[$parameterIndex] ?? null;
474474
$argHash = spl_object_hash($arg->value);
475475

‎src/Rule/ForbidUnsafeArrayKeyRule.php

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace ShipMonk\PHPStan\Rule;
4+
5+
use PhpParser\Node;
6+
use PhpParser\Node\Expr\ArrayDimFetch;
7+
use PhpParser\Node\Expr\ArrayItem;
8+
use PHPStan\Analyser\Scope;
9+
use PHPStan\Rules\IdentifierRuleError;
10+
use PHPStan\Rules\Rule;
11+
use PHPStan\Rules\RuleErrorBuilder;
12+
use PHPStan\Type\IntegerType;
13+
use PHPStan\Type\MixedType;
14+
use PHPStan\Type\StringType;
15+
use PHPStan\Type\Type;
16+
use PHPStan\Type\TypeCombinator;
17+
use PHPStan\Type\VerbosityLevel;
18+
19+
/**
20+
* @implements Rule<Node>
21+
*/
22+
class ForbidUnsafeArrayKeyRule implements Rule
23+
{
24+
25+
private bool $reportMixed;
26+
27+
private bool $reportInsideIsset;
28+
29+
public function __construct(
30+
bool $reportMixed,
31+
bool $reportInsideIsset
32+
)
33+
{
34+
$this->reportMixed = $reportMixed;
35+
$this->reportInsideIsset = $reportInsideIsset;
36+
}
37+
38+
public function getNodeType(): string
39+
{
40+
return Node::class;
41+
}
42+
43+
/**
44+
* @return list<IdentifierRuleError>
45+
*/
46+
public function processNode(
47+
Node $node,
48+
Scope $scope
49+
): array
50+
{
51+
if ($node instanceof ArrayItem && $node->key !== null) {
52+
$keyType = $scope->getType($node->key);
53+
54+
if (!$this->isArrayKey($keyType)) {
55+
return [
56+
RuleErrorBuilder::message('Array key must be integer or string, but ' . $keyType->describe(VerbosityLevel::precise()) . ' given.')
57+
->identifier('shipmonk.unsafeArrayKey')
58+
->build(),
59+
];
60+
}
61+
}
62+
63+
if (
64+
$node instanceof ArrayDimFetch
65+
&& $node->dim !== null
66+
&& !$scope->getType($node->var)->isArray()->no()
67+
) {
68+
if (!$this->reportInsideIsset && $scope->isUndefinedExpressionAllowed($node)) {
69+
return [];
70+
}
71+
72+
$dimType = $scope->getType($node->dim);
73+
74+
if (!$this->isArrayKey($dimType)) {
75+
return [
76+
RuleErrorBuilder::message('Array key must be integer or string, but ' . $dimType->describe(VerbosityLevel::precise()) . ' given.')
77+
->identifier('shipmonk.unsafeArrayKey')
78+
->build(),
79+
];
80+
}
81+
}
82+
83+
return [];
84+
}
85+
86+
private function isArrayKey(Type $type): bool
87+
{
88+
if (!$this->reportMixed && $type instanceof MixedType) {
89+
return true;
90+
}
91+
92+
return $this->containsOnlyTypes($type, [new StringType(), new IntegerType()]);
93+
}
94+
95+
/**
96+
* @param list<Type> $allowedTypes
97+
*/
98+
private function containsOnlyTypes(
99+
Type $checkedType,
100+
array $allowedTypes
101+
): bool
102+
{
103+
$allowedType = TypeCombinator::union(...$allowedTypes);
104+
return $allowedType->isSuperTypeOf($checkedType)->yes();
105+
}
106+
107+
}

‎tests/Rule/ForbidUnsafeArrayKeyRuleTest.php

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Rule;
4+
5+
use LogicException;
6+
use PHPStan\Rules\Rule;
7+
use ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule;
8+
use ShipMonk\PHPStan\RuleTestCase;
9+
10+
/**
11+
* @extends RuleTestCase<ForbidUnsafeArrayKeyRule>
12+
*/
13+
class ForbidUnsafeArrayKeyRuleTest extends RuleTestCase
14+
{
15+
16+
private ?bool $checkMixed = null;
17+
18+
private ?bool $checkInsideIsset = null;
19+
20+
protected function getRule(): Rule
21+
{
22+
if ($this->checkMixed === null) {
23+
throw new LogicException('Property checkMixed must be set');
24+
}
25+
26+
if ($this->checkInsideIsset === null) {
27+
throw new LogicException('Property checkInsideIsset must be set');
28+
}
29+
30+
return new ForbidUnsafeArrayKeyRule(
31+
$this->checkMixed,
32+
$this->checkInsideIsset,
33+
);
34+
}
35+
36+
public function testStrict(): void
37+
{
38+
$this->checkMixed = true;
39+
$this->checkInsideIsset = true;
40+
$this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/default.php');
41+
}
42+
43+
public function testLessStrict(): void
44+
{
45+
$this->checkMixed = false;
46+
$this->checkInsideIsset = false;
47+
$this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/less-strict.php');
48+
}
49+
50+
}

‎tests/Rule/data/ForbidUnsafeArrayKeyRule/default.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 ForbidUnsafeArrayKeyRule\MixedDisabled;
4+
5+
6+
class ArrayKey
7+
{
8+
9+
/**
10+
* @param array-key $arrayKey
11+
*/
12+
public function test(
13+
int $int,
14+
string $string,
15+
float $float,
16+
$arrayKey,
17+
int|string $intOrString,
18+
int|float $intOrFloat,
19+
array $array,
20+
object $object,
21+
\WeakMap $weakMap,
22+
mixed $explicitMixed,
23+
$implicitMixed
24+
) {
25+
$array[$array] = ''; // error: Array key must be integer or string, but array given.
26+
$array[$int] = '';
27+
$array[$string] = '';
28+
$array[$float] = ''; // error: Array key must be integer or string, but float given.
29+
$array[$intOrFloat] = ''; // error: Array key must be integer or string, but float|int given.
30+
$array[$intOrString] = '';
31+
$array[$explicitMixed] = ''; // error: Array key must be integer or string, but mixed given.
32+
$array[$implicitMixed] = ''; // error: Array key must be integer or string, but mixed given.
33+
$array[$arrayKey] = '';
34+
$weakMap[$object] = '';
35+
36+
$a = $array[$float] ?? ''; // error: Array key must be integer or string, but float given.
37+
$b = isset($array[$float]); // error: Array key must be integer or string, but float given.
38+
$c = empty($array[$float]); // error: Array key must be integer or string, but float given.
39+
40+
[
41+
$int => $int,
42+
$string => $string,
43+
$float => $float, // error: Array key must be integer or string, but float given.
44+
$intOrFloat => $intOrFloat, // error: Array key must be integer or string, but float|int given.
45+
$intOrString => $intOrString,
46+
$explicitMixed => $explicitMixed, // error: Array key must be integer or string, but mixed given.
47+
$implicitMixed => $implicitMixed, // error: Array key must be integer or string, but mixed given.
48+
$arrayKey => $arrayKey,
49+
];
50+
}
51+
}
52+
53+

‎tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.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 ForbidUnsafeArrayKeyRule\MixedEnabled;
4+
5+
6+
class ArrayKey
7+
{
8+
9+
/**
10+
* @param array-key $arrayKey
11+
*/
12+
public function test(
13+
int $int,
14+
string $string,
15+
float $float,
16+
$arrayKey,
17+
int|string $intOrString,
18+
int|float $intOrFloat,
19+
array $array,
20+
object $object,
21+
\WeakMap $weakMap,
22+
mixed $explicitMixed,
23+
$implicitMixed
24+
) {
25+
$array[$array] = ''; // error: Array key must be integer or string, but array given.
26+
$array[$int] = '';
27+
$array[$string] = '';
28+
$array[$float] = ''; // error: Array key must be integer or string, but float given.
29+
$array[$intOrFloat] = ''; // error: Array key must be integer or string, but float|int given.
30+
$array[$intOrString] = '';
31+
$array[$explicitMixed] = '';
32+
$array[$implicitMixed] = '';
33+
$array[$arrayKey] = '';
34+
$weakMap[$object] = '';
35+
36+
$a = $array[$float] ?? '';
37+
$b = isset($array[$float]);
38+
$c = empty($array[$float]);
39+
40+
[
41+
$int => $int,
42+
$string => $string,
43+
$float => $float, // error: Array key must be integer or string, but float given.
44+
$intOrFloat => $intOrFloat, // error: Array key must be integer or string, but float|int given.
45+
$intOrString => $intOrString,
46+
$explicitMixed => $explicitMixed,
47+
$implicitMixed => $implicitMixed,
48+
$arrayKey => $arrayKey,
49+
];
50+
}
51+
}
52+
53+

‎tests/RuleTestCase.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,13 @@ private function autofix(string $file, array $analyserErrors): void
111111
$errorsByLines = [];
112112

113113
foreach ($analyserErrors as $analyserError) {
114-
$errorsByLines[$analyserError->getLine()] = $analyserError;
114+
$line = $analyserError->getLine();
115+
116+
if ($line === null) {
117+
throw new LogicException('Error without line number: ' . $analyserError->getMessage());
118+
}
119+
120+
$errorsByLines[$line] = $analyserError;
115121
}
116122

117123
$fileLines = $this->getFileLines($file);

0 commit comments

Comments
 (0)