From 0ab4c2ac4bbe9ffcd3e082421df930913426b36e Mon Sep 17 00:00:00 2001
From: tuqqu <artkurbidaev@gmail.com>
Date: Sun, 24 Sep 2023 20:58:10 +0200
Subject: [PATCH] Introduce NonVariableReferenceReturn issue

---
 config.xsd                                    |  1 +
 docs/contributing/adding_issues.md            |  4 +-
 docs/running_psalm/error_levels.md            |  1 +
 docs/running_psalm/issues.md                  |  1 +
 .../issues/NonVariableReferenceReturn.md      | 11 ++++
 .../Analyzer/Statements/ReturnAnalyzer.php    | 18 +++++
 .../Issue/NonVariableReferenceReturn.php      | 11 ++++
 tests/ClosureTest.php                         | 28 ++++++++
 tests/ReturnTypeTest.php                      | 65 +++++++++++++++++++
 9 files changed, 138 insertions(+), 2 deletions(-)
 create mode 100644 docs/running_psalm/issues/NonVariableReferenceReturn.md
 create mode 100644 src/Psalm/Issue/NonVariableReferenceReturn.php

diff --git a/config.xsd b/config.xsd
index eb5f11e2c21..3d5deb61e90 100644
--- a/config.xsd
+++ b/config.xsd
@@ -350,6 +350,7 @@
             <xs:element name="NonInvariantDocblockPropertyType" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="NonInvariantPropertyType" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="NonStaticSelfCall" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="NonVariableReferenceReturn" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="NoValue" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="NullableReturnStatement" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="NullArgument" type="ArgumentIssueHandlerType" minOccurs="0" />
diff --git a/docs/contributing/adding_issues.md b/docs/contributing/adding_issues.md
index b609d872837..4cc2fabb4b9 100644
--- a/docs/contributing/adding_issues.md
+++ b/docs/contributing/adding_issues.md
@@ -17,8 +17,8 @@ namespace Psalm\Issue;
 
 final class MyNewIssue extends CodeIssue 
 {
-    public const SHORTCODE = 123;
     public const ERROR_LEVEL = 2;
+    public const SHORTCODE = 123;
 }
 ```
 
@@ -26,7 +26,7 @@ For `SHORTCODE` value use `$max_shortcode + 1`. To choose appropriate error leve
 
 There a number of abstract classes you can extend:
 
-* `CodeIssue` - non specific, default issue. It's a base class for all issues.
+* `CodeIssue` - non-specific, default issue. It's a base class for all issues.
 * `ClassIssue` - issue related to a specific class (also interface, trait, enum). These issues can be suppressed for specific classes in `psalm.xml` by using `referencedClass` attribute
 * `PropertyIssue` - issue related to a specific property. Can be targeted by using `referencedProperty` in `psalm.xml`
 * `FunctionIssue` - issue related to a specific function. Can be suppressed with `referencedFunction` attribute.
diff --git a/docs/running_psalm/error_levels.md b/docs/running_psalm/error_levels.md
index 90b5d5351b3..ca2ba58e4e4 100644
--- a/docs/running_psalm/error_levels.md
+++ b/docs/running_psalm/error_levels.md
@@ -100,6 +100,7 @@ Level 5 and above allows a more non-verifiable code, and higher levels are even
 - [ContinueOutsideLoop](issues/ContinueOutsideLoop.md)
 - [InvalidTypeImport](issues/InvalidTypeImport.md)
 - [MethodSignatureMismatch](issues/MethodSignatureMismatch.md)
+- [NonVariableReferenceReturn](issues/NonVariableReferenceReturn.md)
 - [OverriddenMethodAccess](issues/OverriddenMethodAccess.md)
 - [ParamNameMismatch](issues/ParamNameMismatch.md)
 - [ReservedWord](issues/ReservedWord.md)
diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md
index 592225002e7..d05e070a4e7 100644
--- a/docs/running_psalm/issues.md
+++ b/docs/running_psalm/issues.md
@@ -151,6 +151,7 @@
  - [NonInvariantDocblockPropertyType](issues/NonInvariantDocblockPropertyType.md)
  - [NonInvariantPropertyType](issues/NonInvariantPropertyType.md)
  - [NonStaticSelfCall](issues/NonStaticSelfCall.md)
+ - [NonVariableReferenceReturn](issues/NonVariableReferenceReturn.md)
  - [NoValue](issues/NoValue.md)
  - [NullableReturnStatement](issues/NullableReturnStatement.md)
  - [NullArgument](issues/NullArgument.md)
diff --git a/docs/running_psalm/issues/NonVariableReferenceReturn.md b/docs/running_psalm/issues/NonVariableReferenceReturn.md
new file mode 100644
index 00000000000..5dcc759898d
--- /dev/null
+++ b/docs/running_psalm/issues/NonVariableReferenceReturn.md
@@ -0,0 +1,11 @@
+# NonVariableReferenceReturn
+
+Emitted when a function returns by reference expression that is not a variable
+
+```php
+<?php
+
+function &getByRef(): int {
+    return 5;
+}
+```
diff --git a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php
index 2ea1e981544..e8724e93969 100644
--- a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php
+++ b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php
@@ -32,6 +32,7 @@
 use Psalm\Issue\MixedReturnStatement;
 use Psalm\Issue\MixedReturnTypeCoercion;
 use Psalm\Issue\NoValue;
+use Psalm\Issue\NonVariableReferenceReturn;
 use Psalm\Issue\NullableReturnStatement;
 use Psalm\IssueBuffer;
 use Psalm\Storage\FunctionLikeStorage;
@@ -227,6 +228,23 @@ public static function analyze(
 
             $storage = $source->getFunctionLikeStorage($statements_analyzer);
 
+            if ($storage->signature_return_type
+                && $storage->signature_return_type->by_ref
+                && $stmt->expr !== null
+                && !($stmt->expr instanceof PhpParser\Node\Expr\Variable
+                    || $stmt->expr instanceof PhpParser\Node\Expr\PropertyFetch
+                    || $stmt->expr instanceof PhpParser\Node\Expr\StaticPropertyFetch
+                )
+            ) {
+                IssueBuffer::maybeAdd(
+                    new NonVariableReferenceReturn(
+                        'Only variable references should be returned by reference',
+                        new CodeLocation($source, $stmt->expr),
+                    ),
+                    $statements_analyzer->getSuppressedIssues(),
+                );
+            }
+
             $cased_method_id = $source->getCorrectlyCasedMethodId();
 
             if ($stmt->expr && $storage->location) {
diff --git a/src/Psalm/Issue/NonVariableReferenceReturn.php b/src/Psalm/Issue/NonVariableReferenceReturn.php
new file mode 100644
index 00000000000..83d26049550
--- /dev/null
+++ b/src/Psalm/Issue/NonVariableReferenceReturn.php
@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\Issue;
+
+final class NonVariableReferenceReturn extends CodeIssue
+{
+    public const ERROR_LEVEL = 7;
+    public const SHORTCODE = 323;
+}
diff --git a/tests/ClosureTest.php b/tests/ClosureTest.php
index 43d441944fa..43fff6bb036 100644
--- a/tests/ClosureTest.php
+++ b/tests/ClosureTest.php
@@ -957,6 +957,20 @@ function () {
                     }
                     PHP,
             ],
+            'returnByReferenceVariableInClosure' => [
+                'code' => '<?php
+                    function &(): int {
+                        /** @var int $x */
+                        static $x = 1;
+                        return $x;
+                    };
+                ',
+            ],
+            'returnByReferenceVariableInShortClosure' => [
+                'code' => '<?php
+                    fn &(int &$x): int => $x;
+                ',
+            ],
         ];
     }
 
@@ -1428,6 +1442,20 @@ public function f(): int {
                 'ignored_issues' => [],
                 'php_version' => '8.1',
             ],
+            'returnByReferenceNonVariableInClosure' => [
+                'code' => '<?php
+                    function &(): int {
+                        return 45;
+                    };
+                ',
+                'error_message' => 'NonVariableReferenceReturn',
+            ],
+            'returnByReferenceNonVariableInShortClosure' => [
+                'code' => '<?php
+                    fn &(): int => 45;
+                ',
+                'error_message' => 'NonVariableReferenceReturn',
+            ],
         ];
     }
 }
diff --git a/tests/ReturnTypeTest.php b/tests/ReturnTypeTest.php
index 2557fdf8abe..275f093c08e 100644
--- a/tests/ReturnTypeTest.php
+++ b/tests/ReturnTypeTest.php
@@ -1279,6 +1279,40 @@ function aggregate($type) {
                         return $t;
                     }',
             ],
+            'returnByReferenceVariableInStaticMethod' => [
+                'code' => <<<'PHP'
+                    <?php
+                    class Foo {
+                        private static string $foo = "foo";
+
+                        public static function &foo(): string {
+                            return self::$foo;
+                        }
+                    }
+                    PHP,
+            ],
+            'returnByReferenceVariableInInstanceMethod' => [
+                'code' => <<<'PHP'
+                    <?php
+                    class Foo {
+                        private float $foo = 3.3;
+
+                        public function &foo(): float {
+                            return $this->foo;
+                        }
+                    }
+                    PHP,
+            ],
+            'returnByReferenceVariableInFunction' => [
+                'code' => <<<'PHP'
+                    <?php
+                    function &foo(): array {
+                        /** @var array $x */
+                        static $x = [1, 2, 3];
+                        return $x;
+                    }
+                    PHP,
+            ],
         ];
     }
 
@@ -1807,6 +1841,37 @@ public function process(): mixed
                 'ignored_issues' => [],
                 'php_version' => '8.0',
             ],
+            'returnByReferenceNonVariableInStaticMethod' => [
+                'code' => <<<'PHP'
+                    <?php
+                    class Foo {
+                        public static function &foo(string $x): string {
+                            return $x . "bar";
+                        }
+                    }
+                    PHP,
+                'error_message' => 'NonVariableReferenceReturn',
+            ],
+            'returnByReferenceNonVariableInInstanceMethod' => [
+                'code' => <<<'PHP'
+                    <?php
+                    class Foo {
+                        public function &foo(): iterable {
+                            return [] + [1, 2];
+                        }
+                    }
+                    PHP,
+                'error_message' => 'NonVariableReferenceReturn',
+            ],
+            'returnByReferenceNonVariableInFunction' => [
+                'code' => <<<'PHP'
+                    <?php
+                    function &foo(): array {
+                        return [1, 2, 3];
+                    }
+                    PHP,
+                'error_message' => 'NonVariableReferenceReturn',
+            ],
         ];
     }
 }