Check redeclared readonly properties#3041
Conversation
5a3d08c to
5d6a482
Compare
|
Some ideas (I haven't seen the code yet):
I'd accept a PR that does the first bullet point. I think this PR should be that. The second bullet point can be a subject of a future PR. What's your opinion? |
|
Ok, first bullet point seems good. It should already catch many issues. I'll try to implement that. Second bullet-point: I'm not convinced that it's necessary to be so permissive. PHPStan requires the parent to assign the property in the constructor. So IMO it makes sense to assume that it is assigned. As for the possibility of detecting the property assigns in the parent class: I guess it could be useful for use-cases like this: https://phpstan.org/r/56181865-dda9-4e35-bf25-571ef2deb066 ( |
You can't assume that. If someone ignores that error then the property is not assigned in the constructor. I feel like opinionated rules like this shouldn't intervene with each other, it makes the tool less usable. I feel like https://phpstan.org/r/56181865-dda9-4e35-bf25-571ef2deb066 is a useful error, if we omit the possibility to extend the class which can initialize the property before calling
I feel that if we can solve something with collectors, we shouldn't bother the user with new PHPDoc tags. |
Fixes phpstan/phpstan#8101
Readonly properties in PHP work as follows:
PHPStan enforces that readonly properties are assigned in the constructor right from level 0 (
MissingReadOnlyPropertyAssignRule). Therefore, I propose that we prohibit redeclaring a readonly property and simultaneously calling a constructor of the parent which also declares it, or any of its children (they may call their parent's constructor and so on).It could technically be allowed to both redeclare a readonly property and call the parent constructor, as long as the property is not assigned anywhere in the class.
However, I cannot think of a reason why anyone would need to redeclare a readonly property, other than to assign it in the child class, instead of in the parent. Therefore, I didn't add an exception for this case.There is a use-case: adding attributes to the property (phpstan/phpstan#9864).The rule will still reject some valid edge-cases, but IMO it is on par with
MissingReadOnlyPropertyAssignRule. So I also added it as part of level 0 (which strictly speaking breaks the BC promise, but so didMissingReadOnlyPropertyAssignRule- it might need to have an exception for rules which detect code which is highly likely to lead to a runtime error).Additionally, I would like to ask you whether you'd be willing to prohibit redeclaring readonly properties completely (i.e. #1788) as part of strict-rules. My reasoning is as follows:
This would give additional safety for those of us who do not need to redeclare readonly properties, because the general rule that I implemented does not cover everything (e.g. the constructor call could be hidden somehow).