feat: implement isOffsetAccessLegal - #3045
Conversation
There was a problem hiding this comment.
This is the behavior I wanted to revert.
ClosureType (Object not implementing ArrayAcces) throws fatal error.
39c88e3 to
e65f4c2
Compare
e65f4c2 to
7e26cd7
Compare
7e26cd7 to
ed98710
Compare
| public function isOffsetAccessLegal(): TrinaryLogic | ||
| { | ||
| return TrinaryLogic::createYes(); | ||
| } |
There was a problem hiding this comment.
The most important difference between isOffsetAccessible is this line. Types like integer, boolean are not offset accessible but doesn't throw a fatal error on access in isset.
There was a problem hiding this comment.
I don't like this being added to a trait with a name like this. I'd like a separate trait for this method that would be used in all the places where relevant. Or maybe no trait at all, and just put this implementation in all the places where it applies.
| } | ||
|
|
||
| if ($scope->isUndefinedExpressionAllowed($node) && !$isOffsetAccessible->no()) { | ||
| if ($scope->isUndefinedExpressionAllowed($node) && $isOffsetAccessibleType->isOffsetAccessLegal()->yes()) { |
There was a problem hiding this comment.
Maybe it's better to add a tip for !$isOffsetAccessibleType->isOffsetAccessLegal()->yes() case.
- The
isOffsetAccessibleTypeshould be narrowed down to a type excludingObjectTypethat do not implementArrayAccess::class - The class implementing
ArrayAccess::Classshould be markedfinal
There was a problem hiding this comment.
The class implementing ArrayAccess::Class should be marked final
I don't get this part. It doesn't matter if a class is final when it already implements ArrayAccess. What a finality of such class would achieve?
There was a problem hiding this comment.
This was just a mistake. I was mixed up with the fact that we cant say isOffsetAccessLegal()->no() if the class is not final, because child class might implement ArrayClass.
|
This pull request has been marked as ready for review. |
| public function isOffsetAccessLegal(): TrinaryLogic | ||
| { | ||
| return TrinaryLogic::createYes(); | ||
| } |
There was a problem hiding this comment.
I don't like this being added to a trait with a name like this. I'd like a separate trait for this method that would be used in all the places where relevant. Or maybe no trait at all, and just put this implementation in all the places where it applies.
| } | ||
|
|
||
| if ($scope->isUndefinedExpressionAllowed($node) && !$isOffsetAccessible->no()) { | ||
| if ($scope->isUndefinedExpressionAllowed($node) && $isOffsetAccessibleType->isOffsetAccessLegal()->yes()) { |
There was a problem hiding this comment.
The class implementing ArrayAccess::Class should be marked final
I don't get this part. It doesn't matter if a class is final when it already implements ArrayAccess. What a finality of such class would achieve?
| [ | ||
| "Cannot access offset 'path' on iterable<int|string, object>.", | ||
| 26, | ||
| ], |
There was a problem hiding this comment.
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
One levels test needs updating (just re-run it and commit the generated JSON), thanks
|
|
||
| public function isOffsetAccessLegal(): TrinaryLogic | ||
| { | ||
| return TrinaryLogic::createYes(); |
There was a problem hiding this comment.
I think this should be no, and I think we can test it with $p = new parent(); and trying offset access on that.
There was a problem hiding this comment.
You are definitely right! Thank you.
Also, found an interesting result that NonExistentParentClassType behavior has changed from 7.4.
We cannot even declare a NonExistentParentClassType from PHP 8.0.
https://3v4l.org/7TR10
Calling array access is an error across all versions.
https://3v4l.org/ClM1B
There was a problem hiding this comment.
We can just skip the file from linting in Makefile
Line 76 in 78d3d99
|
Awesome, thank you! |
|
Thank you for your swift responses as always! |
3 participants
Details about isOffsetAccessLegal are explained in phpstan/phpstan#8393
ref #1791 (comment)
closes phpstan/phpstan#8393
resolves phpstan/phpstan#10926