Fix bug 8937 with match and treatPhpDocTypesAsCertain #2250
+57
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VincentLanglet
force-pushed
the
bug8937
branch
from
e300fd9 to
ca18fac
Compare
VincentLanglet
commented
Feb 22, 2023
| @@ -65,13 +66,23 @@ public function processNode(Node $node, Scope $scope): array | |||
| $matchCondition, | |||
| $armCondition->getCondition(), | |||
| ); | |||
| $armConditionResult = ($this->treatPhpDocTypesAsCertain ? $armConditionScope->getType($armConditionExpr) : $armConditionScope->getNativeType($armConditionExpr)); | |||
There was a problem hiding this comment.
I split this in nextArmIsDeadForType and nextArmIsDeadForNativeType because
Match arm is unreachableshould rely on$nextArmIsDeadForNativeType || $nextArmIsDeadForType && $this->treatPhpDocTypesAsCertainin order to not be reported in the issue 8932.Match expression does not handle remainingshould rely only onnextArmIsDeadForTypein order to not be reported in the issue 8937.
This way extra checks are allowed when using treatPhpDocTypesAsCertain: false, but phpstan are not complaining if they are missing.
VincentLanglet
force-pushed
the
bug8937
branch
from
ca18fac to
027f3d7
Compare
VincentLanglet
force-pushed
the
bug8937
branch
2 times, most recently
from
c43f1b8 to
b79fd6a
Compare
ondrejmirtes
requested changes
Apr 5, 2023
| $errors = []; | ||
| $armsCount = count($node->getArms()); | ||
| $hasDefault = false; | ||
| foreach ($node->getArms() as $i => $arm) { | ||
| if ($nextArmIsDead) { | ||
| if ($nextArmIsDeadForNativeType || $nextArmIsDeadForType && $this->treatPhpDocTypesAsCertain) { |
There was a problem hiding this comment.
Please use parentheses to disambiguate the condition. And please rebase on top of 1.10.x so we can see whether there are new failures. Thanks.
There was a problem hiding this comment.
It's rebased with parentheses added
|
Thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes phpstan/phpstan#8937
I dunno if it's the best fix, but seems like it's solving both phpstan/phpstan#8932 and phpstan/phpstan#8937.