Add checks for class properties (#77)

* Tests: wrap anonymous class fixtures in methods Previously they were creating an anonymous class in the global namespace outside a function, but this is an unrealistic usage and probably is being effected by #37. This modifies the fixtures to put the class as a return value of a class method. * Tests: cover private/protected class properties * Add check for class property definition * Tests: add 'private static' to anon class fixture * Tests: Add naked static, var to class properties * Treat `var` declarations as properties * Tests: add static function var to anon fixture * Allow bare static property declaration
sirbrillig · Mar 25, 2019 · 22ab15e · 22ab15e
1 parent 4f36541
commit 22ab15e
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 16 deletions.
diff --git a/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
@@ -341,6 +341,38 @@ protected function checkForFunctionPrototype(File $phpcsFile, $stackPtr, $varNam
     return false;
   }
 
+  protected function checkForClassProperty(File $phpcsFile, $stackPtr, $varName, $currScope) {
+    $propertyDeclarationKeywords = [
+      T_PUBLIC,
+      T_PRIVATE,
+      T_PROTECTED,
+      T_VAR,
+    ];
+    $stopAtPtr = $stackPtr - 2;
+    $visibilityPtr = $phpcsFile->findPrevious($propertyDeclarationKeywords, $stackPtr - 1, $stopAtPtr > 0 ? $stopAtPtr : 0);
+    if ($visibilityPtr) {
+      return true;
+    }
+    $staticPtr = $phpcsFile->findPrevious(T_STATIC, $stackPtr - 1, $stopAtPtr > 0 ? $stopAtPtr : 0);
+    if (! $staticPtr) {
+      return false;
+    }
+    $stopAtPtr = $staticPtr - 2;
+    $visibilityPtr = $phpcsFile->findPrevious($propertyDeclarationKeywords, $staticPtr - 1, $stopAtPtr > 0 ? $stopAtPtr : 0);
+    if ($visibilityPtr) {
+      return true;
+    }
+    // it's legal to use `static` to define properties as well as to
+    // define variables, so make sure we are not in a function before
+    // assuming it's a property.
+    $tokens = $phpcsFile->getTokens();
+    $token  = $tokens[$stackPtr];
+    if ($token && !empty($token['conditions']) && end($token['conditions']) !== T_FUNCTION) {
+      return Helpers::areAnyConditionsAClass($token['conditions']);
+    }
+    return false;
+  }
+
   protected function checkForCatchBlock(File $phpcsFile, $stackPtr, $varName, $currScope) {
     $tokens = $phpcsFile->getTokens();
     $token  = $tokens[$stackPtr];
@@ -819,6 +851,11 @@ protected function processVariable(File $phpcsFile, $stackPtr) {
       return;
     }
 
+    if ($this->checkForClassProperty($phpcsFile, $stackPtr, $varName, $currScope)) {
+      Helpers::debug('found class property definition');
+      return;
+    }
+
     // Is the next non-whitespace an assignment?
     if ($this->checkForAssignment($phpcsFile, $stackPtr, $varName, $currScope)) {
       Helpers::debug('found assignment');

diff --git a/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php b/VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
@@ -157,7 +157,7 @@ public function testClassWithMembersWarnings() {
       13,
       18,
       19,
-      64,
+      66,
     ];
     $this->assertEquals($expectedWarnings, $lines);
   }
@@ -534,7 +534,9 @@ public function testAnonymousClassAllowPropertyDefinitions() {
     $phpcsFile = $this->prepareLocalFileForSniffs($this->getSniffFiles(), $fixtureFile);
     $phpcsFile->process();
     $lines = $this->getWarningLineNumbersFromFile($phpcsFile);
-    $expectedWarnings = [];
+    $expectedWarnings = [
+      17,
+    ];
     $this->assertEquals($expectedWarnings, $lines);
     $lines = $this->getErrorLineNumbersFromFile($phpcsFile);
     $expectedErrors = [];

diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/AnonymousClassFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/AnonymousClassFixture.php
@@ -1,11 +1,15 @@
 <?php
 
-new class {
-    public function sayHelloWorld() {
-        echo 'Hello'.$this->getWorld();
-    }
+class ClassWithAnonymousClass {
+    public function getMyClass() {
+        return new class {
+            public function sayHelloWorld() {
+                echo 'Hello'.$this->getWorld();
+            }
 
-    protected function getWorld() {
-        return ' World!';
+            protected function getWorld() {
+                return ' World!';
+            }
+        };
     }
-};
+}
diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/AnonymousClassWithPropertiesFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/AnonymousClassWithPropertiesFixture.php
@@ -1,10 +1,21 @@
 <?php
 
-new class {
-    protected $storedHello;
-    public $helloOptions = [];
-    public function sayHelloWorld() {
-        echo "hello world";
-    }
-};
+class ClassWithAnonymousClass {
+    public function getAnonymousClass() {
+        return new class {
+            protected $storedHello;
+            private static $storedHello2;
+            private $storedHello3;
+            public $helloOptions = [];
+            static $aStaticOne;
+            var $aVarOne;
+            public function sayHelloWorld() {
+                echo "hello world";
+            }
 
+            public function methodWithStaticVar() {
+                static $myStaticVar; // should trigger unused warning
+            }
+        };
+    }
+}
diff --git a/VariableAnalysis/Tests/CodeAnalysis/fixtures/ClassWithMembersFixture.php b/VariableAnalysis/Tests/CodeAnalysis/fixtures/ClassWithMembersFixture.php
@@ -45,6 +45,8 @@ function method_with_member_var() {
 
 class ClassWithMembers {
     public $member_var;
+    private $private_member_var;
+    protected $protected_member_var;
     static $static_member_var;
 
     function method_with_member_var() {