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

feat: implement isOffsetAccessLegal #3045

Merged
merged 12 commits into from
May 7, 2024
+404 −1
Merged

feat: implement isOffsetAccessLegal #3045

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5277d55
feat: implement isOffsetAccessLegal
rajyan May 5, 2024
b81633f
feat: use isOffsetAccessLegal in NonexistentOffsetInArrayDimFetchRule
rajyan May 5, 2024
461cb89
test: add test for issue-10926
rajyan May 5, 2024
ed98710
test: add test for offset access legal
rajyan May 5, 2024
b012509
fix: fix isOffsetAccessLegal for mixed type
rajyan May 5, 2024
fbf47dd
chore: fix miss-leading comments
rajyan May 7, 2024
6ed2ea6
fix: move isOffsetAccessLegal out of trait
rajyan May 7, 2024
dbfc3b8
fix: fix MaybeObjectType isOffsetAccessLegal
rajyan May 7, 2024
4b4af8e
fix: callable and iterable are maybe not isOffsetAccessLegal
rajyan May 7, 2024
631a3a1
fix: NonexistentParentClassType is not offsetAccessLegal
rajyan May 7, 2024
d7951a7
test: update levels test behavior for array access to iterator
rajyan May 7, 2024
9b18667
chore: fix lint
rajyan May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Rules/Arrays/NonexistentOffsetInArrayDimFetchRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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)

return [];
}

Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/AccessoryArrayListType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $this->getIterableKeyType()->isSuperTypeOf($offsetType)->and(TrinaryLogic::createMaybe());
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/AccessoryLiteralStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $offsetType->isInteger()->and(TrinaryLogic::createMaybe());
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/AccessoryNonEmptyStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $offsetType->isInteger()->and(TrinaryLogic::createMaybe());
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/AccessoryNonFalsyStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $offsetType->isInteger()->and(TrinaryLogic::createMaybe());
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/AccessoryNumericStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $offsetType->isInteger()->and(TrinaryLogic::createMaybe());
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/HasOffsetType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
if ($offsetType->isConstantScalarValue()->yes() && $offsetType->equals($this->offsetType)) {
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/HasOffsetValueType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
if ($offsetType->isConstantScalarValue()->yes() && $offsetType->equals($this->offsetType)) {
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/NonEmptyArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createMaybe();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Accessory/OversizedArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createMaybe();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/ArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
$offsetType = $offsetType->toArrayKey();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/ClosureType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,11 @@ function (): string {
);
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createNo();
}

public function isObject(): TrinaryLogic
{
return $this->objectType->isObject();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/IntersectionType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,11 @@ public function isOffsetAccessible(): TrinaryLogic
return $this->intersectResults(static fn (Type $type): TrinaryLogic => $type->isOffsetAccessible());
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return $this->intersectResults(static fn (Type $type): TrinaryLogic => $type->isOffsetAccessLegal());
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
if ($this->isList()->yes() && $this->isIterableAtLeastOnce()->yes()) {
Expand Down
10 changes: 10 additions & 0 deletions src/Type/MixedType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,16 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createMaybe();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
if ($this->subtractedType !== null) {
if ($this->subtractedType->isSuperTypeOf(new ObjectWithoutClassType())->yes()) {
return TrinaryLogic::createYes();
}
}
return TrinaryLogic::createMaybe();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
if ($this->isOffsetAccessible()->no()) {
Expand Down
5 changes: 5 additions & 0 deletions src/Type/NeverType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createYes();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/NullType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createNo();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/ObjectType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,11 @@ public function isOffsetAccessible(): TrinaryLogic
);
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return $this->isOffsetAccessible();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
if ($this->isInstanceOf(ArrayAccess::class)->yes()) {
Expand Down
5 changes: 5 additions & 0 deletions src/Type/StaticType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,11 @@ public function isOffsetAccessible(): TrinaryLogic
return $this->getStaticObjectType()->isOffsetAccessible();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return $this->getStaticObjectType()->isOffsetAccessLegal();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $this->getStaticObjectType()->hasOffsetValueType($offsetType);
Expand Down
5 changes: 5 additions & 0 deletions src/Type/StrictMixedType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createNo();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createNo();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createNo();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/StringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createYes();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $offsetType->isInteger()->and(TrinaryLogic::createMaybe());
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Traits/LateResolvableTypeTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,11 @@ public function isOffsetAccessible(): TrinaryLogic
return $this->resolve()->isOffsetAccessible();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return $this->resolve()->isOffsetAccessLegal();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $this->resolve()->hasOffsetValueType($offsetType);
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Traits/MaybeOffsetAccessibleTypeTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createMaybe();
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return TrinaryLogic::createYes();
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createMaybe();
Expand Down
5 changes: 5 additions & 0 deletions src/Type/Traits/NonOffsetAccessibleTypeTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ public function isOffsetAccessible(): TrinaryLogic
return TrinaryLogic::createNo();
}

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.


public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return TrinaryLogic::createNo();
Expand Down
2 changes: 2 additions & 0 deletions src/Type/Type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ public function isList(): TrinaryLogic;

public function isOffsetAccessible(): TrinaryLogic;

public function isOffsetAccessLegal(): TrinaryLogic;

public function hasOffsetValueType(Type $offsetType): TrinaryLogic;

public function getOffsetValueType(Type $offsetType): Type;
Expand Down
5 changes: 5 additions & 0 deletions src/Type/UnionType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,11 @@ public function isOffsetAccessible(): TrinaryLogic
return $this->unionResults(static fn (Type $type): TrinaryLogic => $type->isOffsetAccessible());
}

public function isOffsetAccessLegal(): TrinaryLogic
{
return $this->unionResults(static fn (Type $type): TrinaryLogic => $type->isOffsetAccessLegal());
}

public function hasOffsetValueType(Type $offsetType): TrinaryLogic
{
return $this->unionResults(static fn (Type $type): TrinaryLogic => $type->hasOffsetValueType($offsetType));
Expand Down
42 changes: 42 additions & 0 deletions tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ public function testRule(): void
'Cannot access offset \'a\' on Closure(): void.',
253,
],
[
'Cannot access offset \'a\' on array{a: 1, b: 1}|(Closure(): void).',
258,
],
Comment on lines +118 to +121
Copy link
Contributor Author

@rajyan rajyan May 5, 2024 •
edited
Loading

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.

[
'Offset null does not exist on array<int, string>.',
310,
Expand Down Expand Up @@ -252,6 +256,10 @@ public function testRuleBleedingEdge(): void
'Cannot access offset \'a\' on Closure(): void.',
253,
],
[
'Cannot access offset \'a\' on array{a: 1, b: 1}|(Closure(): void).',
258,
],
[
'Offset null does not exist on array<int, string>.',
310,
Expand Down Expand Up @@ -737,6 +745,17 @@ public function testBug8166(): void
]);
}

public function testBug10926(): void
{
$this->checkExplicitMixed = true;
$this->analyse([__DIR__ . '/data/bug-10926.php'], [
[
'Cannot access offset \'a\' on stdClass.',
10,
],
]);
}

public function testMixed(): void
{
$this->checkExplicitMixed = true;
Expand All @@ -757,6 +776,29 @@ public function testMixed(): void
]);
}

public function testOffsetAccessLegal(): void
{
$this->checkExplicitMixed = true;
$this->analyse([__DIR__ . '/data/offset-access-legal.php'], [
[
'Cannot access offset 0 on Closure(): void.',
7,
],
[
'Cannot access offset 0 on stdClass.',
12,
],
[
'Cannot access offset 0 on array{\'test\'}|stdClass.',
96,
],
[
'Cannot access offset 0 on array{\'test\'}|(Closure(): void).',
98,
],
]);
}

public function dataReportPossiblyNonexistentArrayOffset(): iterable
{
yield [false, false, []];
Expand Down
12 changes: 12 additions & 0 deletions tests/PHPStan/Rules/Arrays/data/bug-10926.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php // lint >= 8.0

namespace Bug10926;

class HelloWorld
{
public function sayHello(?\stdClass $date): void
{
$date ??= new \stdClass();
echo isset($date['a']);
}
}
Loading
Loading