Skip to content

#429 polyfills should not override reflected internal PHP symbols #431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented Feb 4, 2022

Fixes #429

As discovered in #429 by @weirdan, the Stringable interface is not added to symbols, when
the interface is declared by a vendor/ package.

This leads to the AncestorRemoved BC checker (correctly) kicking in, and reporting BC breaks:

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 <[email protected]>
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"

As discovered by @Ocramius ( #429 (comment) )
the problem is that symfony/polyfill-php80 declares a non-internal Stringable interface, and BetterReflection
rightfully does NOT use it when deciding whether to add Stringable to a class implementing __toString(), because
only an internal PHP class would be a valid candidate.

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.

With this commit, we put the example in a reproducible test scenario

…nternal symbols

As discovered in #429 by @weirdan, the `Stringable` interface is not added to symbols, when
the interface is declared by a `vendor/` package.

This leads to the `AncestorRemoved` BC checker (correctly) kicking in, and reporting BC breaks:

> 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:
>
> ```diff
> commit 4cc0571866e8405658117613a8106f880116decb (HEAD -> add-bcc-2)
> Author: Bruce Weirdan <[email protected]>
> 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"
> ```

As discovered by @Ocramius ( #429 (comment) )
the problem is that `symfony/polyfill-php80` declares a non-internal `Stringable` interface, and `BetterReflection`
rightfully does **NOT** use it when deciding whether to add `Stringable` to a class implementing `__toString()`, because
only an internal PHP class would be a valid candidate.

> 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.

With this commit, we put the example in a reproducible test scenario
@Ocramius Ocramius added the bug label Feb 4, 2022
@Ocramius Ocramius added this to the 6.1.1 milestone Feb 4, 2022
@Ocramius Ocramius self-assigned this Feb 4, 2022
@Ocramius Ocramius changed the title #429 - polyfills should be detected **after** defined internal symbols #429 polyfills should not override reflected internal PHP symbols Feb 4, 2022
…talled.json` dependencies

In PHP, internal symbols always take priority, since the engine already declared: by moving the `PhpInternalSourceLocator`
higher up in the list of aggregated source locators, we prevent internal PHP symbols from being shadowed by their
corresponding polyfills (if any are declared).

This simple change, for example, prevents `symfony/polyfill-php80` from declaring `Stringable` in the context of reflection,
when `Stringable` is already declared by the engine.

Fixes #429
…the `AggregateSourceLocator` produced by `LocateDependenciesViaComposer`

These assertions were quite fragile to start with.
@Ocramius Ocramius merged commit 3487ca1 into 6.1.x Feb 4, 2022
@Ocramius Ocramius deleted the fix/#429-ensure-internal-source-locator-considered-before-vendor-polyfills branch February 4, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding Stringable polyfill causes false positive (Stringable ancestor removed)
2 participants