From 4cbd66909aa6f581de8e353c36a71d891b93d81c Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Mon, 18 Apr 2022 09:08:52 +0900 Subject: [PATCH 1/8] add regression tests for property access with dynamic properties related issues --- .../Properties/AccessPropertiesRuleTest.php | 30 +++++++++++++++++++ .../Rules/Properties/data/bug-3659.php | 17 +++++++++++ .../Rules/Properties/data/bug-6026.php | 23 ++++++++++++++ .../Properties/data/dynamic-properties.php | 19 ++++++++++++ 4 files changed, 89 insertions(+) create mode 100644 tests/PHPStan/Rules/Properties/data/bug-3659.php create mode 100644 tests/PHPStan/Rules/Properties/data/bug-6026.php create mode 100644 tests/PHPStan/Rules/Properties/data/dynamic-properties.php diff --git a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php index 9b6ea50be1..f20f81d46d 100644 --- a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php +++ b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php @@ -577,4 +577,34 @@ public function testBug6899(): void ]); } + public function testBug6026(): void + { + $this->checkThisOnly = false; + $this->checkUnionTypes = true; + $this->analyse([__DIR__ . '/data/bug-6026.php'], []); + } + + public function testBug3659(): void + { + $this->checkThisOnly = false; + $this->checkUnionTypes = true; + $this->analyse([__DIR__ . '/data/bug-3659.php'], []); + } + + public function testDynamicProperties(): void + { + $this->checkThisOnly = false; + $this->checkUnionTypes = true; + $this->analyse([__DIR__ . '/data/dynamic-properties.php'], [ + [ + 'Access to an undefined property DynamicProperties\Foo::$dynamicProperty.', + 11, + ], + [ + 'Access to an undefined property DynamicProperties\Bar::$dynamicProperty.', + 16, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Properties/data/bug-3659.php b/tests/PHPStan/Rules/Properties/data/bug-3659.php new file mode 100644 index 0000000000..de03b1cd42 --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/bug-3659.php @@ -0,0 +1,17 @@ +func2($obj->someProperty ?? null); + } + + public function func2(?string $param): void + { + echo $param ?? 'test'; + } +} + diff --git a/tests/PHPStan/Rules/Properties/data/bug-6026.php b/tests/PHPStan/Rules/Properties/data/bug-6026.php new file mode 100644 index 0000000000..cc939a8036 --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/bug-6026.php @@ -0,0 +1,23 @@ +datalen ?? 0; + $bucketLen = isset($bucket->datalen) ? $bucket->datalen : 0; + + return true; + } +} + diff --git a/tests/PHPStan/Rules/Properties/data/dynamic-properties.php b/tests/PHPStan/Rules/Properties/data/dynamic-properties.php new file mode 100644 index 0000000000..131152fdde --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/dynamic-properties.php @@ -0,0 +1,19 @@ +dynamicProperty); + empty($this->dynamicProperty); + $this->dynamicProperty ?? 'test'; // dynamic properties are not allowed in coalesce + + $bar = new Bar(); + isset($bar->dynamicProperty); + empty($bar->dynamicProperty); + $bar->dynamicProperty ?? 'test'; + } +} + From 1db43fc2d588ad04a67ed757a45ed06a201c957d Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Mon, 18 Apr 2022 09:12:45 +0900 Subject: [PATCH 2/8] allow undefined expression for `ObjectWithoutClassType` property fetch --- src/Analyser/NodeScopeResolver.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 33f264d889..559498a67d 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -127,6 +127,7 @@ use PHPStan\Type\NeverType; use PHPStan\Type\NullType; use PHPStan\Type\ObjectType; +use PHPStan\Type\ObjectWithoutClassType; use PHPStan\Type\StaticType; use PHPStan\Type\StaticTypeFactory; use PHPStan\Type\StringType; @@ -2309,9 +2310,12 @@ function (MutatingScope $scope) use ($expr, $nodeCallback, $context): Expression static fn (): MutatingScope => $rightResult->getScope()->filterByFalseyValue($expr), ); } elseif ($expr instanceof Coalesce) { - // backwards compatibility: coalesce doesn't allow dynamic properties $nonNullabilityResult = $this->ensureNonNullability($scope, $expr->left, false); - if ($expr->left instanceof PropertyFetch || $expr->left instanceof Expr\NullsafePropertyFetch || $expr->left instanceof StaticPropertyFetch) { + // backwards compatibility: coalesce doesn't allow dynamic properties + if ($expr->left instanceof PropertyFetch || $expr->left instanceof Expr\NullsafePropertyFetch) { + $propertyHolderType = $scope->getType($expr->left->var); + $condScope = $nonNullabilityResult->getScope()->setAllowedUndefinedExpression($expr->left, $propertyHolderType->isSuperTypeOf(new ObjectWithoutClassType())->yes()); + } elseif ($expr->left instanceof StaticPropertyFetch) { $condScope = $nonNullabilityResult->getScope()->setAllowedUndefinedExpression($expr->left, false); } else { $condScope = $nonNullabilityResult->getScope(); From 7ed685421d222ebfb0754a3c9eb096d39665a089 Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Mon, 18 Apr 2022 09:13:27 +0900 Subject: [PATCH 3/8] fix test for solved issue --- tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php index f20f81d46d..7856e4a1f0 100644 --- a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php +++ b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php @@ -570,10 +570,6 @@ public function testBug6899(): void 'Cannot access property $prop on string.', 15, ], - [ - 'Cannot access property $prop on object|string.', // open issue https://github.com/phpstan/phpstan/issues/3659, https://github.com/phpstan/phpstan/issues/6026 - 25, - ], ]); } From 4eb80b39febca0ff3658c27694424048e4abcc11 Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Wed, 20 Apr 2022 19:22:49 +0900 Subject: [PATCH 4/8] make isset and coalesce consistent --- src/Analyser/NodeScopeResolver.php | 11 +----- .../Properties/AccessPropertiesRuleTest.php | 35 +------------------ .../Properties/data/dynamic-properties.php | 2 +- 3 files changed, 3 insertions(+), 45 deletions(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 559498a67d..1f2d942960 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -2311,16 +2311,7 @@ function (MutatingScope $scope) use ($expr, $nodeCallback, $context): Expression ); } elseif ($expr instanceof Coalesce) { $nonNullabilityResult = $this->ensureNonNullability($scope, $expr->left, false); - // backwards compatibility: coalesce doesn't allow dynamic properties - if ($expr->left instanceof PropertyFetch || $expr->left instanceof Expr\NullsafePropertyFetch) { - $propertyHolderType = $scope->getType($expr->left->var); - $condScope = $nonNullabilityResult->getScope()->setAllowedUndefinedExpression($expr->left, $propertyHolderType->isSuperTypeOf(new ObjectWithoutClassType())->yes()); - } elseif ($expr->left instanceof StaticPropertyFetch) { - $condScope = $nonNullabilityResult->getScope()->setAllowedUndefinedExpression($expr->left, false); - } else { - $condScope = $nonNullabilityResult->getScope(); - } - $condScope = $this->lookForEnterAllowedUndefinedVariable($condScope, $expr->left, true); + $condScope = $this->lookForEnterAllowedUndefinedVariable($nonNullabilityResult->getScope(), $expr->left, true); $condResult = $this->processExprNode($expr->left, $condScope, $nodeCallback, $context->enterDeep()); $scope = $this->revertNonNullability($condResult->getScope(), $nonNullabilityResult->getSpecifiedExpressions()); $scope = $this->lookForExitAllowedUndefinedVariable($scope, $expr->left); diff --git a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php index 7856e4a1f0..b389fa5940 100644 --- a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php +++ b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php @@ -129,18 +129,6 @@ public function testAccessProperties(): void 'Access to an undefined property TestAccessProperties\FooAccessProperties::$dolor.', 250, ], - [ - 'Access to an undefined property TestAccessProperties\NullCoalesce::$bar.', - 264, - ], - [ - 'Access to an undefined property TestAccessProperties\NullCoalesce::$bar.', - 266, - ], - [ - 'Access to an undefined property TestAccessProperties\NullCoalesce::$bar.', - 270, - ], [ 'Cannot access property $bar on TestAccessProperties\NullCoalesce|null.', 272, @@ -271,18 +259,6 @@ public function testAccessPropertiesWithoutUnionTypes(): void 'Access to an undefined property TestAccessProperties\FooAccessProperties::$dolor.', 250, ], - [ - 'Access to an undefined property TestAccessProperties\NullCoalesce::$bar.', - 264, - ], - [ - 'Access to an undefined property TestAccessProperties\NullCoalesce::$bar.', - 266, - ], - [ - 'Access to an undefined property TestAccessProperties\NullCoalesce::$bar.', - 270, - ], [ 'Cannot access property $bar on TestAccessProperties\NullCoalesce|null.', 272, @@ -591,16 +567,7 @@ public function testDynamicProperties(): void { $this->checkThisOnly = false; $this->checkUnionTypes = true; - $this->analyse([__DIR__ . '/data/dynamic-properties.php'], [ - [ - 'Access to an undefined property DynamicProperties\Foo::$dynamicProperty.', - 11, - ], - [ - 'Access to an undefined property DynamicProperties\Bar::$dynamicProperty.', - 16, - ], - ]); + $this->analyse([__DIR__ . '/data/dynamic-properties.php'], []); } } diff --git a/tests/PHPStan/Rules/Properties/data/dynamic-properties.php b/tests/PHPStan/Rules/Properties/data/dynamic-properties.php index 131152fdde..45d32a9f7c 100644 --- a/tests/PHPStan/Rules/Properties/data/dynamic-properties.php +++ b/tests/PHPStan/Rules/Properties/data/dynamic-properties.php @@ -8,7 +8,7 @@ class Foo { public function doBar() { isset($this->dynamicProperty); empty($this->dynamicProperty); - $this->dynamicProperty ?? 'test'; // dynamic properties are not allowed in coalesce + $this->dynamicProperty ?? 'test'; $bar = new Bar(); isset($bar->dynamicProperty); From 3188cd2efd50ca053a5de4fc867c7b85d144915b Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Wed, 20 Apr 2022 19:29:28 +0900 Subject: [PATCH 5/8] allow overwriting allowedUndefinedExpression because not needed anymore --- src/Analyser/MutatingScope.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Analyser/MutatingScope.php b/src/Analyser/MutatingScope.php index 22709bd06b..fa068bbb7e 100644 --- a/src/Analyser/MutatingScope.php +++ b/src/Analyser/MutatingScope.php @@ -3909,9 +3909,6 @@ public function isInExpressionAssign(Expr $expr): bool public function setAllowedUndefinedExpression(Expr $expr, bool $isAllowed): self { $exprString = $this->getNodeKey($expr); - if (array_key_exists($exprString, $this->currentlyAllowedUndefinedExpressions)) { - return $this; - } $currentlyAllowedUndefinedExpressions = $this->currentlyAllowedUndefinedExpressions; $currentlyAllowedUndefinedExpressions[$exprString] = $isAllowed; From a26a9ba5fd7d8d8e1a728c3c975ebd7a3445c22d Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Wed, 20 Apr 2022 19:45:16 +0900 Subject: [PATCH 6/8] add regression tests for fixed issues --- .../Rules/Properties/AccessPropertiesRuleTest.php | 8 ++++++++ .../Properties/AccessStaticPropertiesRuleTest.php | 5 +++++ tests/PHPStan/Rules/Properties/data/bug-4559.php | 14 ++++++++++++++ tests/PHPStan/Rules/Properties/data/bug-6809.php | 14 ++++++++++++++ 4 files changed, 41 insertions(+) create mode 100644 tests/PHPStan/Rules/Properties/data/bug-4559.php create mode 100644 tests/PHPStan/Rules/Properties/data/bug-6809.php diff --git a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php index b389fa5940..ddfabe3eaf 100644 --- a/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php +++ b/tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php @@ -570,4 +570,12 @@ public function testDynamicProperties(): void $this->analyse([__DIR__ . '/data/dynamic-properties.php'], []); } + + public function testBug4559(): void + { + $this->checkThisOnly = false; + $this->checkUnionTypes = true; + $this->analyse([__DIR__ . '/data/bug-4559.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Properties/AccessStaticPropertiesRuleTest.php b/tests/PHPStan/Rules/Properties/AccessStaticPropertiesRuleTest.php index 44fe1c71fb..e5bdc4d0b7 100644 --- a/tests/PHPStan/Rules/Properties/AccessStaticPropertiesRuleTest.php +++ b/tests/PHPStan/Rules/Properties/AccessStaticPropertiesRuleTest.php @@ -207,4 +207,9 @@ public function testBug5143(): void $this->analyse([__DIR__ . '/data/bug-5143.php'], []); } + public function testBug6809(): void + { + $this->analyse([__DIR__ . '/data/bug-6809.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Properties/data/bug-4559.php b/tests/PHPStan/Rules/Properties/data/bug-4559.php new file mode 100644 index 0000000000..30136d761b --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/bug-4559.php @@ -0,0 +1,14 @@ +error->code)) { + echo $response->error->message ?? ''; + } + } +} diff --git a/tests/PHPStan/Rules/Properties/data/bug-6809.php b/tests/PHPStan/Rules/Properties/data/bug-6809.php new file mode 100644 index 0000000000..54671e11e2 --- /dev/null +++ b/tests/PHPStan/Rules/Properties/data/bug-6809.php @@ -0,0 +1,14 @@ + Date: Wed, 20 Apr 2022 19:47:33 +0900 Subject: [PATCH 7/8] cs fix --- src/Analyser/NodeScopeResolver.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 1f2d942960..50ca42c6db 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -127,7 +127,6 @@ use PHPStan\Type\NeverType; use PHPStan\Type\NullType; use PHPStan\Type\ObjectType; -use PHPStan\Type\ObjectWithoutClassType; use PHPStan\Type\StaticType; use PHPStan\Type\StaticTypeFactory; use PHPStan\Type\StringType; From b1fadee3d50b9bbf448ac3487e161d70e6983d23 Mon Sep 17 00:00:00 2001 From: Yohta Kimura Date: Wed, 20 Apr 2022 20:02:46 +0900 Subject: [PATCH 8/8] fix levels test for coalesce --- tests/PHPStan/Levels/data/coalesce-2.json | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 tests/PHPStan/Levels/data/coalesce-2.json diff --git a/tests/PHPStan/Levels/data/coalesce-2.json b/tests/PHPStan/Levels/data/coalesce-2.json deleted file mode 100644 index 3f41943cf7..0000000000 --- a/tests/PHPStan/Levels/data/coalesce-2.json +++ /dev/null @@ -1,7 +0,0 @@ -[ - { - "message": "Access to an undefined property ReflectionClass::$nonexistent.", - "line": 11, - "ignorable": true - } -] \ No newline at end of file