-
Notifications
You must be signed in to change notification settings - Fork 583
Emit SwitchConditionNode and report always-false switch case comparisons #5854
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
Open
phpstan-bot
wants to merge
17
commits into
phpstan:2.2.x
Choose a base branch
from
phpstan-bot:create-pull-request/patch-pndxujx
base: 2.2.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
ecd6b4b
Emit SwitchConditionNode and report always-false switch case comparisons
ondrejmirtes 1df998c
Add in-trait test coverage for SwitchConditionRule
phpstan-bot 342ddec
Report always-true switch case comparisons
phpstan-bot 07988ec
Treat the last non-default switch case as last for always-true reporting
phpstan-bot efc693e
Use early exit in last-non-default-case loop
phpstan-bot 7f93065
Use @param mixed PHPDoc instead of native mixed type for PHP 7.4
phpstan-bot 3d5be75
Use @return PHPDoc instead of native false/true types for PHP 7.4
phpstan-bot 80a6a1a
Skip version-dependent SwitchConditionRule tests on older PHP
phpstan-bot 522731e
Keep loose comparison with a never operand undecided
phpstan-bot 39fefc7
Report duplicate switch cases
phpstan-bot 3cc85ce
Identify duplicate switch cases by type instead of a hand-rolled key
phpstan-bot 7df67da
fix bad merge
staabm 21ff471
Document that a never switch subject is intentionally not always-false
phpstan-bot d4b98fb
Report always-false switch case on a never subject exhausted by earli…
phpstan-bot 1d57aaa
remove outdated comments
staabm 85b48e7
Report loosely-equal numeric switch cases as duplicates
phpstan-bot ca306a2
I don't see why we should handle only numeric constant values
staabm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Node; | ||
|
|
||
| use PhpParser\Node\Expr; | ||
| use PHPStan\Analyser\Scope; | ||
|
|
||
| /** | ||
| * A single non-default `case` of a `switch`, paired with the scope captured | ||
| * right after the case condition was processed (which already excludes the | ||
| * values matched by earlier terminating cases). | ||
| * | ||
| * @api | ||
| */ | ||
| final class SwitchConditionArm | ||
| { | ||
|
|
||
| public function __construct( | ||
| private Expr $caseCondition, | ||
| private Scope $scope, | ||
| private int $line, | ||
| private bool $isLast, | ||
| ) | ||
| { | ||
| } | ||
|
|
||
| public function getCaseCondition(): Expr | ||
| { | ||
| return $this->caseCondition; | ||
| } | ||
|
|
||
| public function getScope(): Scope | ||
| { | ||
| return $this->scope; | ||
| } | ||
|
|
||
| public function getLine(): int | ||
| { | ||
| return $this->line; | ||
| } | ||
|
|
||
| /** | ||
| * Whether this is the last non-default `case` of the `switch` (only a | ||
| * `default` may follow it), in which case an always-true comparison is fine | ||
| * because it does not make any subsequent `case` unreachable. A trailing | ||
| * `default` is not considered a `case` it would make unreachable. | ||
| */ | ||
| public function isLast(): bool | ||
| { | ||
| return $this->isLast; | ||
| } | ||
|
|
||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Node; | ||
|
|
||
| use Override; | ||
| use PhpParser\Node\Expr; | ||
| use PhpParser\Node\Stmt\Switch_; | ||
| use PhpParser\NodeAbstract; | ||
|
|
||
| /** | ||
| * Virtual node emitted once per `switch` statement. It pairs the switch subject | ||
| * with each non-default `case` condition so rules can inspect the loose `==` | ||
| * comparison the `switch` performs, using the scope captured at each case | ||
| * (which already excludes the values matched by earlier cases). | ||
| * | ||
| * @api | ||
| */ | ||
| final class SwitchConditionNode extends NodeAbstract implements VirtualNode | ||
| { | ||
|
|
||
| /** | ||
| * @param SwitchConditionArm[] $arms | ||
| */ | ||
| public function __construct( | ||
| private Expr $subject, | ||
| private array $arms, | ||
| Switch_ $originalNode, | ||
| ) | ||
| { | ||
| parent::__construct($originalNode->getAttributes()); | ||
| } | ||
|
|
||
| public function getSubject(): Expr | ||
| { | ||
| return $this->subject; | ||
| } | ||
|
|
||
| /** | ||
| * @return SwitchConditionArm[] | ||
| */ | ||
| public function getArms(): array | ||
| { | ||
| return $this->arms; | ||
| } | ||
|
|
||
| #[Override] | ||
| public function getType(): string | ||
| { | ||
| return 'PHPStan_Node_SwitchCondition'; | ||
| } | ||
|
|
||
| /** | ||
| * @return string[] | ||
| */ | ||
| #[Override] | ||
| public function getSubNodeNames(): array | ||
| { | ||
| return []; | ||
| } | ||
|
|
||
| } |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Rules\Comparison; | ||
|
|
||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\BinaryOp\Equal; | ||
| use PHPStan\Analyser\CollectedDataEmitter; | ||
| use PHPStan\Analyser\NodeCallbackInvoker; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Node\Printer\ExprPrinter; | ||
| use PHPStan\Node\SwitchConditionNode; | ||
| use PHPStan\Php\PhpVersion; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
| use PHPStan\Type\Type; | ||
| use PHPStan\Type\VerbosityLevel; | ||
| use function count; | ||
| use function sprintf; | ||
|
|
||
| /** | ||
| * @implements Rule<SwitchConditionNode> | ||
| */ | ||
| final class SwitchConditionRule implements Rule | ||
| { | ||
|
|
||
| public function __construct( | ||
| private ConstantConditionRuleHelper $constantConditionRuleHelper, | ||
| private PossiblyImpureTipHelper $possiblyImpureTipHelper, | ||
| private ConstantConditionInTraitHelper $constantConditionInTraitHelper, | ||
| private ExprPrinter $exprPrinter, | ||
| private PhpVersion $phpVersion, | ||
| private bool $treatPhpDocTypesAsCertain, | ||
| ) | ||
| { | ||
| } | ||
|
|
||
| public function getNodeType(): string | ||
| { | ||
| return SwitchConditionNode::class; | ||
| } | ||
|
|
||
| public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array | ||
| { | ||
| $subject = $node->getSubject(); | ||
| $errors = []; | ||
| $nextCaseIsDeadForType = false; | ||
| $nextCaseIsDeadForNativeType = false; | ||
| $seenCases = []; | ||
|
|
||
| foreach ($node->getArms() as $arm) { | ||
| if ( | ||
| $nextCaseIsDeadForNativeType | ||
| || ($nextCaseIsDeadForType && $this->treatPhpDocTypesAsCertain) | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| $armScope = $arm->getScope(); | ||
| $caseCondition = $arm->getCaseCondition(); | ||
|
|
||
| $caseConditionType = $armScope->getType($caseCondition); | ||
| $finiteTypes = $caseConditionType->getFiniteTypes(); | ||
| if (count($finiteTypes) === 1) { | ||
| $caseValueType = $finiteTypes[0]; | ||
| $firstSeen = null; | ||
| foreach ($seenCases as $seenCase) { | ||
| if ($this->isDuplicateCase($seenCase['type'], $caseValueType)) { | ||
| $firstSeen = $seenCase; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ($firstSeen !== null) { | ||
| $errors[] = RuleErrorBuilder::message(sprintf( | ||
| 'Case %s in switch is a duplicate of case %s on line %d.', | ||
| $this->exprPrinter->printExpr($caseCondition), | ||
| $firstSeen['printed'], | ||
| $firstSeen['line'], | ||
| ))->line($arm->getLine())->identifier('switch.duplicateCase')->build(); | ||
| continue; | ||
| } | ||
|
|
||
| $seenCases[] = [ | ||
| 'type' => $caseValueType, | ||
| 'printed' => $this->exprPrinter->printExpr($caseCondition), | ||
| 'line' => $arm->getLine(), | ||
| ]; | ||
| } | ||
|
|
||
| $conditionExpr = new Equal($subject, $caseCondition); | ||
|
|
||
| $conditionType = $armScope->getType($conditionExpr); | ||
| if (!$this->isConstantBoolean($conditionType)) { | ||
| $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $conditionExpr); | ||
| continue; | ||
| } | ||
| if ($conditionType->isTrue()->yes()) { | ||
| $nextCaseIsDeadForType = true; | ||
| } | ||
|
|
||
| if (!$this->treatPhpDocTypesAsCertain) { | ||
| $conditionNativeType = $armScope->getNativeType($conditionExpr); | ||
| if (!$this->isConstantBoolean($conditionNativeType)) { | ||
| $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $conditionExpr); | ||
| continue; | ||
| } | ||
| if ($conditionNativeType->isTrue()->yes()) { | ||
| $nextCaseIsDeadForNativeType = true; | ||
| } | ||
| } | ||
|
|
||
| $subjectType = $armScope->getType($subject); | ||
| if ($this->isConstantBoolean($subjectType)) { | ||
| $caseConditionStandaloneType = $this->constantConditionRuleHelper->getBooleanType($armScope, $caseCondition); | ||
| if (!$this->isConstantBoolean($caseConditionStandaloneType)) { | ||
| $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $conditionExpr); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if ($conditionType->isFalse()->yes()) { | ||
| $errorBuilder = RuleErrorBuilder::message(sprintf( | ||
| 'Switch condition comparison between %s and %s is always false.', | ||
| $subjectType->describe(VerbosityLevel::value()), | ||
| $caseConditionType->describe(VerbosityLevel::value()), | ||
| ))->line($arm->getLine())->identifier('switch.alwaysFalse'); | ||
| $this->possiblyImpureTipHelper->addTip($armScope, $conditionExpr, $errorBuilder); | ||
| $ruleError = $errorBuilder->build(); | ||
| if ($scope->isInTrait()) { | ||
| $this->constantConditionInTraitHelper->emitError(self::class, $scope, $conditionExpr, false, $ruleError); | ||
| } else { | ||
| $errors[] = $ruleError; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if ($arm->isLast()) { | ||
| $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $conditionExpr); | ||
| continue; | ||
| } | ||
|
|
||
| $errorBuilder = RuleErrorBuilder::message(sprintf( | ||
| 'Switch condition comparison between %s and %s is always true.', | ||
| $subjectType->describe(VerbosityLevel::value()), | ||
| $armScope->getType($caseCondition)->describe(VerbosityLevel::value()), | ||
| ))->line($arm->getLine())->identifier('switch.alwaysTrue') | ||
| ->tip('Remove remaining cases below this one and this error will disappear too.'); | ||
| $this->possiblyImpureTipHelper->addTip($armScope, $conditionExpr, $errorBuilder); | ||
| $ruleError = $errorBuilder->build(); | ||
| if ($scope->isInTrait()) { | ||
| $this->constantConditionInTraitHelper->emitError(self::class, $scope, $conditionExpr, true, $ruleError); | ||
| } else { | ||
| $errors[] = $ruleError; | ||
| } | ||
| } | ||
|
|
||
| return $errors; | ||
| } | ||
|
|
||
| private function isConstantBoolean(Type $type): bool | ||
| { | ||
| return $type->isTrue()->yes() || $type->isFalse()->yes(); | ||
| } | ||
|
|
||
| /** | ||
| * A later `case` is a duplicate of an earlier one when both match the exact | ||
| * same set of subject values. Besides identical values, `switch` compares | ||
| * with loose `==`, so two numerically-equal constants (e.g. 1, '1' and 1.0) | ||
| * are duplicates too - they cannot be told apart by a `switch`. | ||
| */ | ||
| private function isDuplicateCase(Type $seenType, Type $caseValueType): bool | ||
| { | ||
| if ($seenType->equals($caseValueType)) { | ||
| return true; | ||
| } | ||
|
|
||
| return $seenType->looseCompare($caseValueType, $this->phpVersion)->isTrue()->yes(); | ||
| } | ||
|
|
||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.