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

fix error with 0 being considered positive #7487

Merged
merged 1 commit into from Jan 25, 2022
Merged

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Jan 25, 2022

fix a bug introduced in #7473. When combining ints, we were flagging 0, using TPositiveInt and then check if TPositiveInt to add 0 to the combination. When I removed TPositiveInt, I wrongly assumed this had become unused code

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Jan 25, 2022
Comment on lines +707 to +718
'noErrorPushingBigShapeIntoConstant' => [
'code' => '<?php
class DocComment
{
private const PSALM_ANNOTATIONS = [
"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", ""
];
}',
'assertions' => [
],
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this test actually fail before the fix? I tried the same thing with the actual values from DocComment but for some reason it passed fine.

Copy link
Collaborator Author

@orklah orklah Jan 25, 2022

Choose a reason for hiding this comment

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

it does: https://psalm.dev/r/1529295950.

EDIT: see result from Psalm's bot
EDIT2: also see the failing CI on master. I retrieved the error from there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although I noticed that it started failing after 52 values in there. DocComment must have around 55, so maybe you didn't include them all?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@orklah Oh I see what happened, I used --filter for my test, but I forgot paratest doesn't support that, you have to use phpunit directly. I saw the success message and didn't notice it said 0 tests...

@orklah
Copy link
Collaborator Author

orklah commented Jan 25, 2022

Psalm bot, please fetch this for posterity: https://psalm.dev/r/1529295950

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/1529295950
<?php
class DocComment
                            {
                                private const PSALM_ANNOTATIONS = [
                                    "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
                                    "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", ""
                                ];
                            }
Psalm output (using commit 7c8441b):

ERROR: InvalidConstantAssignmentValue - 4:47 - DocComment::PSALM_ANNOTATIONS with declared type non-empty-array<int<1, max>, ''> cannot be assigned type array{'', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', ''}

@orklah orklah merged commit 1cc9d1c into vimeo:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants