Class-level @psalm-suppress on a test case with setUp() is reported as unused

Open
  • Is something wrong?
#214 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
1-2 days
Newbie friendliness
55/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
php
Domain
testing

Research direction

Start by reading TestCaseHandler in the plugin and Psalm’s ClassLikeAnalyzer::registerDocblockSuppressions() and FunctionLikeAnalyzer suppression loops; the issue explains how integer array keys are mistaken for docblock offsets. Check how StatementsSource::addSuppressedIssues() stores plugin suppressions, then update the two TestCaseHandler entries to use string keys and ensure Psalm skips non-integer keys in both analyzer loops. Done when the reported UnusedPsalmSuppress no longer appears for the described test case and unused docblock suppressions still work.

Written by the indexing model from the issue text.

Description

bug

With findUnusedPsalmSuppress="true", a test case that has setUp() (or a #[Before] method) and a class-level @psalm-suppress gets a bogus UnusedPsalmSuppress (with --find-dead-code when the suppressed issue is a dead-code one):

/**
 * @psalm-suppress UnusedFunctionCall
 */
final class MyTestCase extends TestCase {
    protected function setUp(): void {}

    public function testSomething(): void {
        strlen('a');
    }
}

The user's suppression is counted as used. The reported one is the plugin's own: TestCaseHandler appends MissingConstructor to the class storage's suppressed_issues with []=, which gives it key max(offset) + 1. ClassLikeAnalyzer::registerDocblockSuppressions() registers every key as a docblock offset, so the entry is reported unused whenever no MissingConstructor is emitted. When the class has no other suppressions the key is 0, and Psalm then skips unused-suppression tracking for the whole class.

There is no plugin-only fix: a string key (['MissingConstructor'] = 'MissingConstructor', as StatementsSource::addSuppressedIssues() uses) crashes registerDocblockSuppressions(), because IssueBuffer::addUnusedSuppression() takes an int offset. Psalm needs to skip non-int keys there (and in the same loop in FunctionLikeAnalyzer):

+        $docblock_suppressions = array_filter($storage->suppressed_issues, is_int(...), ARRAY_FILTER_USE_KEY);
-        if (count($storage->suppressed_issues) > 1
+        if (count($docblock_suppressions) > 1
...
-        foreach ($storage->suppressed_issues as $offset => $issue_type) {
+        foreach ($docblock_suppressions as $offset => $issue_type) {

With that, the plugin change is to use the string key in both places in TestCaseHandler.

Dominant language
Gherkin
Stars
79
Forks
38
Avg merge
6h 52m
Merged PRs (30d)
28

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from psalm/psalm-plugin-phpunit

All issues in psalm/psalm-plugin-phpunit

Similar issues

More Testing & QA issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.