-
Notifications
You must be signed in to change notification settings - Fork 545
array_key_exist inference is lost when adding a false condition
#4473
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
Conversation
fb48fb4 to
d93abb5
Compare
|
This pull request has been marked as ready for review. |
4b2b7a5 to
821db45
Compare
|
So no issue with this PR ? Thanks for finding it ! |
|
I think it works (but did not re-use your previous implementation) |
6b9f420 to
0844b34
Compare
0844b34 to
d02bcc3
Compare
src/Analyser/TypeSpecifier.php
Outdated
|
|
||
| if ($context->true()) { | ||
| if ( | ||
| $scope->getType($expr->left)->isFalse()->yes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My head hurts 😅
I think all the isTrue/isFalse calls should do toBoolean() beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch. fixed.
thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm maybe we should have a $type->isFalse()->yes()/$type->isTrue()->yes() to $type->toBoolean()->isFalse()->yes()/$type->toBoolean()->isTrue()->yes() mutator.
it would make us aware of false/falsey coverage
.github/workflows/tests.yml
Outdated
| run: | | ||
| php build-infection/bin/infection-config.php \ | ||
| --source-directory='build/PHPStan/Build' \ | ||
| --timeout=180 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened a new ticket, as we need more information to proceed.
infection/infection#2510
even after raising the timeout to 300 it still times out.
so at phpstan-src repo we are so slow, that we can't even check 3 mutants in 15minutes CI runtime.
we can merge for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets fix this in a separate PR
|
Make a build-infection issue for the new mutator idea 😊 |
|
Thank you! |

closes phpstan/phpstan#11276
fixes phpstan/phpstan-phpunit#100