EnforceListReturnRule (#76)

* EnforceListReturnRule * dont use feature toggle from PHPStan * bump phpstan 1.9.1, BC promise of AccessoryArrayListType needed * Fix readme typo
shipmonk-rnd · Jan 31, 2023 · 7e1755d · 7e1755d
1 parent 3a3853f
commit 7e1755d
Show file tree

Hide file tree

Showing 29 changed files with 248 additions and 28 deletions.
diff --git a/README.md b/README.md
@@ -24,6 +24,8 @@ parameters:
             enabled: true
         backedEnumGenerics:
             enabled: true
+        enforceListReturn:
+            enabled: true
         enforceNativeReturnTypehint:
             enabled: true
         enforceReadonlyPublicProperty:
@@ -121,6 +123,20 @@ enum MyEnum: string { // missing @implements tag
 }
 ```
 
+### enforceListReturn
+- Enforces usage of `list<T>` when list is always returned from a class method
+- When only single return with empty array is present in the method, it is not considered as list
+- Does nothing when [list types](https://phpstan.org/blog/phpstan-1-9-0-with-phpdoc-asserts-list-type#list-type) are disabled in PHPStan
+```php
+/**
+ * @return array<string>
+ */
+public function returnList(): array // error, return phpdoc is generic array, but list is always returned
+{
+    return ['item'];
+}
+```
+
 ### enforceNativeReturnTypehint
 - Enforces usage of native return typehints if supported by your PHP version
 - If PHPDoc is present, it deduces needed typehint from that, if not, deduction is performed based on real types returned

diff --git a/composer.json b/composer.json
@@ -12,7 +12,7 @@
     "require": {
         "php": "^7.4 || ^8.0",
         "nikic/php-parser": "^4.14.0",
-        "phpstan/phpstan": "^1.8.7"
+        "phpstan/phpstan": "^1.9.1"
     },
     "require-dev": {
         "editorconfig-checker/editorconfig-checker": "^10.3.0",

diff --git a/rules.neon b/rules.neon
@@ -6,6 +6,8 @@ parameters:
             enabled: true
         backedEnumGenerics:
             enabled: true
+        enforceListReturn:
+            enabled: true
         enforceNativeReturnTypehint:
             enabled: true
         enforceReadonlyPublicProperty:
@@ -65,6 +67,9 @@ parametersSchema:
         backedEnumGenerics: structure([
             enabled: bool()
         ])
+        enforceListReturn: structure([
+            enabled: bool()
+        ])
         enforceNativeReturnTypehint: structure([
             enabled: bool()
         ])
@@ -144,6 +149,8 @@ conditionalTags:
         phpstan.rules.rule: %shipmonkRules.backedEnumGenerics.enabled%
     ShipMonk\PHPStan\Rule\EnforceNativeReturnTypehintRule:
         phpstan.rules.rule: %shipmonkRules.enforceNativeReturnTypehint.enabled%
+    ShipMonk\PHPStan\Rule\EnforceListReturnRule:
+        phpstan.rules.rule: %shipmonkRules.enforceListReturn.enabled%
     ShipMonk\PHPStan\Rule\EnforceReadonlyPublicPropertyRule:
         phpstan.rules.rule: %shipmonkRules.enforceReadonlyPublicProperty.enabled%
     ShipMonk\PHPStan\Rule\ForbidAssignmentNotMatchingVarDocRule:
@@ -205,6 +212,8 @@ services:
         class: ShipMonk\PHPStan\Rule\AllowNamedArgumentOnlyInAttributesRule
     -
         class: ShipMonk\PHPStan\Rule\BackedEnumGenericsRule
+    -
+        class: ShipMonk\PHPStan\Rule\EnforceListReturnRule
     -
         class: ShipMonk\PHPStan\Rule\EnforceNativeReturnTypehintRule
         arguments:

diff --git a/src/Rule/AllowComparingOnlyComparableTypesRule.php b/src/Rule/AllowComparingOnlyComparableTypesRule.php
@@ -33,7 +33,7 @@ public function getNodeType(): string
 
     /**
      * @param BinaryOp $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/AllowNamedArgumentOnlyInAttributesRule.php b/src/Rule/AllowNamedArgumentOnlyInAttributesRule.php
@@ -21,7 +21,7 @@ public function getNodeType(): string
 
     /**
      * @param Arg $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/BackedEnumGenericsRule.php b/src/Rule/BackedEnumGenericsRule.php
@@ -23,7 +23,7 @@ public function getNodeType(): string
 
     /**
      * @param InClassNode $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/EnforceListReturnRule.php b/src/Rule/EnforceListReturnRule.php
@@ -0,0 +1,96 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\MethodReturnStatementsNode;
+use PHPStan\Reflection\MethodReflection;
+use PHPStan\Reflection\ParametersAcceptorSelector;
+use PHPStan\Rules\Rule;
+use PHPStan\Type\Accessory\AccessoryArrayListType;
+use PHPStan\Type\ArrayType;
+use PHPStan\Type\NeverType;
+use PHPStan\Type\VerbosityLevel;
+use function count;
+
+/**
+ * @implements Rule<MethodReturnStatementsNode>
+ */
+class EnforceListReturnRule implements Rule
+{
+
+    public function getNodeType(): string
+    {
+        return MethodReturnStatementsNode::class;
+    }
+
+    /**
+     * @param MethodReturnStatementsNode $node
+     * @return list<string>
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        if (AccessoryArrayListType::isListTypeEnabled() === false) {
+            return [];
+        }
+
+        if ($scope->getClassReflection() === null) {
+            return [];
+        }
+
+        $method = $scope->getFunction();
+
+        if (!$method instanceof MethodReflection) {
+            return [];
+        }
+
+        if ($this->alwaysReturnList($node) && !$this->isMarkedWithListReturn($method)) {
+            return ["Method {$method->getName()} always return list, but is marked as {$this->getReturnPhpDoc($method)}"];
+        }
+
+        return [];
+    }
+
+    private function getReturnPhpDoc(MethodReflection $methodReflection): string
+    {
+        $returnType = ParametersAcceptorSelector::selectSingle($methodReflection->getVariants())->getReturnType();
+        return $returnType->describe(VerbosityLevel::precise());
+    }
+
+    private function alwaysReturnList(MethodReturnStatementsNode $node): bool
+    {
+        $returnStatementsCount = count($node->getReturnStatements());
+
+        if ($returnStatementsCount === 0) {
+            return false;
+        }
+
+        foreach ($node->getReturnStatements() as $returnStatement) {
+            $returnExpr = $returnStatement->getReturnNode()->expr;
+
+            if ($returnExpr === null) {
+                return false;
+            }
+
+            $returnType = $returnStatement->getScope()->getType($returnExpr);
+
+            if (!$returnType->isList()->yes()) {
+                return false;
+            }
+
+            if ($returnStatementsCount === 1 && $returnType instanceof ArrayType && $returnType->getItemType() instanceof NeverType) {
+                return false; // do not consider empty array as list when it is the only return statement
+            }
+        }
+
+        return true;
+    }
+
+    private function isMarkedWithListReturn(MethodReflection $methodReflection): bool
+    {
+        $returnType = ParametersAcceptorSelector::selectSingle($methodReflection->getVariants())->getReturnType();
+        return $returnType->isList()->yes();
+    }
+
+}
diff --git a/src/Rule/EnforceReadonlyPublicPropertyRule.php b/src/Rule/EnforceReadonlyPublicPropertyRule.php
@@ -20,7 +20,7 @@ public function getNodeType(): string
 
     /**
      * @param ClassPropertyNode $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidAssignmentNotMatchingVarDocRule.php b/src/Rule/ForbidAssignmentNotMatchingVarDocRule.php
@@ -40,7 +40,7 @@ public function getNodeType(): string
 
     /**
      * @param Assign $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidCastRule.php b/src/Rule/ForbidCastRule.php
@@ -45,7 +45,7 @@ public function getNodeType(): string
 
     /**
      * @param Cast $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidCustomFunctionsRule.php b/src/Rule/ForbidCustomFunctionsRule.php
@@ -74,7 +74,7 @@ public function getNodeType(): string
 
     /**
      * @param CallLike $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidEnumInFunctionArgumentsRule.php b/src/Rule/ForbidEnumInFunctionArgumentsRule.php
@@ -50,7 +50,7 @@ public function getNodeType(): string
 
     /**
      * @param FuncCall $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidFetchOnMixedRule.php b/src/Rule/ForbidFetchOnMixedRule.php
@@ -34,7 +34,7 @@ public function getNodeType(): string
 
     /**
      * @param PropertyFetch $node
-     * @return string[] errors
+     * @return list<string> errors
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidMatchDefaultArmForEnumsRule.php b/src/Rule/ForbidMatchDefaultArmForEnumsRule.php
@@ -24,7 +24,7 @@ public function getNodeType(): string
 
     /**
      * @param MatchExpressionNode $node
-     * @return RuleError[]
+     * @return list<RuleError>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidMethodCallOnMixedRule.php b/src/Rule/ForbidMethodCallOnMixedRule.php
@@ -34,7 +34,7 @@ public function getNodeType(): string
 
     /**
      * @param MethodCall $node
-     * @return string[] errors
+     * @return list<string> errors
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidNullInAssignOperationsRule.php b/src/Rule/ForbidNullInAssignOperationsRule.php
@@ -52,7 +52,7 @@ public function getNodeType(): string
 
     /**
      * @param AssignOp $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidNullInBinaryOperationsRule.php b/src/Rule/ForbidNullInBinaryOperationsRule.php
@@ -38,7 +38,7 @@ public function getNodeType(): string
 
     /**
      * @param BinaryOp $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidNullInInterpolatedStringRule.php b/src/Rule/ForbidNullInInterpolatedStringRule.php
@@ -30,7 +30,7 @@ public function getNodeType(): string
 
     /**
      * @param Encapsed $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidReturnInConstructorRule.php b/src/Rule/ForbidReturnInConstructorRule.php
@@ -21,7 +21,7 @@ public function getNodeType(): string
 
     /**
      * @param Return_ $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidUnsetClassFieldRule.php b/src/Rule/ForbidUnsetClassFieldRule.php
@@ -21,7 +21,7 @@ public function getNodeType(): string
 
     /**
      * @param Unset_ $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidUnusedExceptionRule.php b/src/Rule/ForbidUnusedExceptionRule.php
@@ -35,7 +35,7 @@ public function getNodeType(): string
 
     /**
      * @param Expr $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {
@@ -52,7 +52,7 @@ public function processNode(Node $node, Scope $scope): array
 
     /**
      * @param MethodCall|StaticCall $node
-     * @return string[]
+     * @return list<string>
      */
     private function processCall(CallLike $node, Scope $scope): array
     {
@@ -68,7 +68,7 @@ private function processCall(CallLike $node, Scope $scope): array
     }
 
     /**
-     * @return string[]
+     * @return list<string>
      */
     private function processNew(New_ $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidUnusedMatchResultRule.php b/src/Rule/ForbidUnusedMatchResultRule.php
@@ -26,7 +26,7 @@ public function getNodeType(): string
 
     /**
      * @param Match_ $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidUselessNullableReturnRule.php b/src/Rule/ForbidUselessNullableReturnRule.php
@@ -27,7 +27,7 @@ public function getNodeType(): string
 
     /**
      * @param MethodReturnStatementsNode $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/ForbidVariableTypeOverwritingRule.php b/src/Rule/ForbidVariableTypeOverwritingRule.php
@@ -36,7 +36,7 @@ public function getNodeType(): string
 
     /**
      * @param Assign $node
-     * @return string[]
+     * @return list<string>
      */
     public function processNode(Node $node, Scope $scope): array
     {

diff --git a/src/Rule/RequirePreviousExceptionPassRule.php b/src/Rule/RequirePreviousExceptionPassRule.php
@@ -52,7 +52,7 @@ public function getNodeType(): string
 
     /**
      * @param TryCatch $node
-     * @return RuleError[]
+     * @return list<RuleError>
      */
     public function processNode(Node $node, Scope $scope): array
     {
@@ -95,7 +95,7 @@ public function processNode(Node $node, Scope $scope): array
     }
 
     /**
-     * @return RuleError[]
+     * @return list<RuleError>
      */
     private function processExceptionCreation(bool $strictTypes, ?string $caughtExceptionVariableName, Type $caughtExceptionType, CallLike $node, Scope $scope): array
     {

diff --git a/src/Rule/UselessPrivatePropertyDefaultValueRule.php b/src/Rule/UselessPrivatePropertyDefaultValueRule.php
@@ -25,7 +25,7 @@ public function getNodeType(): string
 
     /**
      * @param ClassPropertiesNode $node
-     * @return RuleError[]
+     * @return list<RuleError>
      */
     public function processNode(Node $node, Scope $scope): array
     {