Skip to content

Commit cfa0299

Browse files
staabmstaabm
authored
Fixed false-positive with by-ref parameters and array-modifying functions
1 parent d04535d commit cfa0299

File tree

11 files changed

+342
-10
lines changed

11 files changed

+342
-10
lines changed

‎src/Analyser/NodeScopeResolver.php

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5392,6 +5392,7 @@ private function processAssignVar(
53925392
}
53935393
}
53945394

5395+
$scopeBeforeAssignEval = $scope;
53955396
$scope = $result->getScope();
53965397
$truthySpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $assignedExpr, TypeSpecifierContext::createTruthy());
53975398
$falseySpecifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $assignedExpr, TypeSpecifierContext::createFalsey());
@@ -5404,7 +5405,7 @@ private function processAssignVar(
54045405
$conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);
54055406
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);
54065407

5407-
$nodeCallback(new VariableAssignNode($var, $assignedExpr), $result->getScope());
5408+
$nodeCallback(new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval);
54085409
$scope = $scope->assignVariable($var->name, $type, $scope->getNativeType($assignedExpr), TrinaryLogic::createYes());
54095410
foreach ($conditionalExpressions as $exprString => $holders) {
54105411
$scope = $scope->addConditionalExpressions($exprString, $holders);
@@ -5487,6 +5488,7 @@ private function processAssignVar(
54875488
$nativeValueToWrite = $scope->getNativeType($assignedExpr);
54885489
$originalValueToWrite = $valueToWrite;
54895490
$originalNativeValueToWrite = $nativeValueToWrite;
5491+
$scopeBeforeAssignEval = $scope;
54905492

54915493
// 3. eval assigned expr
54925494
$result = $processExprCallback($scope);
@@ -5542,11 +5544,11 @@ private function processAssignVar(
55425544

55435545
if ($varType->isArray()->yes() || !(new ObjectType(ArrayAccess::class))->isSuperTypeOf($varType)->yes()) {
55445546
if ($var instanceof Variable && is_string($var->name)) {
5545-
$nodeCallback(new VariableAssignNode($var, $assignedPropertyExpr), $scope);
5547+
$nodeCallback(new VariableAssignNode($var, $assignedPropertyExpr), $scopeBeforeAssignEval);
55465548
$scope = $scope->assignVariable($var->name, $valueToWrite, $nativeValueToWrite, TrinaryLogic::createYes());
55475549
} else {
55485550
if ($var instanceof PropertyFetch || $var instanceof StaticPropertyFetch) {
5549-
$nodeCallback(new PropertyAssignNode($var, $assignedPropertyExpr, $isAssignOp), $scope);
5551+
$nodeCallback(new PropertyAssignNode($var, $assignedPropertyExpr, $isAssignOp), $scopeBeforeAssignEval);
55505552
if ($var instanceof PropertyFetch && $var->name instanceof Node\Identifier && !$isAssignOp) {
55515553
$scope = $scope->assignInitializedProperty($scope->getType($var->var), $var->name->toString());
55525554
}
@@ -5574,9 +5576,9 @@ private function processAssignVar(
55745576
}
55755577
} else {
55765578
if ($var instanceof Variable) {
5577-
$nodeCallback(new VariableAssignNode($var, $assignedPropertyExpr), $scope);
5579+
$nodeCallback(new VariableAssignNode($var, $assignedPropertyExpr), $scopeBeforeAssignEval);
55785580
} elseif ($var instanceof PropertyFetch || $var instanceof StaticPropertyFetch) {
5579-
$nodeCallback(new PropertyAssignNode($var, $assignedPropertyExpr, $isAssignOp), $scope);
5581+
$nodeCallback(new PropertyAssignNode($var, $assignedPropertyExpr, $isAssignOp), $scopeBeforeAssignEval);
55805582
if ($var instanceof PropertyFetch && $var->name instanceof Node\Identifier && !$isAssignOp) {
55815583
$scope = $scope->assignInitializedProperty($scope->getType($var->var), $var->name->toString());
55825584
}
@@ -5611,6 +5613,7 @@ static function (): void {
56115613
$scope = $propertyNameResult->getScope();
56125614
}
56135615

5616+
$scopeBeforeAssignEval = $scope;
56145617
$result = $processExprCallback($scope);
56155618
$hasYield = $hasYield || $result->hasYield();
56165619
$throwPoints = array_merge($throwPoints, $result->getThrowPoints());
@@ -5625,7 +5628,7 @@ static function (): void {
56255628
if ($propertyName !== null && $propertyHolderType->hasProperty($propertyName)->yes()) {
56265629
$propertyReflection = $propertyHolderType->getProperty($propertyName, $scope);
56275630
$assignedExprType = $scope->getType($assignedExpr);
5628-
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scope);
5631+
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval);
56295632
if ($propertyReflection->canChangeTypeAfterAssignment()) {
56305633
if ($propertyReflection->hasNativeType()) {
56315634
$propertyNativeType = $propertyReflection->getNativeType();
@@ -5671,7 +5674,7 @@ static function (): void {
56715674
} else {
56725675
// fallback
56735676
$assignedExprType = $scope->getType($assignedExpr);
5674-
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scope);
5677+
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval);
56755678
$scope = $scope->assignExpression($var, $assignedExprType, $scope->getNativeType($assignedExpr));
56765679
// simulate dynamic property assign by __set to get throw points
56775680
if (!$propertyHolderType->hasMethod('__set')->no()) {
@@ -5705,6 +5708,7 @@ static function (): void {
57055708
$scope = $propertyNameResult->getScope();
57065709
}
57075710

5711+
$scopeBeforeAssignEval = $scope;
57085712
$result = $processExprCallback($scope);
57095713
$hasYield = $hasYield || $result->hasYield();
57105714
$throwPoints = array_merge($throwPoints, $result->getThrowPoints());
@@ -5714,7 +5718,7 @@ static function (): void {
57145718
if ($propertyName !== null) {
57155719
$propertyReflection = $scope->getPropertyReflection($propertyHolderType, $propertyName);
57165720
$assignedExprType = $scope->getType($assignedExpr);
5717-
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scope);
5721+
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval);
57185722
if ($propertyReflection !== null && $propertyReflection->canChangeTypeAfterAssignment()) {
57195723
if ($propertyReflection->hasNativeType()) {
57205724
$propertyNativeType = $propertyReflection->getNativeType();
@@ -5745,7 +5749,7 @@ static function (): void {
57455749
} else {
57465750
// fallback
57475751
$assignedExprType = $scope->getType($assignedExpr);
5748-
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scope);
5752+
$nodeCallback(new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval);
57495753
$scope = $scope->assignExpression($var, $assignedExprType, $scope->getNativeType($assignedExpr));
57505754
}
57515755
} elseif ($var instanceof List_) {

‎tests/PHPStan/Rules/Properties/TypesAssignedToPropertiesRuleTest.php

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,11 @@ class TypesAssignedToPropertiesRuleTest extends RuleTestCase
1515

1616
private bool $checkExplicitMixed = false;
1717

18+
private bool $checkImplicitMixed = false;
19+
1820
protected function getRule(): Rule
1921
{
20-
return new TypesAssignedToPropertiesRule(new RuleLevelHelper($this->createReflectionProvider(), true, false, true, $this->checkExplicitMixed, false, false, true), new PropertyReflectionFinder());
22+
return new TypesAssignedToPropertiesRule(new RuleLevelHelper($this->createReflectionProvider(), true, false, true, $this->checkExplicitMixed, $this->checkImplicitMixed, false, true), new PropertyReflectionFinder());
2123
}
2224

2325
public function testTypesAssignedToProperties(): void
@@ -779,4 +781,35 @@ public function testPropertyHooks(): void
779781
]);
780782
}
781783

784+
public function testBug13093c(): void
785+
{
786+
$this->analyse([__DIR__ . '/data/bug-13093c.php'], []);
787+
}
788+
789+
public function testBug13093d(): void
790+
{
791+
$this->analyse([__DIR__ . '/data/bug-13093d.php'], []);
792+
}
793+
794+
public function testBug8825(): void
795+
{
796+
$this->checkImplicitMixed = true;
797+
$this->analyse([__DIR__ . '/data/bug-8825.php'], []);
798+
}
799+
800+
public function testBug7844(): void
801+
{
802+
$this->analyse([__DIR__ . '/data/bug-7844.php'], []);
803+
}
804+
805+
public function testBug7844b(): void
806+
{
807+
$this->analyse([__DIR__ . '/data/bug-7844b.php'], []);
808+
}
809+
810+
public function testBug12675(): void
811+
{
812+
$this->analyse([__DIR__ . '/data/bug-12675.php'], []);
813+
}
814+
782815
}

‎tests/PHPStan/Rules/Properties/data/bug-12675.php

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
3+
namespace Bug12675;
4+
5+
class HelloWorld
6+
{
7+
private string $username = "";
8+
private string $domain = "";
9+
10+
public function with_shift(string $email): void
11+
{
12+
$pieces = explode("@", $email);
13+
if (2 !== count($pieces)) {
14+
15+
throw new \Exception("Bad, very bad...");
16+
}
17+
18+
$this->username = array_shift($pieces);
19+
$this->domain = array_shift($pieces);
20+
21+
echo "{$this->username}@{$this->domain}";
22+
}
23+
24+
public function with_pop(string $email): void
25+
{
26+
$pieces = explode("@", $email);
27+
if (2 !== count($pieces)) {
28+
29+
throw new \Exception("Bad, very bad...");
30+
}
31+
32+
$this->domain = array_pop($pieces);
33+
$this->username = array_pop($pieces);
34+
35+
echo "{$this->username}@{$this->domain}";
36+
}
37+
}

‎tests/PHPStan/Rules/Properties/data/bug-13093c.php

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Bug13093c;
6+
7+
use function array_shift;
8+
9+
final class ParallelProcessRunner
10+
{
11+
/**
12+
* @var array<int, string>
13+
*/
14+
private array $nextMutantProcessKillerContainer = [];
15+
16+
private string $prop;
17+
18+
public function fillBucketOnce(array &$killer): int
19+
{
20+
if ($this->nextMutantProcessKillerContainer !== []) {
21+
$this->prop = array_shift($this->nextMutantProcessKillerContainer);
22+
}
23+
24+
return 0;
25+
}
26+
27+
}
28+
29+
final class ParallelProcessRunner2
30+
{
31+
/**
32+
* @var array<int, string>
33+
*/
34+
private array $nextMutantProcessKillerContainer = [];
35+
36+
private string $prop;
37+
38+
public function fillBucketOnce(array &$killer): int
39+
{
40+
$name = 'prop';
41+
if ($this->nextMutantProcessKillerContainer !== []) {
42+
$this->{$name} = array_shift($this->nextMutantProcessKillerContainer);
43+
}
44+
45+
return 0;
46+
}
47+
48+
}
49+

‎tests/PHPStan/Rules/Properties/data/bug-13093d.php

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Bug13093d;
6+
7+
use function array_shift;
8+
9+
final class ParallelProcessRunner
10+
{
11+
/**
12+
* @var array<int, string>
13+
*/
14+
private array $nextMutantProcessKillerContainer = [];
15+
16+
static private string $prop;
17+
18+
public function fillBucketOnce(array &$killer): int
19+
{
20+
if ($this->nextMutantProcessKillerContainer !== []) {
21+
self::$prop = array_shift($this->nextMutantProcessKillerContainer);
22+
}
23+
24+
return 0;
25+
}
26+
27+
}
28+
29+
final class ParallelProcessRunner2
30+
{
31+
/**
32+
* @var array<int, string>
33+
*/
34+
private array $nextMutantProcessKillerContainer = [];
35+
36+
static private string $prop;
37+
38+
public function fillBucketOnce(array &$killer): int
39+
{
40+
$name = 'prop';
41+
if ($this->nextMutantProcessKillerContainer !== []) {
42+
self::${$name} = array_shift($this->nextMutantProcessKillerContainer);
43+
}
44+
45+
return 0;
46+
}
47+
48+
}
49+
50+

‎tests/PHPStan/Rules/Properties/data/bug-7844.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
namespace Bug7844;
4+
5+
class C
6+
{
7+
/** @var float */
8+
public $val = .0;
9+
10+
/** @var array<float> */
11+
public $data = array();
12+
13+
public function foo(): void
14+
{
15+
if (count($this->data) > 0) {
16+
$this->val = array_shift($this->data);
17+
}
18+
}
19+
}
20+

‎tests/PHPStan/Rules/Properties/data/bug-7844b.php

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<?php
2+
3+
namespace Bug7844b;
4+
5+
class Obj {}
6+
7+
class HelloWorld
8+
{
9+
public Obj $p1;
10+
public Obj $p2;
11+
public Obj $p3;
12+
public Obj $p4;
13+
public Obj $p5;
14+
15+
/** @param non-empty-list<Obj> $objs */
16+
public function __construct(array $objs)
17+
{
18+
\assert($objs !== []);
19+
$this->p1 = $objs[0];
20+
21+
\assert($objs !== []);
22+
$this->p2 = $objs[array_key_last($objs)];
23+
24+
\assert($objs !== []);
25+
$this->p3 = \array_pop($objs);
26+
27+
\assert($objs !== []);
28+
$this->p4 = \array_shift($objs);
29+
30+
\assert($objs !== []);
31+
$p = \array_shift($objs);
32+
$this->p5 = $p;
33+
34+
\assert($objs !== []);
35+
$this->doSomething(\array_pop($objs));
36+
}
37+
38+
private function doSomething(Obj $obj): void {}
39+
}

‎tests/PHPStan/Rules/Properties/data/bug-8825.php

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php
2+
3+
namespace Bug8825;
4+
5+
class Endboss
6+
{
7+
8+
private bool $isBool;
9+
10+
/**
11+
* @param mixed[] $actionParameters
12+
*/
13+
public function __construct(
14+
array $actionParameters
15+
)
16+
{
17+
$this->isBool = $actionParameters['my_key'] ?? false;
18+
}
19+
20+
public function use(): void
21+
{
22+
$this->isBool->someMethod();
23+
}
24+
}

‎tests/PHPStan/Rules/Variables/ParameterOutAssignedTypeRuleTest.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,4 +64,14 @@ public function testBenevolentArrayKey(): void
6464
$this->analyse([__DIR__ . '/data/benevolent-array-key.php'], []);
6565
}
6666

67+
public function testBug13093(): void
68+
{
69+
$this->analyse([__DIR__ . '/data/bug-13093.php'], []);
70+
}
71+
72+
public function testBug13093b(): void
73+
{
74+
$this->analyse([__DIR__ . '/data/bug-13093b.php'], []);
75+
}
76+
6777
}

0 commit comments

Comments
 (0)