Report unintended ignoreErrors regexes as warnings

phpstan · Jan 24, 2020 · 3e1ce5d · staabm · Jan 25, 2020 · staabm
1 parent ded4467
commit 3e1ce5d
Show file tree

Hide file tree

Showing 14 changed files with 344 additions and 7 deletions.
diff --git a/composer.json b/composer.json
@@ -7,13 +7,16 @@
 	"require": {
 		"php": "^7.1",
 		"composer/xdebug-handler": "^1.3.0",
+		"hoa/compiler": "3.17.08.08",
+		"hoa/exception": "^1.0",
+		"hoa/regex": "1.17.01.13",
 		"jean85/pretty-package-versions": "^1.0.3",
 		"nette/bootstrap": "^3.0",
 		"nette/di": "^3.0",
 		"nette/neon": "^3.0",
 		"nette/robot-loader": "^3.0.1",
 		"nette/schema": "^1.0",
-		"nette/utils": "^3.0",
+		"nette/utils": "^3.1.1",
 		"nikic/php-parser": "^4.3.0",
 		"ondram/ci-detector": "^3.1",
 		"ondrejmirtes/better-reflection": "^3.5.6",

diff --git a/conf/config.neon b/conf/config.neon
@@ -290,6 +290,11 @@ services:
 		arguments:
 			memoryLimitFile: %memoryLimitFile%
 
+	-
+		class: PHPStan\Command\IgnoredRegexValidator
+		arguments:
+			parser: @regexParser
+
 	-
 		class: PHPStan\Dependency\DependencyDumper
 
@@ -977,6 +982,16 @@ services:
 			constantReflector: @betterReflectionConstantReflector
 		autowired: false
 
+	regexParser:
+		class: Hoa\Compiler\Llk\Parser
+		factory: Hoa\Compiler\Llk\Llk::load(@regexGrammarStream)
+
+	regexGrammarStream:
+		class: Hoa\File\Read
+		arguments:
+			streamName: 'hoa://Library/Regex/Grammar.pp'
+			mode: rb
+
 	runtimeReflectionProvider:
 		class: PHPStan\Reflection\Runtime\RuntimeReflectionProvider
 		autowired: false

diff --git a/src/Analyser/Analyser.php b/src/Analyser/Analyser.php
@@ -3,6 +3,7 @@
 namespace PHPStan\Analyser;
 
 use Nette\Utils\Json;
+use PHPStan\Command\IgnoredRegexValidator;
 use PHPStan\File\FileHelper;
 use PHPStan\PhpDoc\StubValidator;
 use PHPStan\Rules\Registry;
@@ -25,6 +26,9 @@ class Analyser
 	/** @var \PHPStan\PhpDoc\StubValidator */
 	private $stubValidator;
 
+	/** @var \PHPStan\Command\IgnoredRegexValidator */
+	private $ignoredRegexValidator;
+
 	/** @var (string|mixed[])[] */
 	private $ignoreErrors;
 
@@ -43,6 +47,7 @@ class Analyser
 	 * @param \PHPStan\Analyser\NodeScopeResolver $nodeScopeResolver
 	 * @param \PHPStan\File\FileHelper $fileHelper
 	 * @param \PHPStan\PhpDoc\StubValidator $stubValidator
+	 * @param \PHPStan\Command\IgnoredRegexValidator $ignoredRegexValidator
 	 * @param (string|array<string, string>)[] $ignoreErrors
 	 * @param bool $reportUnmatchedIgnoredErrors
 	 * @param int $internalErrorsCountLimit
@@ -53,6 +58,7 @@ public function __construct(
 		NodeScopeResolver $nodeScopeResolver,
 		FileHelper $fileHelper,
 		StubValidator $stubValidator,
+		IgnoredRegexValidator $ignoredRegexValidator,
 		array $ignoreErrors,
 		bool $reportUnmatchedIgnoredErrors,
 		int $internalErrorsCountLimit
@@ -63,6 +69,7 @@ public function __construct(
 		$this->nodeScopeResolver = $nodeScopeResolver;
 		$this->fileHelper = $fileHelper;
 		$this->stubValidator = $stubValidator;
+		$this->ignoredRegexValidator = $ignoredRegexValidator;
 		$this->ignoreErrors = $ignoreErrors;
 		$this->reportUnmatchedIgnoredErrors = $reportUnmatchedIgnoredErrors;
 		$this->internalErrorsCountLimit = $internalErrorsCountLimit;
@@ -87,9 +94,9 @@ public function analyse(
 	): array
 	{
 		$errors = [];
-
 		$otherIgnoreErrors = [];
 		$ignoreErrorsByFile = [];
+		$warnings = [];
 		foreach ($this->ignoreErrors as $i => $ignoreError) {
 			try {
 				if (is_array($ignoreError)) {
@@ -126,12 +133,23 @@ public function analyse(
 					}
 
 					$ignoreMessage = $ignoreError['message'];
+					if (isset($ignoreError['count'])) {
+						continue; // ignoreError coming from baseline will be correct
+					}
+					$ignoredTypes = $this->ignoredRegexValidator->getIgnoredTypes($ignoreMessage);
+					if (count($ignoredTypes) > 0) {
+						$warnings[] = $this->createIgnoredTypesWarning($ignoreMessage, $ignoredTypes);
+					}
 				} else {
 					$otherIgnoreErrors[] = [
 						'index' => $i,
 						'ignoreError' => $ignoreError,
 					];
 					$ignoreMessage = $ignoreError;
+					$ignoredTypes = $this->ignoredRegexValidator->getIgnoredTypes($ignoreMessage);
+					if (count($ignoredTypes) > 0) {
+						$warnings[] = $this->createIgnoredTypesWarning($ignoreMessage, $ignoredTypes);
+					}
 				}
 
 				\Nette\Utils\Strings::match('', $ignoreMessage);
@@ -349,7 +367,28 @@ public function analyse(
 			$errors[] = sprintf('Reached internal errors count limit of %d, exiting...', $this->internalErrorsCountLimit);
 		}
 
-		return $errors;
+		return array_merge($errors, $warnings);
+	}
+
+	/**
+	 * @param string $regex
+	 * @param array<string, string> $ignoredTypes
+	 * @return Error
+	 */
+	private function createIgnoredTypesWarning(string $regex, array $ignoredTypes): Error
+	{
+		return new Error(
+			sprintf("Ignored error %s has an unescaped '|' which leads to ignoring more errors than intended. Use '\\|' instead.\n%s", $regex, sprintf("It ignores all errors containing the following types:\n%s", implode("\n", array_map(static function (string $typeDescription): string {
+				return sprintf('* %s', $typeDescription);
+			}, array_keys($ignoredTypes))))),
+			'placeholder', // this value will never get used
+			null,
+			false,
+			null,
+			null,
+			null,
+			true
+		);
 	}
 
 	/**

diff --git a/src/Analyser/Error.php b/src/Analyser/Error.php
@@ -26,14 +26,18 @@ class Error
 	/** @var string|null */
 	private $tip;
 
+	/** @var bool */
+	private $warning;
+
 	public function __construct(
 		string $message,
 		string $file,
 		?int $line = null,
 		bool $canBeIgnored = true,
 		?string $filePath = null,
 		?string $traitFilePath = null,
-		?string $tip = null
+		?string $tip = null,
+		bool $warning = false
 	)
 	{
 		$this->message = $message;
@@ -43,6 +47,7 @@ public function __construct(
 		$this->filePath = $filePath;
 		$this->traitFilePath = $traitFilePath;
 		$this->tip = $tip;
+		$this->warning = $warning;
 	}
 
 	public function getMessage(): string
@@ -101,4 +106,9 @@ public function withoutTip(): self
 		);
 	}
 
+	public function isWarning(): bool
+	{
+		return $this->warning;
+	}
+
 }
diff --git a/src/Command/AnalyseApplication.php b/src/Command/AnalyseApplication.php
@@ -136,10 +136,15 @@ static function (Node $node, Scope $scope) use (&$hasInferrablePropertyTypesFrom
 
 		$fileSpecificErrors = [];
 		$notFileSpecificErrors = [];
+		$warnings = [];
 		foreach ($errors as $error) {
 			if (is_string($error)) {
 				$notFileSpecificErrors[] = $error;
 			} else {
+				if ($error->isWarning()) {
+					$warnings[] = $error->getMessage();
+					continue;
+				}
 				$fileSpecificErrors[] = $error;
 			}
 		}
@@ -148,6 +153,7 @@ static function (Node $node, Scope $scope) use (&$hasInferrablePropertyTypesFrom
 			new AnalysisResult(
 				$fileSpecificErrors,
 				$notFileSpecificErrors,
+				$warnings,
 				$defaultLevelUsed,
 				$hasInferrablePropertyTypesFromConstructor,
 				$projectConfigFile

diff --git a/src/Command/AnalysisResult.php b/src/Command/AnalysisResult.php
@@ -13,6 +13,9 @@ class AnalysisResult
 	/** @var string[] */
 	private $notFileSpecificErrors;
 
+	/** @var string[] */
+	private $warnings;
+
 	/** @var bool */
 	private $defaultLevelUsed;
 
@@ -25,13 +28,15 @@ class AnalysisResult
 	/**
 	 * @param \PHPStan\Analyser\Error[] $fileSpecificErrors
 	 * @param string[] $notFileSpecificErrors
+	 * @param string[] $warnings
 	 * @param bool $defaultLevelUsed
 	 * @param bool $hasInferrablePropertyTypesFromConstructor
 	 * @param string|null $projectConfigFile
 	 */
 	public function __construct(
 		array $fileSpecificErrors,
 		array $notFileSpecificErrors,
+		array $warnings,
 		bool $defaultLevelUsed,
 		bool $hasInferrablePropertyTypesFromConstructor,
 		?string $projectConfigFile
@@ -54,6 +59,7 @@ static function (Error $a, Error $b): int {
 
 		$this->fileSpecificErrors = $fileSpecificErrors;
 		$this->notFileSpecificErrors = $notFileSpecificErrors;
+		$this->warnings = $warnings;
 		$this->defaultLevelUsed = $defaultLevelUsed;
 		$this->hasInferrablePropertyTypesFromConstructor = $hasInferrablePropertyTypesFromConstructor;
 		$this->projectConfigFile = $projectConfigFile;
@@ -85,6 +91,19 @@ public function getNotFileSpecificErrors(): array
 		return $this->notFileSpecificErrors;
 	}
 
+	/**
+	 * @return string[]
+	 */
+	public function getWarnings(): array
+	{
+		return $this->warnings;
+	}
+
+	public function hasWarnings(): bool
+	{
+		return count($this->warnings) > 0;
+	}
+
 	public function isDefaultLevelUsed(): bool
 	{
 		return $this->defaultLevelUsed;

diff --git a/src/Command/ErrorFormatter/TableErrorFormatter.php b/src/Command/ErrorFormatter/TableErrorFormatter.php
@@ -67,7 +67,7 @@ public function formatErrors(
 			$output->writeLineFormatted('');
 		};
 
-		if (!$analysisResult->hasErrors()) {
+		if (!$analysisResult->hasErrors() && !$analysisResult->hasWarnings()) {
 			$style->success('No errors');
 			if ($this->showTipsOfTheDay) {
 				if ($analysisResult->isDefaultLevelUsed()) {
@@ -122,13 +122,29 @@ public function formatErrors(
 			}, $analysisResult->getNotFileSpecificErrors()));
 		}
 
-		$style->error(sprintf($analysisResult->getTotalErrorsCount() === 1 ? 'Found %d error' : 'Found %d errors', $analysisResult->getTotalErrorsCount()));
+		$warningsCount = count($analysisResult->getWarnings());
+		if ($warningsCount > 0) {
+			$style->table(['', 'Warning'], array_map(static function (string $warning): array {
+				return ['', $warning];
+			}, $analysisResult->getWarnings()));
+		}
+
+		$finalMessage = sprintf($analysisResult->getTotalErrorsCount() === 1 ? 'Found %d error' : 'Found %d errors', $analysisResult->getTotalErrorsCount());
+		if ($warningsCount > 0) {
+			$finalMessage .= sprintf($warningsCount === 1 ? ' and %d warning' : ' and %d warnings', $warningsCount);
+		}
+
+		if ($analysisResult->getTotalErrorsCount() > 0) {
+			$style->error($finalMessage);
+		} else {
+			$style->warning($finalMessage);
+		}
 
 		if ($this->checkMissingTypehints && $this->showTipsOfTheDay) {
 			$showInferPropertiesTip();
 		}
 
-		return 1;
+		return $analysisResult->getTotalErrorsCount() > 0 ? 1 : 0;
 	}
 
 }