forbidReturnValueInYieldingMethod (#152)

Author: janedbal

README.md

@@ -94,6 +94,9 @@ parameters:
             enabled: true
         forbidProtectedEnumMethod:
             enabled: true
+        forbidReturnValueInYieldingMethod:
+            enabled: true
+            reportRegardlessOfReturnType: false
         forbidVariableTypeOverwriting:
             enabled: true
         forbidUnsetClassField:
@@ -580,6 +583,28 @@ enum MyEnum {
 }
 ```
 
+### forbidReturnValueInYieldingMethod
+- Disallows returning values in yielding methods unless marked to return Generator as the value is accessible only via [Generator::getReturn](https://www.php.net/manual/en/generator.getreturn.php)
+- To prevent misuse, this rule can be configured to even stricter mode where it reports such returns regardless of return type declared
+
+```php
+class Get {
+    public static function oneTwoThree(): iterable { // marked as iterable, caller cannot access the return value by Generator::getReturn
+        yield 1;
+        yield 2;
+        return 3;
+    }
+}
+
+iterator_to_array(Get::oneTwoThree()); // [1, 2] - see https://3v4l.org/Leu9j
+```
+```neon
+parameters:
+    shipmonkRules:
+        forbidReturnValueInYieldingMethod:
+            reportRegardlessOfReturnType: true # optional stricter mode, defaults to false
+```
+
 ### forbidVariableTypeOverwriting
 - Restricts variable assignment to those that does not change its type
   - Array append `$array[] = 1;` not yet supported

phpcs.xml.dist

@@ -407,6 +407,7 @@
                 Doctrine\Common\Collections\Collection
             "/>
         </properties>
+        <exclude name="SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingTraversableTypeHintSpecification"/><!-- this has problems with vendor libs, PHPStan checks this much more reliably -->
     </rule>
     <rule ref="SlevomatCodingStandard.TypeHints.LongTypeHints"/>
     <rule ref="SlevomatCodingStandard.TypeHints.ReturnTypeHintSpacing"/>

rules.neon

@@ -73,6 +73,9 @@ parameters:
             enabled: true
         forbidProtectedEnumMethod:
             enabled: true
+        forbidReturnValueInYieldingMethod:
+            enabled: true
+            reportRegardlessOfReturnType: false
         forbidVariableTypeOverwriting:
             enabled: true
         forbidUnsetClassField:
@@ -171,6 +174,10 @@ parametersSchema:
         forbidProtectedEnumMethod: structure([
             enabled: bool()
         ])
+        forbidReturnValueInYieldingMethod: structure([
+            enabled: bool()
+            reportRegardlessOfReturnType: bool()
+        ])
         forbidVariableTypeOverwriting: structure([
             enabled: bool()
         ])
@@ -245,6 +252,8 @@ conditionalTags:
         phpstan.rules.rule: %shipmonkRules.forbidPhpDocNullabilityMismatchWithNativeTypehint.enabled%
     ShipMonk\PHPStan\Rule\ForbidProtectedEnumMethodRule:
         phpstan.rules.rule: %shipmonkRules.forbidProtectedEnumMethod.enabled%
+    ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule:
+        phpstan.rules.rule: %shipmonkRules.forbidReturnValueInYieldingMethod.enabled%
     ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule:
         phpstan.rules.rule: %shipmonkRules.forbidVariableTypeOverwriting.enabled%
     ShipMonk\PHPStan\Rule\ForbidUnsetClassFieldRule:
@@ -350,6 +359,10 @@ services:
         class: ShipMonk\PHPStan\Rule\ForbidPhpDocNullabilityMismatchWithNativeTypehintRule
     -
         class: ShipMonk\PHPStan\Rule\ForbidProtectedEnumMethodRule
+    -
+        class: ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule
+        arguments:
+            reportRegardlessOfReturnType: %shipmonkRules.forbidReturnValueInYieldingMethod.reportRegardlessOfReturnType%
     -
         class: ShipMonk\PHPStan\Rule\ForbidVariableTypeOverwritingRule
     -

src/Rule/ForbidReturnValueInYieldingMethodRule.php

@@ -0,0 +1,98 @@
+<?php declare(strict_types = 1);
+
+namespace ShipMonk\PHPStan\Rule;
+
+use Generator;
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\ClosureReturnStatementsNode;
+use PHPStan\Node\MethodReturnStatementsNode;
+use PHPStan\Node\ReturnStatementsNode;
+use PHPStan\Reflection\ParametersAcceptorSelector;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleError;
+use PHPStan\Rules\RuleErrorBuilder;
+use PHPStan\Type\MixedType;
+use PHPStan\Type\Type;
+use function in_array;
+
+/**
+ * @implements Rule<ReturnStatementsNode>
+ */
+class ForbidReturnValueInYieldingMethodRule implements Rule
+{
+
+    private bool $reportRegardlessOfReturnType;
+
+    public function __construct(bool $reportRegardlessOfReturnType)
+    {
+        $this->reportRegardlessOfReturnType = $reportRegardlessOfReturnType;
+    }
+
+    public function getNodeType(): string
+    {
+        return ReturnStatementsNode::class;
+    }
+
+    /**
+     * @param ReturnStatementsNode $node
+     * @return list<RuleError>
+     */
+    public function processNode(
+        Node $node,
+        Scope $scope
+    ): array
+    {
+        if (!$node->getStatementResult()->hasYield()) {
+            return [];
+        }
+
+        if (!$this->reportRegardlessOfReturnType) {
+            $methodReturnType = $this->getReturnType($node, $scope);
+
+            if (in_array(Generator::class, $methodReturnType->getObjectClassNames(), true)) {
+                return [];
+            }
+        }
+
+        $errors = [];
+
+        foreach ($node->getReturnStatements() as $returnStatement) {
+            $returnNode = $returnStatement->getReturnNode();
+
+            if ($returnNode->expr === null) {
+                continue;
+            }
+
+            $suffix = $this->reportRegardlessOfReturnType
+                ? 'this approach is denied'
+                : 'but this method is not marked to return Generator';
+
+            $callType = $node instanceof MethodReturnStatementsNode // @phpstan-ignore-line ignore bc promise
+                ? 'method'
+                : 'function';
+
+            $errors[] = RuleErrorBuilder::message("Returned value from yielding $callType can be accessed only via Generator::getReturn, $suffix.")
+                ->line($returnNode->getLine())
+                ->build();
+        }
+
+        return $errors;
+    }
+
+    private function getReturnType(ReturnStatementsNode $node, Scope $scope): Type
+    {
+        $methodReflection = $scope->getFunction();
+
+        if ($node instanceof ClosureReturnStatementsNode) { // @phpstan-ignore-line ignore bc promise
+            return $scope->getFunctionType($node->getClosureExpr()->getReturnType(), false, false);
+        }
+
+        if ($methodReflection !== null) {
+            return ParametersAcceptorSelector::selectSingle($methodReflection->getVariants())->getReturnType();
+        }
+
+        return new MixedType();
+    }
+
+}

tests/Rule/ForbidReturnValueInYieldingMethodRuleTest.php

@@ -0,0 +1,39 @@
+<?php declare(strict_types = 1);
+
+namespace Rule;
+
+use LogicException;
+use PHPStan\Rules\Rule;
+use ShipMonk\PHPStan\Rule\ForbidReturnValueInYieldingMethodRule;
+use ShipMonk\PHPStan\RuleTestCase;
+
+/**
+ * @extends RuleTestCase<ForbidReturnValueInYieldingMethodRule>
+ */
+class ForbidReturnValueInYieldingMethodRuleTest extends RuleTestCase
+{
+
+    private ?bool $strictMode = null;
+
+    protected function getRule(): Rule
+    {
+        if ($this->strictMode === null) {
+            throw new LogicException('Strict mode must be set');
+        }
+
+        return new ForbidReturnValueInYieldingMethodRule($this->strictMode);
+    }
+
+    public function testNormalMode(): void
+    {
+        $this->strictMode = false;
+        $this->analyseFile(__DIR__ . '/data/ForbidReturnValueInYieldingMethodRule/normal.php');
+    }
+
+    public function testStrictMode(): void
+    {
+        $this->strictMode = true;
+        $this->analyseFile(__DIR__ . '/data/ForbidReturnValueInYieldingMethodRule/strict.php');
+    }
+
+}

tests/Rule/data/ForbidReturnValueInYieldingMethodRule/normal.php

@@ -0,0 +1,77 @@
+<?php declare(strict_types = 1);
+
+namespace ForbidReturnValueInYieldingMethodRule;
+
+class Normal {
+
+    public function returnMixed() {
+        yield 1;
+        return 2; // error: Returned value from yielding method can be accessed only via Generator::getReturn, but this method is not marked to return Generator.
+    }
+
+    public function returnIterable(): iterable {
+        yield 1;
+        return 2; // error: Returned value from yielding method can be accessed only via Generator::getReturn, but this method is not marked to return Generator.
+    }
+
+    public function returnGenerator(): \Generator {
+        yield 1;
+        return 2;
+    }
+
+    /**
+     * @return iterable&\Generator
+     */
+    public function returnGeneratorAndIterable() {
+        yield 1;
+        return 2;
+    }
+
+}
+
+class NoYield {
+    public function returnGeneratorNoYield(): \Generator {
+        return $this->returnGeneratorByYield();
+    }
+
+    public function returnGeneratorByYield(): \Generator {
+        yield 1;
+    }
+}
+
+class NoValue {
+    public function returnMixed() {
+        yield 1;
+        return;
+    }
+
+    public function returnIterable(): iterable {
+        yield 1;
+        return;
+    }
+
+    public function returnGenerator(): \Generator {
+        yield 1;
+        return;
+    }
+}
+
+function returnIterable(): iterable {
+    yield 1;
+    return 2; // error: Returned value from yielding function can be accessed only via Generator::getReturn, but this method is not marked to return Generator.
+}
+
+function returnGenerator(): \Generator {
+    yield 1;
+    return 2;
+}
+
+function (): iterable {
+    yield 1;
+    return 2; // error: Returned value from yielding function can be accessed only via Generator::getReturn, but this method is not marked to return Generator.
+};
+
+function (): \Generator {
+    yield 1;
+    return 2;
+};

tests/Rule/data/ForbidReturnValueInYieldingMethodRule/strict.php

@@ -0,0 +1,79 @@
+<?php declare(strict_types = 1);
+
+namespace Rule\data\ForbidReturnValueInYieldingMethodRule;
+
+class Strict {
+
+    public function returnMixed() {
+        yield 1;
+        return 2; // error: Returned value from yielding method can be accessed only via Generator::getReturn, this approach is denied.
+    }
+
+    public function returnIterable(): iterable {
+        yield 1;
+        return 2; // error: Returned value from yielding method can be accessed only via Generator::getReturn, this approach is denied.
+    }
+
+    public function returnGenerator(): \Generator {
+        yield 1;
+        return 2; // error: Returned value from yielding method can be accessed only via Generator::getReturn, this approach is denied.
+    }
+
+    /**
+     * @return iterable&\Generator
+     */
+    public function returnGeneratorAndIterable() {
+        yield 1;
+        return 2; // error: Returned value from yielding method can be accessed only via Generator::getReturn, this approach is denied.
+    }
+
+}
+
+class NoYield {
+    public function returnGeneratorNoYield(): \Generator {
+        return $this->returnGeneratorByYield();
+    }
+
+    public function returnGeneratorByYield(): \Generator {
+        yield 1;
+    }
+}
+
+class NoValue {
+    public function returnMixed() {
+        yield 1;
+        return;
+    }
+
+    public function returnIterable(): iterable {
+        yield 1;
+        return;
+    }
+
+    public function returnGenerator(): \Generator {
+        yield 1;
+        return;
+    }
+}
+
+
+function returnIterable(): iterable {
+    yield 1;
+    return 2; // error: Returned value from yielding function can be accessed only via Generator::getReturn, this approach is denied.
+}
+
+function returnGenerator(): \Generator {
+    yield 1;
+    return 2; // error: Returned value from yielding function can be accessed only via Generator::getReturn, this approach is denied.
+}
+
+function (): iterable {
+    yield 1;
+    return 2; // error: Returned value from yielding function can be accessed only via Generator::getReturn, this approach is denied.
+};
+
+function (): \Generator {
+    yield 1;
+    return 2; // error: Returned value from yielding function can be accessed only via Generator::getReturn, this approach is denied.
+};
+