-
Notifications
You must be signed in to change notification settings - Fork 544
Fix non-existent offset when guarding #4539
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
base: 2.1.x
Are you sure you want to change the base?
Conversation
tests/PHPStan/Rules/Arrays/data/report-possibly-nonexistent-array-offset.php
Outdated
Show resolved
Hide resolved
tests/PHPStan/Rules/Arrays/data/report-possibly-nonexistent-array-offset.php
Show resolved
Hide resolved
tests/PHPStan/Rules/Arrays/data/report-possibly-nonexistent-array-offset.php
Show resolved
Hide resolved
| if ( | ||
| $context->true() | ||
| && $expr instanceof Node\Expr\BinaryOp\Smaller | ||
| && $argType->isList()->yes() | ||
| && IntegerRangeType::fromInterval(0, null)->isSuperTypeOf($leftType)->yes() | ||
| ) { | ||
| $dimFetch = new ArrayDimFetch( | ||
| $expr->right->getArgs()[0]->value, | ||
| $expr->left, | ||
| ); | ||
| $result = $result->unionWith( | ||
| $this->create( | ||
| $dimFetch, | ||
| $argType->getIterableValueType(), | ||
| TypeSpecifierContext::createTrue(), | ||
| $scope, | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this PR trying to fix phpstan/phpstan#13773 ?
(since the PR does not at all contain a description.. I can only guess)
if so, you should either adjust the logic added with #4403 or maybe even delete it in case you want to re-implement it in a different place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out! I updated the description of this pr.
I have not opened a issue for this pr. But i will also try and fix phpstan/phpstan#13773 later in a diffrent pr. Here i am trying to fix array access on lists if we explicitly guard the index.
|
There seems to be a regression This did work before this pr. I am wondering if this is also true for the other checks in the method i changed. |
| if (count($c) > 0) { | ||
| $c = array_map(fn() => new stdClass(), $c); | ||
| assertType('non-empty-list<stdClass>', $c); | ||
| assertType('non-empty-list<stdClass>&hasOffsetValue(0, stdClass)', $c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type does not make sense, because non-empty-list<stdClass> already implies that the list has a offset 0 with value stdClass
dc64635 to
2c7e290
Compare
| $context->true() | ||
| && $expr instanceof Node\Expr\BinaryOp\Smaller | ||
| && $argType->isList()->yes() | ||
| && $argExpr instanceof Expr\Variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instanceof Expr\Variable is usually a bug as most logic works for other expressions (e.g. a pure function call can also be remembered).
most of the time this works just by dropping this line
| $this->create( | ||
| $dimFetch, | ||
| $argType->getIterableValueType(), | ||
| TypeSpecifierContext::createTrue(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually we pass thru the $context, instead of creating a new one.
This pr is trying to fix a false positive.
Note that 1) $array is a list 2) $index is positive (or at least >= 0)