Skip to content

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

Merged
ondrejmirtes merged 8 commits intophpstan:1.7.xfrom
rajyan:fix/issue-7229
May 18, 2022
Merged

allow $isOffsetAccessible->maybe() for isUndefinedExpressionAllowed and isSpecified expression#1312
ondrejmirtes merged 8 commits intophpstan:1.7.xfrom
rajyan:fix/issue-7229

Conversation

@rajyan
Copy link
Copy Markdown
Contributor

@rajyan rajyan commented May 14, 2022

@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
Copy Markdown
Contributor Author

rajyan commented May 14, 2022

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

I think I understood it.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

rajyan commented May 15, 2022

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

rajyan commented May 18, 2022

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

@rajyan
Copy link
Copy Markdown
Contributor Author

rajyan commented May 18, 2022

@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
Copy Markdown
Member

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

Development

Successfully merging this pull request may close these issues.

3 participants