TypeDoesNotContainNull: on return value from dom_import_simplexml()

[KevinVanSonsbeek]

When using the dom_import_simplexml() function, psalm gives a TypeDoesNotContainNull warning when performing a null check on the return value. Psalm still indicates the return type to be DOMElement|false while the actual return type is DOMElement|null.

Example: https://psalm.dev/r/42bcd39f32

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/42bcd39f32

<?php

class Foo {
 	public function test(\SimpleXMLElement $xmlElement): \SimpleXMLElement
    {
        $domElement = dom_import_simplexml($xmlElement);
        
        if (null === $domElement) {
            throw new \LogicException('Error');
        }
        
        return $domElement;
    }
}

Psalm output (using commit a2c87b8):

ERROR: TypeDoesNotContainNull - 8:13 - DOMElement|false does not contain null

ERROR: InvalidReturnStatement - 12:16 - The inferred type 'DOMElement|false' does not match the declared return type 'SimpleXMLElement' for Foo::test

ERROR: InvalidReturnType - 4:56 - The declared return type 'SimpleXMLElement' for Foo::test is incorrect, got 'DOMElement|false'

[weirdan]

while the actual return type is DOMElement|null

I'm not sure about that - can you devise a reproducer showing null returned in PHP 8+? For older versions it should be changed to |null, as the code returning null was there since about 18 years ago.

[KevinVanSonsbeek]

Not entirely sure how i can make a reproducer. But going through the source code i did notice that going from 7.4 to 8.0 the return has been changed.
From a RETURN_NULL() to a RETURN_THROWS(). Oddly the dom stub shows the return type as ?DOMElement The stub has been updated on the master branch to be just the DOMElement as the return type. And has been pushed to the php-8.0 branch.

[KevinVanSonsbeek]

But from what i can see, the function can now only return a DOMElement. And it will throw a ValueError on failure. (ArgumentCountError when not enough arguments are supplied)

[orklah]

Indeed, the function doesn't seem to have ever returned false. The return null was kinda legit before 8.0 but was only returned when wrong params were send. Psalm already reports wrong params so adding the null return type would be redundant.

So, all that is needed is to remove false from the type in the callmaps.

We actually have a doc page if you want to contribute on that: https://psalm.dev/docs/contributing/editing_callmaps/

Care for a PR? :)

[KevinVanSonsbeek]

PR is open