Recognize that variable vars are not assignments (#88)

* Tests: Add tests for variable variables * Recognize that variable vars are not assignments When a variable variable is used as the left side of an assignment, there are actually two variables being checked: there is the current one, and the variable one whose name is the value of the current one. In this case, we must not treat the current one as an assignment; it is actually a read. This change updates the logic so that this is the case.
sirbrillig · May 10, 2019 · fd217f2 · fd217f2
1 parent 00d3158
commit fd217f2
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 0 deletions.
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -517,6 +517,12 @@ protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $cur
       return false;
     }
 
+    // Is this a variable variable? If so, it's not an assignment to the current variable.
+    if ($this->checkForVariableVariable($phpcsFile, $stackPtr, $varName, $currScope)) {
+      Helpers::debug('found variable variable');
+      return false;
+    }
+
     // Plain ol' assignment. Simpl(ish).
     $writtenPtr = Helpers::findWhereAssignExecuted($phpcsFile, $assignPtr);
     if ($writtenPtr === false) {
@@ -526,6 +532,22 @@ protected function checkForAssignment(File $phpcsFile, $stackPtr, $varName, $cur
     return true;
   }
 
+  protected function checkForVariableVariable(File $phpcsFile, $stackPtr, $varName, $currScope) {
+    $tokens = $phpcsFile->getTokens();
+    $token = $tokens[$stackPtr];
+
+    if (!isset($tokens[$stackPtr - 1]['code'])) {
+      return false;
+    }
+    if ($tokens[$stackPtr - 1]['code'] === T_DOLLAR) {
+      return true;
+    }
+    if ($tokens[$stackPtr - 1]['code'] === T_OPEN_CURLY_BRACKET && isset($tokens[$stackPtr - 2]['code']) && $tokens[$stackPtr - 2]['code'] === T_DOLLAR) {
+      return true;
+    }
+    return false;
+  }
+
   protected function checkForListShorthandAssignment(File $phpcsFile, $stackPtr, $varName, $currScope) {
     $tokens = $phpcsFile->getTokens();
     $token  = $tokens[$stackPtr];

diff --git a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
@@ -540,6 +540,30 @@ public function testVariableFunctionCallsCountAsUsage() {
     $this->assertEquals($expectedErrors, $lines);
   }
 
+  public function testVariableVariables() {
+    $fixtureFile = $this->getFixture('VariableVariablesFixture.php');
+    $phpcsFile = $this->prepareLocalFileForSniffs($this->getSniffFiles(), $fixtureFile);
+    $phpcsFile->process();
+    $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
+    $expectedWarnings = [
+      4,
+      10,
+      16,
+      22,
+      29,
+      35,
+      41,
+      47,
+      48,
+      52,
+      53,
+    ];
+    $this->assertEquals($expectedWarnings, $lines);
+    $lines = $this->getErrorLineNumbersFromFile($phpcsFile);
+    $expectedErrors = [];
+    $this->assertEquals($expectedErrors, $lines);
+  }
+
   public function testTraitsAllowPropertyDefinitions() {
     $fixtureFile = $this->getFixture('TraitWithPropertiesFixture.php');
     $phpcsFile = $this->prepareLocalFileForSniffs($this->getSniffFiles(), $fixtureFile);

diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/VariableVariablesFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/VariableVariablesFixture.php
@@ -0,0 +1,56 @@
+<?php
+
+function usedVariableVariableInReturn() {
+    $foo = true; // this is used but it requires execution to know that
+    $varName = 'foo';
+    return $$varName;
+}
+
+function usedVariableVariableInEcho() {
+    $foo = true; // this is used but it requires execution to know that
+    $varName = 'foo';
+    echo $$varName;
+}
+
+function usedVariableVariableInLeftAssignment() {
+    $foo = true; // the below is assignment, not a read, so this should be a warning
+    $marName = 'foo';
+    $$marName = false;
+}
+
+function usedVariableVariableInRightAssignment() {
+    $foo = true; // this is used but it requires execution to know that
+    $varName = 'foo';
+    $bar = $$varName;
+    echo $bar;
+}
+
+function usedVariableVariableInEchoWithBraces() {
+    $foo = true; // this is used but it requires execution to know that
+    $varName = 'foo';
+    echo ${$varName};
+}
+
+function usedVariableVariableInLeftAssignmentWithBraces() {
+    $foo = true; // this is used but it requires execution to know that
+    $varName = 'foo';
+    ${$varName} = false;
+}
+
+function usedVariableVariableInEchoWithBracesInString() {
+    $foo = true; // this is used but it requires execution to know that
+    $varName = 'foo';
+    echo "${$varName}";
+}
+
+function undefinedVariableVariableInEcho() {
+    $foo = true; // this should be a warning
+    echo $$varName; // this should be a warning
+}
+
+function usedVariableVariableTwoLevels() {
+    $foo = 'hello'; // this is used but it requires execution to know that
+    $varNameOne = 'foo'; // this is used but it requires execution to know that
+    $varNameTwo = 'varNameOne';
+    return $$$varNameTwo;
+}