Mark DOMNamedNodeMap as taking a covariant DOMNode

[Girgias]

And indicate that DOMNode::$attributes is a node map of DOMAttr.

Not sure how to write a test, nor if this is where I need to amend the stubs. But happy to be guided.

[Girgias]

Okay, I am seeing that the stubs are pulled from: https://github.com/phpstan/php-8-stubs, but how can I augment those?

[ondrejmirtes]

What bug are you trying to fix? With a phpstan.org/try sample I can direct you to the right test file.

[Girgias]

I've reduced some of my code to showcase the issue: https://phpstan.org/r/2ebd6b8b-17d6-47c3-b6e1-f1705c98460e

I am trying to fix the undefined property access to $value (Access to an undefined property DOMNode::$value) because it should "know" that the $attributes property of DOMNode is a node map of DOMAttr

[ondrejmirtes]

So you can verify the "property not found" is no longer reported with a new test in: https://github.com/phpstan/phpstan-src/blob/2.0.x/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php

I'd also add a new file in https://github.com/phpstan/phpstan-src/tree/2.0.x/tests/PHPStan/Analyser/nsrt which gets picked up by tests/PHPStan/Analyser/NodeScopeResolverTest.php with some assertType calls to verify $type->attributes is of the you expect it to be. (See other files in that directory for examples.)

Also there should be a test that tests iterating over DOMNamedNodeMap gives the right type.

Also when I check https://github.com/phpstan/php-8-stubs/blob/main/stubs/ext/dom/DOMNamedNodeMap.php the TNode in the stub could be used as a return type for some of these methods.

Thanks! :)

[Girgias]

I cannot get the tests to run, nor pass localy. Trying to run all of them uses I don't know how much memory, and I cannot run the tests/PHPStan/Analyser/nsrt/DOMLegacyNamedNodeNap.php tests on its own. And I still don't understand why the stub changes don't seem to be taken into account.

[herndlm]

@Girgias you can either run /vendor/bin/phpunit directly (= without paratest = less initial memory usage) or, what Ondřej basically said, run vendor/bin/phpunit tests/PHPStan/Analyser/NodeScopeResolverTest.php which will also assert the types of the new file you put into the nsrt folder.

[Girgias]

@Girgias you can either run /vendor/bin/phpunit directly (= without paratest = less initial memory usage) or, what Ondřej basically said, run vendor/bin/phpunit tests/PHPStan/Analyser/NodeScopeResolverTest.php which will also assert the types of the new file you put into the nsrt folder.

Right, (although needing to run 1050 tests at a time is somewhat suboptimal). But I still cannot get PHPStan to grab updated stubs as that is completely opaque.

[Girgias]

The fundamental issue seems to be that there is no way to overwrite the php-src stubs for PHP 8, which are better, but far from perfect as they do not have generic types.

I'm happy to update the corresponding repo, but that doesn't seem like the best idea. I've submitted a PR to phpstorm-stubs but they also don't seem to be well-equipped to deal with template types.

[ondrejmirtes]

I've adjusted the tests a bit:

1. Only the /nsrt/ files are checked for assertType calls correctness.
2. In AccessPropertiesRuleTest, only errors reported by AccessPropertiesRule are verified, not assertType calls.

So now there are these failures in the /nsrt/ file:

Line 15:
Expected: DOMNamedNodeMap<DOMAttr>
Actual:   DOMNamedNodeMap<DOMAttr>|null - not surprising, there's @var DOMNamedNodeMap<DOMAttr>|null above the property in the stub

Line 17:
Expected: DOMAttr
Actual:   DOMAttr|null

Not surprising, there's no code that would connect $node->hasAttribute
with $node->attributes->getNamedItem non-nullability or vice-versa.
I don't even think it's possible to do that in PHPStan with just PHPDocs.
Sure, $node->hasAttribute and $node->getAttribute would be possible to connect like that,
but not $node->hasAttribute and a nested object, that's currently not supported by `@phpstan-assert-if-true`.

Line 19:
Expected: null
Actual:   DOMAttr|null

Same problem as on line 17.

Line 25:
Expected: DOMNamedNodeMap<DOMAttr>
Actual:   DOMNamedNodeMap<DOMAttr>|null - exactly the same test as on line 15.

Line 30:
Expected: DOMNamedNodeMap<DOMAttr>
Actual:   DOMNamedNodeMap

You're probably expecting the DOMNode::$attributes stub to take effect here too,
but it won't because the $attributes property is redeclared in DOMElement in phpstorm-stubs.

In order for this assert to pass, just declare the $attributes property the same way in DOMElement stub.

Line 35:
Expected: DOMAttr
Actual:   DOMNode

Doesn't pass, because line 30 doesn't pass.

Line 36:
Expected: string
Actual:   *ERROR*

Unknown property at this point.

[Girgias]

So now PHPStan is narrowing too much in DOMElement for seemingly no reason. >->

[ondrejmirtes]

The tests are only failing on PHP 8.1+ because thanks to phpstorm-stubs it's known the type cannot be null on 8.1+:

    /**
     * @var DOMNamedNodeMap
     * A <classname>DOMNamedNodeMap</classname> containing the attributes of this node (if it is a <classname>DOMElement</classname>) or NULL otherwise.
     * @link https://php.net/manual/en/class.domnode.php#domnode.props.attributes
     */
    #[LanguageLevelTypeAware(['8.1' => 'DOMNamedNodeMap'], default: '')]
    public $attributes;

To have different assertions between PHP versions, you need to use // lint ... at the top of the file in /nsrt/. So ideally you'd have file without // lint for the assertions that are the same before/after 8.1, and then you'd have two different files for things that are different before/after 8.1, and mark them both with different // lint at the top of the file.

For example array_column has different assertions for PHP 7, PHP 8, and PHP 8.2+. Seee array-column-php7.php and similar.

[Girgias]

The tests are only failing on PHP 8.1+ because thanks to phpstorm-stubs it's known the type cannot be null on 8.1+:

    /**
     * @var DOMNamedNodeMap
     * A <classname>DOMNamedNodeMap</classname> containing the attributes of this node (if it is a <classname>DOMElement</classname>) or NULL otherwise.
     * @link https://php.net/manual/en/class.domnode.php#domnode.props.attributes
     */
    #[LanguageLevelTypeAware(['8.1' => 'DOMNamedNodeMap'], default: '')]
    public $attributes;

To have different assertions between PHP versions, you need to use // lint ... at the top of the file in /nsrt/. So ideally you'd have file without // lint for the assertions that are the same before/after 8.1, and then you'd have two different files for things that are different before/after 8.1, and mark them both with different // lint at the top of the file.

For example array_column has different assertions for PHP 7, PHP 8, and PHP 8.2+. Seee array-column-php7.php and similar.

Right, the php.net docs were leading me to believe it could be null ... I asked Niels, and he confirmed that it never can be null. I do hate old DOM.

(aside: I updated my project to new DOM and phpstan complains a hella less as it is better organized and has more type info).

[herndlm]

btw @Girgias regarding test runs - I didn't want to be annoying by mentioning the obvious things, but you can always do something like the following to only run your test file and avoid having to wait for all the others and such:

--- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php
+++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php
@@ -25,6 +25,9 @@ class NodeScopeResolverTest extends TypeInferenceTestCase
         */
        private static function findTestFiles(): iterable
        {
+               yield __DIR__ . '/nsrt/DOMLegacyNamedNodeNap.php;
+               return;
+
                foreach (self::findTestDataFilesFromDirectory(__DIR__ . '/nsrt') as $testFile) {
                        yield $testFile;
                }

at least that's how I work regularly, also when I step-debug through some type issues in a specific case and such.

oh, and another thing - for a fix like this, if it would benefit users of PHPStan 1, it might be better to target the still supported 1.12.x branch in the end I suppose. But that's up to you and Ondřej in the end of course.

[Girgias]

btw @Girgias regarding test runs - I didn't want to be annoying by mentioning the obvious things, but you can always do something like the following to only run your test file and avoid having to wait for all the others and such:

--- a/tests/PHPStan/Analyser/NodeScopeResolverTest.php
+++ b/tests/PHPStan/Analyser/NodeScopeResolverTest.php
@@ -25,6 +25,9 @@ class NodeScopeResolverTest extends TypeInferenceTestCase
         */
        private static function findTestFiles(): iterable
        {
+               yield __DIR__ . '/nsrt/DOMLegacyNamedNodeNap.php;
+               return;
+
                foreach (self::findTestDataFilesFromDirectory(__DIR__ . '/nsrt') as $testFile) {
                        yield $testFile;
                }

at least that's how I work regularly, also when I step-debug through some type issues in a specific case and such.

oh, and another thing - for a fix like this, if it would benefit users of PHPStan 1, it might be better to target the still supported 1.12.x branch in the end I suppose. But that's up to you and Ondřej in the end of course.

Oh you should have, it's not because I work on PHP Core that I think of the obvious things to do.

I don't mind backporting, if I'm explained the process, do I target the lower branch first and merge up or is it cherry-picking?

But I still do not understand how PHPStan deals with stubs, the DOMElement::getAttributeNode() stub is wrong for PHPStorms, and trying to overwrite it in the PHPStan stubs seems to do nothing.

[ondrejmirtes]

You mean the line public function getAttributeNode(string $qualifiedName): DOMAttr|DOMNameSpaceNode|false {} you added?

PHPStan's stubs can only override PHPDocs. For native types, phpstorm-stubs and php-8-stubs are treated as the truth without possible workarounds.

[herndlm]

And regarding backporting: the lower branch is targeted (branch would need a rebase and 2.x commits removed), Ondřej regularly merges it up.

[Girgias]

My PR on the PHPStorm stubs has landed, would it be better to just update those instead?

[ondrejmirtes]

I have a weekly workflow that updates phpstorm-stubs here. I just ran it early: #3733

So you can now update 2.0.x and see the effects of your changes there. And you can decide whether this PR is still relevant or not. Thanks.