diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 528084e353..be21fdb25d 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -20,3 +20,4 @@ parameters: reportMethodPurityOverride: true checkDynamicConstantNameValues: true unusedLabel: true + switchConditionAlwaysFalse: true diff --git a/conf/config.level4.neon b/conf/config.level4.neon index cd0b60d4c1..1d2198e3b4 100644 --- a/conf/config.level4.neon +++ b/conf/config.level4.neon @@ -14,6 +14,8 @@ conditionalTags: phpstan.rules.rule: %exceptions.check.tooWideThrowType% PHPStan\Rules\Keywords\UnusedLabelRule: phpstan.rules.rule: %featureToggles.unusedLabel% + PHPStan\Rules\Comparison\SwitchConditionRule: + phpstan.rules.rule: %featureToggles.switchConditionAlwaysFalse% parameters: checkAdvancedIsset: true @@ -35,3 +37,8 @@ services: - class: PHPStan\Rules\Keywords\UnusedLabelRule + + - + class: PHPStan\Rules\Comparison\SwitchConditionRule + arguments: + treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain% diff --git a/conf/config.neon b/conf/config.neon index df8f46567a..96e5846ba0 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -47,6 +47,7 @@ parameters: reportMethodPurityOverride: false checkDynamicConstantNameValues: false unusedLabel: false + switchConditionAlwaysFalse: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index dc07b020e0..e9c585d339 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -49,6 +49,7 @@ parametersSchema: reportMethodPurityOverride: bool() checkDynamicConstantNameValues: bool() unusedLabel: bool() + switchConditionAlwaysFalse: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 9faedfce2f..2aab6f5fea 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -106,6 +106,8 @@ use PHPStan\Node\PropertyHookStatementNode; use PHPStan\Node\ReturnStatement; use PHPStan\Node\StaticMethodCallableNode; +use PHPStan\Node\SwitchConditionArm; +use PHPStan\Node\SwitchConditionNode; use PHPStan\Node\UnreachableStatementNode; use PHPStan\Node\VariableAssignNode; use PHPStan\Node\VarTagChangedExpressionTypeNode; @@ -2005,7 +2007,16 @@ public function processStmtNode( $throwPoints = $condResult->getThrowPoints(); $impurePoints = $condResult->getImpurePoints(); $fullCondExpr = null; - foreach ($stmt->cases as $caseNode) { + $switchConditionArms = []; + $lastNonDefaultCaseKey = null; + foreach ($stmt->cases as $caseKey => $caseNode) { + if ($caseNode->cond === null) { + continue; + } + + $lastNonDefaultCaseKey = $caseKey; + } + foreach ($stmt->cases as $caseKey => $caseNode) { if ($caseNode->cond !== null) { $condExpr = new BinaryOp\Equal($stmt->cond, $caseNode->cond); $fullCondExpr = $fullCondExpr === null ? $condExpr : new BooleanOr($fullCondExpr, $condExpr); @@ -2014,6 +2025,12 @@ public function processStmtNode( $hasYield = $hasYield || $caseResult->hasYield(); $throwPoints = array_merge($throwPoints, $caseResult->getThrowPoints()); $impurePoints = array_merge($impurePoints, $caseResult->getImpurePoints()); + $switchConditionArms[] = new SwitchConditionArm( + $caseNode->cond, + $scopeForBranches, + $caseNode->cond->getStartLine(), + $caseKey === $lastNonDefaultCaseKey, + ); $branchScope = $caseResult->getScope()->filterByTruthyValue($condExpr); } else { $hasDefaultCase = true; @@ -2051,6 +2068,10 @@ public function processStmtNode( } } + if ($switchConditionArms !== []) { + $this->callNodeCallback($nodeCallback, new SwitchConditionNode($stmt->cond, $switchConditionArms, $stmt), $scope, $storage); + } + $exhaustive = $scopeForBranches->getType($stmt->cond) instanceof NeverType; if (!$hasDefaultCase && !$exhaustive) { diff --git a/src/Node/SwitchConditionArm.php b/src/Node/SwitchConditionArm.php new file mode 100644 index 0000000000..1dcfed7992 --- /dev/null +++ b/src/Node/SwitchConditionArm.php @@ -0,0 +1,53 @@ +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; + } + +} diff --git a/src/Node/SwitchConditionNode.php b/src/Node/SwitchConditionNode.php new file mode 100644 index 0000000000..f17c11ae1f --- /dev/null +++ b/src/Node/SwitchConditionNode.php @@ -0,0 +1,61 @@ +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 []; + } + +} diff --git a/src/Reflection/InitializerExprTypeResolver.php b/src/Reflection/InitializerExprTypeResolver.php index 5599286bb8..ddc3751d1f 100644 --- a/src/Reflection/InitializerExprTypeResolver.php +++ b/src/Reflection/InitializerExprTypeResolver.php @@ -1978,6 +1978,10 @@ public function resolveIdenticalType(Type $leftType, Type $rightType): TypeResul */ public function resolveEqualType(Type $leftType, Type $rightType): TypeResult { + if ($leftType instanceof NeverType || $rightType instanceof NeverType) { + return new TypeResult(new ConstantBooleanType(false), []); + } + if ( ($leftType->isEnum()->yes() && $rightType->isTrue()->no()) || ($rightType->isEnum()->yes() && $leftType->isTrue()->no()) diff --git a/src/Rules/Comparison/SwitchConditionRule.php b/src/Rules/Comparison/SwitchConditionRule.php new file mode 100644 index 0000000000..1de05dfe7a --- /dev/null +++ b/src/Rules/Comparison/SwitchConditionRule.php @@ -0,0 +1,129 @@ + + */ +final class SwitchConditionRule implements Rule +{ + + public function __construct( + private ConstantConditionRuleHelper $constantConditionRuleHelper, + private PossiblyImpureTipHelper $possiblyImpureTipHelper, + private ConstantConditionInTraitHelper $constantConditionInTraitHelper, + 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; + + foreach ($node->getArms() as $arm) { + if ( + $nextCaseIsDeadForNativeType + || ($nextCaseIsDeadForType && $this->treatPhpDocTypesAsCertain) + ) { + continue; + } + + $armScope = $arm->getScope(); + $caseCondition = $arm->getCaseCondition(); + $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()), + $armScope->getType($caseCondition)->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(); + } + +} diff --git a/tests/PHPStan/Analyser/nsrt/bcmath-number.php b/tests/PHPStan/Analyser/nsrt/bcmath-number.php index d78887e4ef..e16c681920 100644 --- a/tests/PHPStan/Analyser/nsrt/bcmath-number.php +++ b/tests/PHPStan/Analyser/nsrt/bcmath-number.php @@ -393,8 +393,8 @@ public function bcVsNever(Number $a): void assertType('bool', $a > $b); assertType('bool', $a >= $b); assertType('*NEVER*', $a <=> $b); - assertType('bool', $a == $b); - assertType('bool', $a != $b); + assertType('false', $a == $b); + assertType('true', $a != $b); assertType('*NEVER*', $a & $b); assertType('*NEVER*', $a ^ $b); assertType('*NEVER*', $a | $b); diff --git a/tests/PHPStan/Rules/Comparison/SwitchConditionRuleTest.php b/tests/PHPStan/Rules/Comparison/SwitchConditionRuleTest.php new file mode 100644 index 0000000000..cfdd4e901e --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/SwitchConditionRuleTest.php @@ -0,0 +1,161 @@ + + */ +class SwitchConditionRuleTest extends RuleTestCase +{ + + private bool $treatPhpDocTypesAsCertain = true; + + protected function getRule(): Rule + { + // @phpstan-ignore argument.type + return new CompositeRule([ + new SwitchConditionRule( + new ConstantConditionRuleHelper( + new ImpossibleCheckTypeHelper( + self::createReflectionProvider(), + $this->getTypeSpecifier(), + $this->treatPhpDocTypesAsCertain, + ), + $this->treatPhpDocTypesAsCertain, + ), + new PossiblyImpureTipHelper(true), + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + $this->treatPhpDocTypesAsCertain, + ), + new ConstantConditionInTraitRule(), + ]); + } + + protected function shouldTreatPhpDocTypesAsCertain(): bool + { + return $this->treatPhpDocTypesAsCertain; + } + + public function testRule(): void + { + if (!defined('DUPLICATE_SWITCH_CASE_CONST')) { + define('DUPLICATE_SWITCH_CASE_CONST', 'unknown'); + } + + $this->analyse([__DIR__ . '/data/switch-condition-always-false.php'], [ + [ + 'Switch condition comparison between int|int<3, max> and 1 is always false.', + 46, + ], + [ + 'Switch condition comparison between \'0\' and true is always false.', + 110, + ], + [ + 'Switch condition comparison between \'0\' and null is always false.', + 112, + ], + ]); + } + + #[RequiresPhp('>= 8.1.0')] + public function testRuleEnum(): void + { + $this->analyse([__DIR__ . '/data/switch-condition-always-false-enum.php'], [ + [ + 'Switch condition comparison between SwitchConditionAlwaysFalseEnum\Status::Pending and SwitchConditionAlwaysFalseEnum\Status::Active is always false.', + 24, + ], + ]); + } + + #[RequiresPhp('>= 8.1.0')] + public function testAlwaysTrue(): void + { + $tipText = 'Remove remaining cases below this one and this error will disappear too.'; + $this->analyse([__DIR__ . '/data/switch-condition-always-true.php'], [ + [ + 'Switch condition comparison between SwitchConditionAlwaysTrue\Suit::Diamonds and SwitchConditionAlwaysTrue\Suit::Diamonds is always true.', + 21, + $tipText, + ], + [ + 'Switch condition comparison between 2 and 2 is always true.', + 46, + $tipText, + ], + ]); + } + + #[RequiresPhp('>= 8.1.0')] + public function testImpossibleCases(): void + { + $tipText = 'Remove remaining cases below this one and this error will disappear too.'; + $this->analyse([__DIR__ . '/data/switch-condition-always-false-impossible.php'], [ + [ + 'Switch condition comparison between int and \'foo\' is always false.', + 11, + ], + [ + 'Switch condition comparison between 1|2|3 and 4 is always false.', + 22, + ], + [ + 'Switch condition comparison between \'b\' and \'b\' is always true.', + 37, + $tipText, + ], + [ + 'Switch condition comparison between int<5, max> and 1 is always false.', + 50, + ], + [ + 'Switch condition comparison between *NEVER* and string is always false.', + 66, + ], + ]); + } + + public function testDoNotTreatPhpDocTypesAsCertain(): void + { + $this->treatPhpDocTypesAsCertain = false; + + if (PHP_VERSION_ID < 80000) { + // Before PHP 8.0 a non-numeric string loosely equals 0, so int == 'foo' is not always false. + $this->analyse([__DIR__ . '/data/switch-condition-always-false-native.php'], []); + return; + } + + $this->analyse([__DIR__ . '/data/switch-condition-always-false-native.php'], [ + [ + 'Switch condition comparison between int and \'foo\' is always false.', + 11, + ], + ]); + } + + public function testInTrait(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/switch-condition-in-trait.php'], [ + [ + 'Switch condition comparison between true and false is always false.', + 21, + ], + [ + 'Switch condition comparison between true and true is always true.', + 30, + 'Remove remaining cases below this one and this error will disappear too.', + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-enum.php b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-enum.php new file mode 100644 index 0000000000..0646b061ae --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-enum.php @@ -0,0 +1,55 @@ += 8.1 + +namespace SwitchConditionAlwaysFalseEnum; + +enum Status: string +{ + + case Active = 'active'; + case Inactive = 'inactive'; + case Pending = 'pending'; + +} + +class Foo +{ + + public function doFoo(Status $status): void + { + switch ($status) { + case Status::Active: + break; + case Status::Inactive: + break; + case Status::Active: + break; + } + } + + public function noDuplicates(Status $status): void + { + switch ($status) { + case Status::Active: + break; + case Status::Inactive: + break; + case Status::Pending: + break; + } + } + + public function backedEnumCaseIsNotADuplicateOfItsBackingValue(Status|string $value): void + { + switch ($value) { + case 'active': + break; + case Status::Active: + break; + case Status::Inactive: + break; + case 'inactive': + break; + } + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-impossible.php b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-impossible.php new file mode 100644 index 0000000000..02e88b38d6 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-impossible.php @@ -0,0 +1,71 @@ + $i + */ + public function integerRange(int $i): void + { + switch ($i) { + case 1: + break; + case 10: + break; + } + } + + /** + * @param 'a'|'b' $s + */ + public function nonConstantCaseOnExhaustedSubject(string $s, string $other): void + { + switch ($s) { + case 'a': + case 'b': + break; + case $other: + break; + } + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-native.php b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-native.php new file mode 100644 index 0000000000..03c1a99830 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false-native.php @@ -0,0 +1,27 @@ + $i + */ + public function phpDocOnly(int $i): void + { + switch ($i) { + case 1: + break; + } + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false.php b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false.php new file mode 100644 index 0000000000..1a7e34cffa --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-false.php @@ -0,0 +1,177 @@ +weightInGrams = 1; + break; + case 'kg': + $this->weightInGrams = 1000; + break; + case 'mg': + $this->weightInGrams = 0; + break; + case 'lb': + $this->weightInGrams = 454; + break; + case 'oz': + $this->weightInGrams = 28; + break; + case 'lb': + $this->weightInGrams = 453; + break; + case 'oz': + $this->weightInGrams = 29; + break; + } + } + + public function intCases(int $i): void + { + switch ($i) { + case 1: + break; + case 2: + break; + case 1: + break; + } + } + + public function tripleDuplicate(string $s): void + { + switch ($s) { + case 'x': + break; + case 'y': + break; + case 'x': + break; + case 'x': + break; + } + } + + public function classConstant(string $operator): void + { + switch ($operator) { + case '=': + break; + case '<': + break; + case self::EQ: + break; + } + } + + public function globalConstant(string $s): void + { + switch ($s) { + case 'unknown': + break; + case DUPLICATE_SWITCH_CASE_CONST: + break; + } + } + + public function fallthroughGroups(string $s): void + { + switch ($s) { + case 'a': + case 'b': + doFoo(); + break; + case 'a': + doBar(); + break; + } + } + + /** + * @param mixed $m + */ + public function boolAndNullCases($m): void + { + switch ($m) { + case true: + break; + case null: + break; + case true: + break; + case null: + break; + } + } + + public function defaultIsNotADuplicate(string $s): void + { + switch ($s) { + case 'a': + break; + default: + break; + case 'b': + break; + } + } + + public function nonConstantConditions(string $s, string $foo): void + { + switch ($s) { + case $foo: + break; + case $foo: + break; + case rand() === 1 ? 'a' : 'b': + break; + case rand() === 1 ? 'a' : 'b': + break; + } + } + + /** + * @param mixed $m + */ + public function looseEquality($m): void + { + switch ($m) { + case 1: + break; + case '1': + break; + case 1.0: + break; + case true: + break; + case 0: + break; + case false: + break; + } + } + + public function separateSwitches(string $s): void + { + switch ($s) { + case 'a': + break; + } + + switch ($s) { + case 'a': + break; + } + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/switch-condition-always-true.php b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-true.php new file mode 100644 index 0000000000..1fd95a47f5 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/switch-condition-always-true.php @@ -0,0 +1,65 @@ += 8.1 + +namespace SwitchConditionAlwaysTrue; + +enum Suit +{ + + case Hearts; + case Diamonds; + +} + +class Foo +{ + + public function redundantCaseAfterExhaustiveEnum(Suit $suit): void + { + switch ($suit) { + case Suit::Hearts: + break; + case Suit::Diamonds: + break; + case Suit::Hearts: + break; + } + } + + public function lastCaseAlwaysTrueIsAllowed(Suit $suit): void + { + switch ($suit) { + case Suit::Hearts: + break; + case Suit::Diamonds: + break; + } + } + + /** + * @param 1|2 $i + */ + public function intUnion(int $i): void + { + switch ($i) { + case 1: + break; + case 2: + break; + case 3: + break; + } + } + + public function alwaysTrueBeforeDefault(Suit $suit): void + { + switch ($suit) { + case Suit::Hearts: + break; + case Suit::Diamonds: + break; + default: + break; + } + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/switch-condition-in-trait.php b/tests/PHPStan/Rules/Comparison/data/switch-condition-in-trait.php new file mode 100644 index 0000000000..2827aff347 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/switch-condition-in-trait.php @@ -0,0 +1,98 @@ +doBar(): + break; + } + } + + public function doFoo2(): void + { + // always false + switch (true) { + case $this->doBar2(): + break; + } + } + + public function doFoo3(): void + { + // always true + switch (true) { + case $this->doBar3(): + break; + case $this->doBar3(): + break; + default: + break; + } + } + +} + +class Foo +{ + + use FooTrait; + + /** + * @return false + */ + public function doBar(): bool + { + + } + + /** + * @return false + */ + public function doBar2(): bool + { + + } + + /** + * @return true + */ + public function doBar3(): bool + { + + } + +} + +class FooAnother +{ + + use FooTrait; + + public function doBar(): bool + { + + } + + /** + * @return false + */ + public function doBar2(): bool + { + + } + + /** + * @return true + */ + public function doBar3(): bool + { + + } + +}