diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 528084e353..e250a4bf84 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -20,3 +20,4 @@ parameters: reportMethodPurityOverride: true checkDynamicConstantNameValues: true unusedLabel: true + duplicateCasesInSwitch: true diff --git a/conf/config.level0.neon b/conf/config.level0.neon index 805ea348a9..40f19c3a80 100644 --- a/conf/config.level0.neon +++ b/conf/config.level0.neon @@ -14,6 +14,8 @@ conditionalTags: phpstan.restrictedFunctionUsageExtension: %featureToggles.internalTag% PHPStan\Rules\Classes\NewStaticInAbstractClassStaticMethodRule: phpstan.rules.rule: %featureToggles.newStaticInAbstractClassStaticMethod% + PHPStan\Rules\Comparison\DuplicateCasesInSwitchRule: + phpstan.rules.rule: %featureToggles.duplicateCasesInSwitch% services: - @@ -27,3 +29,6 @@ services: - class: PHPStan\Rules\InternalTag\RestrictedInternalFunctionUsageExtension + + - + class: PHPStan\Rules\Comparison\DuplicateCasesInSwitchRule diff --git a/conf/config.neon b/conf/config.neon index df8f46567a..200faafc15 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -47,6 +47,7 @@ parameters: reportMethodPurityOverride: false checkDynamicConstantNameValues: false unusedLabel: false + duplicateCasesInSwitch: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index dc07b020e0..4fb7be798f 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -49,6 +49,7 @@ parametersSchema: reportMethodPurityOverride: bool() checkDynamicConstantNameValues: bool() unusedLabel: bool() + duplicateCasesInSwitch: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Rules/Comparison/DuplicateCasesInSwitchRule.php b/src/Rules/Comparison/DuplicateCasesInSwitchRule.php new file mode 100644 index 0000000000..21e227da91 --- /dev/null +++ b/src/Rules/Comparison/DuplicateCasesInSwitchRule.php @@ -0,0 +1,83 @@ + + */ +final class DuplicateCasesInSwitchRule implements Rule +{ + + public function __construct(private ExprPrinter $exprPrinter) + { + } + + public function getNodeType(): string + { + return Switch_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $errors = []; + $seenCases = []; + + foreach ($node->cases as $case) { + if ($case->cond === null) { + continue; + } + + $condType = $scope->getType($case->cond); + $scalarValues = $condType->getConstantScalarValues(); + + if (count($scalarValues) === 1) { + $key = ['scalar', $scalarValues[0]]; + } else { + $enumCases = $condType->getEnumCases(); + + if (count($enumCases) !== 1) { + continue; + } + + $key = ['enum', $enumCases[0]->getClassName(), $enumCases[0]->getEnumCaseName()]; + } + + $firstSeen = null; + + foreach ($seenCases as $seenCase) { + if ($seenCase['key'] === $key) { + $firstSeen = $seenCase; + break; + } + } + + if ($firstSeen === null) { + $seenCases[] = [ + 'key' => $key, + 'printed' => $this->exprPrinter->printExpr($case->cond), + 'line' => $case->cond->getStartLine(), + ]; + continue; + } + + $errors[] = RuleErrorBuilder::message(sprintf( + 'Case %s in switch is a duplicate of case %s on line %d.', + $this->exprPrinter->printExpr($case->cond), + $firstSeen['printed'], + $firstSeen['line'], + ))->identifier('switch.duplicateCase')->line($case->getStartLine())->build(); + } + + return $errors; + } + +} diff --git a/tests/PHPStan/Rules/Comparison/DuplicateCasesInSwitchRuleTest.php b/tests/PHPStan/Rules/Comparison/DuplicateCasesInSwitchRuleTest.php new file mode 100644 index 0000000000..b82ba5781a --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/DuplicateCasesInSwitchRuleTest.php @@ -0,0 +1,85 @@ + + */ +class DuplicateCasesInSwitchRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new DuplicateCasesInSwitchRule( + new ExprPrinter(new Printer()), + ); + } + + public function testDuplicateCases(): void + { + if (!defined('DUPLICATE_SWITCH_CASE_CONST')) { + define('DUPLICATE_SWITCH_CASE_CONST', 'unknown'); + } + + $this->analyse([__DIR__ . '/data/duplicate-cases-in-switch.php'], [ + [ + 'Case \'lb\' in switch is a duplicate of case \'lb\' on line 24.', + 30, + ], + [ + 'Case \'oz\' in switch is a duplicate of case \'oz\' on line 27.', + 33, + ], + [ + 'Case 1 in switch is a duplicate of case 1 on line 42.', + 46, + ], + [ + 'Case \'x\' in switch is a duplicate of case \'x\' on line 54.', + 58, + ], + [ + 'Case \'x\' in switch is a duplicate of case \'x\' on line 54.', + 60, + ], + [ + 'Case self::EQ in switch is a duplicate of case \'=\' on line 68.', + 72, + ], + [ + 'Case DUPLICATE_SWITCH_CASE_CONST in switch is a duplicate of case \'unknown\' on line 80.', + 82, + ], + [ + 'Case \'a\' in switch is a duplicate of case \'a\' on line 90.', + 94, + ], + [ + 'Case true in switch is a duplicate of case true on line 103.', + 107, + ], + [ + 'Case null in switch is a duplicate of case null on line 105.', + 109, + ], + ]); + } + + public function testDuplicateCasesEnum(): void + { + $this->analyse([__DIR__ . '/data/duplicate-cases-in-switch-enum.php'], [ + [ + 'Case \DuplicateCasesInSwitchEnum\Status::Active in switch is a duplicate of case \DuplicateCasesInSwitchEnum\Status::Active on line 20.', + 24, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/duplicate-cases-in-switch-enum.php b/tests/PHPStan/Rules/Comparison/data/duplicate-cases-in-switch-enum.php new file mode 100644 index 0000000000..c229691ae7 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/duplicate-cases-in-switch-enum.php @@ -0,0 +1,55 @@ += 8.1 + +namespace DuplicateCasesInSwitchEnum; + +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/duplicate-cases-in-switch.php b/tests/PHPStan/Rules/Comparison/data/duplicate-cases-in-switch.php new file mode 100644 index 0000000000..4cec6400c1 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/duplicate-cases-in-switch.php @@ -0,0 +1,171 @@ +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; + } + } + + public function boolAndNullCases(mixed $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; + } + } + + public function looseEqualityIsNotReported(mixed $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; + } + } + +}