Adding `Stringable` polyfill causes false positive (`Stringable ancestor removed`)

[weirdan]

Live example: https://github.com/vimeo/psalm/runs/5059860222?check_suite_focus=true

Steps to reproduce:

1. Add roave/backward-compatibility-check:6.1 to a project that didn't use symfony/polyfill-php80 and that also has some classes with __toString() method.
2. Commit composer.json (and composer.lock if it's versioned),
3. Run vendor/bin/roave-backward-compatibility-check --from="HEAD^".

Expected

No backward compatibility breaks should be reported as no project files have changed.

Actual

....
[BC] REMOVED: These ancestors of Psalm\Storage\Assertion\InArray have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Storage\FunctionLikeStorage have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Storage\Assertion have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Node\VirtualIdentifier have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Node\VirtualName have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Node\VirtualVarLikeIdentifier have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Node\Name\VirtualRelative have been removed: ["Stringable"]
[BC] REMOVED: These ancestors of Psalm\Node\Name\VirtualFullyQualified have been removed: ["Stringable"]
125 backwards-incompatible changes detected

I also confirmed that it's triggered by symfony/polyfill-php80 by installing BCC into a separate environment and running vendor/bin/roave-backward-compatibility-check --from="HEAD^" with the HEAD commit having just this:

commit 4cc0571866e8405658117613a8106f880116decb (HEAD -> add-bcc-2)
Author: Bruce Weirdan <weirdan@gmail.com>
Date:   Fri Feb 4 03:39:44 2022 +0200

    Add polyfill

diff --git a/composer.json b/composer.json
index f06872480..567e5d275 100644
--- a/composer.json
+++ b/composer.json
@@ -35,7 +35,8 @@
         "openlss/lib-array2xml": "^1.0",
         "sebastian/diff": "^4.0",
         "symfony/console": "^3.4.17 || ^4.1.6 || ^5.0 || ^6.0",
-        "symfony/filesystem": "^5.4 || ^6.0"
+        "symfony/filesystem": "^5.4 || ^6.0",
+        "symfony/polyfill-php80": "^1.24"
     },
     "provide": {
         "psalm/psalm": "self.version"

[Ocramius]

Hmm, is this with the latest roave/better-reflection installed? 🤔

[weirdan]

With roave/better-reflection:5.0.7

[Ocramius]

I just reproduced this locally:

    public function testImplicitStringableAncestorIsNotConsideredRemoved(): void
    {
        $locator       = (new BetterReflection())->astLocator();
        $fromReflector = new DefaultReflector(new StringSourceLocator(
            <<<'PHP'
<?php

interface Stringable { public function __toString(): string; }
class ClassWithImplicitStringableAncestor implements Stringable {
    public function __toString() : string { return 'hi'; }
}
PHP
            ,
            $locator
        ));
        $toReflector   = new DefaultReflector(new AggregateSourceLocator([
            new StringSourceLocator(
                <<<'PHP'
<?php

class ClassWithImplicitStringableAncestor {
    public function __toString() : string { return 'hi'; }
}
PHP
                ,
                $locator
            )
        ]));

        self::assertEmpty(iterator_to_array(
            (new AncestorRemoved())(
                $fromReflector->reflectClass('ClassWithImplicitStringableAncestor'),
                $toReflector->reflectClass('ClassWithImplicitStringableAncestor'),
            )
        ));
    }

This is because Stringable does not exist in the understanding of the codebase by BetterReflection.

By adding the internal source locator, the problem goes away:

    public function testImplicitStringableAncestorIsNotConsideredRemoved(): void
    {
        $locator       = (new BetterReflection())->astLocator();
        $fromReflector = new DefaultReflector(new StringSourceLocator(
            <<<'PHP'
<?php

interface Stringable { public function __toString(): string; }
class ClassWithImplicitStringableAncestor implements Stringable {
    public function __toString() : string { return 'hi'; }
}
PHP
            ,
            $locator
        ));
        $toReflector   = new DefaultReflector(new AggregateSourceLocator([
            new StringSourceLocator(
                <<<'PHP'
<?php

class ClassWithImplicitStringableAncestor {
    public function __toString() : string { return 'hi'; }
}
PHP
                ,
                $locator
            ),
+            new PhpInternalSourceLocator($locator, new ReflectionSourceStubber())
        ]));

        self::assertEmpty(iterator_to_array(
            (new AncestorRemoved())(
                $fromReflector->reflectClass('ClassWithImplicitStringableAncestor'),
                $toReflector->reflectClass('ClassWithImplicitStringableAncestor'),
            )
        ));
    }

This is the behavior in isolation, but in integration, this is probably related to how the whole source locator is set up (currently investigating that).

[weirdan]

Note that contrary to your example, none of the classes we have in Psalm has explicit implements Stringable. Also, Stringable polyfill was added rather than removed. And of course, it wasn't doing anything as the analysis was run on PHP 8.1. if (\PHP_VERSION_ID < 80000) check effectively made it a piece of dead code.

[Ocramius]

In $toReflector, the Stringable is not there. BetterReflection adds Stringable to a class when it sees __toString: https://github.com/Roave/BetterReflection/blob/d5c9a1afe2ed031c1c86aae58d22f70edb586c67/src/Reflection/ReflectionClass.php#L1113-L1144

For that to work, Stringable must be defined in the sources known to BetterReflection (including its internal stubs)

[Ocramius]

Also, Stringable polyfill was added rather than removed.

This is very interesting 🤔

[Ocramius]

I managed to reproduce this on a minimal example with:

<?php

final class Something
{
    public function __toString(): string { return 'hi'; }
}

{
  "autoload": {
    "classmap": ["./sources.php"]
  }
}

Then added a commit to add the PHP 8.0 polyfill:

{
  "autoload": {
    "classmap": ["./sources.php"]
-  }
+  },
+  "require": {
+    "symfony/polyfill-php80": "^1.24"
+  }
}

The report:

❯ ../bin/roave-backward-compatibility-check --from=master --to=broken
Comparing from 70748ab5be383c124fb4d7d198e90b88452693a8 to 377d52e9d8d9c3f0163569e28fc4fe27ac19ef25...
<warning>No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.</warning>
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Writing lock file
Installing dependencies from lock file
Nothing to install, update or remove
<warning>No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.</warning>
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 0 removals
  - Locking symfony/polyfill-php80 (v1.24.0)
Writing lock file
Installing dependencies from lock file
Package operations: 1 install, 0 updates, 0 removals
  - Installing symfony/polyfill-php80 (v1.24.0): Extracting archive
1 package you are using is looking for funding.
Use the `composer fund` command to find out more!
[BC] REMOVED: These ancestors of Something have been removed: ["Stringable"]
1 backwards-incompatible changes detected

It seems like the polyfill breaks fetching the Stringable class definition.

[Ocramius]

Aha, found it. In roave/better-reflection, we only add the Stringable interface to the ancestors in a ReflectionClass of our sources if it is an internal class ( https://github.com/Roave/BetterReflection/blob/d5c9a1afe2ed031c1c86aae58d22f70edb586c67/src/Reflection/ReflectionClass.php#L1113-L1144 )

                    $stringableInterfaceReflection = $this->reflectClassForNamedNode(new Node\Name($stringableClassName));

                    if ($stringableInterfaceReflection->isInternal()) {
                        $interfaces[$stringableClassName] = $stringableInterfaceReflection;
                    }

Obviously, the symfony/polyfill-php80 is not an internal class, and therefore it is skipped. The trouble is that Reflector#reflectClass() finds the one under symfony/polyfill-php80 before looking at the internal source locators.

This could potentially be fixed by sorting the source locators differently (currently trying that).

NOTE: it is OK to ignore Stringable if it didn't come from an internal source locator - that's because nobody knows what nightmarish contraption people may have invented before PHP 7.x.

[Ocramius]

@weirdan can you review #431 please? Should be fairly simple :)

[weirdan]

Thanks!