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

Fix properties for XML-related classes #1526

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Wirone
Copy link

@Wirone Wirone commented Feb 6, 2023

These properties are real (non-dynamic):
https://github.com/php/php-src/blob/master/ext/xmlreader/php_xmlreader.stub.php https://www.php.net/manual/en/class.domnamednodemap.php

Current usage of @property-read annotations makes PHPStan fail on PHP 8.2.

Related to phpstan/phpstan#8629.

These properties are real (non-dynamic):
https://github.com/php/php-src/blob/master/ext/xmlreader/php_xmlreader.stub.php
https://www.php.net/manual/en/class.domnamednodemap.php

Current usage of `@property-read` annotations makes PHPStan fail on PHP 8.2.
@isfedorov
Copy link
Contributor

That's true, they are real, but only for PHP >= 8.1 (https://3v4l.org/V82V3 and https://github.com/php/php-src/blob/PHP-8.0/ext/xmlreader/php_xmlreader_arginfo.h), and stubs should be valid also for older PHP versions that's why tests will fail in this case. Also, @readonly tag isn't supported by phpDocumentor and is phpstan/psalm specific, and PhpStorm doesn't rely on this tag during an inspection run thus, such change will remove some of the valid warnings. We could add real read-only property with an attribute like below

class XMLReader
{
    #[PhpStormStubsElementAvailable(from: 8.1)]
    public readonly int $attributeCount;
}

But in this case, we would get multi-resolve because there is another declaration in property-read that doesn't have since/to versions. So looks like to fix this problem at the moment, we have to add a copy of classes marked with @since/@removed versions in PhpDoc so that class with dynamic properties has @since 5.3/@removed 8.1 and class with real properties @since 8.1

@Wirone
Copy link
Author

Wirone commented Feb 7, 2023

Also, @readonly tag isn't supported by phpDocumentor and is phpstan/psalm specific

I basically took it from other stub: pq/pq.php 🤷‍♂️

Thanks for the response, I'll prepare changes today.

@Wirone
Copy link
Author

Wirone commented Feb 7, 2023

@isfedorov I've prepared changes, please let me know if it's what you were thinking of. I decided to extract DOMNamedNodeMap to separate file in order to not replicate all other DOM classes, I hope it can be loaded properly.

One question: should I remove #[LanguageLevelTypeAware] and #[TentativeType] from definitions for PHP>=8.1? Because since there's @since 8.1 the types are always the same.

But on a side note I have to ask: does it really matter from the code's perspective? 🤔 Those properties were not defined explicitly and were dynamic, but they were there for reading. So basically in the users' code, if e.g. $reader->depth was used, from static analysis perspective it is exactly the same if it's @property-read annotation or actual property.. The only difference would be for code that is operating on reflection level. Is this worth complicating with separate definitions?

@Wirone
Copy link
Author

Wirone commented Feb 17, 2023

@isfedorov friendly reminder 🙂 Those changes are the last PHPStan errors in our App, which we're migrating to PHP8.2, and I really would like to proceed with the process.

@isfedorov
Copy link
Contributor

@Wirone

One question: should I remove #[LanguageLevelTypeAware] and #[TentativeType] from definitions for PHP>=8.1? Because since there's @since 8.1 the types are always the same.

Yes, generally those attributes can be removed for the version for PHP >=8.1.

Those properties were not defined explicitly and were dynamic, but they were there for reading. So basically in the users' code, if e.g. $reader->depth was used, from static analysis perspective it is exactly the same if it's @property-read annotation or actual property..

There are next differences here: PhpStorm highlights write access to property marked with @property-read as error but doesn't highlight write access to regular property with error (it doesn't take into account @readonly tag since it's "unofficial" tag) unless the property has attribute readonly since PHP 8.1

Screenshot 2023-02-20 at 15 41 53

But we can't just convert @property read to public readonly because in this case there will be an error for those who overrides the property with language level < 8.1
Screenshot 2023-02-20 at 15 49 39
That's why we need two versions of class to define properties properly. But actually even in case we declare those classes with since/removed tag we still have a problem that the class usage will be highlighted with Multiple class declaration inspection (filled WI-71469 to fix this) that can be quite noisy, that's why I'm not sure if it makes sense to perform such changes until WI-71469 is fixed as there is a chance that users will be disturbed by MultipleClassDeclaration inspection warning for the sake of fixed PhpStan warning.

@dbu
Copy link

dbu commented Sep 19, 2023

any chance to wrap this up? (coming here after following up phpstan false alert on xmlreader->nodeType with PHP 8.2)

can we maybe do a simple change to convert the the property annotations to property declarations but leave out the complicated problems that require further discussion?

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 this pull request may close these issues.

3 participants