Asymmetric @property types - thinner version #2327
Conversation
| private ?Type $readableType, | ||
| private ?Type $writableType, |
There was a problem hiding this comment.
One downside compared to #2294 is that sharing PropertyTag makes it impossible to e.g. inherit @property definition and then override @property-write in the subclass. (Though I did not bother implementing it yet.)
| { | ||
| assertType('int', $this->asymmetricPropertyRw); | ||
| assertType('int', $this->asymmetricPropertyXw); | ||
| assertType('int|string', $this->asymmetricPropertyRx); |
There was a problem hiding this comment.
This does not look right. The @property should be shadowed by @property-read.
There was a problem hiding this comment.
To me it doesn't make sense to allow both @property and @property-read. My expectation is that @property overwrites everything.
There was a problem hiding this comment.
I would expect the opposite – the more specific @property-read and @property-write should overwrite the less specific @property (at least within the context of a single class). In fact, that was the behaviour before this PR.
There was a problem hiding this comment.
Oh, please send an updating PR that restores this behaviour, and adds some tests for it. Thank you!
There was a problem hiding this comment.
I do not care that much either way – converting my existing @property tags to @property-read when adding @property-write tags is only mildly annoying. Though, if we are counter-intuitively ignoring the more specific tags, we might want to add a check that warns that there are conflicting tags.
There was a problem hiding this comment.
Because the tags are indexed by name, the check cannot really easily be written.
There was a problem hiding this comment.
But backward compatibility is important to me so please research it and send a PR.
| return new AnnotationPropertyReflection( | ||
| $declaringClass, | ||
| TemplateTypeHelper::resolveTemplateTypes( | ||
| $propertyTags[$propertyName]->getType(), | ||
| $propertyTag->getReadableType() ?? $defaultType, |
There was a problem hiding this comment.
Another point of difference from #2294, this will continue to return an incorrect readable type when the property is not readable. It would be more elegant if we used VoidType as the fallback type – then instead of checking if the property is readable/writeable and checking if a value can inhabit the type, we could simply check the value, since void is a bottom type that cannot be inhabited by any value.
There was a problem hiding this comment.
That's NeverType, not VoidType. Yeah, I didn't understand that change. Feel free to update that scenario to NeverType, thank you.
Closes phpstan/phpstan#9062