Skip to content
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

Psalm acts differently depending on what version of PHP it is executed on. #6305

Closed
greg0ire opened this issue Aug 14, 2021 · 8 comments · Fixed by #6310
Closed

Psalm acts differently depending on what version of PHP it is executed on. #6305

greg0ire opened this issue Aug 14, 2021 · 8 comments · Fixed by #6310

Comments

@greg0ire
Copy link
Contributor

greg0ire commented Aug 14, 2021

Context: I'm trying to fix an error that does not happen in the CI of doctrine/orm, but does happen on my machine where I use PHP 7.4.

I tried fixing it with an assert, but now I get the opposite situation: doctrine/orm#8917

I cannot reproduce the issue on psalm.dev, that presumably does not run PHP 8

https://psalm.dev/r/a8f882ff61

On a machine with PHP 8, as well as on the CI, I know get this:

ERROR: RedundantConditionGivenDocblockType - lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:912:13 - Docblock-defined type SimpleXMLElement for $action is never null (see https://psalm.dev/156)
            assert($action !== null);


ERROR: RedundantConditionGivenDocblockType - lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:912:20 - Docblock-defined type SimpleXMLElement ca never contain null (see https://psalm.dev/156)
            assert($action !== null);

Either way, there is a difference in results depending on the version of PHP, and hardcoding phpVersion to 7.1 in psalm.xml does not help. I'm not sure what to try next.

See also: https://symfony-devs.slack.com/archives/C8SFXTD2M/p1628964600095200

@psalm-github-bot
Copy link

psalm-github-bot bot commented Aug 14, 2021

I found these snippets:

https://psalm.dev/r/a8f882ff61
<?php

function bar(SimpleXMLElement $a): void {
    foreach ($a->children() as $child) {
        assert($child !== null);
    }
}
Psalm output (using commit 4c26293):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Aug 14, 2021

Steps to reproduce:

gh repo clone doctrine/orm
cd orm/
gh pr checkout 8917
phpenv local 8.0.7
composer install
vendor/bin/psalm lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php
phpenv local system # 7.4.21
composer update -W
vendor/bin/psalm --clear-cache
vendor/bin/psalm lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php

@orklah
Copy link
Collaborator

orklah commented Aug 14, 2021

Thanks for digging into this @greg0ire !

@weirdan
Copy link
Collaborator

weirdan commented Aug 14, 2021

Under 8.0, $action is inferred as SimpleXMLElement, while under 7.4 it's thought to be SimpleXMLIterator|null

@orklah
Copy link
Collaborator

orklah commented Aug 14, 2021

It's unfortunate, we actually have a test case with a foreach loop over SimpleXmlElement::children, but it doesn't reproduce the issue...

@weirdan
Copy link
Collaborator

weirdan commented Aug 14, 2021

I think this may have to do with inheritance change in 8.0. In 8.0 SimpleXMLIterator::current() is inherited from SimpleXMLElement, and in 7.4 it's not.

@weirdan
Copy link
Collaborator

weirdan commented Aug 14, 2021

Yep. Stubbing SimpleXMLIterator as class SimpleXMLIterator extends SimpleXMLElement {} makes the error consistently appear for both 8.0 and 7.4.

Context: https://externals.io/message/108789 and php/php-src#5234

Still not sure what would be the best way to address this.

@greg0ire
Copy link
Contributor Author

Shouldn't this be changed to SimpleXMLElement|null:

psalm/stubs/DOM.phpstub

Lines 96 to 97 in 4c26293

/** @return SimpleXMLIterator */
public function children(?string $namespaceOrPrefix = null, bool $isPrefix = false)
(like in the docs at https://www.php.net/manual/en/class.simplexmlelement.php?)

weirdan added a commit to weirdan/psalm that referenced this issue Aug 15, 2021
Historically, `SimpleXMLIterator` had only implemented `Iterator` and
`RecursiveIterator` methods. This changed in 8.0, when iterator methods
were moved to `SimpleXMLElement`, and `SimpleXMLIterator` was made a
dummy class extending `SimpleXMLElement`.

Fixes vimeo#6305, in the sense that Psalm would no longer report
different errors depending on the runtime PHP version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants