Add support for references and improve UnusedVariable checks (fixes v…

…imeo#7254).

config.xsd

@@ -403,6 +403,7 @@
             <xs:element name="RedundantPropertyInitializationCheck" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="RedundantIdentityWithTrue" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="ReferenceConstraintViolation" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="ReferenceReusedFromConfusingScope" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="ReservedWord" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="StringIncrement" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedCallable" type="IssueHandlerType" minOccurs="0" />

docs/running_psalm/issues/ReferenceReusedFromConfusingScope.md

@@ -0,0 +1,39 @@
+# ReferenceReusedFromConfusingScope
+
+Emitted when a reference assigned in a potentially confusing scope is reused later.
+Since PHP doesn't scope loops and ifs the same way most other languages do, a common issue is the reuse of a variable
+declared in such a scope. Usually it doesn't matter, because a reassignment to an already-defined variable will just
+reassign it, but if the already-defined variable is a reference it will change the value of the referred to variable.
+
+```php
+<?php
+
+$arr = [1, 2, 3];
+foreach ($arr as &$i) {
+    ++$i;
+}
+
+// ...more code, after which the usage of $i as a reference has been forgotten by the programmer
+
+for ($i = 0; $i < 10; ++$i) {
+    echo $i;
+}
+// $arr is now [2, 3, 10] instead of the expected [2, 3, 4]
+```
+
+To fix the issue, unset the reference after using it in such a scope:
+
+```php
+<?php
+
+$arr = [1, 2, 3];
+foreach ($arr as &$i) {
+    ++$i;
+}
+unset($i);
+
+for ($i = 0; $i < 10; ++$i) {
+    echo $i;
+}
+// $arr is correctly [2, 3, 4]
+```

src/Psalm/Codebase.php

@@ -1967,9 +1967,7 @@ public function addTaintSource(
 
         $this->taint_flow_graph->addSource($source);
 
-        $expr_type->parent_nodes = [
-            $source->id => $source,
-        ];
+        $expr_type->parent_nodes[$source->id] = $source;
     }
 
     /**

src/Psalm/Context.php

@@ -14,6 +14,7 @@
 use Psalm\Type\Atomic\DependentType;
 use Psalm\Type\Atomic\TArray;
 use Psalm\Type\Union;
+use RuntimeException;
 
 use function array_keys;
 use function array_search;
@@ -41,6 +42,54 @@ class Context
      */
     public $vars_possibly_in_scope = [];
 
+    /**
+     * Keeps track of how many times a var_in_scope has been referenced. May not be set for all $vars_in_scope.
+     *
+     * @var array<string, int<0, max>>
+     */
+    public $referenced_counts = [];
+
+    /**
+     * Maps references to referenced variables for the current scope.
+     * With `$b = &$a`, this will contain `['$b' => '$a']`.
+     *
+     * All keys and values in this array are guaranteed to be set in $vars_in_scope.
+     *
+     * To check if a variable was passed or returned by reference, or
+     * references an object property or array item, see Union::$by_ref.
+     *
+     * @var array<string, string>
+     */
+    public $references_in_scope = [];
+
+    /**
+     * Set of references to variables in another scope. These references will be marked as used if they are assigned to.
+     *
+     * @var array<string, true>
+     */
+    public $references_to_external_scope = [];
+
+    /**
+     * A set of globals that are referenced somewhere.
+     * Ideally this shouldn't be needed and GlobalAnalyzer should add an edge to the
+     * DataFlowGraph pointing from the global to its use in another scope, but since that's
+     * difficult this is used as a workaround to always mark referenced globals as used.
+     *
+     * @internal May be removed if GlobalAnalyzer is improved.
+     *
+     * @var array<string, true>
+     */
+    public $referenced_globals = [];
+
+    /**
+     * A set of references that might still be in scope from a scope likely to cause confusion. This applies
+     * to references set inside a loop or if statement, since it's easy to forget about PHP's weird scope
+     * rules, and assinging to a reference will change the referenced variable rather than shadowing it.
+     *
+     * @var array<string, CodeLocation>
+     */
+    public $references_possibly_from_confusing_scope = [];
+
     /**
      * Whether or not we're inside the conditional of an if/where etc.
      *
@@ -375,11 +424,6 @@ class Context
      */
     public $has_returned = false;
 
-    /**
-     * @var array<string, bool>
-     */
-    public $vars_from_global = [];
-
     public function __construct(?string $self = null)
     {
         $this->self = $self;
@@ -458,6 +502,28 @@ public function update(
         }
     }
 
+    /**
+     * Updates the list of possible references from a confusing scope,
+     * such as a reference created in an if and then later reused.
+     */
+    public function updateReferencesPossiblyFromConfusingScope(
+        Context $confusing_scope_context,
+        StatementsAnalyzer $statements_analyzer
+    ): void {
+        foreach ($confusing_scope_context->references_in_scope + $confusing_scope_context->references_to_external_scope
+            as $reference_id => $_
+        ) {
+            if (!isset($this->references_in_scope[$reference_id])
+                && !isset($this->references_to_external_scope[$reference_id])
+                && $reference_location = $statements_analyzer->getFirstAppearance($reference_id)
+            ) {
+                $this->references_possibly_from_confusing_scope[$reference_id] = $reference_location;
+            }
+        }
+        $this->references_possibly_from_confusing_scope +=
+            $confusing_scope_context->references_possibly_from_confusing_scope;
+    }
+
     /**
      * @param  array<string, Union> $new_vars_in_scope
      *
@@ -505,19 +571,39 @@ public static function getNewOrUpdatedVarIds(Context $original_context, Context
         return $redefined_var_ids;
     }
 
-    public function remove(string $remove_var_id): void
+    public function remove(string $remove_var_id, bool $removeDescendents = true): void
     {
-        unset(
-            $this->referenced_var_ids[$remove_var_id],
-            $this->vars_possibly_in_scope[$remove_var_id]
-        );
-
         if (isset($this->vars_in_scope[$remove_var_id])) {
             $existing_type = $this->vars_in_scope[$remove_var_id];
             unset($this->vars_in_scope[$remove_var_id]);
 
-            $this->removeDescendents($remove_var_id, $existing_type);
+            if ($removeDescendents) {
+                $this->removeDescendents($remove_var_id, $existing_type);
+            }
+        }
+        $this->removePossibleReference($remove_var_id);
+        unset($this->vars_possibly_in_scope[$remove_var_id]);
+    }
+
+    /**
+     * Remove a variable from the context which might be a reference to another variable.
+     * Leaves the variable as possibly-in-scope, unlike remove().
+     */
+    public function removePossibleReference(string $remove_var_id): void
+    {
+        if (isset($this->references_in_scope[$remove_var_id])) {
+            $reference_count = &$this->referenced_counts[$this->references_in_scope[$remove_var_id]];
+            if ($reference_count < 1) {
+                throw new RuntimeException("Incorrect referenced count found");
+            }
+            --$reference_count;
         }
+        unset(
+            $this->vars_in_scope[$remove_var_id],
+            $this->referenced_var_ids[$remove_var_id],
+            $this->references_in_scope[$remove_var_id],
+            $this->references_to_external_scope[$remove_var_id],
+        );
     }
 
     /**
@@ -667,7 +753,7 @@ public function removeDescendents(
 
         foreach ($this->vars_in_scope as $var_id => $type) {
             if (preg_match('/' . preg_quote($remove_var_id, '/') . '[\]\[\-]/', $var_id)) {
-                unset($this->vars_in_scope[$var_id]);
+                $this->remove($var_id, false);
             }
 
             foreach ($type->getAtomicTypes() as $atomic_type) {
@@ -698,7 +784,7 @@ public function removeMutableObjectVars(bool $methods_only = false): void
         }
 
         foreach ($vars_to_remove as $var_id) {
-            unset($this->vars_in_scope[$var_id], $this->vars_possibly_in_scope[$var_id]);
+            $this->remove($var_id, false);
         }
 
         $clauses_to_keep = [];

src/Psalm/Internal/Analyzer/ClosureAnalyzer.php

@@ -160,6 +160,7 @@ public static function analyzeExpression(
 
                 if ($use->byRef) {
                     $use_context->vars_in_scope[$use_var_id]->by_ref = true;
+                    $use_context->references_to_external_scope[$use_var_id] = true;
                 }
 
                 $use_context->vars_possibly_in_scope[$use_var_id] = true;

src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php

@@ -763,7 +763,7 @@ public function analyze(
 
             foreach ($context->vars_in_scope as $var => $_) {
                 if (strpos($var, '$this->') !== 0 && $var !== '$this') {
-                    unset($context->vars_in_scope[$var]);
+                    $context->removePossibleReference($var);
                 }
             }
 
@@ -846,10 +846,6 @@ private function checkParamReferences(
                 throw new UnexpectedValueException('$position should not be false here');
             }
 
-            if ($storage->params[$position]->by_ref) {
-                continue;
-            }
-
             if ($storage->params[$position]->promoted_property) {
                 continue;
             }
@@ -976,6 +972,7 @@ private function processParams(
         $project_analyzer = $statements_analyzer->getProjectAnalyzer();
 
         foreach ($params as $offset => $function_param) {
+            $function_param_id = '$' . $function_param->name;
             $signature_type = $function_param->signature_type;
             $signature_type_location = $function_param->signature_type_location;
 
@@ -1064,7 +1061,7 @@ private function processParams(
                         && !$function_param->type->isBool())
                 ) {
                     $param_assignment = DataFlowNode::getForAssignment(
-                        '$' . $function_param->name,
+                        $function_param_id,
                         $function_param->location
                     );
 
@@ -1082,16 +1079,6 @@ private function processParams(
                         $statements_analyzer->data_flow_graph->addPath($type_source, $param_assignment, 'param');
                     }
 
-                    if ($function_param->by_ref
-                        && $codebase->find_unused_variables
-                    ) {
-                        $statements_analyzer->data_flow_graph->addPath(
-                            $param_assignment,
-                            new DataFlowNode('variable-use', 'variable use', null),
-                            'variable-use'
-                        );
-                    }
-
                     if ($storage->variadic) {
                         $this->param_nodes += [$param_assignment->id => $param_assignment];
                     }
@@ -1100,18 +1087,19 @@ private function processParams(
                 }
             }
 
-            $context->vars_in_scope['$' . $function_param->name] = $var_type;
-            $context->vars_possibly_in_scope['$' . $function_param->name] = true;
+            $context->vars_in_scope[$function_param_id] = $var_type;
+            $context->vars_possibly_in_scope[$function_param_id] = true;
 
             if ($function_param->by_ref) {
-                $context->vars_in_scope['$' . $function_param->name]->by_ref = true;
+                $context->vars_in_scope[$function_param_id]->by_ref = true;
+                $context->references_to_external_scope[$function_param_id] = true;
             }
 
             $parser_param = $this->function->getParams()[$offset] ?? null;
 
             if ($function_param->location) {
                 $statements_analyzer->registerVariable(
-                    '$' . $function_param->name,
+                    $function_param_id,
                     $function_param->location,
                     null
                 );
@@ -1147,7 +1135,7 @@ private function processParams(
 
                     IssueBuffer::maybeAdd(
                         new MismatchingDocblockParamType(
-                            'Parameter $' . $function_param->name . ' has wrong type \'' . $param_type .
+                            'Parameter ' . $function_param_id . ' has wrong type \'' . $param_type .
                                 '\', should be \'' . $signature_type . '\'',
                             $function_param->type_location
                         ),
@@ -1248,13 +1236,6 @@ private function processParams(
                 }
             }
 
-            if ($function_param->by_ref) {
-                // register by ref params as having been used, to avoid false positives
-                // @todo change the assignment analysis *just* for byref params
-                // so that we don't have to do this
-                $context->hasVariable('$' . $function_param->name);
-            }
-
             foreach ($function_param->attributes as $attribute) {
                 AttributeAnalyzer::analyze(
                     $this,

src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php

@@ -298,6 +298,15 @@ public static function analyze(
             $value_type->by_ref = true;
         }
 
+        if ($stmt->byRef
+            && $stmt->valueVar instanceof PhpParser\Node\Expr\Variable
+            && is_string($stmt->valueVar->name)
+        ) {
+            // When assigning as reference, it removes any previous
+            // reference, so it's no longer from a previous confusing scope
+            unset($foreach_context->references_possibly_from_confusing_scope['$' . $stmt->valueVar->name]);
+        }
+
         AssignmentAnalyzer::analyze(
             $statements_analyzer,
             $stmt->valueVar,
@@ -311,6 +320,14 @@ public static function analyze(
                 : []
         );
 
+        if ($stmt->byRef
+            && $stmt->valueVar instanceof PhpParser\Node\Expr\Variable
+            && is_string($stmt->valueVar->name)
+        ) {
+            // TODO support references with destructuring
+            $foreach_context->references_to_external_scope['$' . $stmt->valueVar->name] = true;
+        }
+
         foreach ($var_comments as $var_comment) {
             if (!$var_comment->var_id || !$var_comment->type) {
                 continue;

src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php

@@ -97,7 +97,7 @@ public static function analyze(
                     if (preg_match('/' . preg_quote($changed_var_id, '/') . '[\]\[\-]/', $var_id)
                         && !array_key_exists($var_id, $changed_var_ids)
                     ) {
-                        unset($else_context->vars_in_scope[$var_id]);
+                        $else_context->removePossibleReference($var_id);
                     }
                 }
             }
@@ -239,6 +239,12 @@ public static function analyze(
             $outer_context->mergeExceptions($else_context);
         }
 
+        // Track references set in the else to make sure they aren't reused later
+        $outer_context->updateReferencesPossiblyFromConfusingScope(
+            $else_context,
+            $statements_analyzer
+        );
+
         return null;
     }
 }