Merge pull request #10227 from tuqqu/non-variable-reference-return

Introduce NonVariableReferenceReturn issue

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" />

docs/contributing/adding_issues.md

@@ -17,16 +17,16 @@ namespace Psalm\Issue;
 
 final class MyNewIssue extends CodeIssue 
 {
-    public const SHORTCODE = 123;
     public const ERROR_LEVEL = 2;
+    public const SHORTCODE = 123;
 }
 ```
 
 For `SHORTCODE` value use `$max_shortcode + 1`. To choose appropriate error level see [Error levels](../running_psalm/error_levels.md).
 
 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.

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)

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)

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;
+}
+```

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) {

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;
+}

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',
+            ],
         ];
     }
 }

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',
+            ],
         ];
     }
 }