From 83547c9f7b7231e72cb599b46ccd4fb9325509ab Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Tue, 12 Sep 2023 14:27:06 +0200 Subject: [PATCH 1/3] EnforceIteratorToArrayPreserveKeysRule --- README.md | 15 ++++++ rules.neon | 9 ++++ ...EnforceIteratorToArrayPreserveKeysRule.php | 49 +++++++++++++++++++ ...rceIteratorToArrayPreserveKeysRuleTest.php | 25 ++++++++++ .../code.php | 26 ++++++++++ 5 files changed, 124 insertions(+) create mode 100644 src/Rule/EnforceIteratorToArrayPreserveKeysRule.php create mode 100644 tests/Rule/EnforceIteratorToArrayPreserveKeysRuleTest.php create mode 100644 tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php diff --git a/README.md b/README.md index a56370e..e913762 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,8 @@ parameters: superclassToSuffixMapping: [] enforceEnumMatch: enabled: true + enforceIteratorToArrayPreserveKeys: + enabled: true enforceListReturn: enabled: true enforceNativeReturnTypehint: @@ -240,6 +242,19 @@ PHPStan even adds tip about `match` in those cases since `1.10.11`. For those reasons, this rule detects any always-true/false enum comparisons and forces you to rewrite it to `match ($enum)`. +### enforceIteratorToArrayPreserveKeys +- Enforces presence of second parameter in [iterator_to_array](https://www.php.net/manual/en/function.iterator-to-array.php) call (`$preserve_keys`) as the default value `true` is generally dangerous (risk of data loss / failure) +- You can use both `true` and `false` there, but doing so is intentional choice now + +```php +$fn = function () { + yield new stdClass => 1; +}; + +iterator_to_array($fn()); // denied, would fail +``` + + ### enforceListReturn - Enforces usage of `list` when list is always returned from a class method or function - When only single return with empty array is present in the method, it is not considered as list diff --git a/rules.neon b/rules.neon index 1743f13..14bd85b 100644 --- a/rules.neon +++ b/rules.neon @@ -11,6 +11,8 @@ parameters: superclassToSuffixMapping: [] enforceEnumMatch: enabled: true + enforceIteratorToArrayPreserveKeys: + enabled: true enforceListReturn: enabled: true enforceNativeReturnTypehint: @@ -112,6 +114,9 @@ parametersSchema: enforceEnumMatch: structure([ enabled: bool() ]) + enforceIteratorToArrayPreserveKeys: structure([ + enabled: bool() + ]) enforceListReturn: structure([ enabled: bool() ]) @@ -216,6 +221,8 @@ conditionalTags: phpstan.rules.rule: %shipmonkRules.classSuffixNaming.enabled% ShipMonk\PHPStan\Rule\EnforceEnumMatchRule: phpstan.rules.rule: %shipmonkRules.enforceEnumMatch.enabled% + ShipMonk\PHPStan\Rule\EnforceIteratorToArrayPreserveKeysRule: + phpstan.rules.rule: %shipmonkRules.enforceIteratorToArrayPreserveKeys.enabled% ShipMonk\PHPStan\Rule\EnforceNativeReturnTypehintRule: phpstan.rules.rule: %shipmonkRules.enforceNativeReturnTypehint.enabled% ShipMonk\PHPStan\Rule\EnforceListReturnRule: @@ -304,6 +311,8 @@ services: superclassToSuffixMapping: %shipmonkRules.classSuffixNaming.superclassToSuffixMapping% - class: ShipMonk\PHPStan\Rule\EnforceEnumMatchRule + - + class: ShipMonk\PHPStan\Rule\EnforceIteratorToArrayPreserveKeysRule - class: ShipMonk\PHPStan\Rule\EnforceListReturnRule - diff --git a/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php b/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php new file mode 100644 index 0000000..2a7d7a8 --- /dev/null +++ b/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php @@ -0,0 +1,49 @@ + + */ +class EnforceIteratorToArrayPreserveKeysRule implements Rule +{ + + public function getNodeType(): string + { + return FuncCall::class; + } + + /** + * @param FuncCall $node + * @return list + */ + public function processNode(Node $node, Scope $scope): array + { + if (!$node->name instanceof Name) { + return []; + } + + if ($node->name->toString() !== 'iterator_to_array') { + return []; + } + + if (count($node->getArgs()) >= 2) { + return []; + } + + return [RuleErrorBuilder::message('Calling iterator_to_array without 2nd parameter $preserve_keys. Default value true might cause failures or data loss.') + ->line($node->getLine()) + ->identifier('iteratorToArrayWithoutPreserveKeys') + ->build()]; + } + +} diff --git a/tests/Rule/EnforceIteratorToArrayPreserveKeysRuleTest.php b/tests/Rule/EnforceIteratorToArrayPreserveKeysRuleTest.php new file mode 100644 index 0000000..bb5c911 --- /dev/null +++ b/tests/Rule/EnforceIteratorToArrayPreserveKeysRuleTest.php @@ -0,0 +1,25 @@ + + */ +class EnforceIteratorToArrayPreserveKeysRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new EnforceIteratorToArrayPreserveKeysRule(); + } + + public function test(): void + { + $this->analyseFile(__DIR__ . '/data/EnforceIteratorToArrayPreserveKeysRule/code.php'); + } + +} diff --git a/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php b/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php new file mode 100644 index 0000000..6cff954 --- /dev/null +++ b/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php @@ -0,0 +1,26 @@ + 1; +}; + +$dataLoss = function () { + yield 1 => 1; + yield 1 => 2; +}; + +$noKeys = function () { + yield 1; + yield 2; +}; + + +iterator_to_array($objectAsKey()); // error: Calling iterator_to_array without 2nd parameter $preserve_keys. Default value true might cause failures or data loss. +iterator_to_array($dataLoss()); // error: Calling iterator_to_array without 2nd parameter $preserve_keys. Default value true might cause failures or data loss. +iterator_to_array($noKeys()); // error: Calling iterator_to_array without 2nd parameter $preserve_keys. Default value true might cause failures or data loss. + +iterator_to_array($objectAsKey(), false); +iterator_to_array($dataLoss(), false); +iterator_to_array($noKeys(), true); From aa1b159d87acbc2f05bbc093378cdf4a375218c7 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Tue, 12 Sep 2023 14:33:27 +0200 Subject: [PATCH 2/3] No false positive when unpacking --- src/Rule/EnforceIteratorToArrayPreserveKeysRule.php | 8 ++++++++ .../data/EnforceIteratorToArrayPreserveKeysRule/code.php | 1 + 2 files changed, 9 insertions(+) diff --git a/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php b/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php index 2a7d7a8..c0a33e9 100644 --- a/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php +++ b/src/Rule/EnforceIteratorToArrayPreserveKeysRule.php @@ -40,6 +40,14 @@ public function processNode(Node $node, Scope $scope): array return []; } + if (count($node->getArgs()) === 0) { + return []; + } + + if ($node->getArgs()[0]->unpack) { + return []; // not trying to analyse what is being unpacked as this is very non-standard approach here + } + return [RuleErrorBuilder::message('Calling iterator_to_array without 2nd parameter $preserve_keys. Default value true might cause failures or data loss.') ->line($node->getLine()) ->identifier('iteratorToArrayWithoutPreserveKeys') diff --git a/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php b/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php index 6cff954..67453e5 100644 --- a/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php +++ b/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php @@ -24,3 +24,4 @@ iterator_to_array($objectAsKey(), false); iterator_to_array($dataLoss(), false); iterator_to_array($noKeys(), true); +iterator_to_array(... [$noKeys(), true]); From dc5d4afda5694ef27aa40e60faf79f7eee2f8258 Mon Sep 17 00:00:00 2001 From: Jan Nedbal Date: Tue, 12 Sep 2023 14:42:36 +0200 Subject: [PATCH 3/3] Add test for swapped args by named args --- .../Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php b/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php index 67453e5..54745e9 100644 --- a/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php +++ b/tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php @@ -25,3 +25,7 @@ iterator_to_array($dataLoss(), false); iterator_to_array($noKeys(), true); iterator_to_array(... [$noKeys(), true]); +iterator_to_array( + preserve_keys: true, + iterator: $noKeys() +);