Analyzer return and yield in one method, #185

.phpsa.yml

@@ -184,6 +184,10 @@ phpsa:
         property_definition_default_value:
             enabled:              true
 
+        # Checks for using return and yield statements in a one method and discourages it.
+        return_and_yield_in_one_method:
+            enabled:              true
+
         # Using octal, hexadecimal or binary integers is discouraged.
         check_l_number_kind:
             enabled:              true

docs/05_Analyzers.md

@@ -155,6 +155,10 @@ Checks for use of old rand, srand, getrandmax functions and suggests alternative
 
 Checks that regular expressions are syntactically correct.
 
+#### ReturnAndYieldInOneMethod
+
+Checks for using return and yield statements in a one method and discourages it.
+
 #### StaticUsage
 
 Discourages the use of static variables (not properties).

src/Analyzer/Factory.php

@@ -110,6 +110,7 @@ private static function getStatementPasses()
             AnalyzerPass\Statement\YodaCondition::class,
             AnalyzerPass\Statement\ForCondition::class,
             AnalyzerPass\Statement\PropertyDefinitionDefaultValue::class,
+            AnalyzerPass\Statement\ReturnAndYieldInOneMethod::class,
         ];
     }
 

src/Analyzer/Helper/ResolveExpressionTrait.php

@@ -35,25 +35,23 @@ public function resolveFunctionName(FuncCall $funcCall, Context $context)
     }
 
     /**
-     * Return \Generator with Return_ statement(s)
+     * Return \Generator with Yield_ expression(s)
      *
      * @param \PhpParser\Node[] $nodes
      * @return \Generator
      */
-    protected function findReturnStatement(array $nodes)
+    protected function findYieldExpression(array $nodes)
     {
-        return $this->findNode($nodes, Return_::class);
+        return $this->findNode($nodes, Yield_::class);
     }
 
     /**
-     * Return \Generator with Yield_ expression(s)
-     *
      * @param \PhpParser\Node[] $nodes
      * @return \Generator
      */
-    protected function findYieldExpression(array $nodes)
+    protected function findReturnStatement(array $nodes)
     {
-        return $this->findNode($nodes, Yield_::class);
+        return $this->findNode($nodes, Return_::class);
     }
 
     /**

src/Analyzer/Pass/Statement/ReturnAndYieldInOneMethod.php

@@ -0,0 +1,49 @@
+<?php
+
+namespace PHPSA\Analyzer\Pass\Statement;
+
+use PhpParser\Node\Stmt;
+use PHPSA\Analyzer\Helper\DefaultMetadataPassTrait;
+use PHPSA\Analyzer\Helper\ResolveExpressionTrait;
+use PHPSA\Analyzer\Pass;
+use PhpParser\Node\Expr;
+use PHPSA\Context;
+
+class ReturnAndYieldInOneMethod implements Pass\AnalyzerPassInterface
+{
+    const DESCRIPTION = 'Checks for using return and yield statements in a one method and discourages it.';
+
+    use DefaultMetadataPassTrait;
+    use ResolveExpressionTrait;
+
+    /**
+     * @param Stmt $stmt
+     * @param Context $context
+     * @return bool
+     */
+    public function pass(Stmt $stmt, Context $context)
+    {
+        $yieldExists = \PHPSA\generatorHasValue($this->findYieldExpression([$stmt]));
+        if (!$yieldExists) {
+            // YieldFrom is another expression
+            $yieldExists = \PHPSA\generatorHasValue($this->findNode([$stmt], Expr\YieldFrom::class));
+        }
+
+        if ($yieldExists && \PHPSA\generatorHasValue($this->findReturnStatement([$stmt]))) {
+            $context->notice('return_and_yield_in_one_method', 'Do not use return and yield in a one method', $stmt);
+            return true;
+        }
+
+        return false;
+    }
+
+    /**
+     * @return array
+     */
+    public function getRegister()
+    {
+        return [
+            Stmt\ClassMethod::class,
+        ];
+    }
+}

src/functions.php

@@ -22,3 +22,18 @@ function nodeVisitorFactory($stmt, Context $context)
 
     return $context->getExpressionCompiler()->compile($stmt);
 }
+
+/**
+ * Especial to protect HHVM: exception: Need to call next() first
+ *
+ * @param \Generator $generator
+ * @return bool
+ */
+function generatorHasValue(\Generator $generator)
+{
+    foreach ($generator as $v) {
+        return true;
+    }
+
+    return false;
+}

tests/analyze-fixtures/Statement/ReturnAndYieldInOneMethod.php

@@ -0,0 +1,63 @@
+<?php
+
+namespace Tests\Analyze\Fixtures\Statement;
+
+class ReturnAndYieldInOneMethod
+{
+    public function testReturnAndYield($a = true)
+    {
+        if ($a) {
+            return $a;
+        }
+
+        yield $a;
+    }
+
+    public function testReturnAndYieldFrom($a = true)
+    {
+        if ($a) {
+            return $a;
+        }
+
+        yield from [1, 2, 3, 4, 5];
+    }
+
+    public function testReturnOnly($a = true)
+    {
+        if ($a) {
+            return $a;
+        }
+
+        return !$a;
+    }
+
+    public function testYieldOnly($a = true)
+    {
+        if ($a) {
+            yield $a;
+        }
+    }
+
+    public function testVoid(&$a)
+    {
+        $a = false;
+    }
+}
+?>
+----------------------------
+PHPSA\Analyzer\Pass\Statement\ReturnAndYieldInOneMethod
+----------------------------
+[
+    {
+        "type": "return_and_yield_in_one_method",
+        "message": "Do not use return and yield in a one method",
+        "file": "ReturnAndYieldInOneMethod.php",
+        "line": 6
+    },
+    {
+        "type": "return_and_yield_in_one_method",
+        "message": "Do not use return and yield in a one method",
+        "file": "ReturnAndYieldInOneMethod.php",
+        "line": 15
+    }
+]