Detect changes in ENUMs as BC breaks

bin/roave-backward-compatibility-check.php

@@ -96,6 +96,7 @@ static function (string $installationPath) use ($composerIo): Installer {
                     new ClassBased\SkipClassBasedErrors(new ClassBased\MethodRemoved()),
                     new ClassBased\SkipClassBasedErrors(new ClassBased\AncestorRemoved()),
                     new ClassBased\SkipClassBasedErrors(new ClassBased\ClassBecameInternal()),
+                    new ClassBased\SkipClassBasedErrors(new ClassBased\EnumCasesChanged()),
                     new ClassBased\SkipClassBasedErrors(new ClassBased\OpenClassChanged(
                         new ClassBased\MultipleChecksOnAClass(
                             new ClassBased\SkipClassBasedErrors(new ClassBased\ConstantChanged(

src/DetectChanges/BCBreak/ClassBased/EnumCasesChanged.php

@@ -0,0 +1,162 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Roave\BackwardCompatibility\DetectChanges\BCBreak\ClassBased;
+
+use Psl\Regex;
+use Roave\BackwardCompatibility\Change;
+use Roave\BackwardCompatibility\Changes;
+use Roave\BetterReflection\Reflection\ReflectionClass;
+use Roave\BetterReflection\Reflection\ReflectionEnum;
+use Roave\BetterReflection\Reflection\ReflectionEnumCase;
+
+use function array_filter;
+use function array_map;
+
+class EnumCasesChanged implements ClassBased
+{
+    public function __invoke(ReflectionClass $fromClass, ReflectionClass $toClass): Changes
+    {
+        $fromEnumName = $fromClass->getName();
+        $fromKind     = $this->kindOf($fromClass);
+        $toKind       = $this->kindOf($toClass);
+
+        if (! $fromClass instanceof ReflectionEnum && ! $toClass instanceof ReflectionEnum) {
+            return Changes::empty();
+        }
+
+        if (! $fromClass instanceof ReflectionEnum && $toClass instanceof ReflectionEnum) {
+            return Changes::fromList(Change::changed($fromKind . ' ' . $fromEnumName . ' became enum'));
+        }
+
+        if (! $toClass instanceof ReflectionEnum) {
+            return Changes::fromList(Change::changed('enum ' . $fromEnumName . ' became ' . $toKind));
+        }
+
+        $addedCases = array_filter(
+            $toClass->getCases(),
+            static function (ReflectionEnumCase $case) use ($fromClass): bool {
+                if (self::isInternalDocComment($case->getDocComment())) {
+                    return false;
+                }
+
+                return ! $fromClass->hasCase($case->getName());
+            },
+        );
+
+        $removedCases = array_filter(
+            $fromClass->getCases(),
+            static function (ReflectionEnumCase $case) use ($toClass): bool {
+                if (self::isInternalDocComment($case->getDocComment())) {
+                    return false;
+                }
+
+                return ! $toClass->hasCase($case->getName());
+            },
+        );
+
+        $internalisedCases = array_filter(
+            $toClass->getCases(),
+            static function (ReflectionEnumCase $case) use ($fromClass) {
+                if (! self::isInternalDocComment($case->getDocComment())) {
+                    return false;
+                }
+
+                $fromClassCase = $fromClass->getCase($case->getName());
+                if (! $fromClassCase) {
+                    return false;
+                }
+
+                return ! self::isInternalDocComment($fromClassCase->getDocComment());
+            },
+        );
+
+        $nowNotInternalCases = array_filter(
+            $toClass->getCases(),
+            static function (ReflectionEnumCase $case) use ($fromClass) {
+                if (self::isInternalDocComment($case->getDocComment())) {
+                    return false;
+                }
+
+                $fromClassCase = $fromClass->getCase($case->getName());
+                if (! $fromClassCase) {
+                    return false;
+                }
+
+                return self::isInternalDocComment($fromClassCase->getDocComment());
+            },
+        );
+
+        $caseRemovedChanges = array_map(
+            static function (ReflectionEnumCase $case) use ($fromEnumName): Change {
+                $caseName = $case->getName();
+
+                return Change::removed('Case ' . $fromEnumName . '::' . $caseName . ' was removed');
+            },
+            $removedCases,
+        );
+
+        $caseAddedChanges = array_map(
+            static function (ReflectionEnumCase $case) use ($fromEnumName): Change {
+                $caseName = $case->getName();
+
+                return Change::added('Case ' . $fromEnumName . '::' . $caseName . ' was added');
+            },
+            $addedCases,
+        );
+
+        $caseBecameInternalChanges = array_map(
+            static function (ReflectionEnumCase $case) use ($fromEnumName): Change {
+                $caseName = $case->getName();
+
+                return Change::changed('Case ' . $fromEnumName . '::' . $caseName . ' was marked "@internal"');
+            },
+            $internalisedCases,
+        );
+
+        $caseBecameNotInternalChanges = array_map(
+            static function (ReflectionEnumCase $case) use ($fromEnumName): Change {
+                $caseName = $case->getName();
+
+                return Change::changed('Case ' . $fromEnumName . '::' . $caseName . ' had "@internal" removed');
+            },
+            $nowNotInternalCases,
+        );
+
+        return Changes::fromList(
+            ...$caseRemovedChanges,
+            ...$caseAddedChanges,
+            ...$caseBecameInternalChanges,
+            ...$caseBecameNotInternalChanges,
+        );
+    }
+
+    /**
+     * Copied from DetectChanges\BCBreak\ClassBased\ExcludeInternalClass - for now I'm not sure
+     * if there's a good place to put a shared function, and following the 3 strike then refactor rule.
+     */
+    private static function isInternalDocComment(string|null $comment): bool
+    {
+        return $comment !== null
+            && Regex\matches($comment, '/\s+@internal\s+/');
+    }
+
+    /** @psalm-return 'enum'|'interface'|'trait'|'class' */
+    private function kindOf(ReflectionClass $reflectionClass): string
+    {
+        if ($reflectionClass->isEnum()) {
+            return 'enum';
+        }
+
+        if ($reflectionClass->isInterface()) {
+            return 'interface';
+        }
+
+        if ($reflectionClass->isTrait()) {
+            return 'trait';
+        }
+
+        return 'class';
+    }
+}

test/asset/api/new/EnumWithCasesBeingChanged.php

@@ -0,0 +1,42 @@
+<?php
+
+declare(strict_types=1);
+
+namespace RoaveTestAsset;
+
+enum EnumWithCasesBeingChanged
+{
+    // two new months below:
+    case January;
+    case February;
+
+    /** @internal  - tired from the two new Months, reserving March for private use from now on.*/
+    case March;
+
+    case April;
+    case May;
+    case June;
+    case July;
+
+    // We're on holiday in August, not going to allow that month any more.
+    // case August;
+
+    case September;
+    case October;
+    case November;
+    case December;
+
+    /**
+     * For testing now but for use only in a future major version. As it should not be statically referenced from
+     * outside this library, and the library will not pass a reference to it to the outside at runtime, adding it
+     * is not a BC break.
+     * @internal
+     */
+    case Sol;
+
+    /**
+     * Previously internal, now public. This is a BC break as we previously committed to never return an instance
+     * of this case and we no-longer make this commitment.
+     */
+    case UnknownMonth;
+}

test/asset/api/old/EnumWithCasesBeingChanged.php

@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types=1);
+
+namespace RoaveTestAsset;
+
+enum EnumWithCasesBeingChanged
+{
+    case March;
+    case April;
+    case May;
+    case June;
+    case July;
+    case August;
+    case September;
+    case October;
+    case November;
+    Case december; // oops forget this is spelled with a capital D
+
+    /**
+     * @internal - may be removed without notice
+     */
+    case FakeMonth;
+
+    /**
+     * @internal - may be introduced in future
+     */
+    case UnknownMonth;
+}

test/unit/DetectChanges/BCBreak/ClassBased/DummyEnum.php

@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace RoaveTest\BackwardCompatibility\DetectChanges\BCBreak\ClassBased;
+
+enum DummyEnum
+{
+    case someCase;
+}

test/unit/DetectChanges/BCBreak/ClassBased/EnumCasesChangedTest.php

@@ -0,0 +1,97 @@
+<?php
+
+declare(strict_types=1);
+
+namespace RoaveTest\BackwardCompatibility\DetectChanges\BCBreak\ClassBased;
+
+use PHPUnit\Framework\TestCase;
+use Roave\BackwardCompatibility\Change;
+use Roave\BackwardCompatibility\DetectChanges\BCBreak\ClassBased\EnumCasesChanged;
+use Roave\BetterReflection\BetterReflection;
+use Roave\BetterReflection\Reflection\ReflectionClass;
+use Roave\BetterReflection\Reflector\DefaultReflector;
+use Roave\BetterReflection\SourceLocator\Type\SingleFileSourceLocator;
+use stdClass;
+
+use function array_map;
+use function iterator_to_array;
+
+final class EnumCasesChangedTest extends TestCase
+{
+    /**
+     * @param string[] $expectedMessages
+     *
+     * @dataProvider enumsToBeTested
+     */
+    public function testDiffs(
+        ReflectionClass $fromEnum,
+        ReflectionClass $toEnum,
+        array $expectedMessages,
+    ): void {
+        $changes = (new EnumCasesChanged())($fromEnum, $toEnum);
+
+        self::assertSame(
+            $expectedMessages,
+            array_map(static function (Change $change): string {
+                return $change->__toString();
+            }, iterator_to_array($changes)),
+        );
+    }
+
+    public function testReturnsClassBecameEnumError(): void
+    {
+        $changes = (new EnumCasesChanged())(
+            ReflectionClass::createFromName(stdClass::class),
+            ReflectionClass::createFromName(DummyEnum::class),
+        );
+
+        $this->assertEquals(
+            [Change::changed('class stdClass became enum')],
+            iterator_to_array($changes),
+        );
+    }
+
+    public function testReturnsEnumBecameClassError(): void
+    {
+        $changes = (new EnumCasesChanged())(
+            ReflectionClass::createFromName(DummyEnum::class),
+            ReflectionClass::createFromName(stdClass::class),
+        );
+
+        $this->assertEquals(
+            [Change::changed('enum RoaveTest\BackwardCompatibility\DetectChanges\BCBreak\ClassBased\DummyEnum became class')],
+            iterator_to_array($changes),
+        );
+    }
+
+    /**
+     * @return array<string, array<int, ReflectionClass|array<int, string>>>
+     * @psalm-return array<string, array{0: ReflectionClass, 1: ReflectionClass, 2: list<string>}>
+     */
+    public function enumsToBeTested(): array
+    {
+        $locator = (new BetterReflection())->astLocator();
+
+        return [
+            'RoaveTestAsset\\EnumWithCasesBeingChanged' => [
+                (new DefaultReflector(new SingleFileSourceLocator(
+                    __DIR__ . '/../../../../asset/api/old/EnumWithCasesBeingChanged.php',
+                    $locator,
+                )))->reflectClass('RoaveTestAsset\EnumWithCasesBeingChanged'),
+                (new DefaultReflector(new SingleFileSourceLocator(
+                    __DIR__ . '/../../../../asset/api/new/EnumWithCasesBeingChanged.php',
+                    $locator,
+                )))->reflectClass('RoaveTestAsset\EnumWithCasesBeingChanged'),
+                [
+                    '[BC] REMOVED: Case RoaveTestAsset\EnumWithCasesBeingChanged::August was removed',
+                    '[BC] REMOVED: Case RoaveTestAsset\EnumWithCasesBeingChanged::december was removed',
+                    '[BC] ADDED: Case RoaveTestAsset\EnumWithCasesBeingChanged::January was added',
+                    '[BC] ADDED: Case RoaveTestAsset\EnumWithCasesBeingChanged::February was added',
+                    '[BC] ADDED: Case RoaveTestAsset\EnumWithCasesBeingChanged::December was added',
+                    '[BC] CHANGED: Case RoaveTestAsset\EnumWithCasesBeingChanged::March was marked "@internal"',
+                    '[BC] CHANGED: Case RoaveTestAsset\EnumWithCasesBeingChanged::UnknownMonth had "@internal" removed',
+                ],
+            ],
+        ];
+    }
+}