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

feat: implement isOffsetAccessLegal #3045

Merged
merged 12 commits into from
May 7, 2024

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented May 5, 2024

Details about isOffsetAccessLegal are explained in phpstan/phpstan#8393

ref #1791 (comment)
closes phpstan/phpstan#8393
resolves phpstan/phpstan#10926

Comment on lines +118 to +121
[
'Cannot access offset \'a\' on array{a: 1, b: 1}|(Closure(): void).',
258,
],
Copy link
Contributor Author

@rajyan rajyan May 5, 2024

Choose a reason for hiding this comment

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

This is the behavior I wanted to revert.
ClosureType (Object not implementing ArrayAcces) throws fatal error.

@rajyan rajyan force-pushed the feat/offset-access-legal branch from 39c88e3 to e65f4c2 Compare May 5, 2024 03:43
@rajyan rajyan force-pushed the feat/offset-access-legal branch from e65f4c2 to 7e26cd7 Compare May 5, 2024 03:43
@rajyan rajyan force-pushed the feat/offset-access-legal branch from 7e26cd7 to ed98710 Compare May 5, 2024 04:05
Comment on lines 17 to 20
public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -64,7 +64,7 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

if ($scope->isUndefinedExpressionAllowed($node) && !$isOffsetAccessible->no()) {
if ($scope->isUndefinedExpressionAllowed($node) && $isOffsetAccessibleType->isOffsetAccessLegal()->yes()) {
Copy link
Contributor Author

@rajyan rajyan May 5, 2024

Choose a reason for hiding this comment

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

Maybe it's better to add a tip for !$isOffsetAccessibleType->isOffsetAccessLegal()->yes() case.

  • The isOffsetAccessibleType should be narrowed down to a type excluding ObjectType that do not implement ArrayAccess::class
  • The class implementing ArrayAccess::Class should be marked final

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

phpstan/phpstan#10926 (comment)

@rajyan rajyan marked this pull request as ready for review May 5, 2024 06:14
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Otherwise awesome! 👍

Comment on lines 17 to 20
public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -64,7 +64,7 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

if ($scope->isUndefinedExpressionAllowed($node) && !$isOffsetAccessible->no()) {
if ($scope->isUndefinedExpressionAllowed($node) && $isOffsetAccessibleType->isOffsetAccessLegal()->yes()) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Comment on lines +681 to +684
[
"Cannot access offset 'path' on iterable<int|string, object>.",
26,
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

One levels test needs updating (just re-run it and commit the generated JSON), thanks

@@ -152,6 +152,11 @@ public function toArrayKey(): Type
return new ErrorType();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be no, and I think we can test it with $p = new parent(); and trying offset access on that.

Copy link
Contributor Author

@rajyan rajyan May 7, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

We can just skip the file from linting in Makefile

--exclude tests/PHPStan/Rules/Classes/data/instantiation-promoted-properties.php \

@ondrejmirtes ondrejmirtes merged commit e55a83f into phpstan:1.11.x May 7, 2024
443 of 444 checks passed
@ondrejmirtes
Copy link
Member

Awesome, thank you!

@rajyan rajyan deleted the feat/offset-access-legal branch May 7, 2024 12:19
@rajyan
Copy link
Contributor Author

rajyan commented May 7, 2024

Thank you for your swift responses as always!

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.

offsetAccess.nonOffsetAccessible suppressed by isset Improve isOffsetAccessible->no
3 participants