Skip to content

Commit eb04e1d

Browse files
calebdwcalebdw
committed
feat: add option to report ignores without comments
1 parent d39453f commit eb04e1d

File tree

12 files changed

+144
-67
lines changed

12 files changed

+144
-67
lines changed

‎conf/config.neon‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ parameters:
114114
nodesByStringCountMax: 256
115115
reportUnmatchedIgnoredErrors: true
116116
reportIgnoresWithoutIdentifiers: false
117+
reportIgnoresWithoutComments: false
117118
typeAliases: []
118119
universalObjectCratesClasses:
119120
- stdClass
@@ -222,6 +223,7 @@ parameters:
222223
- [parameters, ignoreErrors]
223224
- [parameters, reportUnmatchedIgnoredErrors]
224225
- [parameters, reportIgnoresWithoutIdentifiers]
226+
- [parameters, reportIgnoresWithoutComments]
225227
- [parameters, tipsOfTheDay]
226228
- [parameters, parallel]
227229
- [parameters, internalErrorsCountLimit]

‎conf/parametersSchema.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ parametersSchema:
139139
])
140140
reportUnmatchedIgnoredErrors: bool()
141141
reportIgnoresWithoutIdentifiers: bool()
142+
reportIgnoresWithoutComments: bool()
142143
typeAliases: arrayOf(string())
143144
universalObjectCratesClasses: listOf(string())
144145
stubFiles: listOf(string())

‎src/Analyser/AnalyserResultFinalizer.php‎

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -30,19 +30,18 @@ public function __construct(
3030
private bool $reportUnmatchedIgnoredErrors,
3131
#[AutowiredParameter]
3232
private bool $reportIgnoresWithoutIdentifiers,
33+
#[AutowiredParameter]
34+
private bool $reportIgnoresWithoutComments,
3335
)
3436
{
3537
}
3638

3739
public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $debug): FinalizerResult
3840
{
39-
if (count($analyserResult->getCollectedData()) === 0) {
40-
return $this->addUnmatchedIgnoredErrors($this->addIgnoresWithoutIdentifiersErrors($this->mergeFilteredPhpErrors($analyserResult)), [], []);
41-
}
42-
41+
$hasCollectedData = count($analyserResult->getCollectedData()) > 0;
4342
$hasInternalErrors = count($analyserResult->getInternalErrors()) > 0 || $analyserResult->hasReachedInternalErrorsCountLimit();
44-
if ($hasInternalErrors) {
45-
return $this->addUnmatchedIgnoredErrors($this->addIgnoresWithoutIdentifiersErrors($this->mergeFilteredPhpErrors($analyserResult)), [], []);
43+
if (! $hasCollectedData || $hasInternalErrors) {
44+
return $this->addUnmatchedIgnoredErrors($this->addIgnoresWithoutCommentsOrIdentifiersErrors($this->mergeFilteredPhpErrors($analyserResult)), [], []);
4645
}
4746

4847
$nodeType = CollectedDataNode::class;
@@ -136,7 +135,7 @@ public function finalize(AnalyserResult $analyserResult, bool $onlyFiles, bool $
136135
$allUnmatchedLineIgnores[$file] = $localIgnoresProcessorResult->getUnmatchedLineIgnores();
137136
}
138137

139-
return $this->addUnmatchedIgnoredErrors($this->addIgnoresWithoutIdentifiersErrors(new AnalyserResult(
138+
return $this->addUnmatchedIgnoredErrors($this->addIgnoresWithoutCommentsOrIdentifiersErrors(new AnalyserResult(
140139
array_merge($errors, $analyserResult->getFilteredPhpErrors()),
141140
[],
142141
$analyserResult->getAllPhpErrors(),
@@ -207,7 +206,7 @@ private function addUnmatchedIgnoredErrors(
207206

208207
foreach ($identifiers as $identifier) {
209208
$errors[] = (new Error(
210-
sprintf('No error with identifier %s is reported on line %d.', $identifier, $line),
209+
sprintf('No error with identifier %s is reported on line %d.', $identifier['name'], $line),
211210
$file,
212211
$line,
213212
false,
@@ -239,9 +238,9 @@ private function addUnmatchedIgnoredErrors(
239238
);
240239
}
241240

242-
private function addIgnoresWithoutIdentifiersErrors(AnalyserResult $analyserResult): AnalyserResult
241+
private function addIgnoresWithoutCommentsOrIdentifiersErrors(AnalyserResult $analyserResult): AnalyserResult
243242
{
244-
if (!$this->reportIgnoresWithoutIdentifiers) {
243+
if (!$this->reportIgnoresWithoutComments && !$this->reportIgnoresWithoutIdentifiers) {
245244
return $analyserResult;
246245
}
247246

@@ -253,17 +252,34 @@ private function addIgnoresWithoutIdentifiersErrors(AnalyserResult $analyserResu
253252
}
254253

255254
foreach ($lines as $line => $identifiers) {
256-
if ($identifiers !== null) {
255+
if ($identifiers === null) {
256+
if (!$this->reportIgnoresWithoutIdentifiers) {
257+
continue;
258+
}
259+
$errors[] = (new Error(
260+
sprintf('Error is ignored with no identifiers on line %d.', $line),
261+
$file,
262+
$line,
263+
false,
264+
$file,
265+
))->withIdentifier('ignore.noIdentifier');
257266
continue;
258267
}
259268

260-
$errors[] = (new Error(
261-
sprintf('Error is ignored with no identifiers on line %d.', $line),
262-
$file,
263-
$line,
264-
false,
265-
$file,
266-
))->withIdentifier('ignore.noIdentifier');
269+
foreach ($identifiers as $identifier) {
270+
['name' => $name, 'comment' => $comment] = $identifier;
271+
if ($comment !== null || !$this->reportIgnoresWithoutComments) {
272+
continue;
273+
}
274+
275+
$errors[] = (new Error(
276+
sprintf('Ignore with identifier %s has no comment.', $name),
277+
$file,
278+
$line,
279+
false,
280+
$file,
281+
))->withIdentifier('ignore.noComment');
282+
}
267283
}
268284
}
269285
}

‎src/Analyser/FileAnalyser.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
use const E_WARNING;
4545

4646
/**
47+
* @phpstan-import-type Identifier from FileAnalyserResult
4748
* @phpstan-import-type CollectorData from CollectedData
4849
*/
4950
#[AutowiredService]
@@ -341,15 +342,15 @@ public function analyseFile(
341342

342343
/**
343344
* @param Node[] $nodes
344-
* @return array<int, non-empty-list<string>|null>
345+
* @return array<int, non-empty-list<Identifier>|null>
345346
*/
346347
private function getLinesToIgnoreFromTokens(array $nodes): array
347348
{
348349
if (!isset($nodes[0])) {
349350
return [];
350351
}
351352

352-
/** @var array<int, non-empty-list<string>|null> */
353+
/** @var array<int, non-empty-list<Identifier>|null> */
353354
return $nodes[0]->getAttribute('linesToIgnore', []);
354355
}
355356

‎src/Analyser/FileAnalyserResult.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
use PHPStan\Dependency\RootExportedNode;
77

88
/**
9-
* @phpstan-type LinesToIgnore = array<string, array<int, non-empty-list<string>|null>>
9+
* @phpstan-type Identifier = array{name: string, comment: string|null}
10+
* @phpstan-type LinesToIgnore = array<string, array<int, non-empty-list<Identifier>|null>>
1011
* @phpstan-import-type CollectorData from CollectedData
1112
*/
1213
final class FileAnalyserResult

‎src/Analyser/LocalIgnoresProcessor.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ public function process(
4949
}
5050

5151
foreach ($identifiers as $i => $ignoredIdentifier) {
52-
if ($ignoredIdentifier !== $tmpFileError->getIdentifier()) {
52+
if ($ignoredIdentifier['name'] !== $tmpFileError->getIdentifier()) {
5353
continue;
5454
}
5555

@@ -68,7 +68,7 @@ public function process(
6868
$unmatchedIgnoredIdentifiers = $unmatchedLineIgnores[$tmpFileError->getFile()][$line];
6969
if (is_array($unmatchedIgnoredIdentifiers)) {
7070
foreach ($unmatchedIgnoredIdentifiers as $j => $unmatchedIgnoredIdentifier) {
71-
if ($ignoredIdentifier !== $unmatchedIgnoredIdentifier) {
71+
if ($ignoredIdentifier['name'] !== $unmatchedIgnoredIdentifier['name']) {
7272
continue;
7373
}
7474

‎src/Command/FixerWorkerCommand.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ function (array $errors, array $locallyIgnoredErrors, array $analysedFiles) use
313313
if ($error->getIdentifier() === null) {
314314
continue;
315315
}
316-
if (!in_array($error->getIdentifier(), ['ignore.count', 'ignore.unmatched', 'ignore.unmatchedLine', 'ignore.unmatchedIdentifier', 'ignore.noIdentifier'], true)) {
316+
if (!in_array($error->getIdentifier(), ['ignore.count', 'ignore.unmatched', 'ignore.unmatchedLine', 'ignore.unmatchedIdentifier', 'ignore.noIdentifier', 'ignore.noComment'], true)) {
317317
continue;
318318
}
319319
$ignoreFileErrors[] = $error;

‎src/Parser/RichParser.php‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,16 @@
77
use PhpParser\NodeTraverser;
88
use PhpParser\NodeVisitor\NameResolver;
99
use PhpParser\Token;
10+
use PHPStan\Analyser\FileAnalyserResult;
1011
use PHPStan\Analyser\Ignore\IgnoreLexer;
1112
use PHPStan\Analyser\Ignore\IgnoreParseException;
1213
use PHPStan\DependencyInjection\Container;
1314
use PHPStan\File\FileReader;
1415
use PHPStan\ShouldNotHappenException;
1516
use function array_filter;
17+
use function array_key_last;
1618
use function array_map;
19+
use function array_values;
1720
use function count;
1821
use function implode;
1922
use function in_array;
@@ -30,6 +33,9 @@
3033
use const T_DOC_COMMENT;
3134
use const T_WHITESPACE;
3235

36+
/**
37+
* @phpstan-import-type Identifier from FileAnalyserResult
38+
*/
3339
final class RichParser implements Parser
3440
{
3541

@@ -120,7 +126,7 @@ public function parseString(string $sourceCode): array
120126

121127
/**
122128
* @param Token[] $tokens
123-
* @return array{lines: array<int, non-empty-list<string>|null>, errors: array<int, non-empty-list<string>>}
129+
* @return array{lines: array<int, non-empty-list<Identifier>|null>, errors: array<int, non-empty-list<string>>}
124130
*/
125131
private function getLinesToIgnore(array $tokens): array
126132
{
@@ -277,33 +283,29 @@ private function getLinesToIgnoreForTokenByIgnoreComment(
277283
}
278284

279285
/**
280-
* @return non-empty-list<string>
286+
* @return non-empty-list<Identifier>
281287
* @throws IgnoreParseException
282288
*/
283289
private function parseIdentifiers(string $text, int $ignorePos): array
284290
{
285291
$text = substr($text, $ignorePos + strlen('@phpstan-ignore'));
286-
$originalTokens = $this->ignoreLexer->tokenize($text);
287-
$tokens = [];
288-
289-
foreach ($originalTokens as $originalToken) {
290-
if ($originalToken[IgnoreLexer::TYPE_OFFSET] === IgnoreLexer::TOKEN_WHITESPACE) {
291-
continue;
292-
}
293-
$tokens[] = $originalToken;
294-
}
292+
$tokens = $this->ignoreLexer->tokenize($text);
295293

296294
$c = count($tokens);
297295

298296
$identifiers = [];
297+
$comment = null;
299298
$openParenthesisCount = 0;
300299
$expected = [IgnoreLexer::TOKEN_IDENTIFIER];
300+
$lastTokenTypeLabel = '@phpstan-ignore';
301301

302302
for ($i = 0; $i < $c; $i++) {
303-
$lastTokenTypeLabel = isset($tokenType) ? $this->ignoreLexer->getLabel($tokenType) : '@phpstan-ignore';
303+
if (isset($tokenType) && $tokenType !== IgnoreLexer::TOKEN_WHITESPACE) {
304+
$lastTokenTypeLabel = $this->ignoreLexer->getLabel($tokenType);
305+
}
304306
[IgnoreLexer::VALUE_OFFSET => $content, IgnoreLexer::TYPE_OFFSET => $tokenType, IgnoreLexer::LINE_OFFSET => $tokenLine] = $tokens[$i];
305307

306-
if ($expected !== null && !in_array($tokenType, $expected, true)) {
308+
if ($expected !== null && !in_array($tokenType, [...$expected, IgnoreLexer::TOKEN_WHITESPACE], true)) {
307309
$tokenTypeLabel = $this->ignoreLexer->getLabel($tokenType);
308310
$otherTokenContent = $tokenType === IgnoreLexer::TOKEN_OTHER ? sprintf(" '%s'", $content) : '';
309311
$expectedLabels = implode(' or ', array_map(fn ($token) => $this->ignoreLexer->getLabel($token), $expected));
@@ -312,6 +314,9 @@ private function parseIdentifiers(string $text, int $ignorePos): array
312314
}
313315

314316
if ($tokenType === IgnoreLexer::TOKEN_OPEN_PARENTHESIS) {
317+
if ($openParenthesisCount > 0) {
318+
$comment .= $content;
319+
}
315320
$openParenthesisCount++;
316321
$expected = null;
317322
continue;
@@ -320,17 +325,25 @@ private function parseIdentifiers(string $text, int $ignorePos): array
320325
if ($tokenType === IgnoreLexer::TOKEN_CLOSE_PARENTHESIS) {
321326
$openParenthesisCount--;
322327
if ($openParenthesisCount === 0) {
328+
$key = array_key_last($identifiers);
329+
if ($key !== null) {
330+
$identifiers[$key]['comment'] = $comment;
331+
$comment = null;
332+
}
323333
$expected = [IgnoreLexer::TOKEN_COMMA, IgnoreLexer::TOKEN_END];
334+
} else {
335+
$comment .= $content;
324336
}
325337
continue;
326338
}
327339

328340
if ($openParenthesisCount > 0) {
341+
$comment .= $content;
329342
continue; // waiting for comment end
330343
}
331344

332345
if ($tokenType === IgnoreLexer::TOKEN_IDENTIFIER) {
333-
$identifiers[] = $content;
346+
$identifiers[] = ['name' => $content, 'comment' => null];
334347
$expected = [IgnoreLexer::TOKEN_COMMA, IgnoreLexer::TOKEN_END, IgnoreLexer::TOKEN_OPEN_PARENTHESIS];
335348
continue;
336349
}
@@ -349,7 +362,7 @@ private function parseIdentifiers(string $text, int $ignorePos): array
349362
throw new IgnoreParseException('Missing identifier', 1);
350363
}
351364

352-
return $identifiers;
365+
return array_values($identifiers);
353366
}
354367

355368
}

‎src/Testing/RuleTestCase.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,7 @@ private function gatherAnalyserErrorsWithDelayedErrors(array $files): array
288288
new LocalIgnoresProcessor(),
289289
true,
290290
false,
291+
false,
291292
);
292293

293294
return [

‎tests/PHPStan/Analyser/AnalyserTest.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,28 @@ public function testIgnoresWithoutIdentifiersReported(): void
648648
}
649649
}
650650

651+
#[DataProvider('dataTrueAndFalse')]
652+
public function testIgnoresWithoutCommentsReported(): void
653+
{
654+
$expects = [
655+
[9, 'variable.undefined'],
656+
[12, 'variable.undefined'],
657+
[12, 'wrong.id'],
658+
[13, 'wrong.id'],
659+
[14, 'variable.undefined'],
660+
];
661+
$result = $this->runAnalyser([], false, [
662+
__DIR__ . '/data/ignore-no-comments.php',
663+
], true, false, true);
664+
$this->assertCount(5, $result);
665+
foreach ($expects as $i => $expect) {
666+
$this->assertArrayHasKey($i, $result);
667+
$this->assertInstanceOf(Error::class, $result[$i]);
668+
$this->assertStringContainsString(sprintf('Ignore with identifier %s has no comment.', $expect[1]), $result[$i]->getMessage());
669+
$this->assertSame($expect[0], $result[$i]->getLine());
670+
}
671+
}
672+
651673
#[DataProvider('dataTrueAndFalse')]
652674
public function testIgnoreLine(bool $reportUnmatchedIgnoredErrors): void
653675
{
@@ -758,6 +780,7 @@ private function runAnalyser(
758780
$filePaths,
759781
bool $onlyFiles,
760782
bool $reportIgnoresWithoutIdentifiers = false,
783+
bool $reportIgnoresWithoutComments = false,
761784
): array
762785
{
763786
$analyser = $this->createAnalyser();
@@ -791,6 +814,7 @@ private function runAnalyser(
791814
new LocalIgnoresProcessor(),
792815
$reportUnmatchedIgnoredErrors,
793816
$reportIgnoresWithoutIdentifiers,
817+
$reportIgnoresWithoutComments,
794818
);
795819
$analyserResult = $finalizer->finalize($analyserResult, $onlyFiles, false)->getAnalyserResult();
796820

0 commit comments

Comments
 (0)