Skip to content

Report invalid constructor property promotion for readonly props already defined in parent class constructor#1788

Closed
staabm wants to merge 3 commits intophpstan:1.8.xfrom
staabm:bug8101
Closed

Report invalid constructor property promotion for readonly props already defined in parent class constructor#1788
staabm wants to merge 3 commits intophpstan:1.8.xfrom
staabm:bug8101

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Oct 3, 2022

$node->getName(),
))->nonIgnorable()->build();
} else {
$errors[] = RuleErrorBuilder::message(sprintf(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to come up with a similar example for the static case above this IF-block, therefore only added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just read about 3e383fc which resolves my questions, as readonly properties cannot be static.

@staabm staabm marked this pull request as ready for review October 3, 2022 16:51
@staabm
Copy link
Contributor Author

staabm commented Oct 3, 2022

@herndlm do we need a similar fix for phpdoc based readonly properties?

@herndlm
Copy link
Contributor

herndlm commented Oct 3, 2022

@herndlm do we need a similar fix for phpdoc based readonly properties?

hmm, the phpdoc-based-readonly rules are basically duplicates of the native readonly ones. but there is no equivalent for OverridingPropertyRule and it does not deal with phpdoc-based-readonly yet. The question is then if this is needed. And if yes if it should be added here directly (most likely just (via isReadOnlyByPhpDoc()) or in a dedicated rule 🤔 but either way, nothing you have to do here IMO :)

@ondrejmirtes
Copy link
Member

I can't merge this, because it's allowed by PHP: https://3v4l.org/jPiT7

See: phpstan/phpstan#8101 (comment)

We have to come up with a more clever fix for this. To realize that the property is already assigned (it's promoted) when calling the constructor.

@staabm staabm deleted the bug8101 branch October 21, 2022 15:28
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