From 372456c599c34b9096b85e6efa6a088e8e38b760 Mon Sep 17 00:00:00 2001 From: Sam Graham Date: Wed, 20 Jul 2011 09:12:56 +0100 Subject: [PATCH] Support for static member vars. Report on use of self::$var outside class scope. Consolidate unit test list of warnings and errors. --- README | 1 + Sniffs/CodeAnalysis/VariableAnalysisSniff.php | 56 ++++++ .../CodeAnalysis/VariableAnalysisUnitTest.inc | 22 +++ .../CodeAnalysis/VariableAnalysisUnitTest.php | 167 ++++++++++++------ ruleset.xml | 3 +- 5 files changed, 193 insertions(+), 56 deletions(-) diff --git a/README b/README index 236dc508..230fabe4 100644 --- a/README +++ b/README @@ -7,6 +7,7 @@ variable use. * Warns on use of undefined variables. * Warns if variables are set or declared but never used within that scope. * Warns if variables are redeclared within same scope. + * Warns if self::$static_member is used outside class scope. INSTALLATION diff --git a/Sniffs/CodeAnalysis/VariableAnalysisSniff.php b/Sniffs/CodeAnalysis/VariableAnalysisSniff.php index e053e291..76eecf90 100644 --- a/Sniffs/CodeAnalysis/VariableAnalysisSniff.php +++ b/Sniffs/CodeAnalysis/VariableAnalysisSniff.php @@ -566,6 +566,56 @@ protected function checkForThisWithinClass( return false; } + protected function checkForStaticMember( + PHP_CodeSniffer_File $phpcsFile, + $stackPtr, + $varName, + $currScope + ) { + $tokens = $phpcsFile->getTokens(); + $token = $tokens[$stackPtr]; + + // Are we a static member? + $doubleColonPtr = $stackPtr - 1; + if ($tokens[$doubleColonPtr]['code'] !== T_DOUBLE_COLON) { + return false; + } + $classNamePtr = $stackPtr - 2; + if (($tokens[$classNamePtr]['code'] !== T_STRING) && + ($tokens[$classNamePtr]['code'] !== T_SELF)) { + return false; + } + +//echo "found className " . $tokens[$classNamePtr]['content'] . "\n"; + + // Are we refering to self:: outside a class? + // TODO: not sure this is our business or should be some other sniff. + if ($tokens[$classNamePtr]['code'] === T_SELF) { +//echo "found self, like totally trippin'\n"; + if (!empty($token['conditions'])) { + foreach (array_reverse($token['conditions'], true) as $scopePtr => $scopeCode) { + // self within a closure is invalid + // Note: have to fetch code from $tokens, T_CLOSURE isn't set for conditions codes. + if ($tokens[$scopePtr]['code'] === T_CLOSURE) { + $phpcsFile->addError("Use of self::%s inside closure.", $stackPtr, + 'SelfOutsideClass', + array("\${$varName}")); + return true; + } + if ($scopeCode === T_CLASS) { + return true; + } + } + } + $phpcsFile->addError("Use of self::%s outside class definition.", $stackPtr, + 'SelfOutsideClass', + array("\${$varName}")); + return true; + } + + return true; + } + protected function checkForAssignment( PHP_CodeSniffer_File $phpcsFile, $stackPtr, @@ -827,6 +877,7 @@ protected function processVariable( // Is closure use declaration of a variable defined within containing scope // catch (...) block start // $this within a class (but not within a closure). + // $var part of class::$var static member // Assignment via = // Assignment via list (...) = // Declares as a global @@ -849,6 +900,11 @@ protected function processVariable( return; } + // $var part of class::$var static member + if ($this->checkForStaticMember($phpcsFile, $stackPtr, $varName, $currScope)) { + return; + } + // Is the next non-whitespace an assignment? if ($this->checkForAssignment($phpcsFile, $stackPtr, $varName, $currScope)) { return; diff --git a/Tests/CodeAnalysis/VariableAnalysisUnitTest.inc b/Tests/CodeAnalysis/VariableAnalysisUnitTest.inc index 4b24e76e..87309cad 100644 --- a/Tests/CodeAnalysis/VariableAnalysisUnitTest.inc +++ b/Tests/CodeAnalysis/VariableAnalysisUnitTest.inc @@ -158,15 +158,20 @@ class ClassWithoutMembers { function method_with_member_var() { echo $this->member_var; + echo self::$static_member_var; } } class ClassWithMembers { public $member_var; + static $static_member_var; function method_with_member_var() { echo $this->member_var; echo $this->no_such_member_var; + echo self::$static_member_var; + echo self::$no_such_static_member_var; + echo SomeOtherClass::$external_static_member_var; } } @@ -174,6 +179,11 @@ function function_with_this_outside_class() { return $this->whatever(); } +function function_with_static_members_outside_class() { + echo SomeOtherClass::$external_static_member_var; + return self::$whatever; +} + function function_with_closure($outer_param) { $outer_var = 1; $outer_var2 = 2; @@ -260,6 +270,18 @@ class ClassWithThisInsideClosure { } } +class ClassWithSelfInsideClosure { + static $static_member; + + function method_with_self_inside_closure() { + echo self::$static_member; + array_map(function () { + echo self::$static_member; + }, array()); + echo self::$static_member; + } +} + function function_with_inline_assigns() { echo $var; ($var = 12) && $var; diff --git a/Tests/CodeAnalysis/VariableAnalysisUnitTest.php b/Tests/CodeAnalysis/VariableAnalysisUnitTest.php index 751d6b03..ec97e3e2 100644 --- a/Tests/CodeAnalysis/VariableAnalysisUnitTest.php +++ b/Tests/CodeAnalysis/VariableAnalysisUnitTest.php @@ -31,36 +31,31 @@ class Generic_Tests_CodeAnalysis_VariableAnalysisUnitTest extends AbstractSniffUnitTest { - - /** - * Returns the lines where errors should occur. - * - * The key of the array should represent the line number and the value - * should represent the number of errors that should occur on that line. - * - * @return array(int => int) - */ - public function getErrorList() - { - return array(); - - }//end getErrorList() - - - /** - * Returns the lines where warnings should occur. - * - * The key of the array should represent the line number and the value - * should represent the number of warnings that should occur on that line. - * - * @return array(int => int) - */ - public function getWarningList() - { + private function _getWarningAndErrorList() { // This is a maintainence nightmare. + // Value of a line is either: + // - an int: the number of warnings. + // - an array: the number of warnings and number of errors. + // This is chosen because we mostly warn and because maintaining _two_ + // separate lists of line numbers would drive me insane. + // + // All the fiddling with $base is to make each line number relative + // to the line number of the function the line is in, which in turn + // is relative to the line number of the previous function. + // + // This makes adding new tests only moderately painful rather than + // a total clusterfuck of alterations. + // + // Comments with an unexplained number at the start before the name + // of several variables are probably a reminder that there's multiple + // errors on that line, but I had to pretend only one error because + // AbstractSniffUnitTest doesn't reliably count all the errors. :( + // This unfortunately means some of our test cases could fail and + // we'd never notice. + // TODO: track down that bug, fix and submit upstream. $base = 0; return array( - // function_without_param() line 3 (+3) + // function_without_param() line (+3) ($base += 3) => 0, ($base + 1) => 1, // $var ($base + 2) => 1, // $var @@ -73,20 +68,20 @@ public function getWarningList() ($base + 9) => 1, // $var ($base + 14) => 1, // $var2 ($base + 15) => 1, // $var2 - // function_with_param() line 26 (+23) + // function_with_param() line (+23) // no warnings. ($base += 23) => 0, - // function_with_default_defined_param() line 37 (+11) + // function_with_default_defined_param() line (+11) ($base += 11) => 0, ($base + 0) => 1, // $unused - // function_with_default_null_param() line 48 (+11) + // function_with_default_null_param() line (+11) ($base += 11) => 0, ($base + 0) => 1, // $unused - // function_with_global_var() line 59 (+11) + // function_with_global_var() line (+11) ($base += 11) => 0, ($base + 1) => 1, // $unused ($base + 4) => 1, // $var3 - // function_with_undefined_foreach() line 67 (+8) + // function_with_undefined_foreach() line (+8) ($base += 8) => 0, ($base + 1) => 1, // $array ($base + 5) => 1, // $array @@ -96,13 +91,13 @@ public function getWarningList() ($base + 19) => 1, // 2, // $array, $element4 ($base + 21) => 1, // 3, // $array, $key3, $value4 ($base + 23) => 1, // 3, // $array, $key4, $value4 - // function_with_defined_foreach() line 94 (+27) + // function_with_defined_foreach() line (+27) ($base += 27) => 0, ($base + 18) => 1, // $element3 ($base + 20) => 1, // $element4 ($base + 22) => 1, // 2, // $key3, $value4 ($base + 24) => 1, // 2, // $key4, $value4 - // ClassWithoutMembers->method_without_param() line 123 (+29) + // ClassWithoutMembers->method_without_param() line (+29) ($base += 29) => 0, ($base + 1) => 1, // $var ($base + 2) => 1, // $var @@ -115,20 +110,31 @@ public function getWarningList() ($base + 9) => 1, // $var ($base + 14) => 1, // $var2 ($base + 15) => 1, // $var2 - // ClassWithoutMembers->method_with_param() line 123 (+24) + // ClassWithoutMembers->method_with_param() line (+24) // no warnings. ($base += 24) => 0, - // ClassWithoutMembers->method_with_member_var() line 135 (+12) + // ClassWithoutMembers->method_with_member_var() line (+12) + // no warnings. + // We can't/don't inspect the class inheritence so we can't + // determine that these are undeclared: + // $this->member_var + // self::$static_member_var ($base += 12) => 0, -// TODO: 136 => 1, // $this->member_var - // ClassWithMembers->method_with_member_var() line 143 (+8) - ($base += 8) => 0, -// TODO: 145 => 1, // $this->no_such_member_var - // function_with_this_outside_class() line 149 (+6) - ($base += 6) => 0, + // ClassWithMembers->method_with_member_var() line (+10) + // no warnings. + // We can't/don't inspect the class inheritence so we can't + // determine that these are undeclared: + // $this->no_such_member_var + // self::$no_such_static_member_var + ($base += 10) => 0, + // function_with_this_outside_class() line (+9) + ($base += 9) => 0, ($base + 1) => 1, // $this - // function_with_closure() line 153 (+4) + // function_with_static_members_outside_class() line (+4) ($base += 4) => 0, + ($base + 2) => array(0, 1), // self::$whatever + // function_with_closure() line (+5) + ($base += 5) => 0, ($base + 5) => 1, // $outer_param ($base + 7) => 1, // $outer_var ($base + 8) => 1, // $outer_var2 @@ -141,18 +147,18 @@ public function getWarningList() ($base + 24) => 1, // $inner_param ($base + 25) => 1, // $inner_var ($base + 26) => 1, // $inner_var2 - // function_with_return_by_reference_and_param() line 182 (+29) + // function_with_return_by_reference_and_param() line (+29) // no warnings. ($base += 29) => 0, - // function_with_static_var() line 187 (+5) + // function_with_static_var() line (+5) ($base += 5) => 0, ($base + 1) => 1, // 5, // $static_neg_num, $static_string, $static_string2, // $static_define, $static_constant ($base + 5) => 1, // $var - // function_with_pass_by_reference_param() line 195 (+8) + // function_with_pass_by_reference_param() line (+8) // no warnings. ($base += 8) => 0, - // function_with_pass_by_reference_calls() line 199 (+4) + // function_with_pass_by_reference_calls() line (+4) ($base += 4) => 0, ($base + 1) => 1, // $matches ($base + 2) => 1, // $needle @@ -161,20 +167,23 @@ public function getWarningList() ($base + 6) => 1, // $haystack ($base + 8) => 1, // $needle ($base + 9) => 1, // $haystack - // function_with_try_catch() line 211 (+12) + // function_with_try_catch() line (+12) ($base += 12) => 0, ($base + 1) => 1, // $e ($base + 5) => 1, // $e - // ClassWithThisInsideClosure->method_with_this_inside_closure() line 227 (+16) + // ClassWithThisInsideClosure->method_with_this_inside_closure() line (+16) ($base += 16) => 0, ($base + 3) => 1, // $inner_param ($base + 4) => 1, // $this ($base + 5) => 1, // $this - // function_with_inline_assigns() line 263 (+12) - ($base += 12) => 0, + // ClassWithSelfInsideClosure->method_with_self_inside_closure() line (+15) + ($base += 15) => 0, + ($base + 3) => array(0, 1), // $self::$static_member + // function_with_inline_assigns() line (+9) + ($base += 9) => 0, ($base + 1) => 1, // $var ($base + 4) => 1, // $var2 - // function_with_global_redeclarations() line 274 (+11) + // function_with_global_redeclarations() line (+11) ($base += 11) => 0, ($base + 5) => 1, // $bound ($base + 12) => 1, // $param @@ -182,7 +191,7 @@ public function getWarningList() ($base + 14) => 1, // $bound ($base + 15) => 1, // $local ($base + 16) => 1, // $e - // function_with_static_redeclarations() line 293 (+19) + // function_with_static_redeclarations() line (+19) ($base += 19) => 0, ($base + 2) => 1, // $static ($base + 5) => 1, // $bound @@ -191,12 +200,60 @@ public function getWarningList() ($base + 14) => 1, // $bound ($base + 15) => 1, // $local ($base + 16) => 1, // $e - // function_with_catch_redeclarations() line 312 (+19) + // function_with_catch_redeclarations() line (+19) // no warnings. ($base += 19) => 0, ); + }//end _getWarningAndErrorList() + + /** + * Returns the lines where errors should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of errors that should occur on that line. + * + * @return array(int => int) + */ + public function getErrorList() + { + $errorList = array(); + foreach ($this->_getWarningAndErrorList() as $line => $incidents) { + $errors = null; + if (is_array($incidents)) { + list ($warnings, $errors) = $incidents; + } + if (!empty($errors)) { + $errorList[$line] = $errors; + } + } + return $errorList; + }//end getErrorList() - }//end getWarningList() + + /** + * Returns the lines where warnings should occur. + * + * The key of the array should represent the line number and the value + * should represent the number of warnings that should occur on that line. + * + * @return array(int => int) + */ + public function getWarningList() + { + $warningList = array(); + foreach ($this->_getWarningAndErrorList() as $line => $incidents) { + $warnings = null; + if (is_array($incidents)) { + list ($warnings, $errors) = $incidents; + } else { + $warnings = $incidents; + } + if (!empty($warnings)) { + $warningList[$line] = $warnings; + } + } + return $warningList; + }//end getWarningList() }//end class diff --git a/ruleset.xml b/ruleset.xml index 719e6bed..3160e717 100644 --- a/ruleset.xml +++ b/ruleset.xml @@ -4,10 +4,11 @@ -