ReturnTypeFromStrictTernaryRector: Support complex ternaries (#4515)

rules-tests/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector/Fixture/function.php.inc

@@ -0,0 +1,21 @@
+<?php
+
+namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;
+
+function getValue($number)
+{
+    return $number ? 'a' : 'b';
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;
+
+function getValue($number): string
+{
+    return $number ? 'a' : 'b';
+}
+
+?>

rules-tests/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector/Fixture/skip_phpdocs.php.inc

@@ -0,0 +1,22 @@
+<?php
+
+namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;
+
+final class SkipPhpdocs
+{
+    public function getValue()
+    {
+        $x = false;
+        if (rand(0,1)) {
+            $x = $this->get();
+        }
+        return $x ?: null;
+    }
+
+    /**
+     * @return SkipPhpdocs
+     */
+    public function get() {}
+}
+
+?>

rules-tests/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector/Fixture/skip_wrong_return.php.inc

@@ -0,0 +1,24 @@
+<?php
+
+namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;
+
+final class SkipWrongReturn
+{
+    public function getValue()
+    {
+        $x = false;
+        if (rand(0,1)) {
+            $x = $this->get();
+        }
+
+        if (rand(0,1)) {
+            return $x ?: null;
+        }
+
+        // missing return
+    }
+
+    public function get(): SkipWrongReturn {}
+}
+
+?>

rules-tests/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector/Fixture/ternary_short_var_call.php.inc

@@ -0,0 +1,31 @@
+<?php
+
+namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;
+
+final class CallInShortTernaryVar
+{
+    public function getValue()
+    {
+        return $this->get() ?: null;
+    }
+
+    public function get(): CallInShortTernaryVar|false {}
+}
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture;
+
+final class CallInShortTernaryVar
+{
+    public function getValue(): ?\Rector\Tests\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector\Fixture\CallInShortTernaryVar
+    {
+        return $this->get() ?: null;
+    }
+
+    public function get(): CallInShortTernaryVar|false {}
+}
+
+?>

rules-tests/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector/config/configured_rule.php

@@ -4,8 +4,10 @@
 
 use Rector\Config\RectorConfig;
 
+use Rector\Core\ValueObject\PhpVersionFeature;
 use Rector\TypeDeclaration\Rector\Class_\ReturnTypeFromStrictTernaryRector;
 
 return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->phpVersion(PhpVersionFeature::NULLABLE_TYPE);
     $rectorConfig->rule(ReturnTypeFromStrictTernaryRector::class);
 };

rules/TypeDeclaration/Rector/Class_/ReturnTypeFromStrictTernaryRector.php

@@ -7,18 +7,25 @@
 use PhpParser\Node;
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\ClassConstFetch;
+use PhpParser\Node\Expr\Closure;
 use PhpParser\Node\Expr\ConstFetch;
 use PhpParser\Node\Expr\Ternary;
 use PhpParser\Node\Scalar;
 use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Function_;
 use PhpParser\Node\Stmt\Return_;
 use PHPStan\Analyser\Scope;
 use PHPStan\Type\ConstantType;
 use PHPStan\Type\GeneralizePrecision;
+use PHPStan\Type\MixedType;
 use PHPStan\Type\Type;
+use PHPStan\Type\TypeCombinator;
+use PHPStan\Type\UnionType;
 use Rector\Core\Rector\AbstractScopeAwareRector;
 use Rector\Core\ValueObject\PhpVersionFeature;
 use Rector\PHPStanStaticTypeMapper\Enum\TypeKind;
+use Rector\TypeDeclaration\TypeInferer\ReturnTypeInferer;
 use Rector\TypeDeclaration\ValueObject\TernaryIfElseTypes;
 use Rector\VendorLocker\NodeVendorLocker\ClassMethodReturnTypeOverrideGuard;
 use Rector\VersionBonding\Contract\MinPhpVersionInterface;
@@ -31,7 +38,8 @@
 final class ReturnTypeFromStrictTernaryRector extends AbstractScopeAwareRector implements MinPhpVersionInterface
 {
     public function __construct(
-        private readonly ClassMethodReturnTypeOverrideGuard $classMethodReturnTypeOverrideGuard
+        private readonly ClassMethodReturnTypeOverrideGuard $classMethodReturnTypeOverrideGuard,
+        private readonly ReturnTypeInferer $returnTypeInferer
     ) {
     }
 
@@ -68,119 +76,71 @@ public function getValue($number): int
      */
     public function getNodeTypes(): array
     {
-        return [Class_::class];
+        return [ClassMethod::class, Function_::class, Closure::class];
     }
 
     /**
-     * @param Class_ $node
+     * @param ClassMethod|Function_|Closure $node
      */
     public function refactorWithScope(Node $node, Scope $scope): ?Node
     {
-        $hasChanged = false;
-
-        foreach ($node->getMethods() as $classMethod) {
-            if ($classMethod->returnType instanceof Node) {
-                continue;
-            }
-
-            $onlyStmt = $classMethod->stmts[0] ?? null;
-            if (! $onlyStmt instanceof Return_) {
-                continue;
-            }
-
-            if (! $onlyStmt->expr instanceof Ternary) {
-                continue;
-            }
-
-            $ternary = $onlyStmt->expr;
-
-            // has scalar in if/else of ternary
-            $ternaryIfElseTypes = $this->matchScalarTernaryIfElseTypes($ternary);
-            if (! $ternaryIfElseTypes instanceof TernaryIfElseTypes) {
-                continue;
-            }
-
-            $ifType = $ternaryIfElseTypes->getFirstType();
-            $elseType = $ternaryIfElseTypes->getSecondType();
+        if ($this->shouldSkip($node, $scope)) {
+            return null;
+        }
 
-            if (! $this->areTypesEqual($ifType, $elseType)) {
-                continue;
-            }
+        if ($node->stmts === null) {
+            return null;
+        }
 
-            if ($this->classMethodReturnTypeOverrideGuard->shouldSkipClassMethod($classMethod, $scope)) {
-                continue;
-            }
+        $returns = $this->betterNodeFinder->findInstanceOf($node->stmts, Return_::class);
+        if (count($returns) !== 1) {
+            return null;
+        }
 
-            $returnTypeNode = $this->staticTypeMapper->mapPHPStanTypeToPhpParserNode($ifType, TypeKind::RETURN);
-            if (! $returnTypeNode instanceof Node) {
-                continue;
-            }
+        $return = $returns[0];
+        if (! $return->expr instanceof Ternary) {
+            return null;
+        }
+        $ternary = $return->expr;
 
-            $classMethod->returnType = $returnTypeNode;
-            $hasChanged = true;
+        $nativeTernaryType = $scope->getNativeType($ternary);
+        if ($nativeTernaryType instanceof MixedType) {
+            return null;
         }
 
-        if ($hasChanged) {
-            return $node;
+        $ternaryType = $this->staticTypeMapper->mapPhpParserNodePHPStanType($ternary);
+        $returnTypeNode = $this->staticTypeMapper->mapPHPStanTypeToPhpParserNode($ternaryType, TypeKind::RETURN);
+        if (! $returnTypeNode instanceof Node) {
+            return null;
         }
 
-        return null;
+        $node->returnType = $returnTypeNode;
+        return $node;
     }
 
     public function provideMinPhpVersion(): int
     {
         return PhpVersionFeature::SCALAR_TYPES;
     }
 
-    private function isAlwaysScalarExpr(?Expr $expr): bool
-    {
-        // check if Scalar node
-        if ($expr instanceof Scalar) {
+    private function shouldSkip(ClassMethod|Function_|Closure $node, Scope $scope): bool {
+        if ($node->returnType !== null) {
             return true;
         }
 
-        // check if constant
-        if ($expr instanceof ConstFetch) {
+        $returnType = $this->returnTypeInferer->inferFunctionLike($node);
+        $returnType = TypeCombinator::removeNull($returnType);
+        if ($returnType instanceof UnionType) {
             return true;
         }
 
-        // check if class constant
-        return $expr instanceof ClassConstFetch;
-    }
-
-    private function areTypesEqual(Type $firstType, Type $secondType): bool
-    {
-        // this is needed to make comparison tolerant to constant values, e.g. 5 and 10 are same only then
-        if ($firstType instanceof ConstantType) {
-            $firstType = $firstType->generalize(GeneralizePrecision::lessSpecific());
-        }
-
-        if ($secondType instanceof ConstantType) {
-            $secondType = $secondType->generalize(GeneralizePrecision::lessSpecific());
+        if ($node instanceof ClassMethod) {
+            if ($this->classMethodReturnTypeOverrideGuard->shouldSkipClassMethod($node, $scope)) {
+                return true;
+            }
         }
 
-        return $firstType->equals($secondType);
+        return false;
     }
 
-    private function matchScalarTernaryIfElseTypes(Ternary $ternary): ?TernaryIfElseTypes
-    {
-        if (! $this->isAlwaysScalarExpr($ternary->if)) {
-            return null;
-        }
-
-        if (! $this->isAlwaysScalarExpr($ternary->else)) {
-            return null;
-        }
-
-        /** @var Node\Expr $if */
-        $if = $ternary->if;
-
-        /** @var Node\Expr $else */
-        $else = $ternary->else;
-
-        $ifType = $this->staticTypeMapper->mapPhpParserNodePHPStanType($if);
-        $elseType = $this->staticTypeMapper->mapPhpParserNodePHPStanType($else);
-
-        return new TernaryIfElseTypes($ifType, $elseType);
-    }
 }