Improve static declaration checks (#267)

* Add test for `new` keyword in static initializer

* Add tests for static and nullable class properties

* Adjust processVariableAsStaticDeclaration to be more liberal

* Add test for static methods with a parameter

* Just examine the token at the start of the statement

* Remove T_MATCH_ARROW which did not exist in php 5

* Improve static variable tests by covering static methods with vars

* Refactor exclusion of static functions for phpcs 3.5.6

* Make FunctionWithReferenceFixture test code more clear

Author: sirbrillig

Tests/VariableAnalysisSniff/VariableAnalysisTest.php

@@ -300,20 +300,25 @@ public function testFunctionWithReferenceWarnings()
 		$phpcsFile->process();
 		$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
 		$expectedWarnings = [
-			8,
-			20,
-			32,
-			33,
-			34,
-			36,
-			37,
-			39,
-			40,
+			10,
+			11,
+			12,
+			13,
+			14,
+			16,
+			29,
+			41,
+			42,
+			43,
 			46,
-			59,
-			60,
-			64,
+			52,
+			56,
+			57,
+			63,
+			76,
+			77,
 			81,
+			98,
 		];
 		$this->assertSame($expectedWarnings, $lines);
 	}
@@ -330,18 +335,23 @@ public function testFunctionWithReferenceWarningsAllowsCustomFunctions()
 		$phpcsFile->process();
 		$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
 		$expectedWarnings = [
-			8,
-			20,
-			32,
-			33,
-			34,
-			36,
-			37,
-			39,
-			40,
+			10,
+			11,
+			12,
+			13,
+			14,
+			16,
+			29,
+			41,
+			42,
+			43,
 			46,
-			64,
+			52,
+			56,
+			57,
+			63,
 			81,
+			98,
 		];
 		$this->assertSame($expectedWarnings, $lines);
 	}
@@ -358,19 +368,24 @@ public function testFunctionWithReferenceWarningsAllowsWordPressFunctionsIfSet()
 		$phpcsFile->process();
 		$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
 		$expectedWarnings = [
-			8,
-			20,
-			32,
-			33,
-			34,
-			36,
-			37,
-			39,
-			40,
+			10,
+			11,
+			12,
+			13,
+			14,
+			16,
+			29,
+			41,
+			42,
+			43,
 			46,
-			59,
-			60,
-			81,
+			52,
+			56,
+			57,
+			63,
+			76,
+			77,
+			98,
 		];
 		$this->assertSame($expectedWarnings, $lines);
 	}

Tests/VariableAnalysisSniff/fixtures/ClassWithMembersFixture.php

@@ -45,8 +45,8 @@ function method_with_member_var() {
 
 class ClassWithMembers {
     public $member_var;
-    private $private_member_var;
-    protected $protected_member_var;
+    private ?string $private_member_var;
+    protected string $protected_member_var;
     static $static_member_var;
 
     function method_with_member_var() {
@@ -123,3 +123,40 @@ public function __construct(
   ) {
   }
 }
+
+class ClassWithStaticProperties {
+  static $static_simple;
+  public static $static_with_visibility;
+  public static $static_with_visibility_unused;
+  public static int $static_with_visibility_and_type;
+  public static ?int $static_with_visibility_and_nullable_type;
+
+  public function use_vars() {
+    echo self::$static_simple;
+    echo self::$static_with_visibility;
+    echo self::$static_with_visibility_and_type;
+    echo self::$static_with_visibility_and_nullable_type;
+  }
+
+  public static function getIntOrNull($value) {
+    return is_int($value) ? $value : null;
+  }
+
+  static function getIntOrNull2($value) {
+    return is_int($value) ? $value : null;
+  }
+}
+
+abstract class AbstractClassWithStaticProperties {
+  static $static_simple;
+  public static $static_with_visibility;
+  public static $static_with_visibility_unused;
+  public static int $static_with_visibility_and_type;
+  public static ?int $static_with_visibility_and_nullable_type;
+
+  public function use_vars();
+
+  public static function getIntOrNull($value);
+
+  static function getIntOrNull2($value);
+}

Tests/VariableAnalysisSniff/fixtures/FunctionWithReferenceFixture.php

@@ -5,7 +5,16 @@ function /*comment*/ &function_with_return_by_reference_and_param($param) {
 }
 
 function function_with_static_var() {
-    static $static1, $static_num = 12, $static_neg_num = -1.5, $static_string = 'abc', $static_string2 = "def", $static_define = MYDEFINE, $static_constant = MyClass::CONSTANT, $static2;
+  static $static1,
+    $static_num = 12,
+    $static_neg_num = -1.5, // Unused variable $static_neg_num
+    $static_string = 'abc', // Unused variable $static_string
+    $static_string2 = "def", // Unused variable $static_string2
+    $static_define = MYDEFINE, // Unused variable $static_define
+    $static_constant = MyClass::CONSTANT, // Unused variable $static_constant
+    $static2,
+    $static_new_unused = new Foobar(), // Unused variable $static_new_unused
+    $static_new = new Foobar();
     static $static_heredoc = <<<END_OF_HEREDOC
 this is an ugly but valid way to continue after a heredoc
 END_OF_HEREDOC
@@ -17,33 +26,41 @@ function function_with_static_var() {
     echo $static1;
     echo $static_num;
     echo $static2;
-    echo $var;
+    echo $var; // Undefined variable $var
     echo $static_heredoc;
     echo $static3;
     echo $static_nowdoc;
-    echo $static4;
+    echo $static4 . $static_new;
 }
 
 function function_with_pass_by_reference_param(&$param) {
     echo $param;
 }
 
 function function_with_pass_by_reference_calls() {
-    echo $matches;
-    echo $needle;
-    echo $haystack;
+    echo $matches; // Undefined variable $matches
+    echo $needle; // Undefined variable $needle
+    echo $haystack; // Undefined variable $haystack
     preg_match('/(abc)/', 'defabcghi', /* comment */ $matches);
-    preg_match($needle,   'defabcghi', $matches);
-    preg_match('/(abc)/', $haystack,   $matches);
+    preg_match(
+      $needle, // Undefined variable $needle
+      'defabcghi',
+      $matches
+    );
+    preg_match(
+      '/(abc)/',
+      $haystack, // Undefined variable $haystack
+      $matches
+    );
     echo $matches;
-    echo $needle;
-    echo $haystack;
+    echo $needle; // Undefined variable $needle
+    echo $haystack; // Undefined variable $haystack
     $stmt = 'whatever';
     $var1 = 'one';
     $var2 = 'two';
     echo $var1;
     echo $var2;
-    echo $var3;
+    echo $var3; // Undefined variable $var3
     maxdb_stmt_bind_result /*comment*/ ($stmt, $var1, $var2, $var3);
     echo $var1;
     echo $var2;
@@ -56,12 +73,12 @@ function function_with_pass_by_ref_assign_only_arg(&  /*comment*/  $return_value
 
 function function_with_ignored_reference_call() {
     $foo = 'bar';
-    my_reference_function($foo, $baz, $bip);
-    another_reference_function($foo, $foo2, $foo3);
+    my_reference_function($foo, $baz, $bip); // Undefined variable $bar, Undefined variable $bip
+    another_reference_function($foo, $foo2, $foo3); // Undefined variable $foo2, Undefined variable $foo3
 }
 
 function function_with_wordpress_reference_calls() {
-    wp_parse_str('foo=bar', $vars);
+    wp_parse_str('foo=bar', $vars); // Undefined variable $vars
 }
 
 function function_with_array_walk($userNameParts) {
@@ -78,7 +95,7 @@ function function_with_foreach_with_reference($derivatives, $base_plugin_definit
   foreach ($derivatives as &$entry) {
     $entry .= $base_plugin_definition;
   }
-  foreach ($derivatives as &$unused) { // unused variable
+  foreach ($derivatives as &$unused) { // Unused variable $unused
     $base_plugin_definition .= '1';
   }
   return $derivatives;

VariableAnalysis/Lib/Helpers.php

@@ -159,11 +159,13 @@ public static function isTokenInsideFunctionCallArgument(File $phpcsFile, $stack
 	}
 
 	/**
-	 * Find the index of the function keyword for a token in a function definition's arguments
+	 * Find the index of the function keyword for a token in a function
+	 * definition's parameters.
 	 *
 	 * Does not work for tokens inside the "use".
 	 *
-	 * Will also work for the parenthesis that make up the function definition's arguments list.
+	 * Will also work for the parenthesis that make up the function definition's
+	 * parameters list.
 	 *
 	 * For arguments inside a function call, rather than a definition, use
 	 * `getFunctionIndexForFunctionCallArgument`.
@@ -256,6 +258,10 @@ public static function getUseIndexForUseImport(File $phpcsFile, $stackPtr)
 	}
 
 	/**
+	 * Return the index of a function's name token from inside the function.
+	 *
+	 * $stackPtr must be inside the function body or parameters for this to work.
+	 *
 	 * @param File $phpcsFile
 	 * @param int  $stackPtr
 	 *

VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php

@@ -1275,6 +1275,33 @@ protected function processVariableAsGlobalDeclaration(File $phpcsFile, $stackPtr
 	}
 
 	/**
+	 * Process a variable as a static declaration within a function.
+	 *
+	 * This will not operate on variables that are written a class definition
+	 * like `static $foo;` or `public static ?int $foo = 'bar';` because class
+	 * properties (static or instance) are currently not tracked by this sniff.
+	 * This is because a class property might be unused inside the class, but
+	 * used outside the class (we cannot easily know if it is unused); this is
+	 * also because it's common and legal to define class properties when they
+	 * are assigned and that assignment can happen outside a class (we cannot
+	 * easily know if the use of a property is undefined). These sorts of checks
+	 * are better performed by static analysis tools that can see a whole project
+	 * rather than a linter which can only easily see a file or some lines.
+	 *
+	 * If found, such a variable will be marked as declared (and possibly
+	 * assigned, if it includes an initial value) within the scope of the
+	 * function body.
+	 *
+	 * This will not operate on variables that use late static binding
+	 * (`static::$foobar`) or the parameters of static methods even though they
+	 * include the word `static` in the same statement.
+	 *
+	 * This only finds the defintions of static variables. Their use is handled
+	 * by `processVariableAsStaticMember()`.
+	 *
+	 * Can be called for any token and will return false if the variable is not
+	 * of this type.
+	 *
 	 * @param File   $phpcsFile
 	 * @param int    $stackPtr
 	 * @param string $varName
@@ -1286,55 +1313,36 @@ protected function processVariableAsStaticDeclaration(File $phpcsFile, $stackPtr
 	{
 		$tokens = $phpcsFile->getTokens();
 
-		// Are we a static declaration?
-		// Static declarations are a bit more complicated than globals, since they
-		// can contain assignments. The assignment is compile-time however so can
-		// only be constant values, which makes life manageable.
-		//
-		// Just to complicate matters further, late static binding constants
-		// take the form static::CONSTANT and are invalid within static variable
-		// assignments, but we don't want to accidentally match their use of the
-		// static keyword.
-		//
-		// Valid values are:
-		//   number         T_MINUS T_LNUMBER T_DNUMBER
-		//   string         T_CONSTANT_ENCAPSED_STRING
-		//   heredoc        T_START_HEREDOC T_HEREDOC T_END_HEREDOC
-		//   nowdoc         T_START_NOWDOC T_NOWDOC T_END_NOWDOC
-		//   define         T_STRING
-		//   class constant T_STRING T_DOUBLE_COLON T_STRING
-		// Search backwards for first token that isn't whitespace, comma, variable,
-		// equals, or on the list of assignable constant values above.
-		$find = [
-			T_WHITESPACE => T_WHITESPACE,
-			T_VARIABLE => T_VARIABLE,
-			T_COMMA => T_COMMA,
-			T_EQUAL => T_EQUAL,
-			T_MINUS => T_MINUS,
-			T_LNUMBER => T_LNUMBER,
-			T_DNUMBER => T_DNUMBER,
-			T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING,
-			T_STRING => T_STRING,
-			T_DOUBLE_COLON => T_DOUBLE_COLON,
-		];
-		$find += Tokens::$heredocTokens;
+		// Search backwards for a `static` keyword that occurs before the start of the statement.
+		$startOfStatement = $phpcsFile->findPrevious([T_SEMICOLON, T_OPEN_CURLY_BRACKET], $stackPtr - 1, null, false, null, true);
+		$staticPtr = $phpcsFile->findPrevious([T_STATIC], $stackPtr - 1, null, false, null, true);
+		if (! is_int($startOfStatement)) {
+			$line = $tokens[$stackPtr]['line'];
+			throw new \Exception("Could not find start of statement on line {$line}");
+		}
+		if (! is_int($staticPtr)) {
+			return false;
+		}
+		// PHPCS is bad at finding the start of statements so we have to do it ourselves.
+		if ($staticPtr < $startOfStatement) {
+			return false;
+		}
 
-		$staticPtr = $phpcsFile->findPrevious($find, $stackPtr - 1, null, true, null, true);
-		if (($staticPtr === false) || ($tokens[$staticPtr]['code'] !== T_STATIC)) {
+		// Is the token inside function parameters? If so, this is not a static
+		// declaration because we must be inside a function body.
+		if (Helpers::isTokenFunctionParameter($phpcsFile, $stackPtr)) {
 			return false;
 		}
 
-		// Is it a late static binding static::?
-		// If so, this isn't the static keyword we're looking for, but since
-		// static:: isn't allowed in a compile-time constant, we also know
-		// we can't be part of a static declaration anyway, so there's no
-		// need to look any further.
+		// Is the keyword a late static binding? If so, this isn't the static
+		// keyword we're looking for, but since static:: isn't allowed in a
+		// compile-time constant, we also know we can't be part of a static
+		// declaration anyway, so there's no need to look any further.
 		$lateStaticBindingPtr = $phpcsFile->findNext(T_WHITESPACE, $staticPtr + 1, null, true, null, true);
 		if (($lateStaticBindingPtr !== false) && ($tokens[$lateStaticBindingPtr]['code'] === T_DOUBLE_COLON)) {
 			return false;
 		}
 
-		// It's a static declaration.
 		$this->markVariableDeclaration($varName, ScopeType::STATICSCOPE, null, $stackPtr, $currScope);
 		if (Helpers::getNextAssignPointer($phpcsFile, $stackPtr) !== null) {
 			$this->markVariableAssignment($varName, $stackPtr, $currScope);