From 25b61d974f5ca1cb00d7d2be4dce750553787b87 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Sun, 1 Mar 2020 13:32:17 +0100 Subject: [PATCH] Isset rule for checking always-defined/never-defined properties, array offsets etc. (level 4) - bleeding edge --- conf/config.level4.neon | 5 + src/Reflection/ResolvedPropertyReflection.php | 5 + src/Rules/IssetCheck.php | 12 +- .../Properties/FoundPropertyReflection.php | 17 ++- src/Rules/Variables/IssetRule.php | 42 +++++ src/Rules/Variables/NullCoalesceRule.php | 4 +- .../PHPStan/Rules/Variables/IssetRuleTest.php | 84 ++++++++++ .../Rules/Variables/NullCoalesceRuleTest.php | 10 +- tests/PHPStan/Rules/Variables/data/isset.php | 143 ++++++++++++++++++ .../Rules/Variables/data/null-coalesce.php | 5 + 10 files changed, 315 insertions(+), 12 deletions(-) create mode 100644 src/Rules/Variables/IssetRule.php create mode 100644 tests/PHPStan/Rules/Variables/IssetRuleTest.php create mode 100644 tests/PHPStan/Rules/Variables/data/isset.php diff --git a/conf/config.level4.neon b/conf/config.level4.neon index a5211b0582..a879768acd 100644 --- a/conf/config.level4.neon +++ b/conf/config.level4.neon @@ -15,6 +15,8 @@ rules: - PHPStan\Rules\TooWideTypehints\TooWideFunctionReturnTypehintRule conditionalTags: + PHPStan\Rules\Variables\IssetRule: + phpstan.rules.rule: %featureToggles.nullCoalesce% PHPStan\Rules\Variables\NullCoalesceRule: phpstan.rules.rule: %featureToggles.nullCoalesce% @@ -121,5 +123,8 @@ services: tags: - phpstan.rules.rule + - + class: PHPStan\Rules\Variables\IssetRule + - class: PHPStan\Rules\Variables\NullCoalesceRule diff --git a/src/Reflection/ResolvedPropertyReflection.php b/src/Reflection/ResolvedPropertyReflection.php index a629bbd133..0d8042bc90 100644 --- a/src/Reflection/ResolvedPropertyReflection.php +++ b/src/Reflection/ResolvedPropertyReflection.php @@ -29,6 +29,11 @@ public function __construct(PropertyReflection $reflection, TemplateTypeMap $tem $this->templateTypeMap = $templateTypeMap; } + public function getOriginalReflection(): PropertyReflection + { + return $this->reflection; + } + public function getDeclaringClass(): ClassReflection { return $this->reflection->getDeclaringClass(); diff --git a/src/Rules/IssetCheck.php b/src/Rules/IssetCheck.php index 4cb847af48..922e5a287e 100644 --- a/src/Rules/IssetCheck.php +++ b/src/Rules/IssetCheck.php @@ -50,7 +50,7 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, ?Ru if ($hasOffsetValue->no()) { return $error ?? RuleErrorBuilder::message( sprintf( - 'Offset %s on %s on left side of %s does not exist.', + 'Offset %s on %s %s does not exist.', $dimType->describe(VerbosityLevel::value()), $type->describe(VerbosityLevel::value()), $operatorDescription @@ -67,7 +67,7 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, ?Ru if ($hasOffsetValue->yes()) { $error = $error ?? $this->generateError($type->getOffsetValueType($dimType), sprintf( - 'Offset %s on %s on left side of %s always exists and', + 'Offset %s on %s %s always exists and', $dimType->describe(VerbosityLevel::value()), $type->describe(VerbosityLevel::value()), $operatorDescription @@ -89,12 +89,16 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, ?Ru return null; } + if (!$propertyReflection->isNative()) { + return null; + } + $propertyDescription = $this->propertyDescriptor->describeProperty($propertyReflection, $expr); $propertyType = $propertyReflection->getWritableType(); $error = $error ?? $this->generateError( $propertyReflection->getWritableType(), - sprintf('%s (%s) on left side of %s', $propertyDescription, $propertyType->describe(VerbosityLevel::typeOnly()), $operatorDescription) + sprintf('%s (%s) %s', $propertyDescription, $propertyType->describe(VerbosityLevel::typeOnly()), $operatorDescription) ); if ($error !== null) { @@ -110,7 +114,7 @@ public function check(Expr $expr, Scope $scope, string $operatorDescription, ?Ru return $error; } - return $error ?? $this->generateError($scope->getType($expr), sprintf('Left side of %s', $operatorDescription)); + return $error ?? $this->generateError($scope->getType($expr), sprintf('Expression %s', $operatorDescription)); } private function generateError(Type $type, string $message): ?RuleError diff --git a/src/Rules/Properties/FoundPropertyReflection.php b/src/Rules/Properties/FoundPropertyReflection.php index 7e99013bd3..4a5efde71a 100644 --- a/src/Rules/Properties/FoundPropertyReflection.php +++ b/src/Rules/Properties/FoundPropertyReflection.php @@ -5,6 +5,7 @@ use PHPStan\Reflection\ClassReflection; use PHPStan\Reflection\Php\PhpPropertyReflection; use PHPStan\Reflection\PropertyReflection; +use PHPStan\Reflection\ResolvedPropertyReflection; use PHPStan\TrinaryLogic; use PHPStan\Type\Type; @@ -98,16 +99,26 @@ public function isInternal(): TrinaryLogic public function isNative(): bool { - return $this->originalPropertyReflection instanceof PhpPropertyReflection; + $reflection = $this->originalPropertyReflection; + if ($reflection instanceof ResolvedPropertyReflection) { + $reflection = $reflection->getOriginalReflection(); + } + + return $reflection instanceof PhpPropertyReflection; } public function getNativeType(): ?Type { - if (!$this->originalPropertyReflection instanceof PhpPropertyReflection) { + $reflection = $this->originalPropertyReflection; + if ($reflection instanceof ResolvedPropertyReflection) { + $reflection = $reflection->getOriginalReflection(); + } + + if (!$reflection instanceof PhpPropertyReflection) { return null; } - return $this->originalPropertyReflection->getNativeType(); + return $reflection->getNativeType(); } } diff --git a/src/Rules/Variables/IssetRule.php b/src/Rules/Variables/IssetRule.php new file mode 100644 index 0000000000..13bd73a003 --- /dev/null +++ b/src/Rules/Variables/IssetRule.php @@ -0,0 +1,42 @@ + + */ +class IssetRule implements \PHPStan\Rules\Rule +{ + + /** @var IssetCheck */ + private $issetCheck; + + public function __construct(IssetCheck $issetCheck) + { + $this->issetCheck = $issetCheck; + } + + public function getNodeType(): string + { + return Node\Expr\Isset_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $messages = []; + foreach ($node->vars as $var) { + $error = $this->issetCheck->check($var, $scope, 'in isset()'); + if ($error === null) { + continue; + } + $messages[] = $error; + } + + return $messages; + } + +} diff --git a/src/Rules/Variables/NullCoalesceRule.php b/src/Rules/Variables/NullCoalesceRule.php index b9edcba03e..2eaafaf089 100644 --- a/src/Rules/Variables/NullCoalesceRule.php +++ b/src/Rules/Variables/NullCoalesceRule.php @@ -28,9 +28,9 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { if ($node instanceof Node\Expr\BinaryOp\Coalesce) { - $error = $this->issetCheck->check($node->left, $scope, '??'); + $error = $this->issetCheck->check($node->left, $scope, 'on left side of ??'); } elseif ($node instanceof Node\Expr\AssignOp\Coalesce) { - $error = $this->issetCheck->check($node->var, $scope, '??='); + $error = $this->issetCheck->check($node->var, $scope, 'on left side of ??='); } else { return []; } diff --git a/tests/PHPStan/Rules/Variables/IssetRuleTest.php b/tests/PHPStan/Rules/Variables/IssetRuleTest.php new file mode 100644 index 0000000000..dfe66c011e --- /dev/null +++ b/tests/PHPStan/Rules/Variables/IssetRuleTest.php @@ -0,0 +1,84 @@ + + */ +class IssetRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new IssetRule(new IssetCheck(new PropertyDescriptor(), new PropertyReflectionFinder())); + } + + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/isset.php'], [ + [ + 'Property IssetRule\FooCoalesce::$string (string) in isset() is not nullable.', + 32, + ], + [ + 'Offset \'string\' on array(1, 2, 3) in isset() does not exist.', + 45, + ], + [ + 'Offset \'string\' on array(array(1), array(2), array(3)) in isset() does not exist.', + 49, + ], + [ + 'Offset \'dim\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()) in isset() always exists and is not nullable.', + 67, + ], + [ + 'Offset \'b\' on array() in isset() does not exist.', + 79, + ], + [ + 'Property IssetRule\FooCoalesce::$string (string) in isset() is not nullable.', + 85, + ], + [ + 'Property IssetRule\FooCoalesce::$alwaysNull (null) in isset() is always null.', + 87, + ], + [ + 'Property IssetRule\FooCoalesce::$string (string) in isset() is not nullable.', + 89, + ], + [ + 'Static property IssetRule\FooCoalesce::$staticString (string) in isset() is not nullable.', + 95, + ], + [ + 'Static property IssetRule\FooCoalesce::$staticAlwaysNull (null) in isset() is always null.', + 97, + ], + [ + 'Property IssetRule\FooCoalesce::$string (string) in isset() is not nullable.', + 116, + ], + [ + 'Property IssetRule\FooCoalesce::$alwaysNull (null) in isset() is always null.', + 118, + ], + [ + 'Static property IssetRule\FooCoalesce::$staticAlwaysNull (null) in isset() is always null.', + 123, + ], + [ + 'Static property IssetRule\FooCoalesce::$staticString (string) in isset() is not nullable.', + 124, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index a1fd0c694e..5a77cdabbe 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -42,7 +42,7 @@ public function testCoalesceRule(): void 79, ], [ - 'Left side of ?? is not nullable.', + 'Expression on left side of ?? is not nullable.', 81, ], [ @@ -74,11 +74,11 @@ public function testCoalesceRule(): void 122, ], [ - 'Left side of ?? is not nullable.', + 'Expression on left side of ?? is not nullable.', 124, ], [ - 'Left side of ?? is always null.', + 'Expression on left side of ?? is always null.', 125, ], [ @@ -89,6 +89,10 @@ public function testCoalesceRule(): void 'Static property CoalesceRule\FooCoalesce::$staticString (string) on left side of ?? is not nullable.', 131, ], + [ + 'Property ReflectionClass::$name (class-string) on left side of ?? is not nullable.', + 136, + ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/isset.php b/tests/PHPStan/Rules/Variables/data/isset.php new file mode 100644 index 0000000000..dca6dac087 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/isset.php @@ -0,0 +1,143 @@ +string) ? $this->string : null; + } +} + +function coalesce() +{ + + $scalar = 3; + + echo isset($scalar) ? $scalar : 4; + + $array = [1, 2, 3]; + + echo isset($array['string']) ? $array['string'] : 0; + + $multiDimArray = [[1], [2], [3]]; + + echo isset($multiDimArray['string']) ? $multiDimArray['string'] : 0; + + echo isset($doesNotExist) ? $doesNotExist : 0; + + if (rand() > 0.5) { + $maybeVariable = 3; + } + + echo isset($maybeVariable) ? $maybeVariable : 0; + + $fixedDimArray = [ + 'dim' => 1, + 'dim-null' => rand() > 0.5 ? null : 1, + 'dim-null-offset' => ['a' => rand() > 0.5 ? true : null], + 'dim-empty' => [] + ]; + + // Always set + echo isset($fixedDimArray['dim']) ? $fixedDimArray['dim'] : 0; + + // Maybe set + echo isset($fixedDimArray['dim-null']) ? $fixedDimArray['dim-null'] : 0; + + // Never set, then unknown + echo isset($fixedDimArray['dim-null-not-set']['a']) ? $fixedDimArray['dim-null-not-set']['a'] : 0; + + // Always set, then always set + echo isset($fixedDimArray['dim-null-offset']['a']) ? $fixedDimArray['dim-null-offset']['a'] : 0; + + // Always set, then never set + echo isset($fixedDimArray['dim-empty']['b']) ? $fixedDimArray['dim-empty']['b'] : 0; + + $foo = new FooCoalesce(); + + echo isset($foo->stringOrNull) ? $foo->stringOrNull : ''; + + echo isset($foo->string) ? $foo->string : ''; + + echo isset($foo->alwaysNull) ? $foo->alwaysNull : ''; + + echo isset($foo->fooCoalesce->string) ? $foo->fooCoalesce->string : ''; + + echo isset($foo->fooCoalesceOrNull->string) ? $foo->fooCoalesceOrNull->string : ''; + + echo isset(FooCoalesce::$staticStringOrNull) ? FooCoalesce::$staticStringOrNull : ''; + + echo isset(FooCoalesce::$staticString) ? FooCoalesce::$staticString : ''; + + echo isset(FooCoalesce::$staticAlwaysNull) ? FooCoalesce::$staticAlwaysNull : ''; +} + +/** + * @param array $array + */ +function coalesceStringOffset(array $array) +{ + echo isset($array['string']) ? $array['string'] : 0; +} + +function alwaysNullCoalesce (?string $a): void +{ + if (!is_string($a)) { + echo isset($a) ? $a : 'foo'; + } +} + +function (): void { + echo isset((new FooCoalesce())->string) ? (new FooCoalesce())->string : 'foo'; + echo isset((new FooCoalesce())->stringOrNull) ? (new FooCoalesce())->stringOrNull : 'foo'; + echo isset((new FooCoalesce())->alwaysNull) ? (new FooCoalesce())->alwaysNull : 'foo'; +}; + +function (FooCoalesce $foo): void +{ + echo isset($foo::$staticAlwaysNull) ? $foo::$staticAlwaysNull : 'foo'; + echo isset($foo::$staticString) ? $foo::$staticString : 'foo'; + echo isset($foo::$staticStringOrNull) ? $foo::$staticStringOrNull : 'foo'; +}; + +/** + * @property int $integerProperty + * @property FooCoalesce $foo + */ +class SomeMagicProperties +{ + +} + +function (SomeMagicProperties $foo, \stdClass $std): void { + echo isset($foo->integerProperty) ? $foo->integerProperty : null; + + echo isset($foo->foo->string) ? $foo->foo->string : null; + + echo isset($std->foo) ? $std->foo : null; +}; diff --git a/tests/PHPStan/Rules/Variables/data/null-coalesce.php b/tests/PHPStan/Rules/Variables/data/null-coalesce.php index b53f16cc74..a694fa67aa 100644 --- a/tests/PHPStan/Rules/Variables/data/null-coalesce.php +++ b/tests/PHPStan/Rules/Variables/data/null-coalesce.php @@ -131,3 +131,8 @@ function (FooCoalesce $foo): void echo $foo::$staticString ?? 'foo'; echo $foo::$staticStringOrNull ?? 'foo'; }; + +function (\ReflectionClass $ref): void { + echo $ref->name ?? 'foo'; + echo $ref->nonexistent ?? 'bar'; +};