Merge pull request #10505 from kkmuffme/fix-readonly-properties-by-re…

…ference-not-reported Add error when using readonly property in by-ref arg
vimeo · Jan 9, 2024 · b4ad9c0 · b4ad9c0
2 parents 6c98f25 + 9e463bb
commit b4ad9c0
Show file tree

Hide file tree

Showing 3 changed files with 156 additions and 0 deletions.
diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php
@@ -2,11 +2,13 @@
 
 namespace Psalm\Internal\Analyzer\Statements\Expression\Call;
 
+use InvalidArgumentException;
 use PhpParser;
 use Psalm\CodeLocation;
 use Psalm\Codebase;
 use Psalm\Context;
 use Psalm\Internal\Analyzer\AttributesAnalyzer;
+use Psalm\Internal\Analyzer\Statements\Expression\Assignment\InstancePropertyAssignmentAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\AssignmentAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\CallAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\ExpressionIdentifier;
@@ -45,6 +47,7 @@
 use Psalm\Type\Atomic\TKeyedArray;
 use Psalm\Type\Atomic\TList;
 use Psalm\Type\Atomic\TLiteralString;
+use Psalm\Type\Atomic\TNamedObject;
 use Psalm\Type\Atomic\TNonEmptyArray;
 use Psalm\Type\Atomic\TTemplateParam;
 use Psalm\Type\Union;
@@ -1253,6 +1256,42 @@ private static function evaluateArbitraryParam(
         return null;
     }
 
+    private static function handleByRefReadonlyArg(
+        StatementsAnalyzer $statements_analyzer,
+        Context $context,
+        PhpParser\Node\Expr\PropertyFetch $stmt,
+        string $fq_class_name,
+        string $prop_name
+    ): void {
+        $property_id = $fq_class_name . '::$' . $prop_name;
+
+        $codebase = $statements_analyzer->getCodebase();
+        $declaring_property_class = (string) $codebase->properties->getDeclaringClassForProperty(
+            $property_id,
+            true,
+            $statements_analyzer,
+        );
+
+        try {
+            $declaring_class_storage = $codebase->classlike_storage_provider->get($declaring_property_class);
+        } catch (InvalidArgumentException $_) {
+            return;
+        }
+
+        if (isset($declaring_class_storage->properties[$prop_name])) {
+            $property_storage = $declaring_class_storage->properties[$prop_name];
+
+            InstancePropertyAssignmentAnalyzer::trackPropertyImpurity(
+                $statements_analyzer,
+                $stmt,
+                $property_id,
+                $property_storage,
+                $declaring_class_storage,
+                $context,
+            );
+        }
+    }
+
     /**
      * @return false|null
      */
@@ -1274,6 +1313,46 @@ private static function handleByRefFunctionArg(
             'reset', 'end', 'next', 'prev', 'array_pop', 'array_shift',
         ];
 
+        if ($arg->value instanceof PhpParser\Node\Expr\PropertyFetch
+            && $arg->value->name instanceof PhpParser\Node\Identifier) {
+            $prop_name = $arg->value->name->name;
+            if (!empty($statements_analyzer->getFQCLN())) {
+                $fq_class_name = $statements_analyzer->getFQCLN();
+
+                self::handleByRefReadonlyArg(
+                    $statements_analyzer,
+                    $context,
+                    $arg->value,
+                    $fq_class_name,
+                    $prop_name,
+                );
+            } else {
+                // @todo atm only works for simple fetch, $a->foo, not $a->foo->bar
+                // I guess there's a function to do this, but I couldn't locate it
+                $var_id = ExpressionIdentifier::getVarId(
+                    $arg->value->var,
+                    $statements_analyzer->getFQCLN(),
+                    $statements_analyzer,
+                );
+
+                if ($var_id && isset($context->vars_in_scope[$var_id])) {
+                    foreach ($context->vars_in_scope[$var_id]->getAtomicTypes() as $atomic_type) {
+                        if ($atomic_type instanceof TNamedObject) {
+                            $fq_class_name = $atomic_type->value;
+
+                            self::handleByRefReadonlyArg(
+                                $statements_analyzer,
+                                $context,
+                                $arg->value,
+                                $fq_class_name,
+                                $prop_name,
+                            );
+                        }
+                    }
+                }
+            }
+        }
+
         if (($var_id && isset($context->vars_in_scope[$var_id]))
             || ($method_id
                 && in_array(

diff --git a/src/Psalm/Type/Atomic/TKeyedArray.php b/src/Psalm/Type/Atomic/TKeyedArray.php
@@ -112,6 +112,8 @@ public function setProperties(array $properties): self
         if ($cloned->is_list) {
             $last_k = -1;
             $had_possibly_undefined = false;
+
+            /** @psalm-suppress InaccessibleProperty */
             ksort($cloned->properties);
             foreach ($cloned->properties as $k => $v) {
                 if (is_string($k) || $last_k !== ($k-1) || ($had_possibly_undefined && !$v->possibly_undefined)) {

diff --git a/tests/ImmutableAnnotationTest.php b/tests/ImmutableAnnotationTest.php
@@ -764,6 +764,81 @@ public function getShortMutating() : string {
                     }',
                 'error_message' => 'ImpurePropertyAssignment',
             ],
+            'readonlyByRefInClass' => [
+                'code' => '<?php
+                    namespace World;
+
+                    final class Foo
+                    {
+                        /**
+                         * @readonly
+                         */
+                        public array $values;
+
+                        public function __construct(array $values)
+                        {
+                            $this->values = $values;
+                        }
+
+                        /**
+                         * @return mixed
+                         */
+                        public function bar()
+                        {
+                            return reset($this->values);
+                        }
+                    }',
+                'error_message' => 'InaccessibleProperty',
+            ],
+            'readonlyByRef' => [
+                'code' => '<?php
+                    namespace World;
+
+                    final class Foo
+                    {
+                        /**
+                         * @readonly
+                         */
+                        public array $values;
+
+                        public function __construct(array $values)
+                        {
+                            $this->values = $values;
+                        }
+                    }
+
+                    $x = new Foo([]);
+                    reset($x->values);',
+                'error_message' => 'InaccessibleProperty',
+            ],
+            'readonlyByRefCustomFunction' => [
+                'code' => '<?php
+                    namespace World;
+
+                    final class Foo
+                    {
+                        /**
+                         * @readonly
+                         */
+                        public array $values;
+
+                        public function __construct(array $values)
+                        {
+                            $this->values = $values;
+                        }
+                    }
+
+                    /**
+                     * @param string $a
+                     * @param array $b
+                     * @return void
+                     */
+                    function bar($a, &$b) {}
+
+                    $x = new Foo([]);
+                    bar("hello", $x->values);',
+                'error_message' => 'InaccessibleProperty',
+            ],
             'preventUnset' => [
                 'code' => '<?php
                     /**