From 26d9f61672c5110564cc6fbfecc42d27cbe79cb4 Mon Sep 17 00:00:00 2001 From: Correl Roush Date: Fri, 23 May 2008 21:13:36 +0000 Subject: [PATCH] Polished up and enabled the undefined variable scan by default Mantis: 2691 git-svn-id: file:///srv/svn/scanner/trunk@20 a0501263-5b7a-4423-a8ba-1edf086583e7 --- modules/scanner_variables.php | 27 +++++++++++++++++++++------ parser.php | 34 ++++++++++++++++++++++++++++++++-- scanner.php | 2 +- tests/ParserTests.php | 2 +- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/modules/scanner_variables.php b/modules/scanner_variables.php index 7bc0f49..8607ae7 100644 --- a/modules/scanner_variables.php +++ b/modules/scanner_variables.php @@ -9,25 +9,40 @@ class VariableModule extends ScannerModule { function parserCallback( $object ) { $pattern = '/\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*/'; $matches = array(); + $variable = preg_match($pattern, $object['name'], $matches) > 0 ? $matches[0] : false; $scope = "{$object['in_class']}::{$object['in_function']}"; if (!isset($this->assigned_variables[$scope] ) ) $this->assigned_variables[$scope] = array(); if ($object['type'] == PHPPARSER_ASSIGNMENT) { - //$this->fault($object, 0, "Assignment: {$object['name']}"); - list($variable, $value) = explode('=', $object['name']); - $this->assigned_variables[$scope][] = $variable; + list($var, $value) = explode('=', $object['name']); + if ($variable == $var) { + // Regular variable assignment + //$this->fault($object, 0, "Assignment: {$object['name']}"); + $this->assigned_variables[$scope][] = $var; + } else { + // Array index assignment + //$this->fault($object, 0, "Array index assignment: [{$variable}] {$object['name']}"); + if ( + !in_array($variable, $this->assigned_variables[$scope]) + && (empty($object['in_class']) && $variable == '$this') + ) { + $this->fault($object, FAULT_MINOR, "Array key assignment on previously undefined variable: $var"); + } + $this->assigned_variables[$scope][] = $variable; + } } if ( $object['type'] == PHPPARSER_VARIABLE // Cannot yet accurately scan the global scope, so functions only && !empty($object['in_function']) - && !in_array($object['name'], $this->assigned_variables[$scope]) - && !in_array($object['name'], array( + && !in_array($variable, $this->assigned_variables[$scope]) + && !in_array($variable, array( // Superglobals are exempt, obviously '$GLOBALS', '$_SERVER', '$_GET', '$_POST', '$_FILES', '$_COOKIE', '$_SESSION', '$_REQUEST', '$_ENV' )) + && (empty($object['in_class']) && $variable == '$this') ) { - $this->fault($object, FAULT_MEDIUM, "Undefined Variable: {$object['name']}"); + $this->fault($object, FAULT_MEDIUM, "Undefined Variable: $variable"); } } } diff --git a/parser.php b/parser.php index 4ec0e8d..f5b89d1 100644 --- a/parser.php +++ b/parser.php @@ -89,14 +89,16 @@ class PHPParser { $function = $functionname = null; $string = $last_token = null; $switch = array(); - $expression = ''; + $expressions = array(''); + $expression = &$expressions[0]; $assignment = false; $line_text = ''; $internal_functions = get_defined_functions(); $internal_functions = $internal_functions['internal']; $local_functions = array(); $local_classes = array(); - $variables = array(); + $expression_variables = array(array()); + $variables = &$expression_variables[0]; $open_blocks = array( 0 ); $in_string = false; $buffer = array(T_INLINE_HTML => ''); @@ -168,6 +170,25 @@ class PHPParser { $in_foreach_params = false; $in_list = false; break; + case '[': + $expressions[] = ''; + $expression_variables[] = array(); + $variables = &$expression_variables[count($expression_variables) - 1]; + break; + case ']': + /* + TODO: Subexpression reporting is NOT quite as accurate as it should be... + See the unit test failure for an example. The assigned variable is reported fine, which is good, + but the value it is assigned will not be. Since this isn't awfully important, and the fact that + everything else does is, I'm leaving this as-is for now. + */ + $e = array_pop($expressions); + $expression = &$expressions[count($expressions) - 1]; + $expression .= trim($e); + array_pop($expression_variables); + $variables = &$expression_variables[count($expression_variables) - 1]; + $variables[count($variables) - 1] = end($variables) . '[' . trim($e) . ']'; + break; default: /* Should be able to add a hook here later on to catch the use of defines, which are basically just T_STRINGs that PHP can't find anything else to @@ -236,6 +257,8 @@ class PHPParser { ) ); } } else { $expression .= $token; } + if ($expression !== end($expressions)) + $expression = &$expressions[count($expressions) - 1]; if( !empty($buffer[T_INLINE_HTML]) && (bool)($this->fetch_mode & PHPPARSER_FETCH_INLINE_HTML) ) { $this->foundObject( array( 'type' => PHPPARSER_INLINE_HTML, @@ -336,6 +359,13 @@ class PHPParser { break; case T_VARIABLE: if ($in_list) { + /* + TODO: Proper fix for this. + Has to do with the less-than-perfect sub-expressions. If an entry in a list() is a keyed value of an array (i.e. $var[$key]), + everything sort of falls apart. + */ + if (!is_array($variables[count($variables) - 1]) ) + $variables[count($variables) - 1] = array(end($variables)); $variables[count($variables) - 1][] = $text; } else { $variables[] = $text; diff --git a/scanner.php b/scanner.php index 4f3bace..334588c 100644 --- a/scanner.php +++ b/scanner.php @@ -266,7 +266,7 @@ for( $i = 1; $i < $argc; $i++ ) { } } } -if (empty($config['modules'])) $config['modules'] = array('lint', 'functions', 'pattern'); +if (empty($config['modules'])) $config['modules'] = array('lint', 'functions', 'variables', 'pattern'); if( count( $files ) == 0 ) { if( count( $revisions ) > 0 ) { diff --git a/tests/ParserTests.php b/tests/ParserTests.php index cdaa7d4..01793b4 100644 --- a/tests/ParserTests.php +++ b/tests/ParserTests.php @@ -116,7 +116,7 @@ class ParserTests extends PHPUnit_Framework_TestCase { '$hash["answer"]=42', '$data[$index[0]]="complex"', '$index=0', - '$data[$index = 0]="nested"', + '$data[0]="nested"', ); $this->assertEquals($expected, $assignments); }