-
Notifications
You must be signed in to change notification settings - Fork 574
Consider ArrayAccess side effects for dead catch analysis #5117
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2090,6 +2090,18 @@ private function processStmtNode( | |||||||||||||||||
| $throwPoints = array_merge($throwPoints, $exprResult->getThrowPoints()); | ||||||||||||||||||
| $impurePoints = array_merge($impurePoints, $exprResult->getImpurePoints()); | ||||||||||||||||||
| if ($var instanceof ArrayDimFetch && $var->dim !== null) { | ||||||||||||||||||
| $varType = $scope->getType($var->var); | ||||||||||||||||||
| if (!$varType->isArray()->yes() && !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) { | ||||||||||||||||||
| $throwPoints = array_merge($throwPoints, $this->processExprNode( | ||||||||||||||||||
| $stmt, | ||||||||||||||||||
| new MethodCall($this->deepNodeCloner->cloneNode($var->var), 'offsetUnset'), | ||||||||||||||||||
| $scope, | ||||||||||||||||||
| $storage, | ||||||||||||||||||
| new NoopNodeCallback(), | ||||||||||||||||||
| ExpressionContext::createDeep(), | ||||||||||||||||||
| )->getThrowPoints()); | ||||||||||||||||||
|
Comment on lines
+2096
to
+2103
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have similar but different code already in phpstan-src/src/Analyser/NodeScopeResolver.php Lines 6428 to 6435 in 94731cc
I wonder why
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The place before |
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| $clonedVar = $this->deepNodeCloner->cloneNode($var->var); | ||||||||||||||||||
| $traverser = new NodeTraverser(); | ||||||||||||||||||
| $traverser->addVisitor(new class () extends NodeVisitorAbstract { | ||||||||||||||||||
|
|
@@ -3612,6 +3624,18 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context, $sto | |||||||||||||||||
| $impurePoints = array_merge($impurePoints, $result->getImpurePoints()); | ||||||||||||||||||
| $isAlwaysTerminating = $isAlwaysTerminating || $result->isAlwaysTerminating(); | ||||||||||||||||||
| $scope = $result->getScope(); | ||||||||||||||||||
|
|
||||||||||||||||||
| $varType = $scope->getType($expr->var); | ||||||||||||||||||
| if (!$varType->isArray()->yes() && !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) { | ||||||||||||||||||
| $throwPoints = array_merge($throwPoints, $this->processExprNode( | ||||||||||||||||||
| $stmt, | ||||||||||||||||||
| new MethodCall($this->deepNodeCloner->cloneNode($expr->var), 'offsetGet'), | ||||||||||||||||||
| $scope, | ||||||||||||||||||
| $storage, | ||||||||||||||||||
| new NoopNodeCallback(), | ||||||||||||||||||
| ExpressionContext::createDeep(), | ||||||||||||||||||
| )->getThrowPoints()); | ||||||||||||||||||
| } | ||||||||||||||||||
| } elseif ($expr instanceof Array_) { | ||||||||||||||||||
| $itemNodes = []; | ||||||||||||||||||
| $hasYield = false; | ||||||||||||||||||
|
|
@@ -3909,6 +3933,24 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context, $sto | |||||||||||||||||
| $impurePoints = array_merge($impurePoints, $result->getImpurePoints()); | ||||||||||||||||||
| $isAlwaysTerminating = $isAlwaysTerminating || $result->isAlwaysTerminating(); | ||||||||||||||||||
| $nonNullabilityResults[] = $nonNullabilityResult; | ||||||||||||||||||
|
|
||||||||||||||||||
| if (!($var instanceof ArrayDimFetch)) { | ||||||||||||||||||
| continue; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| $varType = $scope->getType($var->var); | ||||||||||||||||||
| if ($varType->isArray()->yes() || (new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->no()) { | ||||||||||||||||||
| continue; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| $throwPoints = array_merge($throwPoints, $this->processExprNode( | ||||||||||||||||||
| $stmt, | ||||||||||||||||||
| new MethodCall($this->deepNodeCloner->cloneNode($var->var), 'offsetExists'), | ||||||||||||||||||
| $scope, | ||||||||||||||||||
| $storage, | ||||||||||||||||||
| new NoopNodeCallback(), | ||||||||||||||||||
| ExpressionContext::createDeep(), | ||||||||||||||||||
| )->getThrowPoints()); | ||||||||||||||||||
| } | ||||||||||||||||||
| foreach (array_reverse($expr->vars) as $var) { | ||||||||||||||||||
| $scope = $this->lookForUnsetAllowedUndefinedExpressions($scope, $var); | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| <?php | ||
|
|
||
| namespace Bug11427; | ||
|
|
||
| /** @implements \ArrayAccess<int, int> */ | ||
| class C implements \ArrayAccess { | ||
| #[\ReturnTypeWillChange] | ||
| public function offsetExists($offset) { | ||
| throw new \Exception("exists"); | ||
| } | ||
|
|
||
| #[\ReturnTypeWillChange] | ||
| public function offsetGet($offset) { | ||
| throw new \Exception("get"); | ||
| } | ||
|
|
||
| #[\ReturnTypeWillChange] | ||
| public function offsetSet($offset, $value) { | ||
| throw new \Exception("set"); | ||
| } | ||
|
|
||
| #[\ReturnTypeWillChange] | ||
| public function offsetUnset($offset) { | ||
| throw new \Exception("unset"); | ||
| } | ||
| } | ||
|
|
||
| function test(C $c): void { | ||
| try { | ||
| $x = isset($c[1]); | ||
| } catch (\Exception $e) { | ||
| // offsetExists can throw | ||
| } | ||
|
|
||
| try { | ||
| $x = $c[1]; | ||
| } catch (\Exception $e) { | ||
| // offsetGet can throw | ||
| } | ||
|
|
||
| try { | ||
| $c[1] = 1; | ||
| } catch (\Exception $e) { | ||
| // offsetSet can throw | ||
| } | ||
|
|
||
| try { | ||
| unset($c[1]); | ||
| } catch (\Exception $e) { | ||
| // offsetUnset can throw | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Union type where isArray() returns maybe and isSuperTypeOf(ArrayAccess) returns maybe. | ||
| * This ensures the conditions in NodeScopeResolver are tested with types | ||
| * that distinguish !->yes() from ->no() and !->no() from ->yes(). | ||
| * | ||
| * @param array<int, int>|C $c | ||
| */ | ||
| function testArrayOrArrayAccess($c): void { | ||
| try { | ||
| $x = isset($c[1]); | ||
| } catch (\Exception $e) { | ||
| // offsetExists can throw when $c is C | ||
| } | ||
|
|
||
| try { | ||
| $x = $c[1]; | ||
| } catch (\Exception $e) { | ||
| // offsetGet can throw when $c is C | ||
| } | ||
|
|
||
| try { | ||
| $c[1] = 1; | ||
| } catch (\Exception $e) { | ||
| // offsetSet can throw when $c is C | ||
| } | ||
|
|
||
| try { | ||
| unset($c[1]); | ||
| } catch (\Exception $e) { | ||
| // offsetUnset can throw when $c is 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 is the only occurrence where you still use a deep-context. intentional?
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.
The
$contextis not an ExpressionContext but a StatementContext here, so I have to create a context (like it's done some line before)