From 1234a6ea7a2e92f41fa842cb109f806bcd5ad52e Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Tue, 9 Jul 2024 15:34:36 +0200 Subject: [PATCH 1/4] ForbidUnsafeArrayKeyRule --- README.md | 26 +++++ rules.neon | 13 +++ .../ForbidCheckedExceptionInCallableRule.php | 2 +- src/Rule/ForbidUnsafeArrayKeyRule.php | 97 +++++++++++++++++++ tests/Rule/ForbidUnsafeArrayKeyRuleTest.php | 41 ++++++++ .../mixed-disabled.php | 38 ++++++++ .../mixed-enabled.php | 38 ++++++++ tests/RuleTestCase.php | 3 +- 8 files changed, 256 insertions(+), 2 deletions(-) create mode 100644 src/Rule/ForbidUnsafeArrayKeyRule.php create mode 100644 tests/Rule/ForbidUnsafeArrayKeyRuleTest.php create mode 100644 tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php create mode 100644 tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php diff --git a/README.md b/README.md index 0ca6738..5062e0a 100644 --- a/README.md +++ b/README.md @@ -86,6 +86,9 @@ parameters: forbidReturnValueInYieldingMethod: enabled: true reportRegardlessOfReturnType: true + forbidUnsafeArrayKey: + enabled: true + reportMixed: true forbidVariableTypeOverwriting: enabled: true forbidUnsetClassField: @@ -610,6 +613,29 @@ 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 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 +``` + + ### 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..200759e 100644 --- a/rules.neon +++ b/rules.neon @@ -65,6 +65,9 @@ parameters: forbidReturnValueInYieldingMethod: enabled: true reportRegardlessOfReturnType: true + forbidUnsafeArrayKey: + enabled: true + reportMixed: true forbidVariableTypeOverwriting: enabled: true forbidUnsetClassField: @@ -177,6 +180,10 @@ parametersSchema: enabled: bool() reportRegardlessOfReturnType: bool() ]) + forbidUnsafeArrayKey: structure([ + enabled: bool() + reportMixed: bool() + ]) forbidVariableTypeOverwriting: structure([ enabled: bool() ]) @@ -259,6 +266,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 +378,10 @@ services: class: ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule arguments: reportRegardlessOfReturnType: %shipmonkRules.forbidReturnValueInYieldingMethod.reportRegardlessOfReturnType% + - + class: ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule + arguments: + reportMixed: %shipmonkRules.forbidUnsafeArrayKey.reportMixed% - 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..03865f0 --- /dev/null +++ b/src/Rule/ForbidUnsafeArrayKeyRule.php @@ -0,0 +1,97 @@ + + */ +class ForbidUnsafeArrayKeyRule implements Rule +{ + + private bool $reportMixed; + + public function __construct(bool $reportMixed) + { + $this->reportMixed = $reportMixed; + } + + 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() + ) { + $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..a102c52 --- /dev/null +++ b/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php @@ -0,0 +1,41 @@ + + */ +class ForbidUnsafeArrayKeyRuleTest extends RuleTestCase +{ + + private ?bool $checkMixed = null; + + protected function getRule(): Rule + { + if ($this->checkMixed === null) { + throw new LogicException('Property checkMixed must be set'); + } + + return new ForbidUnsafeArrayKeyRule( + $this->checkMixed, + ); + } + + public function testMixedDisabled(): void + { + $this->checkMixed = false; + $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php'); + } + + public function testMixedEnabled(): void + { + $this->checkMixed = true; + $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php'); + } + +} diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php new file mode 100644 index 0000000..b5bff9d --- /dev/null +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php @@ -0,0 +1,38 @@ +getLine()] = $analyserError; + $line = $analyserError->getLine() ?? -1; + $errorsByLines[$line] = $analyserError; } $fileLines = $this->getFileLines($file); From 375ac1373bb5cbec607d72fe8c765ddfd29db91d Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Tue, 9 Jul 2024 16:25:47 +0200 Subject: [PATCH 2/4] Test even ArrayItem node --- .../data/ForbidUnsafeArrayKeyRule/mixed-disabled.php | 11 +++++++++++ .../data/ForbidUnsafeArrayKeyRule/mixed-enabled.php | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php index b5bff9d..dd7485e 100644 --- a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php @@ -32,6 +32,17 @@ public function test( $array[$implicitMixed] = ''; $array[$arrayKey] = ''; $weakMap[$object] = ''; + + [ + $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, + ]; } } diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php index 0e343f0..89cdd5d 100644 --- a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php @@ -32,6 +32,17 @@ public function test( $array[$implicitMixed] = ''; // error: Array key must be integer or string, but mixed given. $array[$arrayKey] = ''; $weakMap[$object] = ''; + + [ + $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, + ]; } } From cd60f87be37583524b7095da091af87c75abea40 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Wed, 10 Jul 2024 13:26:03 +0200 Subject: [PATCH 3/4] Implement reportInsideIsset flag --- README.md | 5 ++++- rules.neon | 3 +++ src/Rule/ForbidUnsafeArrayKeyRule.php | 12 ++++++++++- tests/Rule/ForbidUnsafeArrayKeyRuleTest.php | 21 +++++++++++++------ .../{mixed-enabled.php => default.php} | 6 +++++- .../{mixed-disabled.php => less-strict.php} | 6 +++++- 6 files changed, 43 insertions(+), 10 deletions(-) rename tests/Rule/data/ForbidUnsafeArrayKeyRule/{mixed-enabled.php => default.php} (82%) rename tests/Rule/data/ForbidUnsafeArrayKeyRule/{mixed-disabled.php => less-strict.php} (89%) diff --git a/README.md b/README.md index 5062e0a..69cd304 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,7 @@ parameters: forbidUnsafeArrayKey: enabled: true reportMixed: true + reportInsideIsset: true forbidVariableTypeOverwriting: enabled: true forbidUnsetClassField: @@ -619,7 +620,8 @@ parameters: - 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 configuration +- 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) @@ -633,6 +635,7 @@ parameters: shipmonkRules: forbidUnsafeArrayKey: reportMixed: false # defaults to true + reportInsideIsset: false # defaults to true ``` diff --git a/rules.neon b/rules.neon index 200759e..b7e7145 100644 --- a/rules.neon +++ b/rules.neon @@ -68,6 +68,7 @@ parameters: forbidUnsafeArrayKey: enabled: true reportMixed: true + reportInsideIsset: true forbidVariableTypeOverwriting: enabled: true forbidUnsetClassField: @@ -183,6 +184,7 @@ parametersSchema: forbidUnsafeArrayKey: structure([ enabled: bool() reportMixed: bool() + reportInsideIsset: bool() ]) forbidVariableTypeOverwriting: structure([ enabled: bool() @@ -382,6 +384,7 @@ services: class: ShipMonk\PHPStan\Rule\ForbidUnsafeArrayKeyRule arguments: reportMixed: %shipmonkRules.forbidUnsafeArrayKey.reportMixed% + reportInsideIsset: %shipmonkRules.forbidUnsafeArrayKey.reportInsideIsset% - class: ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule - diff --git a/src/Rule/ForbidUnsafeArrayKeyRule.php b/src/Rule/ForbidUnsafeArrayKeyRule.php index 03865f0..5edeb76 100644 --- a/src/Rule/ForbidUnsafeArrayKeyRule.php +++ b/src/Rule/ForbidUnsafeArrayKeyRule.php @@ -24,9 +24,15 @@ class ForbidUnsafeArrayKeyRule implements Rule private bool $reportMixed; - public function __construct(bool $reportMixed) + private bool $reportInsideIsset; + + public function __construct( + bool $reportMixed, + bool $reportInsideIsset + ) { $this->reportMixed = $reportMixed; + $this->reportInsideIsset = $reportInsideIsset; } public function getNodeType(): string @@ -59,6 +65,10 @@ public function processNode( && $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)) { diff --git a/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php b/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php index a102c52..2f303c0 100644 --- a/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php +++ b/tests/Rule/ForbidUnsafeArrayKeyRuleTest.php @@ -15,27 +15,36 @@ 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 testMixedDisabled(): void + public function testStrict(): void { - $this->checkMixed = false; - $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php'); + $this->checkMixed = true; + $this->checkInsideIsset = true; + $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/default.php'); } - public function testMixedEnabled(): void + public function testLessStrict(): void { - $this->checkMixed = true; - $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php'); + $this->checkMixed = false; + $this->checkInsideIsset = false; + $this->analyseFile(__DIR__ . '/data/ForbidUnsafeArrayKeyRule/less-strict.php'); } } diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php similarity index 82% rename from tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php rename to tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php index 89cdd5d..cd878fb 100644 --- a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-enabled.php +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/default.php @@ -1,6 +1,6 @@ $int, $string => $string, diff --git a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php b/tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php similarity index 89% rename from tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php rename to tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php index dd7485e..c45b202 100644 --- a/tests/Rule/data/ForbidUnsafeArrayKeyRule/mixed-disabled.php +++ b/tests/Rule/data/ForbidUnsafeArrayKeyRule/less-strict.php @@ -1,6 +1,6 @@ $int, $string => $string, From e3efd023e75a24699108e596351d1babf57f5dbf Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Wed, 10 Jul 2024 17:19:39 +0200 Subject: [PATCH 4/4] Stricter RuleTestCase --- tests/RuleTestCase.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/RuleTestCase.php b/tests/RuleTestCase.php index bc4dd39..51a99eb 100644 --- a/tests/RuleTestCase.php +++ b/tests/RuleTestCase.php @@ -111,7 +111,12 @@ private function autofix(string $file, array $analyserErrors): void $errorsByLines = []; foreach ($analyserErrors as $analyserError) { - $line = $analyserError->getLine() ?? -1; + $line = $analyserError->getLine(); + + if ($line === null) { + throw new LogicException('Error without line number: ' . $analyserError->getMessage()); + } + $errorsByLines[$line] = $analyserError; }