Handle nested destructuring assignment (#129)

* Add test for nested list() destructuring

* Add phpcsutils as dependency

* Use Lists::getAssignments to determine list assignment

* Add short list nested destructuring test

* Use Lists::getAssignments to determine short list assignments

* Remove null coalesce to support php 5

* Wrap getAssignments in try/catch

It shouldn't be able to fail since we already search for the opening
bracket, but it seems to fail in CI.

* Add phpcsutils to circleci dependencies

* Add namespace to Exception

* Prevent findPrevious from returning true

This will never happen but the return type can include `true` so we must
look for it to satisfy the static analysis.

* Try another way to test for bool return value from findPrevious

The comparison with true was sometimes breaking phpstan.

Author: sirbrillig

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -9,6 +9,7 @@
 use PHP_CodeSniffer\Sniffs\Sniff;
 use PHP_CodeSniffer\Files\File;
 use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Utils\Lists;
 
 class VariableAnalysisSniff implements Sniff {
   /**
@@ -821,18 +822,23 @@ protected function checkForListShorthandAssignment(File $phpcsFile, $stackPtr, $
     }
 
     // OK, we're a [ ... ] construct... are we being assigned to?
-    $closePtr = Helpers::findContainingClosingSquareBracket($phpcsFile, $stackPtr);
-    if (! is_int($closePtr)) {
+    try {
+      $assignments = Lists::getAssignments($phpcsFile, $openPtr);
+    } catch (\Exception $error) {
       return false;
     }
-    $assignPtr = Helpers::getNextAssignPointer($phpcsFile, $closePtr);
-    if (! is_int($assignPtr)) {
+    $matchingAssignment = array_reduce($assignments, function ($thisAssignment, array $assignment) use ($stackPtr) {
+      if (isset($assignment['assignment_token']) && $assignment['assignment_token'] === $stackPtr) {
+        return $assignment;
+      }
+      return $thisAssignment;
+    });
+    if (! $matchingAssignment) {
       return false;
     }
 
     // Yes, we're being assigned.
-    $writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr);
-    $this->markVariableAssignment($varName, $writtenPtr, $currScope);
+    $this->markVariableAssignment($varName, $stackPtr, $currScope);
     return true;
   }
 
@@ -854,20 +860,28 @@ protected function checkForListAssignment(File $phpcsFile, $stackPtr, $varName,
     }
 
     $prevPtr = $phpcsFile->findPrevious(Tokens::$emptyTokens, $openPtr - 1, null, true, null, true);
-    if (($prevPtr === false) || ($tokens[$prevPtr]['code'] !== T_LIST)) {
+    if ((is_bool($prevPtr)) || ($tokens[$prevPtr]['code'] !== T_LIST)) {
       return false;
     }
 
     // OK, we're a list (...) construct... are we being assigned to?
-    $closePtr = $tokens[$openPtr]['parenthesis_closer'];
-    $assignPtr = Helpers::getNextAssignPointer($phpcsFile, $closePtr);
-    if (! is_int($assignPtr)) {
+    try {
+      $assignments = Lists::getAssignments($phpcsFile, $prevPtr);
+    } catch (\Exception $error) {
+      return false;
+    }
+    $matchingAssignment = array_reduce($assignments, function ($thisAssignment, array $assignment) use ($stackPtr) {
+      if (isset($assignment['assignment_token']) && $assignment['assignment_token'] === $stackPtr) {
+        return $assignment;
+      }
+      return $thisAssignment;
+    });
+    if (! $matchingAssignment) {
       return false;
     }
 
     // Yes, we're being assigned.
-    $writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr);
-    $this->markVariableAssignment($varName, $writtenPtr, $currScope);
+    $this->markVariableAssignment($varName, $stackPtr, $currScope);
     return true;
   }
 

VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php

@@ -739,6 +739,10 @@ public function testAllowDestructuringAssignment() {
     $expectedWarnings = [
       4,
       12,
+      26,
+      28,
+      43,
+      45,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }

VariableAnalysis/Tests/CodeAnalysis/fixtures/DestructuringFixture.php

@@ -14,3 +14,37 @@ function function_with_destructuring_assignment_using_list() {
     echo $b;
     echo $c;
 }
+
+function function_with_nested_destructure_using_list() {
+    list(
+        $foo,
+        list(
+            $bar,
+        )
+    ) = [ 'foo', [ 'bar'  ]  ];
+    list(
+        $baz, // unused
+        list(
+            $bap, //unused
+        )
+    ) = [ 'foo', [ 'bar'  ]  ];
+    echo $foo;
+    echo $bar;
+}
+
+function function_with_nested_destructure_using_short_list() {
+    [
+        $foo,
+        [
+            $bar,
+        ]
+    ] = [ 'foo', [ 'bar'  ]  ];
+    [
+        $baz, // unused
+        [
+            $bap, //unused
+        ]
+    ] = [ 'foo', [ 'bar'  ]  ];
+    echo $foo;
+    echo $bar;
+}

composer.circleci.json

@@ -33,7 +33,8 @@
     },
     "require" : {
         "php" : ">=5.6.0",
-        "squizlabs/php_codesniffer": "^3.1"
+        "squizlabs/php_codesniffer": "^3.1",
+        "phpcsstandards/phpcsutils": "^1.0"
     },
     "require-dev": {
         "phpunit/phpunit": "^5.0 || ^6.5 || ^7.0 || ^8.0"

composer.json

@@ -35,7 +35,8 @@
     },
     "require" : {
         "php" : ">=5.6.0",
-        "squizlabs/php_codesniffer": "^3.1"
+        "squizlabs/php_codesniffer": "^3.1",
+        "phpcsstandards/phpcsutils": "^1.0"
     },
     "require-dev": {
         "dealerdirect/phpcodesniffer-composer-installer": "^0.4.4 || ^0.5 || ^0.6",