feat: grandfather current static analysis failures

[bishopb]

Description of the change

We want to begin using static analysis to improve the code base, but we don't want to go back through the 760+ current issues before doing so. This commit uses the Psalm baseline functionality to grandfather in the current failures and adds psalm static analysis to the test cycle. This will catch future issues in the code, while allowing for gradual remediation of historical issues.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[bxsx]

We want to begin using static analysis to improve the code base, but we don't want to go back through the 760+ current issues before doing so. This commit uses the Psalm baseline functionality to grandfather in the current failures and adds psalm static analysis to the test cycle. This will catch future issues in the code, while allowing for gradual remediation of historical issues.

I would expect to see the result of the report before approving the silent quality degradation.

[bishopb]

We want to begin using static analysis to improve the code base, but we don't want to go back through the 760+ current issues before doing so. This commit uses the Psalm baseline functionality to grandfather in the current failures and adds psalm static analysis to the test cycle. This will catch future issues in the code, while allowing for gradual remediation of historical issues.

I would expect to see the result of the report before approving the silent quality degradation.

@bxsx, do you mean you want to see the baseline as set in this PR, or that you want to see the baseline every time the CI runs?

For the first scenario, that information is available in the file psalm.baseline. I've also reproduced it here from a run without the baseline:

ERROR: AssignmentToVoid - src/Rollbar.php:110:9 - Cannot assign $result to type void (see https://psalm.dev/121)
        $result = self::$logger->log($level, $toLog, (array)$extra);


ERROR: ParamNameMismatch - src/RollbarLogger.php:92:33 - Argument 2 of Rollbar\RollbarLogger::log has wrong name $toLog, expecting $message as defined by Psr\Log\LoggerInterface::log (see https://psalm.dev/230)
    public function log($level, $toLog, array $context = array())


ERROR: UndefinedDocblockClass - src/Senders/FluentSender.php:17:13 - Docblock-defined class, interface or enum named Fluent\Logger\FluentLogger does not exist (see https://psalm.dev/200)
     * @var FluentLogger FluentLogger instance


ERROR: UndefinedDocblockClass - src/Senders/FluentSender.php:19:5 - Docblock-defined class, interface or enum named Fluent\Logger\FluentLogger does not exist (see https://psalm.dev/200)
    /**
     * @var FluentLogger FluentLogger instance
     */
    private $fluentLogger = null;


ERROR: UndefinedClass - src/Senders/FluentSender.php:29:27 - Class, interface or enum named Fluent\Logger\FluentLogger does not exist (see https://psalm.dev/019)
    private $fluentPort = FluentLogger::DEFAULT_LISTEN_PORT;


ERROR: UndefinedDocblockClass - src/Senders/FluentSender.php:80:20 - Docblock-defined class, interface or enum named Fluent\Logger\FluentLogger does not exist (see https://psalm.dev/200)
        $success = $this->fluentLogger->post($this->fluentTag, $scrubbedPayload);


ERROR: UndefinedClass - src/Senders/FluentSender.php:109:35 - Class, interface or enum named Fluent\Logger\FluentLogger does not exist (see https://psalm.dev/019)
        $this->fluentLogger = new FluentLogger($this->fluentHost, $this->fluentPort);

For the second scenario, Psalm does not seem to provide a way to show the baseline while also ignoring it. Consequently, to show this on each one we'd have to either:

1. Run psalm twice. Once ignoring baseline and the exit code, then again respecting the baseline and the exit code.
2. Write a psalm plugin to manipulate the Pslam engine into outputting the baseline errors it finds
3. Write a quick script to convert psalm.baseline XML into a more pleasant format.

Finally, I'll clarify there's no quality degradation: we're merely accepting the current state. Any code changes that introduce new violations will appear as errors, thus preventing us from worsening the code. However, it does make sense that unless someone comes around later and fixes these, they'll be "dark" forever.

[bxsx]

@bishopb, thanks for the detailed response!

It seems psalm does not grandfather all issues but errors, and the number of error occurrences is not that high. I expected to see 760+ items in the baseline file ;-)

Any code changes that introduce new violations will appear as errors, thus preventing us from worsening the code. However, it does make sense that unless someone comes around later and fixes these, they'll be "dark" forever.

The baseline file only stores occurrence counters without any fingerprints. It seems relatively easy to cheat the "system". Although this is a very rare case.

What worries me the most is the pitfall of the aforementioned darkness. I wonder if we could add additional reporting to the CI pipeline. Can we run --update-baseline or something similar in the CI pipeline to track eventual progression? This would be much more reliable than backlogging such errors.

[bxsx]

What worries me the most is the pitfall of the aforementioned darkness. I wonder if we could add additional reporting to the CI pipeline. Can we run --update-baseline or something similar in the CI pipeline to track eventual progression? This would be much more reliable than backlogging such errors.

Oh, ok. You already provided the answer to this too :)
So how about using --update-baseline instead of manipulating with the psalm engine?

[bishopb]

So how about using --update-baseline instead of manipulating with the psalm engine?

Yes, @bxsx. Here's what the additional output looks like:

------------------------------
3 errors fixed

In synthetic tests (n=100), running with --update-baseline required on average 412% more wall time (4.1s vs 0.8s) and 280% more CPU time (0.744s vs 0.196s).

Finally, to clarify, the baseline file is a committed artifact so --update-baseline doesn't automatically self-heal our repository: its display is simply an indicator of potential progress, not actual progress.

Personally, I'm uncertain of its value, so for now I'm merging this PR. We can easily add --update-baseline in a subsequent PR if you'd like.