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

Assign readonly property value using list is not handled #7119

Closed
niconoe- opened this issue Apr 26, 2022 · 5 comments · Fixed by phpstan/phpstan-src#1405
Closed

Assign readonly property value using list is not handled #7119

niconoe- opened this issue Apr 26, 2022 · 5 comments · Fixed by phpstan/phpstan-src#1405
Labels
Milestone

Comments

@niconoe-
Copy link
Contributor

Bug report

When using the list syntax or its short version to assign values to readonly properties in a constructor, the assignation is not taken into account, and this creates a false positive.

Code snippet that reproduces the problem

Here is an example of snippet that throws an error

Expected output

No error.

Did PHPStan help you today? Did it make you happy in any way?

PHPStan is helping me everyday and I'll never thanks enough people who are making this better and better everyday! A huge THANK YOU!

@ondrejmirtes
Copy link
Member

FYI @herndlm This one is worth investigating. The thing that we should take advantage of is that array destructuring is correctly handled in TypesAssignedToPropertiesRule so the readonly check should take advantage of PropertyAssignNode (ClassStatementsGatherer uses it to create PropertyWrite so I don't really know what's wrong). The array destructuring probably creates PropertyRead too which isn't desired.

@herndlm
Copy link
Contributor

herndlm commented Jun 8, 2022

Yeah, sounds like you're guess is correct. ClassStatementsGatherer basically finds the PropertyFetch but does not exit early in https://github.com/phpstan/phpstan-src/blob/1.7.11/src/Node/ClassStatementsGatherer.php#L186 since Scope::isInExpressionAssign returns false and then it creates the PropertyRead.

I'm not entirely sure yet how to fix this, let me know if you have an idea already. Currently I'm looking into passing around enterExpressionAssign and calling Scope::enterExpressionAssign in NodeScopeResolver since doing this for the outer expression is not enough. I have the feeling that there is a better way to "remember" that we're currently in an assignment expression, but not sure yet.

@ondrejmirtes
Copy link
Member

Yes, that's probably the right way :)

@niconoe-
Copy link
Contributor Author

niconoe- commented Jun 8, 2022

Thank you very much for taking care of this! You rock!

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants