Fix #1311: Reduce false positives for loop variable prefix warnings

includes/Checker/Checks/Plugin_Repo/Prefixing_Check.php

@@ -25,6 +25,14 @@ class Prefixing_Check extends Abstract_PHP_CodeSniffer_Check {
 	use Prefix_Utils;
 	use Stable_Check;
 
+	/**
+	 * Cache of file contents for line-level checks.
+	 *
+	 * @since n.e.x.t
+	 * @var array<string, array<int, string>>
+	 */
+	private $file_line_cache = array();
+
 	/**
 	 * Gets the categories for the check.
 	 *
@@ -106,10 +114,150 @@ public function get_documentation_url(): string {
 	 * @param int          $severity Severity level. Default is 5.
 	 */
 	protected function add_result_message_for_file( Check_Result $result, $error, $message, $code, $file, $line = 0, $column = 0, string $docs = '', $severity = 5 ) {
+		// Suppress false positives where a variable sits inside a `foreach` `as` clause
+		// or a `for` loop initializer at the top level of a file. PHPCS' WPCS sniff
+		// treats these as global variable definitions because the enclosing function
+		// early-return does not apply to file top-level scope, but they are loop-local.
+		if ( 'WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedVariableFound' === $code
+			&& $this->is_variable_in_loop_control( $file, $line )
+		) {
+			return;
+		}
+
 		// Update error type and severity.
 		$error    = false;
 		$severity = 6;
 
 		parent::add_result_message_for_file( $result, $error, $message, $code, $file, $line, $column, $docs, $severity );
 	}
+
+	/**
+	 * Determines whether the variable on the given file/line sits inside a
+	 * `foreach` `as` clause or a `for` loop initializer.
+	 *
+	 * Used to filter out false-positive prefix warnings for loop-local variables
+	 * at the top level of a plugin file, where the WPCS sniff's normal
+	 * function-scope early-return does not apply.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param string $file Absolute path to the file.
+	 * @param int    $line 1-based line number to inspect.
+	 * @return bool True when the variable lives inside a loop control structure.
+	 */
+	private function is_variable_in_loop_control( $file, $line ) {
+		if ( $line <= 0 || ! is_string( $file ) || '' === $file || ! file_exists( $file ) ) {
+			return false;
+		}
+
+		if ( ! isset( $this->file_line_cache[ $file ] ) ) {
+			$contents = file_get_contents( $file ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
+			if ( false === $contents ) {
+				return false;
+			}
+			$this->file_line_cache[ $file ] = preg_split( '/\r\n|\r|\n/', $contents );
+		}
+
+		$lines = $this->file_line_cache[ $file ];
+		if ( ! isset( $lines[ $line - 1 ] ) ) {
+			return false;
+		}
+
+		$source = $lines[ $line - 1 ];
+
+		if ( $this->is_in_single_line_loop_header( $source ) ) {
+			return true;
+		}
+
+		return $this->is_in_multiline_loop_header( $lines, $line );
+	}
+
+	/**
+	 * Checks whether a single source line contains a `foreach ... as $var` or
+	 * `for ( ... $var = ...` loop header.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param string $source Source line content.
+	 * @return bool True when the line is a single-line loop header.
+	 */
+	private function is_in_single_line_loop_header( $source ) {
+		// foreach ( $items as $key => $value ).
+		if ( preg_match( '/\bforeach\s*\([^)]*\bas\s+\$/', $source ) ) {
+			return true;
+		}
+
+		// for ( $i = 0, $j = 10; ... ) — match any `$var =` inside parens.
+		if ( preg_match( '/\bfor\s*\([^)]*?\$\w+\s*=/', $source ) ) {
+			return true;
+		}
+
+		return false;
+	}
+
+	/**
+	 * Checks whether the reported line is part of a multi-line `foreach` or
+	 * `for` header whose opener sits on a previous line.
+	 *
+	 * Looks back up to 10 lines for `foreach (` or `for (` with an unclosed
+	 * opening parenthesis, then walks forward to the reported line checking
+	 * for the `as` keyword (foreach) or `$var =` initializer (for).
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param array<int, string> $lines File contents split by line.
+	 * @param int                $line  1-based line number being inspected.
+	 * @return bool True when the line is inside a multi-line loop header.
+	 */
+	private function is_in_multiline_loop_header( $lines, $line ) {
+		$min_back = max( 0, $line - 11 );
+
+		for ( $i = $line - 2; $i >= $min_back; $i-- ) {
+			if ( ! isset( $lines[ $i ] ) ) {
+				continue;
+			}
+			if ( ! preg_match( '/\b(foreach|for)\s*\(\s*$/', $lines[ $i ] ) ) {
+				continue;
+			}
+			if ( $this->loop_header_contains_variable( $lines, $i + 1, $line ) ) {
+				return true;
+			}
+		}
+
+		return false;
+	}
+
+	/**
+	 * Walks forward from the loop opener to the reported line and checks
+	 * for an `as` keyword (foreach) or `$var =` initializer (for) before
+	 * any statement-terminating `;` or paren-closing `)`.
+	 *
+	 * @since n.e.x.t
+	 *
+	 * @param array<int, string> $lines File contents split by line.
+	 * @param int                $start 1-based line index just after the opener.
+	 * @param int                $end   1-based line index of the reported line.
+	 * @return bool True when a loop header variable is found.
+	 */
+	private function loop_header_contains_variable( $lines, $start, $end ) {
+		for ( $j = $start; $j <= $end; $j++ ) {
+			if ( ! isset( $lines[ $j - 1 ] ) ) {
+				continue;
+			}
+			$scan = $lines[ $j - 1 ];
+			if ( preg_match( '/\bas\b/', $scan ) ) {
+				return true;
+			}
+			if ( preg_match( '/\$\w+\s*=/', $scan ) ) {
+				return true;
+			}
+			if ( strpos( $scan, ';' ) !== false ) {
+				return false;
+			}
+			if ( strpos( $scan, ')' ) !== false ) {
+				return false;
+			}
+		}
+		return false;
+	}
 }

tests/phpunit/testdata/plugins/test-plugin-prefixing-foreach/load.php

@@ -0,0 +1,78 @@
+<?php
+/**
+ * Plugin Name: Test Plugin Prefixing Foreach
+ * Plugin URI: https://github.com/WordPress/plugin-check
+ * Description: Test plugin for the Prefixing check loop variable false positives.
+ * Requires at least: 6.0
+ * Requires PHP: 7.2
+ * Version: 1.0.0
+ * Author: WordPress Performance Team
+ * Author URI: https://make.wordpress.org/performance/
+ * License: GPLv2 or later
+ * License URI: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * Text Domain: test-plugin-prefixing-foreach
+ *
+ * @package test-plugin-prefixing-foreach
+ */
+
+$items = array( 'a', 'b', 'c' );
+
+// foreach: $key and $value are loop-local, must not be flagged.
+foreach ( $items as $key => $value ) {
+	echo $value;
+}
+
+// foreach without key: $value is loop-local, must not be flagged.
+foreach ( $items as $value ) {
+	echo $value;
+}
+
+// for: $i is loop-local, must not be flagged.
+for ( $i = 0; $i < 10; $i++ ) {
+	echo $i;
+}
+
+// Multi-line foreach: opener on previous line, $key/$value on the
+// `as` line. Both loop-local, must not be flagged.
+foreach (
+	$items
+	as $key => $value
+) {
+	echo $value;
+}
+
+// Multi-line for: opener on previous line, $i on the init line.
+// Loop-local, must not be flagged.
+for (
+	$i = 0;
+	$i < 10;
+	$i++
+) {
+	echo $i;
+}
+
+// Multi-line foreach with split `as` and `$var` on different lines.
+// Opener on line 1, `as` on line 3, `$key` on line 4. $key is still
+// a loop header variable, must not be flagged.
+foreach (
+	$items
+	as
+	$key => $value
+) {
+	echo $value;
+}
+
+// Multi-line for with multi-init on different lines. $i and $j both
+// declared in init; both loop-local, must not be flagged.
+for (
+	$i = 0,
+	$j = 10;
+	$i < $j;
+	$i++, $j--
+) {
+	echo $i;
+}
+
+function test_pp_foreach_helper() {
+	return true;
+}

tests/phpunit/tests/Checker/Checks/Prefixing_Check_Tests.php

@@ -28,4 +28,34 @@ public function test_run_with_errors() {
 		$this->assertCount( 1, wp_list_filter( $warnings['load.php'][28][1], array( 'code' => 'WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound' ) ) );
 		$this->assertCount( 1, wp_list_filter( $warnings['load.php'][41][1], array( 'code' => 'WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedFunctionFound' ) ) );
 	}
+
+	public function test_loop_variables_not_flagged() {
+		$check         = new Prefixing_Check();
+		$check_context = new Check_Context( UNIT_TESTS_PLUGIN_DIR . 'test-plugin-prefixing-foreach/load.php' );
+		$check_result  = new Check_Result( $check_context );
+
+		$check->run( $check_result );
+
+		$warnings = $check_result->get_warnings();
+
+		// Single-line foreach/for header lines: variables declared in
+		// `as $key => $value` or `for ( $i = 0; ...` are loop-local and
+		// must not be reported.
+		$this->assertArrayNotHasKey( 21, $warnings['load.php'] ?? array() );
+		$this->assertArrayNotHasKey( 26, $warnings['load.php'] ?? array() );
+		$this->assertArrayNotHasKey( 31, $warnings['load.php'] ?? array() );
+
+		// Multi-line foreach/for with `as`/`$var` on the same line.
+		$this->assertArrayNotHasKey( 39, $warnings['load.php'] ?? array() );
+		$this->assertArrayNotHasKey( 47, $warnings['load.php'] ?? array() );
+
+		// Multi-line foreach with `as` and `$var` on separate lines.
+		// $key on line 60 is still a loop header variable.
+		$this->assertArrayNotHasKey( 60, $warnings['load.php'] ?? array() );
+
+		// Multi-line for with multi-init on separate lines.
+		// $i (line 68) and $j (line 69) are both init expressions.
+		$this->assertArrayNotHasKey( 68, $warnings['load.php'] ?? array() );
+		$this->assertArrayNotHasKey( 69, $warnings['load.php'] ?? array() );
+	}
 }