From 957600623cecb344775518c4b98a376010453bc1 Mon Sep 17 00:00:00 2001
From: Matthew Brown <github@muglug.com>
Date: Mon, 20 Nov 2017 00:12:17 -0500
Subject: [PATCH] Fix issue with byref template params leaking

---
 .../Statements/Expression/CallChecker.php     | 82 +++++++++++++------
 src/Psalm/Stubs/CoreGenericFunctions.php      |  6 +-
 tests/TemplateTest.php                        | 21 +++++
 3 files changed, 81 insertions(+), 28 deletions(-)

diff --git a/src/Psalm/Checker/Statements/Expression/CallChecker.php b/src/Psalm/Checker/Statements/Expression/CallChecker.php
index 28edec812..f695a5f64 100644
--- a/src/Psalm/Checker/Statements/Expression/CallChecker.php
+++ b/src/Psalm/Checker/Statements/Expression/CallChecker.php
@@ -1768,25 +1768,13 @@ class CallChecker
                         ) === false) {
                             return false;
                         }
-
-                        continue;
                     }
-
-                    ExpressionChecker::assignByRefParam(
-                        $statements_checker,
-                        $arg->value,
-                        $by_ref_type,
-                        $context,
-                        $method_id && (strpos($method_id, '::') !== false || !FunctionChecker::inCallMap($method_id))
-                    );
                 } else {
                     if ($arg->value instanceof PhpParser\Node\Expr\Variable) {
                         if (ExpressionChecker::analyzeVariable(
                             $statements_checker,
                             $arg->value,
-                            $context,
-                            $by_ref,
-                            $by_ref_type
+                            $context
                         ) === false) {
                             return false;
                         }
@@ -1922,24 +1910,66 @@ class CallChecker
 
             if ($function_param
                 && $function_param->by_ref
-                && ($arg->value instanceof PhpParser\Node\Scalar
+            ) {
+                if ($arg->value instanceof PhpParser\Node\Scalar
                     || $arg->value instanceof PhpParser\Node\Expr\ClassConstFetch
                     || $arg->value instanceof PhpParser\Node\Expr\ConstFetch
                     || $arg->value instanceof PhpParser\Node\Expr\FuncCall
                     || $arg->value instanceof PhpParser\Node\Expr\MethodCall
-                )
-            ) {
-                if (IssueBuffer::accepts(
-                    new InvalidPassByReference(
-                        'Parameter ' . ($argument_offset + 1) . ' of ' . $method_id . ' expects a variable',
-                        $code_location
-                    ),
-                    $statements_checker->getSuppressedIssues()
-                )) {
-                    return false;
+                ) {
+                    if (IssueBuffer::accepts(
+                        new InvalidPassByReference(
+                            'Parameter ' . ($argument_offset + 1) . ' of ' . $method_id . ' expects a variable',
+                            $code_location
+                        ),
+                        $statements_checker->getSuppressedIssues()
+                    )) {
+                        return false;
+                    }
+
+                    continue;
                 }
 
-                break;
+                if (!in_array(
+                    $method_id,
+                    [
+                        'shuffle', 'sort', 'rsort', 'usort', 'ksort', 'asort',
+                        'krsort', 'arsort', 'natcasesort', 'natsort', 'reset',
+                        'end', 'next', 'prev', 'array_pop', 'array_shift',
+                        'array_push', 'array_unshift',
+                    ],
+                    true
+                )) {
+                    $by_ref_type = null;
+
+                    if ($last_param) {
+                        if ($argument_offset < count($function_params)) {
+                            $by_ref_type = $function_params[$argument_offset]->type;
+                        } else {
+                            $by_ref_type = $last_param->type;
+                        }
+
+                        if ($template_types && $by_ref_type) {
+                            if ($generic_params === null) {
+                                $generic_params = [];
+                            }
+
+                            $by_ref_type = clone $by_ref_type;
+
+                            $by_ref_type->replaceTemplateTypes($template_types, $generic_params);
+                        }
+                    }
+
+                    $by_ref_type = $by_ref_type ?: Type::getMixed();
+
+                    ExpressionChecker::assignByRefParam(
+                        $statements_checker,
+                        $arg->value,
+                        $by_ref_type,
+                        $context,
+                        $method_id && (strpos($method_id, '::') !== false || !FunctionChecker::inCallMap($method_id))
+                    );
+                }
             }
 
             if (isset($arg->value->inferredType)) {
@@ -1948,7 +1978,7 @@ class CallChecker
 
                     if ($function_param->is_variadic) {
                         if (!$param_type->hasArray() || !$param_type->types['array'] instanceof TArray) {
-                            break;
+                            continue;
                         }
 
                         $param_type = clone $param_type->types['array']->type_params[1];
diff --git a/src/Psalm/Stubs/CoreGenericFunctions.php b/src/Psalm/Stubs/CoreGenericFunctions.php
index ab72fafc5..c53cf63c7 100644
--- a/src/Psalm/Stubs/CoreGenericFunctions.php
+++ b/src/Psalm/Stubs/CoreGenericFunctions.php
@@ -84,17 +84,19 @@ function array_diff(array $arr, array $arr2, array $arr3 = null, array $arr4 = n
 function array_diff_key(array $arr, array $arr2, array $arr3 = null, array $arr4 = null) {}
 
 /**
+ * @template TKey
  * @template TValue
  *
- * @param array<mixed, TValue> $arr
+ * @param array<TKey, TValue> $arr
  * @return TValue
  */
 function array_shift(array &$arr) {}
 
 /**
+ * @template TKey
  * @template TValue
  *
- * @param array<mixed, TValue> $arr
+ * @param array<TKey, TValue> $arr
  * @return TValue
  */
 function array_pop(array &$arr) {}
diff --git a/tests/TemplateTest.php b/tests/TemplateTest.php
index d95402aec..d1073bc9e 100644
--- a/tests/TemplateTest.php
+++ b/tests/TemplateTest.php
@@ -362,6 +362,27 @@ class TemplateTest extends TestCase
                     '$b' => 'array<int, string>',
                 ],
             ],
+            'genericArrayPop' => [
+                '<?php
+                    /**
+                     * @template TValue
+                     *
+                     * @param array<mixed, TValue> $arr
+                     * @return TValue
+                     */
+                    function my_array_pop(array &$arr) {
+                        return array_pop($arr);
+                    }
+
+                    /** @var mixed */
+                    $b = ["a" => 5, "c" => 6];
+                    $a = my_array_pop($b);',
+                'assertions' => [
+                    '$a' => 'mixed',
+                    '$b' => 'array<mixed, mixed>',
+                ],
+                'error_levels' => ['MixedAssignment', 'MixedArgument'],
+            ],
         ];
     }