Mark concatenation inside another expression as variable read (#186)

* Add test case for concatination on the same line as assignment

* Remove array functions from getPassByReferenceFunctions

The list of pass-by-reference functions is only currently used to
determine if a function's argument might be counted as a define. All
arguments passed to the function are counted as a read. Functions like
`array_pop()` will throw a PHP warning if used with an undefined
variable, so this removes them. There are probably many others in this
list that can be removed, but this is a start.

* Add variations of concat-and-assign to test fixture

* Add function expression to assign-and-concat test

* Mark assignments as read if they are part of another expression

* Fix test for undefined variable after array assignment in loop

* Just check for RHS

* Add check for inside function calls

* Replace bool casts for indices with is_int

Indices can sometimes be 0.

Author: sirbrillig

Tests/VariableAnalysisSniff/WhileLoopTest.php

@@ -0,0 +1,18 @@
+<?php
+namespace VariableAnalysis\Tests\VariableAnalysisSniff;
+
+use VariableAnalysis\Tests\BaseTestCase;
+
+class WhileLoopTest extends BaseTestCase {
+  public function testFunctionWithWhileLoopWarnings() {
+    $fixtureFile = $this->getFixture('FunctionWithWhileLoopFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($fixtureFile);
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+      38,
+      46,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+  }
+}

Tests/VariableAnalysisSniff/fixtures/FunctionWithWhileLoopFixture.php

@@ -0,0 +1,74 @@
+<?php
+
+function loopAndPush($parts) {
+  $suggestions = [];
+  while ($part = array_shift($parts)) {
+    $suggestions[] = $part;
+  }
+  return $suggestions;
+}
+
+function concatAndAssignAndPush($parts) {
+  $suggestions = [];
+  $suggestion = 'block';
+  while ($part = array_shift($parts)) {
+    $suggestions[] = $suggestion .= '__' . strtr($part, '-', '_');
+  }
+  return $suggestions;
+}
+
+function concatAndAssignAndPushWithoutLoop() {
+  $suggestions = [];
+  $suggestion = 'block';
+  $suggestions[] = $suggestion .= '__';
+  return $suggestions;
+}
+
+function concatAndPush($parts) {
+  $suggestions = [];
+  $suggestion = 'block';
+  while ($part = array_shift($parts)) {
+    $suggestions[] = $suggestion . '__' . strtr($part, '-', '_');
+  }
+  return $suggestions;
+}
+
+function whileLoopAssignWithUndefinedShift() {
+  $suggestions = [];
+  while ($part = array_shift($parts)) { // undefined variable parts
+    $suggestions[] = $part;
+  }
+  return $suggestions;
+}
+
+function loopAndPushWithUndefinedArray($parts) {
+  while ($part = array_shift($parts)) {
+    $suggestions[] = $part; // undefined variable suggestions
+  }
+  return $suggestions;
+}
+
+function concatAndAssignAndPush2($parts) {
+  $suggestions = [];
+  $suggestion = 'block';
+  while ($part = array_shift($parts)) {
+    $suggestions[] = ( $suggestion .= '__' . strtr($part, '-', '_') );
+  }
+  return $suggestions;
+}
+
+function concatAndAssignAndPush3($parts) {
+  $suggestions = [];
+  $suggestion = 'block';
+  while ($part = array_shift($parts)) {
+    $suggestions[] = getPrefix() . $suggestion .= '__' . strtr($part, '-', '_');
+  }
+  return $suggestions;
+}
+
+function concatAndAssignAndPush4($parts) {
+  $suggestion = 'block';
+  while ($part = array_shift($parts)) {
+    addToSuggestion($suggestion .= $part);
+  }
+}

VariableAnalysis/Lib/Constants.php

@@ -8,6 +8,10 @@ class Constants {
    *  by reference, the arguments are numbered starting from 1 and an elipsis '...'
    *  means all argument numbers after the previous should be considered pass-by-reference.
    *
+   *  This does not need to cover all pass-by-reference arguments, only the
+   *  ones which can be passed an undefined variable (eg: `$matches` in
+   *  `preg_match`) and will define that variable.
+   *
    *  @return array[]
    */
   public static function getPassByReferenceFunctions() {
@@ -28,16 +32,6 @@ public static function getPassByReferenceFunctions() {
       'apcu_fetch' => [2],
       'apcu_inc' => [3],
       'areConfusable' => [3],
-      'array_multisort' => [1],
-      'array_pop' => [1],
-      'array_push' => [1],
-      'array_replace' => [1],
-      'array_replace_recursive' => [1, 2, 3, '...'],
-      'array_shift' => [1],
-      'array_splice' => [1],
-      'array_unshift' => [1],
-      'array_walk' => [1],
-      'array_walk_recursive' => [1],
       'arsort' => [1],
       'asort' => [1],
       'bindColumn' => [2],

VariableAnalysis/Lib/Helpers.php

@@ -97,7 +97,17 @@ public static function areConditionsWithinFunctionBeforeClass(array $conditions)
    * @return bool
    */
   public static function isTokenInsideFunctionDefinitionArgumentList(File $phpcsFile, $stackPtr) {
-    return (bool) self::getFunctionIndexForFunctionArgument($phpcsFile, $stackPtr);
+    return is_int(self::getFunctionIndexForFunctionArgument($phpcsFile, $stackPtr));
+  }
+
+  /**
+   * @param File $phpcsFile
+   * @param int $stackPtr
+   *
+   * @return bool
+   */
+  public static function isTokenInsideFunctionCall(File $phpcsFile, $stackPtr) {
+    return is_int(self::getFunctionIndexForFunctionCallArgument($phpcsFile, $stackPtr));
   }
 
   /**
@@ -163,7 +173,7 @@ public static function getFunctionIndexForFunctionArgument(File $phpcsFile, $sta
    * @return bool
    */
   public static function isTokenInsideFunctionUseImport(File $phpcsFile, $stackPtr) {
-    return (bool) self::getUseIndexForUseImport($phpcsFile, $stackPtr);
+    return is_int(self::getUseIndexForUseImport($phpcsFile, $stackPtr));
   }
 
   /**
@@ -303,7 +313,7 @@ public static function findVariableScope(File $phpcsFile, $stackPtr) {
     $token = $tokens[$stackPtr];
 
     $arrowFunctionIndex = self::getContainingArrowFunctionIndex($phpcsFile, $stackPtr);
-    $isTokenInsideArrowFunctionBody = (bool) $arrowFunctionIndex;
+    $isTokenInsideArrowFunctionBody = is_int($arrowFunctionIndex);
     if ($isTokenInsideArrowFunctionBody) {
       // Get the list of variables defined by the arrow function
       // If this matches any of them, the scope is the arrow function,
@@ -851,4 +861,22 @@ public static function isVariableInsideUnset(File $phpcsFile, $stackPtr) {
     }
     return false;
   }
+
+  /**
+   * @param File $phpcsFile
+   * @param int $stackPtr
+   *
+   * @return bool
+   */
+  public static function isTokenInsideAssignmentRHS(File $phpcsFile, $stackPtr) {
+    $previousStatementPtr = $phpcsFile->findPrevious([T_SEMICOLON, T_CLOSE_CURLY_BRACKET, T_OPEN_CURLY_BRACKET, T_COMMA], $stackPtr - 1);
+    if (! is_int($previousStatementPtr)) {
+      $previousStatementPtr = 1;
+    }
+    $previousTokenPtr = $phpcsFile->findPrevious([T_EQUAL], $stackPtr - 1, $previousStatementPtr);
+    if (is_int($previousTokenPtr)) {
+      return true;
+    }
+    return false;
+  }
 }

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -1399,6 +1399,11 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
 
     // Is the next non-whitespace an assignment?
     if ($this->processVariableAsAssignment($phpcsFile, $stackPtr, $varName, $currScope)) {
+      if (Helpers::isTokenInsideAssignmentRHS($phpcsFile, $stackPtr) || Helpers::isTokenInsideFunctionCall($phpcsFile, $stackPtr)) {
+        Helpers::debug("found assignment that's also inside an expression");
+        $this->markVariableRead($varName, $stackPtr, $currScope);
+        return;
+      }
       Helpers::debug('found assignment');
       return;
     }