Detect enum duplicated values

[kubk]

Fixes: phpstan/phpstan#9184

[phpstan-bot]

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

[ondrejmirtes]

You could check more things here. Like:

1. Wrong case value type against the type declared in $node->scalarType.
2. Missing case value in backed enum, and extra case value in case of non-backed enum.

All three cases here should fail: https://phpstan.org/r/3ca06994-1a6e-4686-a57c-6083c6bdb34e

[kubk]

Thanks for the suggestions, I'll implement them.

[kubk]

@ondrejmirtes Done. 3 tests are failing but they seem like unrelated to my changes.

[kubk]

Here I had to use early return (if (!$isStringBackedWithoutStringCase) {) because linter forced me to do that.

Early return is a great rule to reduce code complexity, but in my opinion in this situation it made the code a bit harder to follow. Because instead of this code:

foreach ($enumCases as $enumCase) {
  if ($isIntBackedWithoutIntValue) {
   // add error
  }
  if ($isStringBackedWithoutStringValue) {
   // add error
  }
}

we got:

foreach ($enumCases as $enumCase) {
  if ($isIntBackedWithoutIntValue) {
   // add error
  }
  if (!$isStringBackedWithoutStringValue) {
    continue;
  }
  // add error
}

I saw that this rule was even suppressed once in phpstan repository:

https://github.com/phpstan/phpstan-deprecation-rules/blob/9d366debb01a8097106174e46c9aa138d3bf1c01/src/Rules/Deprecations/FetchingDeprecatedConstRule.php#L30

[herndlm]

You might want to look into fine-tuning https://github.com/slevomat/coding-standard/blob/master/doc/control-structures.md#slevomatcodingstandardcontrolstructuresearlyexit-

I obviously can't speak for Ondřej, but the last time a similar thing like this was discussed it sounded like he is open to look into adapting it.

[ondrejmirtes]

Hi, I just pushed some cosmetic changes that make the rule more consistent with others.

I still need a few changes from you:

1. Missing case value in backed enum should have a special message - put if ($stmt->expr === null) below if ($node->scalarType === null) and handle it separately. This should make the test suite not crash again.
2. Wrong scalar type like float or object is not yet reported AFAIK.

[kubk]

Hi @ondrejmirtes

I've implemented what you asked. Let's summarize what is done:

1. Detecting duplicate values.
Covered code: https://github.com/phpstan/phpstan-src/pull/2371/files#diff-ff846fb46bb3972dcf0a281f7d686cd1fbc84f2d282966d6acdd0d7965e94a77R86

The rule checks that there are at least 2 duplicate case values.

2. Inconsistent case type and backed type
Covered code: https://github.com/phpstan/phpstan-src/pull/2371/files#diff-ff846fb46bb3972dcf0a281f7d686cd1fbc84f2d282966d6acdd0d7965e94a77R105

The rule checks that enum's scalar type and all the case types match.

3. Enum isn't backed, but has a backed case
Covered code: https://github.com/phpstan/phpstan-src/pull/2371/files#diff-ff846fb46bb3972dcf0a281f7d686cd1fbc84f2d282966d6acdd0d7965e94a77R113

The rule triggers when scalar type is empty, but a case isn't

4. Enum is backed, but one of the cases has value missed
Covered code: https://github.com/phpstan/phpstan-src/pull/2371/files#diff-ff846fb46bb3972dcf0a281f7d686cd1fbc84f2d282966d6acdd0d7965e94a77R106

UPD: I have added one more example because I mistakenly hardcoded the error message: https://github.com/phpstan/phpstan-src/pull/2371/files#diff-ff846fb46bb3972dcf0a281f7d686cd1fbc84f2d282966d6acdd0d7965e94a77R110

The rule triggers when scalar type isn't empty, but a case is.

[kubk]

@ondrejmirtes

Wrong scalar type like float or object is not yet reported AFAIK.

It has been already implemented:

phpstan-src/tests/PHPStan/Rules/Classes/data/enum-sanity.php

Line 67 in a27b32e

enum BackedEnumWithFloatType: float

phpstan-src/tests/PHPStan/Rules/Classes/EnumSanityRuleTest.php

Line 64 in a27b32e

'Backed enum EnumSanity\BackedEnumWithFloatType can have only "int" or "string" type.',

[kubk]

@ondrejmirtes I have a question regarding your fix: 7edd4dd

phpstan-src/src/Rules/Classes/EnumSanityRule.php

Lines 191 to 200 in 7edd4dd

	foreach ($enumCases as $caseValue => $caseNames) {
	if (count($caseNames) <= 1) {
	continue;
	}
	$errors[] = RuleErrorBuilder::message(sprintf(
	'Enum %s has duplicate value %s for %s %s.',
	$node->namespacedName->toString(),
	$caseValue,
	count($caseNames) === 1 ? 'case' : 'cases',

You've replaced 'cases' with count($caseNames) === 1 ? 'case' : 'cases'.

This line has triggered PHPStan, because due to line 192 it will always evaluate to false (it's great that PHPStan noticed it!). But as I understand the code should be reverted. $caseNames is a map, the example structure is ['a' => ['case1', 'case2'], 'b' => 'case3'] for the following enum:

enum Test: string
{
  case case1 = 'a';
  case case2 = 'a';
  case case3 = 'b';
}

So when we see that a value has at least 2 corresponding cases - we report duplication. So I brought it back to make PHPStan check pass. Let me know if it needs to be improved somehow.

[ondrejmirtes]

Thank you.