-
Notifications
You must be signed in to change notification settings - Fork 584
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
Changes from all commits
ecd6b4b
1df998c
342ddec
07988ec
efc693e
7f93065
3d5be75
80a6a1a
522731e
39fefc7
3cc85ce
7df67da
21ff471
d4b98fb
1d57aaa
85b48e7
ca306a2
95594cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||
| } | ||
|
|
||
| } |
| 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 []; | ||
| } | ||
|
|
||
| } |
| 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()) { | ||
|
Check warning on line 97 in src/Rules/Comparison/SwitchConditionRule.php
|
||
| $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()) { | ||
|
Check warning on line 107 in src/Rules/Comparison/SwitchConditionRule.php
|
||
| $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()) { | ||
|
Check warning on line 121 in src/Rules/Comparison/SwitchConditionRule.php
|
||
| $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(); | ||
|
Check warning on line 162 in src/Rules/Comparison/SwitchConditionRule.php
|
||
| } | ||
|
|
||
| /** | ||
| * 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(); | ||
|
Check warning on line 177 in src/Rules/Comparison/SwitchConditionRule.php
|
||
| } | ||
|
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.