Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow suppressing UnusedPsalmSuppress, remove unused suppressions. #7133

Merged
merged 3 commits into from Dec 11, 2021

Conversation

AndrolGenhald
Copy link
Collaborator

No description provided.

@AndrolGenhald
Copy link
Collaborator Author

It looks like the CI workflow doesn't run psalm on itself on Windows 🙁
I was planning to have that tell me which suppressions need to be added back in. I guess I'll have to wait till I can get a Windows vm set up to run it.
If someone already has a Windows environment set up or if modifying the CI workflow to add that wouldn't be too hard I'd love a bit of help with that.

@AndrolGenhald AndrolGenhald marked this pull request as draft December 11, 2021 19:37
@orklah
Copy link
Collaborator

orklah commented Dec 11, 2021

Here's the report when running psalm on your branch on windows:

ERROR: UnnecessaryVarAnnotation - src/Psalm/Internal/Algebra/FormulaGenerator.php:152:38 - The @var string annotation for $var is unnecessary (see https://psalm.dev/212)
                            /** @var string */


ERROR: UnnecessaryVarAnnotation - src/Psalm/Internal/Algebra/FormulaGenerator.php:423:30 - The @var string annotation for $var is unnecessary (see https://psalm.dev/212)
                    /** @var string */


ERROR: UnusedPsalmSuppress - src/Psalm/Internal/Codebase/Reflection.php:428:33 - This suppression is never used (see https://psalm.dev/207)
            /** @psalm-suppress MixedArgument */


ERROR: UndefinedConstant - src/Psalm/Internal/Fork/Pool.php:370:56 - Const SIGTERM is not defined (see https://psalm.dev/020)
                                posix_kill($child_pid, SIGTERM);


ERROR: MixedArgument - src/Psalm/Internal/Fork/Pool.php:370:56 - Argument 2 of posix_kill cannot be mixed, expecting int (see https://psalm.dev/030)
                                posix_kill($child_pid, SIGTERM);


ERROR: UndefinedConstant - src/Psalm/Internal/Fork/Pool.php:420:44 - Const SIGALRM is not defined (see https://psalm.dev/020)
                    posix_kill($child_pid, SIGALRM);


ERROR: MixedArgument - src/Psalm/Internal/Fork/Pool.php:420:44 - Argument 2 of posix_kill cannot be mixed, expecting int (see https://psalm.dev/030)
                    posix_kill($child_pid, SIGALRM);


ERROR: UndefinedConstant - src/Psalm/Internal/Fork/Pool.php:432:39 - Const SIGALRM is not defined (see https://psalm.dev/020)
                    if ($term_sig !== SIGALRM) {


ERROR: ParamNameMismatch - src/Psalm/Internal/Fork/PsalmRestarter.php:36:40 - Argument 1 of Psalm\Internal\Fork\PsalmRestarter::requiresRestart has wrong name $default, expecting $isLoaded as defined by Composer\XdebugHandler\Xdebug
Handler::requiresRestart (see https://psalm.dev/230)
    protected function requiresRestart($default): bool


ERROR: RedundantConditionGivenDocblockType - src/Psalm/Type/Reconciler.php:1023:13 - Docblock-defined type string for $array_key_offset is never false (see https://psalm.dev/156)
        if (isset($existing_types[$base_key]) && $array_key_offset !== false) {


ERROR: RedundantConditionGivenDocblockType - src/Psalm/Type/Reconciler.php:1023:50 - Docblock-defined type string can never contain false (see https://psalm.dev/156)
        if (isset($existing_types[$base_key]) && $array_key_offset !== false) {


@AndrolGenhald
Copy link
Collaborator Author

ERROR: UnnecessaryVarAnnotation - src/Psalm/Internal/Algebra/FormulaGenerator.php:152:38 - The @var string annotation for $var is unnecessary (see https://psalm.dev/212)
                            /** @var string */

Well that's interesting, I'll have to look into why that didn't show up anywhere else.

@orklah
Copy link
Collaborator

orklah commented Dec 11, 2021

Yeah, pretty weird in fact. This is my personal machine I always use when working on Psalm and I never saw this error before, so it's likely something to do with your changes

@AndrolGenhald
Copy link
Collaborator Author

It looks like it's because psalm infers it to string|false, so this is a separate issue, and the Windows run is incorrect, or it's inferring it differently somehow.

@AndrolGenhald
Copy link
Collaborator Author

AndrolGenhald commented Dec 11, 2021

This one is also weird:

ERROR: UnusedPsalmSuppress - src/Psalm/Internal/Codebase/Reflection.php:428:33 - This suppression is never used (see https://psalm.dev/207)
            /** @psalm-suppress MixedArgument */

Do some of these show up when running on master as well?

Maybe it would be worthwhile to add a psalm run on Windows to the CI workflow.

@AndrolGenhald
Copy link
Collaborator Author

I added back the suppressions that seem related to this issue, I think the other issues from the Windows run are all preexisting.

@orklah
Copy link
Collaborator

orklah commented Dec 11, 2021

As I said, I run psalm on itself on windows very often.

I just ran it on master and there is no errors.

This is very likely something do do with your changes. That may have triggered a bug somewhere?

@orklah
Copy link
Collaborator

orklah commented Dec 11, 2021

Just ran it again with your changes

I only have a single error left:

ERROR: ParamNameMismatch - src/Psalm/Internal/Fork/PsalmRestarter.php:36:40 - Argument 1 of Psalm\Internal\Fork\PsalmRestarter::requiresRestart has wrong name $default, expecting $isLoaded as defined by Composer\XdebugHandler\Xdebug
Handler::requiresRestart (see https://psalm.dev/230)
    protected function requiresRestart($default): bool

I guess fixing/suppressing errors fixed the weird cases we saw

@AndrolGenhald
Copy link
Collaborator Author

That's very odd. Do you normally clear the psalm cache before running it? That's caught me a few times, although I'm not sure how that could cause these issues I think the cache has some bugs hidden in it, but I've never taken the time to put together a simple reproducer.
A composer update should fix the last issue: https://github.com/composer/xdebug-handler/releases/tag/2.0.0

@AndrolGenhald AndrolGenhald marked this pull request as ready for review December 11, 2021 21:53
@orklah
Copy link
Collaborator

orklah commented Dec 11, 2021

I always run psalm with --no-cache. Changing psalm itself leads to weird bug with cache indeed. Also, I just cloned the repo so I don't think cache could have been reused from my main Psalm repo.

Nice catch about composer. I guess our lock file is out of date...

@@ -186,8 +187,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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind explaining what this condition does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's there so /** @psalm-suppress UnusedPsalmSuppress */ still shows up as an unused suppression. I could have done array_values($storage->suppressed_issues) === ["UnusedPsalmSuppress"] but I figured this is both faster, shorter, and allows other cases to early out as well. Should I add a comment there so it's a bit more obvious?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Yes please, it wasn't immediately obvious to me and I had the context :p

But yeah, I see what this does now, thanks!

@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label Dec 11, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 11, 2021

Thanks!

@orklah orklah merged commit f79f857 into vimeo:master Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants