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

Feature/disallow dynamic property option #1234

Merged

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Apr 20, 2022

Implemented bool option in NodeScopeResolver as mentioned in #1223 (comment) to optionally restore the behavior before #1223.
Strictly, it is not exactly the same behavior as before because it disallows dynamic properties in isset and empty too, but I believe making them consistent is better.

Also, fixes phpstan/phpstan#3171 because it's related.

@rajyan
Copy link
Contributor Author

rajyan commented Apr 20, 2022

I'll add some tests for the feature.

@rajyan
Copy link
Contributor Author

rajyan commented Apr 20, 2022

It might be better to make this as an option for AccessPropertiesRule and AccessStaticPropertiesRule?

@ondrejmirtes
Copy link
Member

I worry that this logic needs to be deep in NodeScopeResolver, or not?

If not then yeah, parameter for a rule is nicer.

@rajyan rajyan force-pushed the feature/disallow-dynamic-propertiy-option branch from 1ab1af1 to e64bc9a Compare April 21, 2022 00:40
@rajyan
Copy link
Contributor Author

rajyan commented Apr 21, 2022

I worry that this logic needs to be deep in NodeScopeResolver, or not?

I think it's possible now. currentlyAllowedUndefinedExpressions in NodeScopeResolver is saying that dynamic property inside isset is a valid PHP with no error. I believe it would be valid even after dynamic properties is deprecated/removed after PHP 8.2. AccessPropertiesRule can check the currentlyAllowedUndefinedExpressions and change the behavior by $checkDynamicProperties like this, which I think makes more sense 👍

@rajyan
Copy link
Contributor Author

rajyan commented Apr 21, 2022

While this option can optionally restore the behavior before #1223 (comment), it would restore the false positive phpstan/phpstan#3171 too.
It's because AccessPropertiesRule is currently treating Class|null and dynamic property on Class (which are both $type->canAccessProperties()->maybe()) in the same way. It's a different issue and I'll fix it separately.

Should be fixed by specifying types in isset.
Related issue phpstan/phpstan#3171
@ondrejmirtes
Copy link
Member

Thank you! Can you please send an update to https://phpstan.org/config-reference#stricter-analysis as well? There's an "Edit this page" link at the bottom :)

I'll update phpstan-strict-rules so that the playground with strict rules + bleedingEdge enabled will have this option on.

@ondrejmirtes
Copy link
Member

@ondrejmirtes
Copy link
Member

I'm testing this in the real world and it's really nicely done! The impact is really minimal and exactly in the right weird places where I'd expect it 👍

@rajyan rajyan deleted the feature/disallow-dynamic-propertiy-option branch April 21, 2022 13:33
@rajyan
Copy link
Contributor Author

rajyan commented Apr 21, 2022

Thanks! I'm glad that I could help solving dynamic properties related issues 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants