diff --git a/conf/config.neon b/conf/config.neon index 19056af1ab0..8bfc9daeac4 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -118,6 +118,7 @@ parameters: resolvedPhpDocBlockCacheCountMax: 2048 nameScopeMapMemoryCacheCountMax: 2048 reportUnmatchedIgnoredErrors: true + reportIgnoresWithoutComments: false typeAliases: [] universalObjectCratesClasses: - stdClass diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index 269dc27c838..e68783e1a0b 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -143,6 +143,7 @@ parametersSchema: nameScopeMapMemoryCacheCountMax: int() ]) reportUnmatchedIgnoredErrors: bool() + reportIgnoresWithoutComments: bool() typeAliases: arrayOf(string()) universalObjectCratesClasses: listOf(string()) stubFiles: listOf(string()) diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 4da2feb9988..2212ddc850b 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -1 +1,3 @@ includes: [ build/phpstan.neon ] +parameters: + reportIgnoresWithoutComments: true diff --git a/src/Analyser/AnalyserResultFinalizer.php b/src/Analyser/AnalyserResultFinalizer.php index 6c3ab6b2c26..9a0638c2100 100644 --- a/src/Analyser/AnalyserResultFinalizer.php +++ b/src/Analyser/AnalyserResultFinalizer.php @@ -28,18 +28,17 @@ public function __construct( private LocalIgnoresProcessor $localIgnoresProcessor, #[AutowiredParameter] private bool $reportUnmatchedIgnoredErrors, + #[AutowiredParameter] + private bool $reportIgnoresWithoutComments, ) { } public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $debug): FinalizerResult { - if (count($analyserResult->getCollectedData()) === 0) { - return $this->addUnmatchedIgnoredErrors($this->mergeFilteredPhpErrors($analyserResult), [], []); - } - + $hasCollectedData = count($analyserResult->getCollectedData()) > 0; $hasInternalErrors = count($analyserResult->getInternalErrors()) > 0 || $analyserResult->hasReachedInternalErrorsCountLimit(); - if ($hasInternalErrors) { + if (! $hasCollectedData || $hasInternalErrors) { return $this->addUnmatchedIgnoredErrors($this->mergeFilteredPhpErrors($analyserResult), [], []); } @@ -205,7 +204,7 @@ private function addUnmatchedIgnoredErrors( foreach ($identifiers as $identifier) { $errors[] = (new Error( - sprintf('No error with identifier %s is reported on line %d.', $identifier, $line), + sprintf('No error with identifier %s is reported on line %d.', $identifier['name'], $line), $file, $line, false, diff --git a/src/Analyser/FileAnalyser.php b/src/Analyser/FileAnalyser.php index 7748903cbfe..ab7fe45f1f0 100644 --- a/src/Analyser/FileAnalyser.php +++ b/src/Analyser/FileAnalyser.php @@ -60,6 +60,8 @@ public function __construct( private IgnoreErrorExtensionProvider $ignoreErrorExtensionProvider, private RuleErrorTransformer $ruleErrorTransformer, private LocalIgnoresProcessor $localIgnoresProcessor, + #[AutowiredParameter] + private bool $reportIgnoresWithoutComments = false, ) { } @@ -129,6 +131,30 @@ public function analyseFile( $temporaryFileErrors = $nodeCallback->getTemporaryFileErrors(); $processedFiles = $nodeCallback->getProcessedFiles(); + if ($this->reportIgnoresWithoutComments) { + foreach ($linesToIgnore[$file] ?? [] as $line => $identifiers) { + if ($identifiers === null) { + continue; + } + + foreach ($identifiers as $identifier) { + if ($identifier['comment'] !== null) { + continue; + } + + $fileErrors[] = (new Error( + sprintf('Ignore with identifier %s has no comment.', $identifier['name']), + $file, + $line, + false, + $file, + null, + 'Explain why this ignore is necessary in parentheses after the identifier.', + ))->withIdentifier('ignore.noComment'); + } + } + } + $localIgnoresProcessorResult = $this->localIgnoresProcessor->process( $temporaryFileErrors, $linesToIgnore, diff --git a/src/Analyser/FileAnalyserCallback.php b/src/Analyser/FileAnalyserCallback.php index 474ccee859f..304513f7f4e 100644 --- a/src/Analyser/FileAnalyserCallback.php +++ b/src/Analyser/FileAnalyserCallback.php @@ -21,6 +21,7 @@ /** * @phpstan-import-type CollectorData from CollectedData + * @phpstan-import-type Identifier from FileAnalyserResult * @phpstan-import-type LinesToIgnore from FileAnalyserResult */ final class FileAnalyserCallback @@ -231,7 +232,7 @@ public function __invoke(Node $node, Scope $scope): void /** * @param Node[] $nodes - * @return array|null> + * @return array|null> */ private function getLinesToIgnoreFromTokens(array $nodes): array { @@ -239,7 +240,7 @@ private function getLinesToIgnoreFromTokens(array $nodes): array return []; } - /** @var array|null> */ + /** @var array|null> */ return $nodes[0]->getAttribute('linesToIgnore', []); } diff --git a/src/Analyser/FileAnalyserResult.php b/src/Analyser/FileAnalyserResult.php index 4a80d9ae9ce..320b736c6b7 100644 --- a/src/Analyser/FileAnalyserResult.php +++ b/src/Analyser/FileAnalyserResult.php @@ -6,7 +6,8 @@ use PHPStan\Dependency\RootExportedNode; /** - * @phpstan-type LinesToIgnore = array|null>> + * @phpstan-type Identifier = array{name: string, comment: string|null} + * @phpstan-type LinesToIgnore = array|null>> * @phpstan-import-type CollectorData from CollectedData */ final class FileAnalyserResult diff --git a/src/Analyser/LocalIgnoresProcessor.php b/src/Analyser/LocalIgnoresProcessor.php index 192b9092511..b7b0f289ada 100644 --- a/src/Analyser/LocalIgnoresProcessor.php +++ b/src/Analyser/LocalIgnoresProcessor.php @@ -49,7 +49,7 @@ public function process( } foreach ($identifiers as $i => $ignoredIdentifier) { - if ($ignoredIdentifier !== $tmpFileError->getIdentifier()) { + if ($ignoredIdentifier['name'] !== $tmpFileError->getIdentifier()) { continue; } @@ -68,7 +68,7 @@ public function process( $unmatchedIgnoredIdentifiers = $unmatchedLineIgnores[$tmpFileError->getFile()][$line]; if (is_array($unmatchedIgnoredIdentifiers)) { foreach ($unmatchedIgnoredIdentifiers as $j => $unmatchedIgnoredIdentifier) { - if ($ignoredIdentifier !== $unmatchedIgnoredIdentifier) { + if ($ignoredIdentifier['name'] !== $unmatchedIgnoredIdentifier['name']) { continue; } diff --git a/src/Command/FixerWorkerCommand.php b/src/Command/FixerWorkerCommand.php index 7d55af74bb7..a0d24fbaa56 100644 --- a/src/Command/FixerWorkerCommand.php +++ b/src/Command/FixerWorkerCommand.php @@ -313,7 +313,7 @@ function (array $errors, array $locallyIgnoredErrors, array $analysedFiles) use if ($error->getIdentifier() === null) { continue; } - if (!in_array($error->getIdentifier(), ['ignore.count', 'ignore.unmatched', 'ignore.unmatchedLine', 'ignore.unmatchedIdentifier'], true)) { + if (!in_array($error->getIdentifier(), ['ignore.count', 'ignore.unmatched', 'ignore.unmatchedLine', 'ignore.unmatchedIdentifier', 'ignore.noComment'], true)) { continue; } $ignoreFileErrors[] = $error; diff --git a/src/Parser/RichParser.php b/src/Parser/RichParser.php index d49d98bf2db..4d3f0bdb4b9 100644 --- a/src/Parser/RichParser.php +++ b/src/Parser/RichParser.php @@ -7,12 +7,14 @@ use PhpParser\NodeTraverser; use PhpParser\NodeVisitor\NameResolver; use PhpParser\Token; +use PHPStan\Analyser\FileAnalyserResult; use PHPStan\Analyser\Ignore\IgnoreLexer; use PHPStan\Analyser\Ignore\IgnoreParseException; use PHPStan\DependencyInjection\Container; use PHPStan\File\FileReader; use PHPStan\ShouldNotHappenException; use function array_filter; +use function array_key_last; use function array_map; use function count; use function implode; @@ -30,6 +32,9 @@ use const T_DOC_COMMENT; use const T_WHITESPACE; +/** + * @phpstan-import-type Identifier from FileAnalyserResult + */ final class RichParser implements Parser { @@ -120,7 +125,7 @@ public function parseString(string $sourceCode): array /** * @param Token[] $tokens - * @return array{lines: array|null>, errors: array>} + * @return array{lines: array|null>, errors: array>} */ private function getLinesToIgnore(array $tokens): array { @@ -277,33 +282,29 @@ private function getLinesToIgnoreForTokenByIgnoreComment( } /** - * @return non-empty-list + * @return non-empty-list * @throws IgnoreParseException */ private function parseIdentifiers(string $text, int $ignorePos): array { $text = substr($text, $ignorePos + strlen('@phpstan-ignore')); - $originalTokens = $this->ignoreLexer->tokenize($text); - $tokens = []; - - foreach ($originalTokens as $originalToken) { - if ($originalToken[IgnoreLexer::TYPE_OFFSET] === IgnoreLexer::TOKEN_WHITESPACE) { - continue; - } - $tokens[] = $originalToken; - } + $tokens = $this->ignoreLexer->tokenize($text); $c = count($tokens); $identifiers = []; + $comment = null; $openParenthesisCount = 0; $expected = [IgnoreLexer::TOKEN_IDENTIFIER]; + $lastTokenTypeLabel = '@phpstan-ignore'; for ($i = 0; $i < $c; $i++) { - $lastTokenTypeLabel = isset($tokenType) ? $this->ignoreLexer->getLabel($tokenType) : '@phpstan-ignore'; + if (isset($tokenType) && $tokenType !== IgnoreLexer::TOKEN_WHITESPACE) { + $lastTokenTypeLabel = $this->ignoreLexer->getLabel($tokenType); + } [IgnoreLexer::VALUE_OFFSET => $content, IgnoreLexer::TYPE_OFFSET => $tokenType, IgnoreLexer::LINE_OFFSET => $tokenLine] = $tokens[$i]; - if ($expected !== null && !in_array($tokenType, $expected, true)) { + if ($expected !== null && !in_array($tokenType, [...$expected, IgnoreLexer::TOKEN_WHITESPACE], true)) { $tokenTypeLabel = $this->ignoreLexer->getLabel($tokenType); $otherTokenContent = $tokenType === IgnoreLexer::TOKEN_OTHER ? sprintf(" '%s'", $content) : ''; $expectedLabels = implode(' or ', array_map(fn ($token) => $this->ignoreLexer->getLabel($token), $expected)); @@ -312,6 +313,9 @@ private function parseIdentifiers(string $text, int $ignorePos): array } if ($tokenType === IgnoreLexer::TOKEN_OPEN_PARENTHESIS) { + if ($openParenthesisCount > 0) { + $comment .= $content; + } $openParenthesisCount++; $expected = null; continue; @@ -320,17 +324,25 @@ private function parseIdentifiers(string $text, int $ignorePos): array if ($tokenType === IgnoreLexer::TOKEN_CLOSE_PARENTHESIS) { $openParenthesisCount--; if ($openParenthesisCount === 0) { + $key = array_key_last($identifiers); + if ($key !== null) { + $identifiers[$key]['comment'] = $comment; + $comment = null; + } $expected = [IgnoreLexer::TOKEN_COMMA, IgnoreLexer::TOKEN_END]; + } else { + $comment .= $content; } continue; } if ($openParenthesisCount > 0) { + $comment .= $content; continue; // waiting for comment end } if ($tokenType === IgnoreLexer::TOKEN_IDENTIFIER) { - $identifiers[] = $content; + $identifiers[] = ['name' => $content, 'comment' => null]; $expected = [IgnoreLexer::TOKEN_COMMA, IgnoreLexer::TOKEN_END, IgnoreLexer::TOKEN_OPEN_PARENTHESIS]; continue; } @@ -349,6 +361,7 @@ private function parseIdentifiers(string $text, int $ignorePos): array throw new IgnoreParseException('Missing identifier', 1); } + /** @phpstan-ignore return.type (return type is correct, not sure why it's being changed from array shape to key-value shape) */ return $identifiers; } diff --git a/src/Testing/RuleTestCase.php b/src/Testing/RuleTestCase.php index 9ce1c5004f7..2fd899bb630 100644 --- a/src/Testing/RuleTestCase.php +++ b/src/Testing/RuleTestCase.php @@ -327,6 +327,7 @@ private function gatherAnalyserErrorsWithDelayedErrors(array $files): array self::createScopeFactory($reflectionProvider, self::getContainer()->getService('typeSpecifier')), new LocalIgnoresProcessor(), true, + false, ); return [ diff --git a/src/dumpType.php b/src/dumpType.php index 5fe276ceaff..531dc3df327 100644 --- a/src/dumpType.php +++ b/src/dumpType.php @@ -40,3 +40,8 @@ function dumpPhpDocType($value, ...$values) // phpcs:ignore Squiz.Functions.Glob { return null; } + +function test(): string +{ + return 1; // @phpstan-ignore return.type +} diff --git a/tests/PHPStan/Analyser/AnalyserTest.php b/tests/PHPStan/Analyser/AnalyserTest.php index 5cd268dfea9..1069f6dda7a 100644 --- a/tests/PHPStan/Analyser/AnalyserTest.php +++ b/tests/PHPStan/Analyser/AnalyserTest.php @@ -638,6 +638,28 @@ public function testIgnoreNextLineUnmatched(): void } } + #[DataProvider('dataTrueAndFalse')] + public function testIgnoresWithoutCommentsReported(): void + { + $expects = [ + [9, 'variable.undefined'], + [12, 'variable.undefined'], + [12, 'wrong.id'], + [13, 'wrong.id'], + [14, 'variable.undefined'], + ]; + $result = $this->runAnalyser([], false, [ + __DIR__ . '/data/ignore-no-comments.php', + ], true, true); + $this->assertCount(5, $result); + foreach ($expects as $i => $expect) { + $this->assertArrayHasKey($i, $result); + $this->assertInstanceOf(Error::class, $result[$i]); + $this->assertStringContainsString(sprintf('Ignore with identifier %s has no comment.', $expect[1]), $result[$i]->getMessage()); + $this->assertSame($expect[0], $result[$i]->getLine()); + } + } + #[DataProvider('dataTrueAndFalse')] public function testIgnoreLine(bool $reportUnmatchedIgnoredErrors): void { @@ -747,9 +769,10 @@ private function runAnalyser( bool $reportUnmatchedIgnoredErrors, $filePaths, bool $onlyFiles, + bool $reportIgnoresWithoutComments = false, ): array { - $analyser = $this->createAnalyser(); + $analyser = $this->createAnalyser($reportIgnoresWithoutComments); if (is_string($filePaths)) { $filePaths = [$filePaths]; @@ -779,6 +802,7 @@ private function runAnalyser( ), new LocalIgnoresProcessor(), $reportUnmatchedIgnoredErrors, + $reportIgnoresWithoutComments, ); $analyserResult = $finalizer->finalize($analyserResult, $onlyFiles, false)->getAnalyserResult(); @@ -795,7 +819,7 @@ private function runAnalyser( ); } - private function createAnalyser(): Analyser + private function createAnalyser(bool $reportIgnoresWithoutComments = false): Analyser { $ruleRegistry = new DirectRuleRegistry([ new AlwaysFailRule(), @@ -850,6 +874,7 @@ private function createAnalyser(): Analyser new IgnoreErrorExtensionProvider(new NetteContainer(new Container([]))), $container->getByType(RuleErrorTransformer::class), new LocalIgnoresProcessor(), + $reportIgnoresWithoutComments, ); return new Analyser( diff --git a/tests/PHPStan/Analyser/data/ignore-no-comments.php b/tests/PHPStan/Analyser/data/ignore-no-comments.php new file mode 100644 index 00000000000..b79581a17ae --- /dev/null +++ b/tests/PHPStan/Analyser/data/ignore-no-comments.php @@ -0,0 +1,18 @@ + ['test'], + 2 => [['name' => 'test', 'comment' => null]], ], ]; @@ -58,7 +62,7 @@ public static function dataLinesToIgnore(): iterable ' ['return.ref'], + 2 => [['name' => 'return.ref', 'comment' => null]], ], ]; @@ -66,7 +70,7 @@ public static function dataLinesToIgnore(): iterable ' ['return.ref', 'return.non'], + 2 => [['name' => 'return.ref', 'comment' => null], ['name' => 'return.non', 'comment' => null]], ], ]; @@ -74,7 +78,7 @@ public static function dataLinesToIgnore(): iterable ' ['return.ref', 'return.non'], + 2 => [['name' => 'return.ref', 'comment' => null], ['name' => 'return.non', 'comment' => 'foo']], ], ]; @@ -82,7 +86,7 @@ public static function dataLinesToIgnore(): iterable ' ['return.ref', 'return.non'], + 2 => [['name' => 'return.ref', 'comment' => null], ['name' => 'return.non', 'comment' => 'foo, because...']], ], ]; @@ -90,7 +94,7 @@ public static function dataLinesToIgnore(): iterable ' ['test'], + 2 => [['name' => 'test', 'comment' => null]], ], ]; @@ -98,7 +102,7 @@ public static function dataLinesToIgnore(): iterable ' ['test'], + 2 => [['name' => 'test', 'comment' => null]], ], ]; @@ -106,7 +110,7 @@ public static function dataLinesToIgnore(): iterable ' ['test'], + 2 => [['name' => 'test', 'comment' => null]], ], ]; @@ -114,7 +118,7 @@ public static function dataLinesToIgnore(): iterable ' ['test'], + 2 => [['name' => 'test', 'comment' => null]], ], ]; @@ -123,7 +127,7 @@ public static function dataLinesToIgnore(): iterable '// @phpstan-ignore test' . PHP_EOL . 'test();', [ - 3 => ['test'], + 3 => [['name' => 'test', 'comment' => null]], ], ]; @@ -132,7 +136,7 @@ public static function dataLinesToIgnore(): iterable '// @phpstan-ignore test' . PHP_EOL . 'test(); // @phpstan-ignore test', [ - 3 => ['test', 'test'], + 3 => [['name' => 'test', 'comment' => null], ['name' => 'test', 'comment' => null]], ], ]; @@ -141,7 +145,7 @@ public static function dataLinesToIgnore(): iterable ' // @phpstan-ignore test' . PHP_EOL . 'test();', [ - 3 => ['test'], + 3 => [['name' => 'test', 'comment' => null]], ], ]; @@ -153,7 +157,7 @@ public static function dataLinesToIgnore(): iterable ' */' . PHP_EOL . 'test();', [ - 6 => ['test'], + 6 => [['name' => 'test', 'comment' => null]], ], ]; @@ -163,7 +167,7 @@ public static function dataLinesToIgnore(): iterable '/** @phpstan-ignore test */' . PHP_EOL . 'test();', [ - 4 => ['test'], + 4 => [['name' => 'test', 'comment' => null]], ], ]; @@ -174,7 +178,7 @@ public static function dataLinesToIgnore(): iterable ' * @phpstan-ignore test' . PHP_EOL . ' */ test();', [ - 5 => ['test'], + 5 => [['name' => 'test', 'comment' => null]], ], ]; @@ -185,7 +189,7 @@ public static function dataLinesToIgnore(): iterable ' * @phpstan-ignore test' . PHP_EOL . ' */', [ - 3 => ['test'], + 3 => [['name' => 'test', 'comment' => null]], ], ]; @@ -194,7 +198,7 @@ public static function dataLinesToIgnore(): iterable PHP_EOL . '/** @phpstan-ignore test */' . PHP_EOL, [ - 4 => ['test'], + 4 => [['name' => 'test', 'comment' => null]], ], ]; @@ -204,7 +208,7 @@ public static function dataLinesToIgnore(): iterable '/** @phpstan-ignore test */' . PHP_EOL . 'doFoo();' . PHP_EOL, [ - 4 => ['test'], + 4 => [['name' => 'test', 'comment' => null]], ], ]; @@ -213,7 +217,7 @@ public static function dataLinesToIgnore(): iterable PHP_EOL . '/** @phpstan-ignore test */', [ - 3 => ['test'], + 3 => [['name' => 'test', 'comment' => null]], ], ]; @@ -221,7 +225,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier', 'identifier2'], + 2 => [['name' => 'identifier', 'comment' => 'comment'], ['name' => 'identifier2', 'comment' => 'comment2']], ], ]; @@ -229,7 +233,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier', 'identifier2', 'identifier3'], + 2 => [['name' => 'identifier', 'comment' => 'comment1'], ['name' => 'identifier2', 'comment' => null], ['name' => 'identifier3', 'comment' => 'comment3']], ], ]; @@ -237,7 +241,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier'], + 2 => [['name' => 'identifier', 'comment' => 'comment with inner (parenthesis)']], ], ]; @@ -245,7 +249,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier'], + 2 => [['name' => 'identifier', 'comment' => '(((multi!)))']], ], ]; @@ -253,7 +257,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier'], + 2 => [['name' => 'identifier', 'comment' => 'var_export() is used intentionally']], ], ]; @@ -261,7 +265,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier'], + 2 => [['name' => 'identifier', 'comment' => 'FileSystem::write() does not support LOCK_EX']], ], ]; @@ -269,7 +273,7 @@ public static function dataLinesToIgnore(): iterable ' ['identifier'], + 2 => [['name' => 'identifier', 'comment' => 'type ensured in self::createClient()']], ], ]; @@ -287,7 +291,7 @@ public static function dataLinesToIgnore(): iterable ' }' . PHP_EOL . '}', [ - 10 => ['variable.undefined'], + 10 => [['name' => 'variable.undefined', 'comment' => null]], ], ]; @@ -308,13 +312,13 @@ public static function dataLinesToIgnore(): iterable ' }' . PHP_EOL . '}', [ - 13 => ['variable.undefined'], + 13 => [['name' => 'variable.undefined', 'comment' => null]], ], ]; } /** - * @param array|null> $expectedLines + * @param array|null> $expectedLines */ #[DataProvider('dataLinesToIgnore')] public function testLinesToIgnore(string $code, array $expectedLines): void