diff --git a/README.md b/README.md index 0ca6738..69cd304 100644 --- a/README.md +++ b/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 diff --git a/rules.neon b/rules.neon index dc2b5db..b7e7145 100644 --- a/rules.neon +++ b/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 - diff --git a/src/Rule/ForbidCheckedExceptionInCallableRule.php b/src/Rule/ForbidCheckedExceptionInCallableRule.php index 17df83b..6b43c2c 100644 --- a/src/Rule/ForbidCheckedExceptionInCallableRule.php +++ b/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); diff --git a/src/Rule/ForbidUnsafeArrayKeyRule.php b/src/Rule/ForbidUnsafeArrayKeyRule.php new file mode 100644 index 0000000..5edeb76 --- /dev/null +++ b/src/Rule/ForbidUnsafeArrayKeyRule.php @@ -0,0 +1,107 @@ + + */ +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 + */ + 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 $allowedTypes + */ + private function containsOnlyTypes( + Type $checkedType, + array $allowedTypes + ): bool + { + $allowedType = TypeCombinator::union(...$allowedTypes); + return $allowedType->isSuperTypeOf($checkedType)->yes(); + } + +} diff --git a/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php b/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php new file mode 100644 index 0000000..2f303c0 --- /dev/null +++ b/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php @@ -0,0 +1,50 @@ + + */ +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'); + } + +} diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php new file mode 100644 index 0000000..cd878fb --- /dev/null +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php @@ -0,0 +1,53 @@ + $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, + ]; + } +} + + diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php new file mode 100644 index 0000000..c45b202 --- /dev/null +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php @@ -0,0 +1,53 @@ + $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, + ]; + } +} + + diff --git a/tests/RuleTestCase.php b/tests/RuleTestCase.php index c503a67..51a99eb 100644 --- a/tests/RuleTestCase.php +++ b/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);