Make sniffs for global variables skip dynamic names

The actual names could be pretty much everything, especially for a tool like PHPCS that really doesn't understand the program flow. Therefore, just avoid emitting any warning, and just skip ahead whenever a dynamic identifier is detected. Also: - Make a conditional explicitly check for T_VARIABLE, instead of checking that the token is not whitespace or comma and then proceeding to assume that it must be a variable. - Avoid overwriting a function parameter with a loop variable. Bug: T341393 Change-Id: If3555e448b7ae1785835b788785466d6f5d64a92
wikimedia · Jul 7, 2023 · 482d9e5 · 482d9e5
1 parent e8ccb98
commit 482d9e5
Show file tree

Hide file tree

Showing 6 changed files with 20 additions and 10 deletions.
diff --git a/MediaWiki/Sniffs/NamingConventions/ValidGlobalNameSniff.php b/MediaWiki/Sniffs/NamingConventions/ValidGlobalNameSniff.php
@@ -74,9 +74,8 @@ public function process( File $phpcsFile, $stackPtr ) {
 		$semicolonIndex = $phpcsFile->findNext( T_SEMICOLON, $stackPtr + 1 );
 
 		while ( $nameIndex < $semicolonIndex ) {
-			if ( $tokens[$nameIndex ]['code'] !== T_WHITESPACE
-				&& $tokens[$nameIndex ]['code'] !== T_COMMA
-			) {
+			// Note, this skips dynamic identifiers.
+			if ( $tokens[$nameIndex ]['code'] === T_VARIABLE && $tokens[$nameIndex - 1]['code'] !== T_DOLLAR ) {
 				$globalName = $tokens[$nameIndex]['content'];
 
 				if ( in_array( $globalName, $this->ignoreList ) ||

diff --git a/MediaWiki/Sniffs/VariableAnalysis/UnusedGlobalVariablesSniff.php b/MediaWiki/Sniffs/VariableAnalysis/UnusedGlobalVariablesSniff.php
@@ -64,7 +64,8 @@ public function process( File $phpcsFile, $stackPtr ) {
 
 			if ( $code === T_GLOBAL ) {
 				$endOfGlobal = $phpcsFile->findEndOfStatement( $i, T_COMMA );
-			} elseif ( $code === T_VARIABLE ) {
+			} elseif ( $code === T_VARIABLE && $tokens[$i - 1]['code'] !== T_DOLLAR ) {
+				// Note, this skips dynamic variable names.
 				$variableName = $tokens[$i]['content'];
 				if ( $i < $endOfGlobal ) {
 					$globalVariables[$variableName] = $i;
@@ -82,10 +83,10 @@ public function process( File $phpcsFile, $stackPtr ) {
 			}
 		}
 
-		foreach ( $globalVariables as $variableName => $stackPtr ) {
+		foreach ( $globalVariables as $variableName => $variableIndex ) {
 			$phpcsFile->addWarning(
 				'Global %s is never used.',
-				$stackPtr,
+				$variableIndex,
 				'UnusedGlobal' . $variableName,
 				[ $variableName ]
 			);

diff --git a/MediaWiki/Tests/files/NamingConventions/case_global_name.php b/MediaWiki/Tests/files/NamingConventions/case_global_name.php
@@ -9,8 +9,10 @@ function wfFailedExamples() {
 	// Testing with a valid variable in the middle to ensure that the global after
 	// it is still checked, see T279968
 	global $wgsomething, $wgValidName, $LocalInterwikis;
+	global $thisIsWrong, $$thisShouldBeSkipped;
 	$wgsomething = $wgValidName;
 	$LocalInterwikis = false;
+	echo $thisIsWrong;
 }
 
 /**
@@ -19,6 +21,8 @@ function wfFailedExamples() {
  */
 function wfPassedExamples() {
 	global $wgSomething, $wgLocalInterwikis, $wg3dProcessor;
+	global $$dynamicNameShouldBeSkipped;
+	global $$$$$$thisShouldAlsoBeSkipped;
 	$wgSomething = 5;
 	$wgLocalInterwikis = false;
 	$wg3dProcessor = null;

diff --git a/MediaWiki/Tests/files/NamingConventions/case_global_name.php.expect b/MediaWiki/Tests/files/NamingConventions/case_global_name.php.expect
@@ -1,4 +1,6 @@
  11 | ERROR | Global variable "$wgsomething" should use CamelCase: "$wgWgsomething"
     |       | (MediaWiki.NamingConventions.ValidGlobalName.CamelCase)
  11 | ERROR | Global variable "$LocalInterwikis" is lacking an allowed prefix ('wg'). Should be
-    |       | "$wgLocalInterwikis". (MediaWiki.NamingConventions.ValidGlobalName.allowedPrefix)
+    |       | "$wgLocalInterwikis". (MediaWiki.NamingConventions.ValidGlobalName.allowedPrefix)
+ 12 | ERROR | Global variable "$thisIsWrong" is lacking an allowed prefix ('wg'). Should be
+    |       | "$wgThisIsWrong". (MediaWiki.NamingConventions.ValidGlobalName.allowedPrefix)
diff --git a/MediaWiki/Tests/files/VariableAnalysis/used_global_variables.php b/MediaWiki/Tests/files/VariableAnalysis/used_global_variables.php
@@ -9,6 +9,7 @@ function wfFailedExamples() {
 	global $wgSomething;
 	global $wgSameLine,
 		$wgNextLine;
+	global $wgThisIsNotUsed, $$skipThisButNotThePreviousOne;
 
 	static function () {
 		global $wgSomething, $wgClosureUnused;
@@ -32,6 +33,7 @@ function wfPassedExamples() {
 	global $wgTwo, $wgClosure,
 		$wgThree,
 		$wgFour;
+	global $$dynamicNameShouldBeSkipped;
 
 	// global variable used via heredoc.
 	$foo = <<<PHP

diff --git a/MediaWiki/Tests/files/VariableAnalysis/used_global_variables.php.expect b/MediaWiki/Tests/files/VariableAnalysis/used_global_variables.php.expect
@@ -4,7 +4,9 @@
     |         | (MediaWiki.VariableAnalysis.UnusedGlobalVariables.UnusedGlobal$wgSameLine)
  11 | WARNING | Global $wgNextLine is never used.
     |         | (MediaWiki.VariableAnalysis.UnusedGlobalVariables.UnusedGlobal$wgNextLine)
- 14 | WARNING | Global $wgClosureUnused is never used.
+ 12 | WARNING | Global $wgThisIsNotUsed is never used.
+    |         | (MediaWiki.VariableAnalysis.UnusedGlobalVariables.UnusedGlobal$wgThisIsNotUsed)
+ 15 | WARNING | Global $wgClosureUnused is never used.
     |         | (MediaWiki.VariableAnalysis.UnusedGlobalVariables.UnusedGlobal$wgClosureUnused)
- 20 | WARNING | Global $wgAnonUnused is never used.
-    |         | (MediaWiki.VariableAnalysis.UnusedGlobalVariables.UnusedGlobal$wgAnonUnused)
+ 21 | WARNING | Global $wgAnonUnused is never used.
+    |         | (MediaWiki.VariableAnalysis.UnusedGlobalVariables.UnusedGlobal$wgAnonUnused)