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

make assigning property via reference impure #3082

Merged
merged 3 commits into from
May 30, 2024
Merged

make assigning property via reference impure #3082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c2bb307
make assigning property via reference impure
schlndh May 17, 2024
ae76f1d
use separate impure identifier for property assignment by ref
schlndh May 18, 2024
b8a4d88
test error message for impure assignment by reference
schlndh May 18, 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/Analyser/ImpurePoint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use PHPStan\Node\VirtualNode;

/**
* @phpstan-type ImpurePointIdentifier = 'echo'|'die'|'exit'|'propertyAssign'|'propertyUnset'|'methodCall'|'new'|'functionCall'|'include'|'require'|'print'|'eval'|'superglobal'|'yield'|'yieldFrom'|'static'|'global'|'betweenPhpTags'
* @phpstan-type ImpurePointIdentifier = 'echo'|'die'|'exit'|'propertyAssign'|'propertyAssignByRef'|'propertyUnset'|'methodCall'|'new'|'functionCall'|'include'|'require'|'print'|'eval'|'superglobal'|'yield'|'yieldFrom'|'static'|'global'|'betweenPhpTags'
* @api
*/
class ImpurePoint
Expand Down
18 changes: 17 additions & 1 deletion src/Analyser/NodeScopeResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2135,7 +2135,23 @@ public function processExprNode(Node\Stmt $stmt, Expr $expr, MutatingScope $scop
$nodeCallback,
$context,
function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context): ExpressionResult {
$impurePoints = [];
if ($expr instanceof AssignRef) {
$referencedExpr = $expr->expr;
while ($referencedExpr instanceof ArrayDimFetch) {
$referencedExpr = $referencedExpr->var;
}

if ($referencedExpr instanceof PropertyFetch || $referencedExpr instanceof StaticPropertyFetch) {
$impurePoints[] = new ImpurePoint(
$scope,
$expr,
'propertyAssignByRef',
'property assignment by reference',
schlndh marked this conversation as resolved.
Show resolved Hide resolved
false,
Copy link
Member

Choose a reason for hiding this comment

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

Should be true, right? There isn't a case where this might still be pure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be pure:

I set certain = false, because theoretically the referenced variable may already be set, and the reference may never be written. In which case it would be pure. But I can't imagine why the reference would be used in such a way in the first place.

<?php

class Foo
{
    public ?int $foo;
    
    public function bar(): void
    {
        $x = &$this->foo;
    }
}

$foo = new Foo;
$foo->foo = 5;
$foo->bar();
var_dump($foo->foo);

Copy link
Member

Choose a reason for hiding this comment

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

Alright, cool :)

);
}

$scope = $scope->enterExpressionAssign($expr->expr);
}

Expand All @@ -2150,7 +2166,7 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context): Exp
$result = $this->processExprNode($stmt, $expr->expr, $scope, $nodeCallback, $context->enterDeep());
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$impurePoints = array_merge($impurePoints, $result->getImpurePoints());
$scope = $result->getScope();

if ($expr instanceof AssignRef) {
Expand Down
10 changes: 10 additions & 0 deletions tests/PHPStan/Rules/Pure/PureMethodRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,14 @@ public function testPureConstructor(): void
]);
}

public function testImpureAssignRef(): void
{
$this->analyse([__DIR__ . '/data/impure-assign-ref.php'], [
[
'Possibly impure property assignment by reference in pure method ImpureAssignRef\HelloWorld::bar6().',
49,
],
]);
}

}
63 changes: 63 additions & 0 deletions tests/PHPStan/Rules/Pure/data/impure-assign-ref.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php declare(strict_types = 1); // lint >= 7.4

namespace ImpureAssignRef;

class HelloWorld
{
public int $value = 0;
/** @var array<int, int> */
public array $arr = [];
/** @var array<int, \stdClass> */
public array $objectArr = [];
public static int $staticValue = 0;
/** @var array<int, int> */
public static array $staticArr = [];

private function bar1(): void
{
$value = &$this->value;
$value = 1;
}

private function bar2(): void
{
$value = &$this->arr[0];
$value = 1;
}

private function bar3(): void
{
$value = &self::$staticValue;
$value = 1;
}

private function bar4(): void
{
$value = &self::$staticArr[0];
$value = 1;
}

private function bar5(self $other): void
{
$value = &$other->value;
$value = 1;
}

/** @phpstan-pure */
private function bar6(): int
{
$value = &$this->objectArr[0]->foo;

return 1;
}

public function foo(): void
{
$this->bar1();
$this->bar2();
$this->bar3();
$this->bar4();
$this->bar5(new self());
$this->bar6();
}
}
Loading