-
Notifications
You must be signed in to change notification settings - Fork 515
Fix missing detection of dead code in expressions #4090
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
hmm I wonder whether I should add this
etc...? edit: I added it in all places where I could think of a example which I was able to test |
I think the new behaviour makes sense but the errors are unfortunate
|
This pull request has been marked as ready for review. |
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 is StatementResultTest that tests "is always terminating". Please add a similar test for ExpressionResult. Thank you.
I think this should be ready to land. thanks for the feedback |
src/Analyser/NodeScopeResolver.php
Outdated
@@ -3289,7 +3289,7 @@ static function (): void { | |||
return new ExpressionResult( | |||
$leftMergedWithRightScope, | |||
$leftResult->hasYield() || $rightResult->hasYield(), | |||
false, | |||
$leftResult->isAlwaysTerminating() || $rightResult->isAlwaysTerminating(), |
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.
The right side might never be executed so this should only mention the left side.
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.
$x && exit();
should be false
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.
it is false
, see https://github.com/phpstan/phpstan-src/pull/4090/files#diff-fa85cdb8d6b73f343f042bbbe433ecc95119bf3dc5e76cc7291563d1c49cbd0dR87
I guess you was looking at an outdated version of the PR while commenting
I swear GitHub has some kind of caching problem with the new "Files changed" UI. Sorry. |
Thank you! |
closes phpstan/phpstan#13232
closes phpstan/phpstan#11909