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/undefined expression allowed #1174

Merged

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Apr 4, 2022

fixes phpstan/phpstan#6107
fixes phpstan/phpstan#5971
fixes phpstan/phpstan#5337
fixes phpstan/phpstan#6899

This PR introduces a new method isUndefinedExpressionAllowed to Scope interface and new property currentlyAllowedUndefinedExpression of MutatingScope.

Currently, access to undefined property is allowed in assign. In Isset_ and Empty_ it is simulating an assign by lookForEnterExpressionAssign to avoid undefined propeties, but it changes from read access to write access which are causing false positives/negatives like issues in above.
The purpose of this feature is to separate isInExpressionAssign and undefined property to avoid those issues.
Also, this feature can make allowing/disallowing undefined expressions much more flexibly like 59d89a5
(this line is added for backwards compatibility that Coalesce doesn't allow dynamic properties)
and may be easier to make strict analysis after #[AllowDynamicProperties] is introduced in PHP 8.2 :)
#1104 (comment)

@rajyan
Copy link
Contributor Author

rajyan commented Apr 4, 2022

@ondrejmirtes
Copy link
Member

I'm blown away by this, it's awesome! Additionally, I love when people start working on a change X (#1104), realize that changeset Y can be contributed separately (#1174) and once it's merged, they go back to implementing X again :)

I'm ready to merge this, I just want to ask you to add regression tests for the linked issues you know this fixes.

@ondrejmirtes
Copy link
Member

Also - consider basing this on 1.5.x since it can be considered a bugfix for all those issues.

@rajyan
Copy link
Contributor Author

rajyan commented Apr 4, 2022

I'm blown away by this, it's awesome! Additionally, I love when people start working on a change X (#1104), realize that changeset Y can be contributed separately (#1174) and once it's merged, they go back to implementing X again :)

I'm happy to hear your comments! I think I need two more fixes and can finally complete #1104 :)

@rajyan
Copy link
Contributor Author

rajyan commented Apr 4, 2022

I'm ready to merge this, I just want to ask you to add regression tests for the linked issues you know this fixes.

I'll fix and add tests and do some small refactoring today. I'll mark it ready when it's finished!

@rajyan
Copy link
Contributor Author

rajyan commented Apr 5, 2022

The failing integration test is from json_decode + checkExplicitMixed (property access is not allowed in strict mixed). I'm not sure how to solve this problem as in phpstan/phpstan#1874
but I think this PR #993 may solve the problem.

@rajyan rajyan force-pushed the feature/undefined-expression-allowed branch from cd29f92 to 641ea52 Compare April 5, 2022 00:20
@rajyan rajyan changed the base branch from 1.6.x to 1.5.x April 5, 2022 00:20
@rajyan rajyan force-pushed the feature/undefined-expression-allowed branch from 641ea52 to db78bf5 Compare April 5, 2022 00:25
@rajyan rajyan marked this pull request as ready for review April 5, 2022 00:32
@rajyan
Copy link
Contributor Author

rajyan commented Apr 5, 2022

Rebased and added some regression tests. This PR is ready for review!
The failing test is related to this PR, but not sure how to fix.
#1174 (comment)

@rajyan
Copy link
Contributor Author

rajyan commented Apr 5, 2022

I'll fix this one next

[
'Cannot access property $prop on object|string.', // open issue https://github.com/phpstan/phpstan/issues/3659, https://github.com/phpstan/phpstan/issues/6026
25,
],
(this feature makes easier to fix the issue :))
and fix TypeSpecifier for Isset_. Then complete #1104 👍

@ondrejmirtes
Copy link
Member

Awesome, thank you!

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