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

@@ -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($var, $value) = explode('=', $object['name']);
-			list($variable, $value) = explode('=', $object['name']);
+			if ($variable == $var) {
-			$this->assigned_variables[$scope][] = $variable;
+				// 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($variable, $this->assigned_variables[$scope])
-			&& !in_array($object['name'], array(
+			&& !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");
 		}
 	}
 }

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;

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 ) {

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);
 	}