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.

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

allow $isOffsetAccessible->maybe() for isUndefinedExpressionAllowed and isSpecified expression #1312

Merged
merged 8 commits into from
May 18, 2022
Merged

allow $isOffsetAccessible->maybe() for isUndefinedExpressionAllowed and isSpecified expression #1312

merged 8 commits into from
May 18, 2022

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented May 14, 2022 •
edited

@rajyan rajyan changed the title allow !$isOffsetAccessible->yes() for isUndefinedExpressionAllowed … allow $isOffsetAccessible->maybe() for isUndefinedExpressionAllowed and isSpecified expression May 14, 2022
@rajyan rajyan marked this pull request as ready for review May 14, 2022 00:56
@rajyan
Copy link
Contributor Author

rajyan commented May 14, 2022 •
edited

I'm not sure enough for what this test is intended to do, so cannot change it yet. c36df00

I think I understood it.

staabm
staabm reviewed May 14, 2022
View reviewed changes
tests/PHPStan/Levels/data/arrayDimFetches.php
@@ -55,7 +55,7 @@ public function doBaz($a, $b): void
*/
public function iterableOffset($iterable): void
{
unset($iterable['foo']);
var_dump($iterable['foo']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover Debug output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just wanted to change this line to some kind of function call that doesn’t cause isUndefinedExpressionAllowed === true (that means something except isset, unset, empty)

Copy link
Member

Choose a reason for hiding this comment

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

Please add new code instead of changing existing tests :) Thanks.

@rajyan
Copy link
Contributor Author

rajyan commented May 15, 2022 •
edited

@ondrejmirtes
Sorry for lack of explanation.
The purpose of this pull request is to make isUndefinedExpressionAllowed === true && $isOffsetAccessible->maybe() and isSpecified === true && $isOffsetAccessible->maybe() which is a kind of breaking change in some cases so we need some fix in tests too (see aeb00d9).

This change can make
https://phpstan.org/r/ea075526-a29b-428a-a5f7-39b90266a2f8

this code not report, and I believe this shouldn't be reported because this is a valid PHP code.

#1312 (comment)

unset is also a method that causes isUndefinedExpressionAllowed === true so the result for this test changes.
c36df00
Looking at this commit I thought that unset was not important here, because the intention of this test was to test reportMaybes for level 7 and unset isn't related. This is why I had to change the existing test.

Although, I remembered now this issue phpstan/phpstan#7073 and noticed that I have to be careful for array access on object type (because it is a fatal error) and this PR needs some modification anyway.

@rajyan rajyan marked this pull request as draft May 15, 2022 13:36
@ondrejmirtes
Copy link
Member

All of that is totally fine, I just want both unset and var_dump tested in that file. It's OK if the error for unset() changes/disappears.

@rajyan
Copy link
Contributor Author

rajyan commented May 15, 2022

I see, thanks.
I’ll mark this ready after fixing that and fixing problems about object type.
Thank you very much for reviewing!

@rajyan
Copy link
Contributor Author

rajyan commented May 18, 2022

https://3v4l.org/HrulF
Memo for canAccessProperties and isOffsetAccessible.

@rajyan
Copy link
Contributor Author

rajyan commented May 18, 2022 •
edited

@ondrejmirtes
I'll mark this ready.

I will implement a more strict behavior for canAccessProperties and isOffsetAccessible maybes under isUndefinedExpressionAllowed === true in another pull request using the result of https://3v4l.org/HrulF

memo
php/php-src@008bfcc

@rajyan rajyan marked this pull request as ready for review May 18, 2022 00:45
@ondrejmirtes ondrejmirtes merged commit 28f4503 into phpstan:1.7.x May 18, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@rajyan rajyan deleted the fix/issue-7229 branch November 17, 2022 23:53
@rajyan rajyan mentioned this pull request Nov 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes left review comments

@staabm staabm staabm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rajyan @ondrejmirtes @staabm