Skip to content

Feature/undefined expression allowed #1174

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.

Sign up for GitHub

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

Jump to bottom
Merged
ondrejmirtes merged 8 commits into from
Apr 5, 2022
Merged

Feature/undefined expression allowed #1174

ondrejmirtes merged 8 commits into from
Apr 5, 2022

Conversation

@rajyan
Copy link
Contributor

@rajyan rajyan commented Apr 4, 2022 •
edited
Loading

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 •
edited
Loading

memo for failing test https://github.com/pmmp/PocketMine-MP/blob/e0b07ff3087b652407439a29c941f3b66ca92c86/src/pocketmine/Server.php#L2105-L2109

@ondrejmirtes
Copy link
Member

ondrejmirtes 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 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

ondrejmirtes commented Apr 4, 2022

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

I'll fix this one next

phpstan-src/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php

Lines 573 to 576 in db78bf5

[
'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

ondrejmirtes commented Apr 5, 2022

Awesome, thank you!

@ondrejmirtes ondrejmirtes merged commit 356250a into phpstan:1.5.x Apr 5, 2022
This was referenced Apr 5, 2022
Closed
Closed
Closed
@rajyan rajyan deleted the feature/undefined-expression-allowed branch April 5, 2022 02:49
@rajyan rajyan mentioned this pull request Apr 5, 2022
Merged
@ondrejmirtes ondrejmirtes mentioned this pull request Apr 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

2 participants

@rajyan @ondrejmirtes