ForbidUnsafeArrayKeyRule (#254)

README.md

@@ -86,6 +86,10 @@ parameters:
         forbidReturnValueInYieldingMethod:
             enabled: true
             reportRegardlessOfReturnType: true
+        forbidUnsafeArrayKey:
+            enabled: true
+            reportMixed: true
+            reportInsideIsset: true
         forbidVariableTypeOverwriting:
             enabled: true
         forbidUnsetClassField:
@@ -610,6 +614,31 @@ parameters:
             reportRegardlessOfReturnType: true # optional stricter mode, defaults to false
 ```
 
+
+### forbidUnsafeArrayKey
+- Denies non-int non-string array keys
+- PHP casts `null`, `float` and `bool` to some nearest int/string
+  - You should rather do that intentionally and explicitly
+  - Those types are the main difference to default PHPStan behaviour which allows using them as array keys
+- You can exclude reporting `mixed` keys via `reportMixed` configuration
+- You can exclude reporting `isset($array[$invalid])` and `$array[$invalid] ?? null` via `reportInsideIsset` configuration
+
+```php
+$taxRates = [ // denied, float key gets casted to int (causing $taxRates to contain one item)
+    1.15 => 'reduced',
+    1.21 => 'standard',
+];
+```
+
+```neon
+parameters:
+    shipmonkRules:
+        forbidUnsafeArrayKey:
+            reportMixed: false # defaults to true
+            reportInsideIsset: false # defaults to true
+```
+
+
 ### forbidVariableTypeOverwriting
 - Restricts variable assignment to those that does not change its type
   - Array append `$array[] = 1;` not yet supported

rules.neon

@@ -65,6 +65,10 @@ parameters:
         forbidReturnValueInYieldingMethod:
             enabled: true
             reportRegardlessOfReturnType: true
+        forbidUnsafeArrayKey:
+            enabled: true
+            reportMixed: true
+            reportInsideIsset: true
         forbidVariableTypeOverwriting:
             enabled: true
         forbidUnsetClassField:
@@ -177,6 +181,11 @@ parametersSchema:
             enabled: bool()
             reportRegardlessOfReturnType: bool()
         ])
+        forbidUnsafeArrayKey: structure([
+            enabled: bool()
+            reportMixed: bool()
+            reportInsideIsset: bool()
+        ])
         forbidVariableTypeOverwriting: structure([
             enabled: bool()
         ])
@@ -259,6 +268,8 @@ conditionalTags:
         phpstan.rules.rule: %shipmonkRules.forbidProtectedEnumMethod.enabled%
     ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule:
         phpstan.rules.rule: %shipmonkRules.forbidReturnValueInYieldingMethod.enabled%
+    ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule:
+        phpstan.rules.rule: %shipmonkRules.forbidUnsafeArrayKey.enabled%
     ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule:
         phpstan.rules.rule: %shipmonkRules.forbidVariableTypeOverwriting.enabled%
     ShipMonk\PHPStan\Rule\ForbidUnsetClassFieldRule:
@@ -369,6 +380,11 @@ services:
         class: ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule
         arguments:
             reportRegardlessOfReturnType: %shipmonkRules.forbidReturnValueInYieldingMethod.reportRegardlessOfReturnType%
+    -
+        class: ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule
+        arguments:
+            reportMixed: %shipmonkRules.forbidUnsafeArrayKey.reportMixed%
+            reportInsideIsset: %shipmonkRules.forbidUnsafeArrayKey.reportInsideIsset%
     -
         class: ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule
     -

src/Rule/ForbidCheckedExceptionInCallableRule.php

@@ -469,7 +469,7 @@ private function whitelistAllowedCallables(CallLike $node, Scope $scope): void
         $parameters = $parametersAcceptor->getParameters();
 
         foreach ($args as $index => $arg) {
-            $parameterIndex = $this->getParameterIndex($arg, $index, $parameters);
+            $parameterIndex = $this->getParameterIndex($arg, $index, $parameters) ?? -1;
             $parameter = $parameters[$parameterIndex] ?? null;
             $argHash = spl_object_hash($arg->value);
 

src/Rule/ForbidUnsafeArrayKeyRule.php

@@ -0,0 +1,107 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\ArrayDimFetch;
+use PhpParser\Node\Expr\ArrayItem;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\IdentifierRuleError;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+use PHPStan\Type\IntegerType;
+use PHPStan\Type\MixedType;
+use PHPStan\Type\StringType;
+use PHPStan\Type\Type;
+use PHPStan\Type\TypeCombinator;
+use PHPStan\Type\VerbosityLevel;
+
+/**
+ * @implements Rule<Node>
+ */
+class ForbidUnsafeArrayKeyRule implements Rule
+{
+
+    private bool $reportMixed;
+
+    private bool $reportInsideIsset;
+
+    public function __construct(
+        bool $reportMixed,
+        bool $reportInsideIsset
+    )
+    {
+        $this->reportMixed = $reportMixed;
+        $this->reportInsideIsset = $reportInsideIsset;
+    }
+
+    public function getNodeType(): string
+    {
+        return Node::class;
+    }
+
+    /**
+     * @return list<IdentifierRuleError>
+     */
+    public function processNode(
+        Node $node,
+        Scope $scope
+    ): array
+    {
+        if ($node instanceof ArrayItem && $node->key !== null) {
+            $keyType = $scope->getType($node->key);
+
+            if (!$this->isArrayKey($keyType)) {
+                return [
+                    RuleErrorBuilder::message('Array key must be integer or string, but ' . $keyType->describe(VerbosityLevel::precise()) . ' given.')
+                        ->identifier('shipmonk.unsafeArrayKey')
+                        ->build(),
+                ];
+            }
+        }
+
+        if (
+            $node instanceof ArrayDimFetch
+            && $node->dim !== null
+            && !$scope->getType($node->var)->isArray()->no()
+        ) {
+            if (!$this->reportInsideIsset && $scope->isUndefinedExpressionAllowed($node)) {
+                return [];
+            }
+
+            $dimType = $scope->getType($node->dim);
+
+            if (!$this->isArrayKey($dimType)) {
+                return [
+                    RuleErrorBuilder::message('Array key must be integer or string, but ' . $dimType->describe(VerbosityLevel::precise()) . ' given.')
+                        ->identifier('shipmonk.unsafeArrayKey')
+                        ->build(),
+                ];
+            }
+        }
+
+        return [];
+    }
+
+    private function isArrayKey(Type $type): bool
+    {
+        if (!$this->reportMixed && $type instanceof MixedType) {
+            return true;
+        }
+
+        return $this->containsOnlyTypes($type, [new StringType(), new IntegerType()]);
+    }
+
+    /**
+     * @param list<Type> $allowedTypes
+     */
+    private function containsOnlyTypes(
+        Type $checkedType,
+        array $allowedTypes
+    ): bool
+    {
+        $allowedType = TypeCombinator::union(...$allowedTypes);
+        return $allowedType->isSuperTypeOf($checkedType)->yes();
+    }
+
+}

tests/Rule/ForbidUnsafeArrayKeyRuleTest.php

@@ -0,0 +1,50 @@
+<?php declare(strict_types = 1);
+
+namespace Rule;
+
+use LogicException;
+use PHPStan\Rules\Rule;
+use ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule;
+use ShipMonk\PHPStan\RuleTestCase;
+
+/**
+ * @extends RuleTestCase<ForbidUnsafeArrayKeyRule>
+ */
+class ForbidUnsafeArrayKeyRuleTest extends RuleTestCase
+{
+
+    private ?bool $checkMixed = null;
+
+    private ?bool $checkInsideIsset = null;
+
+    protected function getRule(): Rule
+    {
+        if ($this->checkMixed === null) {
+            throw new LogicException('Property checkMixed must be set');
+        }
+
+        if ($this->checkInsideIsset === null) {
+            throw new LogicException('Property checkInsideIsset must be set');
+        }
+
+        return new ForbidUnsafeArrayKeyRule(
+            $this->checkMixed,
+            $this->checkInsideIsset,
+        );
+    }
+
+    public function testStrict(): void
+    {
+        $this->checkMixed = true;
+        $this->checkInsideIsset = true;
+        $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/default.php');
+    }
+
+    public function testLessStrict(): void
+    {
+        $this->checkMixed = false;
+        $this->checkInsideIsset = false;
+        $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/less-strict.php');
+    }
+
+}

tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php

@@ -0,0 +1,53 @@
+<?php declare(strict_types = 1);
+
+namespace ForbidUnsafeArrayKeyRule\MixedDisabled;
+
+
+class ArrayKey
+{
+
+    /**
+     * @param array-key $arrayKey
+     */
+    public function test(
+        int $int,
+        string $string,
+        float $float,
+        $arrayKey,
+        int|string $intOrString,
+        int|float $intOrFloat,
+        array $array,
+        object $object,
+        \WeakMap $weakMap,
+        mixed $explicitMixed,
+        $implicitMixed
+    ) {
+        $array[$array] = ''; // error: Array key must be integer or string, but array given.
+        $array[$int] = '';
+        $array[$string] = '';
+        $array[$float] = ''; // error: Array key must be integer or string, but float given.
+        $array[$intOrFloat] = ''; // error: Array key must be integer or string, but float|int given.
+        $array[$intOrString] = '';
+        $array[$explicitMixed] = ''; // error: Array key must be integer or string, but mixed given.
+        $array[$implicitMixed] = ''; // error: Array key must be integer or string, but mixed given.
+        $array[$arrayKey] = '';
+        $weakMap[$object] = '';
+
+        $a = $array[$float] ?? ''; // error: Array key must be integer or string, but float given.
+        $b = isset($array[$float]); // error: Array key must be integer or string, but float given.
+        $c = empty($array[$float]); // error: Array key must be integer or string, but float given.
+
+        [
+            $int => $int,
+            $string => $string,
+            $float => $float, // error: Array key must be integer or string, but float given.
+            $intOrFloat => $intOrFloat, // error: Array key must be integer or string, but float|int given.
+            $intOrString => $intOrString,
+            $explicitMixed => $explicitMixed, // error: Array key must be integer or string, but mixed given.
+            $implicitMixed => $implicitMixed, // error: Array key must be integer or string, but mixed given.
+            $arrayKey => $arrayKey,
+        ];
+    }
+}
+
+

tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php

@@ -0,0 +1,53 @@
+<?php declare(strict_types = 1);
+
+namespace ForbidUnsafeArrayKeyRule\MixedEnabled;
+
+
+class ArrayKey
+{
+
+    /**
+     * @param array-key $arrayKey
+     */
+    public function test(
+        int $int,
+        string $string,
+        float $float,
+        $arrayKey,
+        int|string $intOrString,
+        int|float $intOrFloat,
+        array $array,
+        object $object,
+        \WeakMap $weakMap,
+        mixed $explicitMixed,
+        $implicitMixed
+    ) {
+        $array[$array] = ''; // error: Array key must be integer or string, but array given.
+        $array[$int] = '';
+        $array[$string] = '';
+        $array[$float] = ''; // error: Array key must be integer or string, but float given.
+        $array[$intOrFloat] = ''; // error: Array key must be integer or string, but float|int given.
+        $array[$intOrString] = '';
+        $array[$explicitMixed] = '';
+        $array[$implicitMixed] = '';
+        $array[$arrayKey] = '';
+        $weakMap[$object] = '';
+
+        $a = $array[$float] ?? '';
+        $b = isset($array[$float]);
+        $c = empty($array[$float]);
+
+        [
+            $int => $int,
+            $string => $string,
+            $float => $float, // error: Array key must be integer or string, but float given.
+            $intOrFloat => $intOrFloat, // error: Array key must be integer or string, but float|int given.
+            $intOrString => $intOrString,
+            $explicitMixed => $explicitMixed,
+            $implicitMixed => $implicitMixed,
+            $arrayKey => $arrayKey,
+        ];
+    }
+}
+
+

tests/RuleTestCase.php

@@ -111,7 +111,13 @@ private function autofix(string $file, array $analyserErrors): void
         $errorsByLines = [];
 
         foreach ($analyserErrors as $analyserError) {
-            $errorsByLines[$analyserError->getLine()] = $analyserError;
+            $line = $analyserError->getLine();
+
+            if ($line === null) {
+                throw new LogicException('Error without line number: ' . $analyserError->getMessage());
+            }
+
+            $errorsByLines[$line] = $analyserError;
         }
 
         $fileLines = $this->getFileLines($file);