Merge pull request #7133 from AndrolGenhald/remove-unused-suppressed

Allow suppressing UnusedPsalmSuppress, remove unused suppressions.
vimeo · Dec 11, 2021 · f79f857 · f79f857
2 parents b663841 + 225af97
commit f79f857
Show file tree

Hide file tree

Showing 24 changed files with 62 additions and 67 deletions.
diff --git a/psalm.xml.dist b/psalm.xml.dist
@@ -16,6 +16,7 @@
     xsi:schemaLocation="https://getpsalm.org/schema/config config.xsd"
     limitMethodComplexity="true"
     errorBaseline="psalm-baseline.xml"
+    findUnusedPsalmSuppress="true"
 >
     <stubs>
         <file name="stubs/phpparser.phpstub"/>

diff --git a/src/Psalm/CodeLocation.php b/src/Psalm/CodeLocation.php
@@ -137,9 +137,6 @@ public function setCommentLine(int $line): void
         $this->docblock_line_number = $line;
     }
 
-    /**
-     * @psalm-suppress MixedArrayAccess
-     */
     private function calculateRealLocation(): void
     {
         if ($this->have_recalculated) {

diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php
@@ -850,7 +850,6 @@ private static function processConfigDeprecations(
     /**
      * @psalm-suppress MixedMethodCall
      * @psalm-suppress MixedAssignment
-     * @psalm-suppress MixedOperand
      * @psalm-suppress MixedArgument
      * @psalm-suppress MixedPropertyFetch
      *
@@ -1294,8 +1293,6 @@ public function getPluginClasses(): array
 
     /**
      * Initialises all the plugins (done once the config is fully loaded)
-     *
-     * @psalm-suppress MixedAssignment
      */
     public function initializePlugins(ProjectAnalyzer $project_analyzer): void
     {
@@ -2065,10 +2062,6 @@ public function setIncludeCollector(IncludeCollector $include_collector): void
         $this->include_collector = $include_collector;
     }
 
-    /**
-     * @psalm-suppress MixedAssignment
-     * @psalm-suppress MixedArrayAccess
-     */
     public function visitComposerAutoloadFiles(ProjectAnalyzer $project_analyzer, ?Progress $progress = null): void
     {
         if ($progress === null) {

diff --git a/src/Psalm/Config/Creator.php b/src/Psalm/Config/Creator.php
@@ -208,7 +208,6 @@ public static function getPaths(string $current_dir, ?string $suggested_dir): ar
      * @return list<string>
      * @psalm-suppress MixedAssignment
      * @psalm-suppress MixedArgument
-     * @psalm-suppress PossiblyUndefinedArrayOffset
      */
     private static function getPsr4Or0Paths(string $current_dir, array $composer_json): array
     {

diff --git a/src/Psalm/FileBasedPluginAdapter.php b/src/Psalm/FileBasedPluginAdapter.php
@@ -36,9 +36,6 @@ public function __construct(string $path, Config $config, Codebase $codebase)
         $this->codebase = $codebase;
     }
 
-    /**
-     * @psalm-suppress PossiblyUnusedParam
-     */
     public function __invoke(RegistrationInterface $registration, ?SimpleXMLElement $config = null): void
     {
         $fq_class_name = $this->getPluginClassForPath($this->path);

diff --git a/src/Psalm/Internal/Analyzer/FunctionLike/ReturnTypeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLike/ReturnTypeAnalyzer.php
@@ -67,7 +67,6 @@ class ReturnTypeAnalyzer
      * @return  false|null
      *
      * @psalm-suppress PossiblyUnusedReturnValue unused but seems important
-     * @psalm-suppress ComplexMethod to be refactored
      */
     public static function verifyReturnType(
         FunctionLike $function,

diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
@@ -55,6 +55,7 @@
 use function array_search;
 use function count;
 use function end;
+use function in_array;
 use function is_string;
 use function md5;
 use function microtime;
@@ -185,8 +186,12 @@ public function analyze(
         $project_analyzer = $this->getProjectAnalyzer();
 
         if ($codebase->track_unused_suppressions && !isset($storage->suppressed_issues[0])) {
-            foreach ($storage->suppressed_issues as $offset => $issue_name) {
-                IssueBuffer::addUnusedSuppression($this->getFilePath(), $offset, $issue_name);
+            if (count($storage->suppressed_issues) === 1 // UnusedPsalmSuppress by itself should be marked as unused
+                || !in_array("UnusedPsalmSuppress", $storage->suppressed_issues)
+            ) {
+                foreach ($storage->suppressed_issues as $offset => $issue_name) {
+                    IssueBuffer::addUnusedSuppression($this->getFilePath(), $offset, $issue_name);
+                }
             }
         }
 

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/SimpleTypeInferer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/SimpleTypeInferer.php
@@ -683,7 +683,6 @@ private static function handleUnpackedArray(
                     $array_creation_info->property_types[$new_offset] = $property_value;
                 }
             } elseif ($unpacked_atomic_type instanceof Type\Atomic\TArray) {
-                /** @psalm-suppress PossiblyUndefinedArrayOffset provably true, but Psalm can’t see it */
                 if ($unpacked_atomic_type->type_params[1]->isEmpty()) {
                     continue;
                 }

diff --git a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php
@@ -57,8 +57,10 @@
 use function array_keys;
 use function array_merge;
 use function array_search;
+use function count;
 use function fwrite;
 use function get_class;
+use function in_array;
 use function is_string;
 use function preg_split;
 use function reset;
@@ -420,18 +422,25 @@ private static function analyzeStatement(
                     foreach ($suppressed as $offset => $suppress_entry) {
                         foreach (DocComment::parseSuppressList($suppress_entry) as $issue_offset => $issue_type) {
                             $new_issues[$issue_offset + $offset] = $issue_type;
+                        }
+                    }
 
+                    if ($codebase->track_unused_suppressions
+                        && (
+                            (count($new_issues) === 1) // UnusedPsalmSuppress by itself should be marked as unused
+                            || !in_array("UnusedPsalmSuppress", $new_issues)
+                        )
+                    ) {
+                        foreach ($new_issues as $offset => $issue_type) {
                             if ($issue_type === 'InaccessibleMethod') {
                                 continue;
                             }
 
-                            if ($codebase->track_unused_suppressions) {
-                                IssueBuffer::addUnusedSuppression(
-                                    $statements_analyzer->getFilePath(),
-                                    $issue_offset + $offset,
-                                    $issue_type
-                                );
-                            }
+                            IssueBuffer::addUnusedSuppression(
+                                $statements_analyzer->getFilePath(),
+                                $offset,
+                                $issue_type
+                            );
                         }
                     }
 

diff --git a/src/Psalm/Internal/CliUtils.php b/src/Psalm/Internal/CliUtils.php
@@ -58,7 +58,6 @@ public static function requireAutoloaders(
 
         $psalm_dir = dirname(__DIR__, 3);
 
-        /** @psalm-suppress UndefinedConstant */
         $in_phar = Phar::running() || strpos(__NAMESPACE__, 'HumbugBox');
 
         if ($in_phar) {
@@ -154,9 +153,7 @@ public static function requireAutoloaders(
     }
 
     /**
-     * @psalm-suppress MixedArrayAccess
      * @psalm-suppress MixedAssignment
-     * @psalm-suppress PossiblyUndefinedStringArrayOffset
      */
     public static function getVendorDir(string $current_dir): string
     {

diff --git a/src/Psalm/Internal/Codebase/InternalCallMapHandler.php b/src/Psalm/Internal/Codebase/InternalCallMapHandler.php
@@ -338,9 +338,6 @@ public static function getCallablesFromCallMap(string $function_id): ?array
      * Gets the method/function call map
      *
      * @return array<string, array<int|string, string>>
-     * @psalm-suppress MixedInferredReturnType as the use of require buggers things up
-     * @psalm-suppress MixedReturnStatement
-     * @psalm-suppress MixedReturnTypeCoercion
      */
     public static function getCallMap(): array
     {
@@ -397,7 +394,6 @@ public static function getCallMap(): array
                  *     }>,
                  *     removed: array<string, array<int|string, string>>
                  * }
-                 * @psalm-suppress UnresolvableInclude
                  */
                 $diff_call_map = require($delta_file);
 

diff --git a/src/Psalm/Internal/Codebase/Reflection.php b/src/Psalm/Internal/Codebase/Reflection.php
@@ -422,9 +422,6 @@ public static function getPsalmTypeFromReflectionType(?ReflectionType $reflectio
             return Type::getMixed();
         }
 
-        /**
-         * @psalm-suppress UndefinedClass,TypeDoesNotContainType
-         */
         if ($reflection_type instanceof ReflectionNamedType) {
             $type = $reflection_type->getName();
         } elseif ($reflection_type instanceof ReflectionUnionType) {

diff --git a/src/Psalm/Internal/Fork/Pool.php b/src/Psalm/Internal/Fork/Pool.php
@@ -368,7 +368,9 @@ private function readResultsFromChildren(): array
                             // Kill all children
                             foreach ($this->child_pid_list as $child_pid) {
                                 /**
-                                 * @psalm-suppress UndefinedConstant - does not exist on windows
+                                 * SIGTERM does not exist on windows
+                                 * @psalm-suppress UnusedPsalmSuppress
+                                 * @psalm-suppress UndefinedConstant
                                  * @psalm-suppress MixedArgument
                                  */
                                 posix_kill($child_pid, SIGTERM);
@@ -422,7 +424,9 @@ public function wait(): array
 
                 if ($process_lookup) {
                     /**
-                     * @psalm-suppress UndefinedConstant - does not exist on windows
+                     * SIGALRM does not exist on windows
+                     * @psalm-suppress UnusedPsalmSuppress
+                     * @psalm-suppress UndefinedConstant
                      * @psalm-suppress MixedArgument
                      */
                     posix_kill($child_pid, SIGALRM);
@@ -438,7 +442,9 @@ public function wait(): array
                     $term_sig = pcntl_wtermsig($status);
 
                     /**
-                     * @psalm-suppress UndefinedConstant - does not exist on windows
+                     * SIGALRM does not exist on windows
+                     * @psalm-suppress UnusedPsalmSuppress
+                     * @psalm-suppress UndefinedConstant
                      */
                     if ($term_sig !== SIGALRM) {
                         $this->did_have_error = true;

diff --git a/src/Psalm/Internal/LanguageServer/LanguageServer.php b/src/Psalm/Internal/LanguageServer/LanguageServer.php
@@ -140,7 +140,6 @@ function (Message $msg): Generator {
                         // Invoke the method handler to get a result
                         /**
                          * @var Promise
-                         * @psalm-suppress UndefinedDocblockClass
                          */
                         $dispatched = $this->dispatch($msg->body);
                         /** @psalm-suppress MixedAssignment */

diff --git a/src/Psalm/Internal/LanguageServer/ProtocolStreamReader.php b/src/Psalm/Internal/LanguageServer/ProtocolStreamReader.php
@@ -51,9 +51,6 @@ public function __construct($input)
         asyncCall(
             /**
              * @return Generator<int, Promise<?string>, ?string, void>
-             * @psalm-suppress MixedReturnTypeCoercion
-             * @psalm-suppress MixedArgument in old Amp versions
-             * @psalm-suppress MixedAssignment in old Amp versions
              */
             function () use ($input): Generator {
                 while ($this->is_accepting_new_requests) {
@@ -119,7 +116,6 @@ private function readMessages(string $buffer): int
                             $this->emit('message', [$msg]);
                             /**
                              * @psalm-suppress DocblockTypeContradiction
-                             * @psalm-suppress RedundantConditionGivenDocblockType
                              */
                             if (!$this->is_accepting_new_requests) {
                                 // If we fork, don't read any bytes in the input buffer from the worker process.

diff --git a/src/Psalm/Internal/PhpVisitor/CheckTrivialExprVisitor.php b/src/Psalm/Internal/PhpVisitor/CheckTrivialExprVisitor.php
@@ -15,10 +15,6 @@ class CheckTrivialExprVisitor extends PhpParser\NodeVisitorAbstract
 
     private function checkNonTrivialExpr(PhpParser\Node\Expr $node): bool
     {
-        /**
-         * @psalm-suppress UndefinedClass
-         * @psalm-suppress TypeDoesNotContainType
-         */
         if ($node instanceof PhpParser\Node\Expr\ArrayDimFetch
             || $node instanceof PhpParser\Node\Expr\Closure
             || $node instanceof PhpParser\Node\Expr\ClosureUse

diff --git a/src/Psalm/Internal/PhpVisitor/OffsetShifterVisitor.php b/src/Psalm/Internal/PhpVisitor/OffsetShifterVisitor.php
@@ -54,9 +54,6 @@ public function enterNode(PhpParser\Node $node): ?int
             $node->setAttribute('comments', $new_comments);
         }
 
-        /**
-         * @psalm-suppress MixedOperand
-         */
         $node->setAttribute(
             'startFilePos',
             $attrs['startFilePos'] + $this->file_offset + ($this->extra_offsets[$attrs['startFilePos']] ?? 0)

diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeDocblockParser.php b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeDocblockParser.php
@@ -46,8 +46,6 @@ class ClassLikeDocblockParser
 {
     /**
      * @throws DocblockParseException if there was a problem parsing the docblock
-     *
-     * @psalm-suppress MixedArrayAccess
      */
     public static function parse(
         Node $node,

diff --git a/src/Psalm/Internal/Provider/FakeFileProvider.php b/src/Psalm/Internal/Provider/FakeFileProvider.php
@@ -49,9 +49,6 @@ public function getModifiedTime(string $file_path): int
         return $this->fake_file_times[$file_path] ?? parent::getModifiedTime($file_path);
     }
 
-    /**
-     * @psalm-suppress InvalidPropertyAssignmentValue because microtime is needed for cache busting
-     */
     public function registerFile(string $file_path, string $file_contents): void
     {
         $this->fake_files[$file_path] = $file_contents;

diff --git a/src/Psalm/Internal/Provider/ParserCacheProvider.php b/src/Psalm/Internal/Provider/ParserCacheProvider.php
@@ -71,8 +71,6 @@ public function __construct(Config $config, bool $use_file_cache = true)
 
     /**
      * @return list<PhpParser\Node\Stmt>|null
-     *
-     * @psalm-suppress UndefinedFunction
      */
     public function loadStatementsFromCache(
         string $file_path,
@@ -116,8 +114,6 @@ public function loadStatementsFromCache(
 
     /**
      * @return list<PhpParser\Node\Stmt>|null
-     *
-     * @psalm-suppress UndefinedFunction
      */
     public function loadExistingStatementsFromCache(string $file_path): ?array
     {
@@ -217,9 +213,6 @@ private function getExistingFileContentHashes(): array
 
     /**
      * @param  list<PhpParser\Node\Stmt>        $stmts
-     *
-     *
-     * @psalm-suppress UndefinedFunction
      */
     public function saveStatementsToCache(
         string $file_path,

diff --git a/src/Psalm/Internal/Provider/StatementsProvider.php b/src/Psalm/Internal/Provider/StatementsProvider.php
@@ -150,7 +150,6 @@ public function getStatementsForFile(string $file_path, string $php_version, ?Pr
 
             $existing_statements = $this->parser_cache_provider->loadExistingStatementsFromCache($file_path);
 
-            /** @psalm-suppress DocblockTypeContradiction */
             if ($existing_statements && !$existing_statements[0] instanceof PhpParser\Node\Stmt) {
                 $existing_statements = null;
             }

diff --git a/src/Psalm/Internal/Type/TypeTokenizer.php b/src/Psalm/Internal/Type/TypeTokenizer.php
@@ -97,7 +97,6 @@ class TypeTokenizer
      *
      * @return list<array{string, int}>
      *
-     * @psalm-suppress ComplexMethod
      * @psalm-suppress PossiblyUndefinedIntArrayOffset
      */
     public static function tokenize(string $string_type, bool $ignore_space = true): array

diff --git a/tests/IssueSuppressionTest.php b/tests/IssueSuppressionTest.php
@@ -371,6 +371,24 @@ class Foo {
                     }
                 ',
             ],
+            'suppressUnusedSuppression' => [
+                '<?php
+                    class Foo {
+                        /**
+                         * @psalm-suppress UnusedPsalmSuppress, MissingPropertyType
+                         */
+                        public string $bar = "baz";
+
+                        /**
+                         * @psalm-suppress UnusedPsalmSuppress, MissingReturnType
+                         */
+                        public function foobar(): string
+                        {
+                            return "foobar";
+                        }
+                    }
+                ',
+            ],
         ];
     }
 
@@ -408,6 +426,17 @@ public function b() {
                     function foo($s = Foo::BAR) : void {}',
                 'error_message' => 'UndefinedClass',
             ],
+            'suppressUnusedSuppressionByItselfIsNotSuppressed' => [
+                '<?php
+                    class Foo {
+                        /**
+                         * @psalm-suppress UnusedPsalmSuppress
+                         */
+                        public string $bar = "baz";
+                    }
+                ',
+                'error_message' => 'UnusedPsalmSuppress',
+            ],
         ];
     }
 }
diff --git a/tests/TestConfig.php b/tests/TestConfig.php
@@ -15,9 +15,6 @@ class TestConfig extends Config
     /** @var ProjectFileFilter|null */
     private static $cached_project_files = null;
 
-    /**
-     * @psalm-suppress PossiblyNullPropertyAssignmentValue because cache_directory isn't strictly nullable
-     */
     public function __construct()
     {
         parent::__construct();