Fatal error when inadvertently passing the same file twice and using parallel execution

[rodrigoprimo]

Describe the bug

While testing #1112, I noticed that a fatal error occurs when parallel execution is used and the same file is passed to PHPCS twice.

To reproduce

Steps to reproduce the behavior:

1. Run bin/phpcs test.php test.php --parallel=2
2. See error message displayed

PHP Fatal error:  Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: trim(): Passing null to parameter #1 ($string) of type string is deprecated in src/Files/LocalFile.php on line 32 in src/Runner.php:624
Stack trace:
#0 [internal function]: PHP_CodeSniffer\Runner->handleErrors()
#1 src/Files/LocalFile.php(32): trim()
#2 src/Files/FileList.php(197): PHP_CodeSniffer\Files\LocalFile->__construct()
#3 src/Runner.php(504): PHP_CodeSniffer\Files\FileList->current()
#4 src/Runner.php(120): PHP_CodeSniffer\Runner->run()
#5 bin/phpcs(14): PHP_CodeSniffer\Runner->runPHPCS()
#6 {main}
  thrown in src/Runner.php on line 624
EE 2 / 2 (100%)
PHP Fatal error:  Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: One or more child processes failed to run in src/Runner.php:562
Stack trace:
#0 src/Runner.php(120): PHP_CodeSniffer\Runner->run()
#1 bin/phpcs(14): PHP_CodeSniffer\Runner->runPHPCS()
#2 {main}
  thrown in src/Runner.php on line 562

Expected behavior

I would expect PHPCS to check the file only once instead of failing with a fatal error.

Versions (please complete the following information)

Operating System	Ubuntu 24.04
PHP version	8.3
PHP_CodeSniffer version	master
Standard	N/A
Install type	git clone

Additional context

I believe the problem is in the FileList class. When the same file is passed twice, it adds it only once to the FileList::$files array, but increments the FileList::$numFiles twice:

PHP_CodeSniffer/src/Files/FileList.php

Lines 134 to 137 in 8b83793

	foreach ($iterator as $path) {
	$this->files[$path] = $file;
	$this->numFiles++;
	}

This causes problems when the Runner class uses the total number of files to calculate the number of files per batch and which files to run in which batch:

PHP_CodeSniffer/src/Runner.php

Lines 463 to 563 in 0632c4c

	// Batching and forking.
	$childProcs = [];
	$numPerBatch = ceil($numFiles / $this->config->parallel);
	for ($batch = 0; $batch < $this->config->parallel; $batch++) {
	$startAt = ($batch * $numPerBatch);
	if ($startAt >= $numFiles) {
	break;
	}
	$endAt = ($startAt + $numPerBatch);
	if ($endAt > $numFiles) {
	$endAt = $numFiles;
	}
	$childOutFilename = tempnam(sys_get_temp_dir(), 'phpcs-child');
	$pid = pcntl_fork();
	if ($pid === -1) {
	throw new RuntimeException('Failed to create child process');
	} else if ($pid !== 0) {
	$childProcs[$pid] = $childOutFilename;
	} else {
	// Move forward to the start of the batch.
	$todo->rewind();
	for ($i = 0; $i < $startAt; $i++) {
	$todo->next();
	}
	// Reset the reporter to make sure only figures from this
	// file batch are recorded.
	$this->reporter->totalFiles = 0;
	$this->reporter->totalErrors = 0;
	$this->reporter->totalWarnings = 0;
	$this->reporter->totalFixable = 0;
	$this->reporter->totalFixed = 0;
	// Process the files.
	$pathsProcessed = [];
	ob_start();
	for ($i = $startAt; $i < $endAt; $i++) {
	$path = $todo->key();
	$file = $todo->current();
	if ($file->ignored === true) {
	$todo->next();
	continue;
	}
	$currDir = dirname($path);
	if ($lastDir !== $currDir) {
	if (PHP_CODESNIFFER_VERBOSITY > 0) {
	echo 'Changing into directory '.Common::stripBasepath($currDir, $this->config->basepath).PHP_EOL;
	}
	$lastDir = $currDir;
	}
	$this->processFile($file);
	$pathsProcessed[] = $path;
	$todo->next();
	}//end for
	$debugOutput = ob_get_contents();
	ob_end_clean();
	// Write information about the run to the filesystem
	// so it can be picked up by the main process.
	$childOutput = [
	'totalFiles' => $this->reporter->totalFiles,
	'totalErrors' => $this->reporter->totalErrors,
	'totalWarnings' => $this->reporter->totalWarnings,
	'totalFixable' => $this->reporter->totalFixable,
	'totalFixed' => $this->reporter->totalFixed,
	];
	$output = '<'.'?php'."\n".' $childOutput = ';
	$output .= var_export($childOutput, true);
	$output .= ";\n\$debugOutput = ";
	$output .= var_export($debugOutput, true);
	if ($this->config->cache === true) {
	$childCache = [];
	foreach ($pathsProcessed as $path) {
	$childCache[$path] = Cache::get($path);
	}
	$output .= ";\n\$childCache = ";
	$output .= var_export($childCache, true);
	}
	$output .= ";\n?".'>';
	file_put_contents($childOutFilename, $output);
	exit();
	}//end if
	}//end for
	$success = $this->processChildProcs($childProcs);
	if ($success === false) {
	throw new RuntimeException('One or more child processes failed to run');
	}

Please confirm

• I have searched the issue list and am not opening a duplicate issue.
• I have read the Contribution Guidelines and this is not a support question.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the master branch of PHP_CodeSniffer.

[jrfnl]

@rodrigoprimo Thanks for finding this. While this is primarily an error in the user input, I do agree that preventing the fatal makes sense and could be as straight-forward as just checking via isset($this->files[$path]) === false before adding the file.

Looking at the FileList class, a similar code snippet is used in multiple places in the class, so this may need a critical look to see which of those need changing and/or whether it makes sense to add a (private) function to prevent duplicate code.

Also seems like something for which it should be quite straight-forward to add a test to the test suite.

[jrfnl]

For the record: this looks to be a duplicate of the following upstream issues, which look to never have been addressed, so will be nice to fix all these issues in one go 😉

• Undefined index when running in parallel with same paths defined in different styles squizlabs/PHP_CodeSniffer#3264
• Parallel running with multiple file directives leads to "passing null to non-nullable" notice on PHP 8.1 squizlabs/PHP_CodeSniffer#3677

Open PR squizlabs/PHP_CodeSniffer#3676 attempts to fix squizlabs/PHP_CodeSniffer#3677, but is not the correct fix.
The analysis by @rodrigoprimo above about where this originates from shows the right solution direction.

[jrfnl]

Brain fart - needs verification of viability - I've now started wondering if the numFiles property should even exist... and if it does need to exist (for performance reasons or something), if it shouldn't be set just once at the end of the __construct() method via a count($this->files) instead of doing the incrementing whenever a new file is being added... ? I'd expect that would solve the issue too.