From 2a5e0d8f395017529290dbe821966a0d440141fa Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Sun, 18 Aug 2019 14:27:50 -0400 Subject: [PATCH] Fix #1444 - track unused suppressions --- config.xsd | 1 + docs/running_psalm/issues.md | 9 +++ src/Psalm/CodeLocation/Raw.php | 3 +- src/Psalm/Codebase.php | 13 +++- .../Internal/Analyzer/CommentAnalyzer.php | 6 +- .../Analyzer/FunctionLikeAnalyzer.php | 6 ++ .../Internal/Analyzer/ProjectAnalyzer.php | 8 +++ .../Internal/Analyzer/StatementsAnalyzer.php | 9 ++- src/Psalm/Internal/Codebase/Analyzer.php | 12 +++- src/Psalm/Issue/UnusedPsalmSuppress.php | 6 ++ src/Psalm/IssueBuffer.php | 64 ++++++++++++++++++- src/command_functions.php | 3 + src/psalm.php | 7 +- tests/DocumentationTest.php | 1 + tests/IssueSuppressionTest.php | 49 ++++++++++++++ tests/TestCase.php | 4 ++ 16 files changed, 190 insertions(+), 11 deletions(-) create mode 100644 src/Psalm/Issue/UnusedPsalmSuppress.php diff --git a/config.xsd b/config.xsd index b1b159cbe..1ba5c0fb5 100644 --- a/config.xsd +++ b/config.xsd @@ -344,6 +344,7 @@ + diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md index e4da143e6..0b56bd474 100644 --- a/docs/running_psalm/issues.md +++ b/docs/running_psalm/issues.md @@ -2431,6 +2431,15 @@ $a = new A(); echo $a->getFoo(); ``` +### UnusedPsalmSuppress + +Emitted when `--find-unused-psalm-suppress` is turned on and Psalm cannot find any uses of a given `@psalm-suppress` annotation + +```php +/** @psalm-suppress InvalidArgument */ +echo strpos("hello", "e"); +``` + ### UnusedVariable Emitted when `--find-dead-code` is turned on and Psalm cannot find any references to a variable, once instantiated diff --git a/src/Psalm/CodeLocation/Raw.php b/src/Psalm/CodeLocation/Raw.php index 6b82d2227..bb69070e8 100644 --- a/src/Psalm/CodeLocation/Raw.php +++ b/src/Psalm/CodeLocation/Raw.php @@ -14,6 +14,7 @@ class Raw extends \Psalm\CodeLocation public function __construct( string $file_contents, string $file_path, + string $file_name, int $file_start, int $file_end ) { @@ -22,7 +23,7 @@ class Raw extends \Psalm\CodeLocation $this->raw_file_start = $this->file_start; $this->raw_file_end = $this->file_end; $this->file_path = $file_path; - $this->file_name = $file_path; + $this->file_name = $file_name; $this->single_line = false; $this->preview_start = $this->file_start; diff --git a/src/Psalm/Codebase.php b/src/Psalm/Codebase.php index afc52a2c6..623899f04 100644 --- a/src/Psalm/Codebase.php +++ b/src/Psalm/Codebase.php @@ -272,7 +272,10 @@ class Codebase */ public $php_minor_version = PHP_MINOR_VERSION; - + /** + * @var bool + */ + public $track_unused_suppressions = false; public function __construct( Config $config, @@ -1032,7 +1035,13 @@ class Codebase $file_contents = $this->getFileContents($file_path); - return new CodeLocation\Raw($file_contents, $file_path, (int) $symbol_parts[0], (int) $symbol_parts[1]); + return new CodeLocation\Raw( + $file_contents, + $file_path, + $this->config->shortenFileName($file_path), + (int) $symbol_parts[0], + (int) $symbol_parts[1] + ); } try { diff --git a/src/Psalm/Internal/Analyzer/CommentAnalyzer.php b/src/Psalm/Internal/Analyzer/CommentAnalyzer.php index 127d9508a..5de7c9497 100644 --- a/src/Psalm/Internal/Analyzer/CommentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/CommentAnalyzer.php @@ -518,7 +518,7 @@ class CommentAnalyzer if (isset($parsed_docblock['specials']['psalm-suppress'])) { foreach ($parsed_docblock['specials']['psalm-suppress'] as $offset => $suppress_entry) { - $info->suppressed_issues[$offset] = preg_split('/[\s]+/', $suppress_entry)[0]; + $info->suppressed_issues[$offset + $comment->getFilePos()] = preg_split('/[\s]+/', $suppress_entry)[0]; } } @@ -866,8 +866,8 @@ class CommentAnalyzer } if (isset($parsed_docblock['specials']['psalm-suppress'])) { - foreach ($parsed_docblock['specials']['psalm-suppress'] as $suppress_entry) { - $info->suppressed_issues[] = preg_split('/[\s]+/', $suppress_entry)[0]; + foreach ($parsed_docblock['specials']['psalm-suppress'] as $offset => $suppress_entry) { + $info->suppressed_issues[$offset + $comment->getFilePos()] = preg_split('/[\s]+/', $suppress_entry)[0]; } } diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index f6966be9e..6b67d9fc0 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -1016,6 +1016,12 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements } } + if ($codebase->track_unused_suppressions) { + foreach ($storage->suppressed_issues as $offset => $issue_name) { + IssueBuffer::addUnusedSuppression($this->getFilePath(), $offset, $issue_name); + } + } + foreach ($storage->throws as $expected_exception => $_) { if (isset($storage->throw_locations[$expected_exception])) { if (ClassLikeAnalyzer::checkFullyQualifiedClassLikeName( diff --git a/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php b/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php index 4caf9d239..c1a6da8f4 100644 --- a/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php @@ -558,6 +558,14 @@ class ProjectAnalyzer $this->codebase->taint = new Taint(); } + /** + * @return void + */ + public function trackUnusedSuppressions() + { + $this->codebase->track_unused_suppressions = true; + } + public function interpretRefactors() : void { if (!$this->codebase->alter_code) { diff --git a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php index 9f87901e5..c62eb7e64 100644 --- a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php @@ -297,8 +297,13 @@ class StatementsAnalyzer extends SourceAnalyzer implements StatementsSource ); if ($suppressed) { - $new_issues = array_diff($suppressed, $this->source->getSuppressedIssues()); - /** @psalm-suppress MixedTypeCoercion */ + $new_issues = []; + + foreach ($suppressed as $offset => $issue_type) { + $new_issues[$offset + $docblock->getFilePos()] = $issue_type; + IssueBuffer::addUnusedSuppression($this->getFilePath(), $offset, $issue_type); + } + $this->addSuppressedIssues($new_issues); } } diff --git a/src/Psalm/Internal/Codebase/Analyzer.php b/src/Psalm/Internal/Codebase/Analyzer.php index 91e6aa7f9..601f7a2d1 100644 --- a/src/Psalm/Internal/Codebase/Analyzer.php +++ b/src/Psalm/Internal/Codebase/Analyzer.php @@ -69,7 +69,8 @@ use function usort; * class_method_locations: array>, * class_property_locations: array>, * possible_method_param_types: array>, - * taint_data: ?\Psalm\Internal\Codebase\Taint + * taint_data: ?\Psalm\Internal\Codebase\Taint, + * unused_suppressions: array> * } */ @@ -408,6 +409,7 @@ class Analyzer 'class_property_locations' => $rerun ? [] : $file_reference_provider->getAllClassPropertyLocations(), 'possible_method_param_types' => $rerun ? [] : $analyzer->getPossibleMethodParamTypes(), 'taint_data' => $codebase->taint, + 'unused_suppressions' => $codebase->track_unused_suppressions ? IssueBuffer::getUnusedSuppressions() : [], ]; // @codingStandardsIgnoreEnd }, @@ -427,6 +429,10 @@ class Analyzer foreach ($forked_pool_data as $pool_data) { IssueBuffer::addIssues($pool_data['issues']); + if ($codebase->track_unused_suppressions) { + IssueBuffer::addUnusedSuppressions($pool_data['unused_suppressions']); + } + if ($codebase->taint && $pool_data['taint_data']) { $codebase->taint->addThreadData($pool_data['taint_data']); } @@ -531,6 +537,10 @@ class Analyzer $codebase->file_reference_provider->addIssue($issue_data['file_path'], $issue_data); } } + + if ($codebase->track_unused_suppressions) { + IssueBuffer::processUnusedSuppressions($codebase->file_provider); + } } /** diff --git a/src/Psalm/Issue/UnusedPsalmSuppress.php b/src/Psalm/Issue/UnusedPsalmSuppress.php new file mode 100644 index 000000000..453585450 --- /dev/null +++ b/src/Psalm/Issue/UnusedPsalmSuppress.php @@ -0,0 +1,6 @@ +> */ protected static $recorded_issues = []; + /** + * @var array> + */ + protected static $unused_suppressions = []; + /** * @param CodeIssue $e * @param string[] $suppressed_issues @@ -73,6 +79,15 @@ class IssueBuffer return self::add($e); } + public static function addUnusedSuppression(string $file_path, int $offset, string $issue_type) : void + { + if (!isset(self::$unused_suppressions[$file_path])) { + self::$unused_suppressions[$file_path] = []; + } + + self::$unused_suppressions[$file_path][$offset] = $offset + \strlen($issue_type) - 1; + } + /** * @param CodeIssue $e * @param string[] $suppressed_issues @@ -85,14 +100,17 @@ class IssueBuffer $fqcn_parts = explode('\\', get_class($e)); $issue_type = array_pop($fqcn_parts); + $file_path = $e->getFilePath(); - if (!$config->reportIssueInFile($issue_type, $e->getFilePath())) { + if (!$config->reportIssueInFile($issue_type, $file_path)) { return true; } $suppressed_issue_position = array_search($issue_type, $suppressed_issues); if ($suppressed_issue_position !== false) { + /** @psalm-suppress MixedArrayTypeCoercion */ + unset(self::$unused_suppressions[$file_path][$suppressed_issue_position]); return true; } @@ -102,6 +120,8 @@ class IssueBuffer $suppressed_issue_position = array_search($parent_issue_type, $suppressed_issues); if ($suppressed_issue_position !== false) { + /** @psalm-suppress MixedArrayTypeCoercion */ + unset(self::$unused_suppressions[$file_path][$suppressed_issue_position]); return true; } } @@ -190,6 +210,47 @@ class IssueBuffer return self::$issues_data; } + /** + * @return array> + */ + public static function getUnusedSuppressions() : array + { + return self::$unused_suppressions; + } + + public static function addUnusedSuppressions(array $unused_suppressions) : void + { + self::$unused_suppressions += $unused_suppressions; + } + + public static function processUnusedSuppressions(\Psalm\Internal\Provider\FileProvider $file_provider) : void + { + $config = Config::getInstance(); + + foreach (self::$unused_suppressions as $file_path => $offsets) { + if (!$offsets) { + continue; + } + + $file_contents = $file_provider->getContents($file_path); + + foreach ($offsets as $start => $end) { + self::add( + new UnusedPsalmSuppress( + 'This suppression is never used', + new CodeLocation\Raw( + $file_contents, + $file_path, + $config->shortenFileName($file_path), + $start, + $end + ) + ) + ); + } + } + } + /** * @return int */ @@ -492,6 +553,7 @@ class IssueBuffer self::$recording_level = 0; self::$recorded_issues = []; self::$console_issues = []; + self::$unused_suppressions = []; } /** diff --git a/src/command_functions.php b/src/command_functions.php index 19d37b266..b2a26ea06 100644 --- a/src/command_functions.php +++ b/src/command_functions.php @@ -321,6 +321,9 @@ Options: --find-unused-code[=auto] Look for unused code. Options are 'auto' or 'always'. If no value is specified, default is 'auto' + --find-unused-psalm-suppress + Finds all @psalm-suppress annotations that aren’t used + --find-references-to=[class|method|property] Searches the codebase for references to the given fully-qualified class or method, where method is in the format class::methodName diff --git a/src/psalm.php b/src/psalm.php index 6d358c1e2..25fdc22ac 100644 --- a/src/psalm.php +++ b/src/psalm.php @@ -62,7 +62,8 @@ $valid_long_options = [ 'shepherd::', 'no-progress', 'include-php-versions', // used for baseline - 'track-tainted-input' + 'track-tainted-input', + 'find-unused-psalm-suppress', ]; gc_collect_cycles(); @@ -511,6 +512,10 @@ if (isset($options['track-tainted-input'])) { $project_analyzer->trackTaintedInputs(); } +if (isset($options['find-unused-psalm-suppress'])) { + $project_analyzer->trackUnusedSuppressions(); +} + /** @var string $plugin_path */ foreach ($plugins as $plugin_path) { $config->addPluginPath($plugin_path); diff --git a/tests/DocumentationTest.php b/tests/DocumentationTest.php index 97ed054bb..21202030d 100644 --- a/tests/DocumentationTest.php +++ b/tests/DocumentationTest.php @@ -143,6 +143,7 @@ class DocumentationTest extends TestCase if ($check_references) { $this->project_analyzer->getCodebase()->reportUnusedCode(); + $this->project_analyzer->trackUnusedSuppressions(); } foreach ($error_levels as $error_level) { diff --git a/tests/IssueSuppressionTest.php b/tests/IssueSuppressionTest.php index 3475a100f..6eb170c3d 100644 --- a/tests/IssueSuppressionTest.php +++ b/tests/IssueSuppressionTest.php @@ -8,6 +8,55 @@ class IssueSuppressionTest extends TestCase use Traits\ValidCodeAnalysisTestTrait; use Traits\InvalidCodeAnalysisTestTrait; + /** + * @return void + */ + public function testIssueSuppressedOnFunction() + { + $this->expectException(\Psalm\Exception\CodeException::class); + $this->expectExceptionMessage('UnusedPsalmSuppress'); + + $this->project_analyzer->trackUnusedSuppressions(); + + $this->addFile( + 'somefile.php', + 'barBar()->bat()->baz()->bam()->bas()->bee()->bet()->bes()->bis(); + } + }' + ); + + $this->analyzeFile('somefile.php', new \Psalm\Context()); + } + + /** + * @return void + */ + public function testIssueSuppressedOnStatement() + { + $this->expectException(\Psalm\Exception\CodeException::class); + $this->expectExceptionMessage('UnusedPsalmSuppress'); + + $this->project_analyzer->trackUnusedSuppressions(); + + $this->addFile( + 'somefile.php', + 'analyzeFile('somefile.php', new \Psalm\Context()); + } + /** * @return iterable,error_levels?:string[]}> */ diff --git a/tests/TestCase.php b/tests/TestCase.php index 2b330527a..fba1cb020 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -119,6 +119,10 @@ class TestCase extends BaseTestCase $file_analyzer->analyze($context); } } + + if ($codebase->track_unused_suppressions) { + \Psalm\IssueBuffer::processUnusedSuppressions($codebase->file_provider); + } } /**