EnforceIteratorToArrayPreserveKeysRule (#154)

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<T>` 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

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
     -

src/Rule/EnforceIteratorToArrayPreserveKeysRule.php

@@ -0,0 +1,57 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use PhpParser\Node;
+use PhpParser\Node\Expr\FuncCall;
+use PhpParser\Node\Name;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+use function count;
+
+/**
+ * @implements Rule<FuncCall>
+ */
+class EnforceIteratorToArrayPreserveKeysRule implements Rule
+{
+
+    public function getNodeType(): string
+    {
+        return FuncCall::class;
+    }
+
+    /**
+     * @param FuncCall $node
+     * @return list<RuleError>
+     */
+    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 [];
+        }
+
+        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')
+            ->build()];
+    }
+
+}

tests/Rule/EnforceIteratorToArrayPreserveKeysRuleTest.php

@@ -0,0 +1,25 @@
+<?php declare(strict_types = 1);
+
+namespace Rule;
+
+use PHPStan\Rules\Rule;
+use ShipMonk\PHPStan\Rule\EnforceIteratorToArrayPreserveKeysRule;
+use ShipMonk\PHPStan\RuleTestCase;
+
+/**
+ * @extends RuleTestCase<EnforceIteratorToArrayPreserveKeysRule>
+ */
+class EnforceIteratorToArrayPreserveKeysRuleTest extends RuleTestCase
+{
+
+    protected function getRule(): Rule
+    {
+        return new EnforceIteratorToArrayPreserveKeysRule();
+    }
+
+    public function test(): void
+    {
+        $this->analyseFile(__DIR__ . '/data/EnforceIteratorToArrayPreserveKeysRule/code.php');
+    }
+
+}

tests/Rule/data/EnforceIteratorToArrayPreserveKeysRule/code.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace EnforceIteratorToArrayPreserveKeysRule;
+
+$objectAsKey = function () {
+    yield new \stdClass => 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);
+iterator_to_array(... [$noKeys(), true]);
+iterator_to_array(
+    preserve_keys: true,
+    iterator: $noKeys()
+);