From 2b50d5622dc126b3d1a6e8b4969b00d47c875539 Mon Sep 17 00:00:00 2001 From: "l.gersen@visymo.com" Date: Wed, 22 Jan 2020 15:23:47 +0100 Subject: [PATCH] Add support for method calls, PropertyFetch and StaticPropertyFetch --- src/Rules/Variables/CoalesceRule.php | 126 +++++++++++++++--- .../Rules/Variables/CoalesceRuleTest.php | 91 +++++++++++-- .../Rules/Variables/data/coalesce-assign.php | 68 +++++++++- .../PHPStan/Rules/Variables/data/coalesce.php | 64 ++++++++- 4 files changed, 315 insertions(+), 34 deletions(-) diff --git a/src/Rules/Variables/CoalesceRule.php b/src/Rules/Variables/CoalesceRule.php index 83827385062..d8032d45dfd 100644 --- a/src/Rules/Variables/CoalesceRule.php +++ b/src/Rules/Variables/CoalesceRule.php @@ -3,10 +3,14 @@ namespace PHPStan\Rules\Variables; use PhpParser\Node; +use PhpParser\Node\Expr; use PHPStan\Analyser\Scope; +use PHPStan\Rules\Properties\PropertyDescriptor; +use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\RuleError; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\NullType; +use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; /** @@ -15,6 +19,21 @@ class CoalesceRule implements \PHPStan\Rules\Rule { + /** @var \PHPStan\Rules\Properties\PropertyDescriptor */ + private $propertyDescriptor; + + /** @var \PHPStan\Rules\Properties\PropertyReflectionFinder */ + private $propertyReflectionFinder; + + public function __construct( + PropertyDescriptor $propertyDescriptor, + PropertyReflectionFinder $propertyReflectionFinder + ) + { + $this->propertyDescriptor = $propertyDescriptor; + $this->propertyReflectionFinder = $propertyReflectionFinder; + } + public function getNodeType(): string { return \PhpParser\Node\Expr::class; @@ -39,7 +58,23 @@ public function processNode(Node $node, Scope $scope): array private function canBeCoalesced(Node $node, Scope $scope, string $action, ?RuleError $error = null): ?RuleError { - if ($node instanceof Node\Expr\Variable && is_string($node->name)) { + if ($node instanceof \PhpParser\Node\Expr\FuncCall || + $node instanceof \PhpParser\Node\Expr\MethodCall || + $node instanceof \PhpParser\Node\Expr\StaticCall) { + + if ($node->name instanceof Expr) { + return null; + } + + $resultingType = $scope->getType($node); + + $error = $this->generateError( + $resultingType, + $node, + sprintf('%s of return value for call to \'%s\'', $action, $node->name->toString()) + ); + + } elseif ($node instanceof Node\Expr\Variable && is_string($node->name)) { $hasVariable = $scope->hasVariableType($node->name); @@ -51,11 +86,16 @@ private function canBeCoalesced(Node $node, Scope $scope, string $action, ?RuleE $variableType = $scope->getVariableType($node->name); - // For hasVariable->maybe a coalesce is valid - if ($hasVariable->yes() && $variableType->isSuperTypeOf(new NullType())->no()) { - return $error ?? RuleErrorBuilder::message( - sprintf('%s of variable $%s, which cannot be null.', $action, $node->name) - )->line($node->getLine())->build(); + if ($hasVariable->maybe()) { + return null; + } + + if ($hasVariable->yes()) { + return $error ?? $this->generateError( + $variableType, + $node, + sprintf('%s of variable $%s', $action, $node->name) + ); } } elseif ($node instanceof Node\Expr\ArrayDimFetch && $node->dim !== null) { @@ -81,24 +121,76 @@ private function canBeCoalesced(Node $node, Scope $scope, string $action, ?RuleE // If offset is cannot be null, store this error message and see if one of the earlier offsets is. // E.g. $array['a']['b']['c'] ?? null; is a valid coalesce if a OR b or C might be null. - if ($hasOffsetValue->yes() && $type->getOffsetValueType($dimType)->isSuperTypeOf(new NullType())->no()) { - $error = RuleErrorBuilder::message( - sprintf( - '%s of offset %s on %s, which cannot be null.', - $action, - $dimType->describe(VerbosityLevel::value()), - $type->describe(VerbosityLevel::value()) - ) - )->line($node->getLine())->build(); - - return $this->canBeCoalesced($node->var, $scope, $action, $error); + if ($hasOffsetValue->yes()) { + + $error = $error ?? $this->generateError($type->getOffsetValueType($dimType), $node, sprintf( + '%s of offset %s on %s', + $action, + $dimType->describe(VerbosityLevel::value()), + $type->describe(VerbosityLevel::value()) + )); + + if ($error !== null) { + return $this->canBeCoalesced($node->var, $scope, $action, $error); + } } // Has offset, it is nullable return null; + + } elseif ($node instanceof Node\Expr\PropertyFetch || $node instanceof Node\Expr\StaticPropertyFetch) { + + $propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($node, $scope); + + if ($propertyReflection === null) { + return null; + } + + $propertyDescription = $this->propertyDescriptor->describeProperty($propertyReflection, $node); + $propertyType = $propertyReflection->getWritableType(); + + $error = $error ?? $this->generateError( + $propertyReflection->getWritableType(), + $node, + sprintf('%s of %s (%s)', $action, $propertyDescription, $propertyType->describe(VerbosityLevel::typeOnly())) + ); + + if ($error !== null && property_exists($node, 'var')) { + return $this->canBeCoalesced($node->var, $scope, $action, $error); + } + + } + + return $error; + } + + private function generateError(Type $type, Node $node, string $message): ?RuleError + { + $nullType = new NullType(); + + if ($type->equals($nullType)) { + return $this->generateAlwaysNullError($node, $message); + } + + if ($type->isSuperTypeOf($nullType)->no()) { + return $this->generateNeverNullError($node, $message); } return null; } + private function generateAlwaysNullError(Node $node, string $message): RuleError + { + return RuleErrorBuilder::message( + sprintf('%s, which is always null.', $message) + )->line($node->getLine())->build(); + } + + private function generateNeverNullError(Node $node, string $message): RuleError + { + return RuleErrorBuilder::message( + sprintf('%s, which cannot be null.', $message) + )->line($node->getLine())->build(); + } + } diff --git a/tests/PHPStan/Rules/Variables/CoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/CoalesceRuleTest.php index 21916edf0f0..05c57c81362 100644 --- a/tests/PHPStan/Rules/Variables/CoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/CoalesceRuleTest.php @@ -2,6 +2,9 @@ namespace PHPStan\Rules\Variables; +use PHPStan\Rules\Properties\PropertyDescriptor; +use PHPStan\Rules\Properties\PropertyReflectionFinder; + /** * @extends \PHPStan\Testing\RuleTestCase */ @@ -10,35 +13,68 @@ class CoalesceRuleTest extends \PHPStan\Testing\RuleTestCase protected function getRule(): \PHPStan\Rules\Rule { - return new CoalesceRule(); + return new CoalesceRule(new PropertyDescriptor(), new PropertyReflectionFinder()); } public function testCoalesceRule(): void { + require_once __DIR__ . '/data/coalesce.php'; $this->analyse([__DIR__ . '/data/coalesce.php'], [ + [ + 'Coalesce of Property CoalesceRule\FooCoalesce::$string (string), which cannot be null.', + 32, + ], [ 'Coalesce of variable $scalar, which cannot be null.', - 8, + 41, ], [ 'Coalesce of invalid offset \'string\' on array(1, 2, 3).', - 12, + 45, ], [ 'Coalesce of invalid offset \'string\' on array(array(1), array(2), array(3)).', - 16, + 49, ], [ 'Coalesce of undefined variable $doesNotExist.', - 18, + 51, ], [ 'Coalesce of offset \'dim\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()), which cannot be null.', - 34, + 67, ], [ 'Coalesce of invalid offset \'b\' on array().', - 46, + 79, + ], + [ + 'Coalesce of return value for call to \'rand\', which cannot be null.', + 81, + ], + [ + 'Coalesce of Property CoalesceRule\FooCoalesce::$string (string), which cannot be null.', + 89, + ], + [ + 'Coalesce of Property CoalesceRule\FooCoalesce::$alwaysNull (null), which is always null.', + 91, + ], + [ + 'Coalesce of Property CoalesceRule\FooCoalesce::$string (string), which cannot be null.', + 93, + ], + [ + 'Coalesce of Static property CoalesceRule\FooCoalesce::$staticString (string), which cannot be null.', + 99, + ], + [ + 'Coalesce of Static property CoalesceRule\FooCoalesce::$staticAlwaysNull (null), which is always null.', + 101, + ], + [ + 'Coalesce of variable $a, which is always null.', + 115, ], ]); } @@ -49,30 +85,59 @@ public function testCoalesceAssignRule(): void $this->markTestSkipped('Test requires PHP 7.4.'); } + require_once __DIR__ . '/data/coalesce-assign.php'; $this->analyse([__DIR__ . '/data/coalesce-assign.php'], [ + [ + 'Null-coalescing assignment of Property CoalesceAssignRule\FooCoalesce::$string (string), which cannot be null.', + 32, + ], [ 'Null-coalescing assignment of variable $scalar, which cannot be null.', - 8, + 41, ], [ 'Null-coalescing assignment of invalid offset \'string\' on array(1, 2, 3).', - 12, + 45, ], [ 'Null-coalescing assignment of invalid offset \'string\' on array(array(1), array(2), array(3)).', - 16, + 49, ], [ 'Null-coalescing assignment of undefined variable $doesNotExist.', - 18, + 51, ], [ 'Null-coalescing assignment of offset \'dim\' on array(\'dim\' => 1, \'dim-null\' => 1|null, \'dim-null-offset\' => array(\'a\' => true|null), \'dim-empty\' => array()), which cannot be null.', - 34, + 67, ], [ 'Null-coalescing assignment of invalid offset \'b\' on array().', - 46, + 79, + ], + [ + 'Null-coalescing assignment of Property CoalesceAssignRule\FooCoalesce::$string (string), which cannot be null.', + 89, + ], + [ + 'Null-coalescing assignment of Property CoalesceAssignRule\FooCoalesce::$alwaysNull (null), which is always null.', + 91, + ], + [ + 'Null-coalescing assignment of Property CoalesceAssignRule\FooCoalesce::$string (string), which cannot be null.', + 93, + ], + [ + 'Null-coalescing assignment of Static property CoalesceAssignRule\FooCoalesce::$staticString (string), which cannot be null.', + 99, + ], + [ + 'Null-coalescing assignment of Static property CoalesceAssignRule\FooCoalesce::$staticAlwaysNull (null), which is always null.', + 101, + ], + [ + 'Null-coalescing assignment of variable $a, which is always null.', + 115, ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/coalesce-assign.php b/tests/PHPStan/Rules/Variables/data/coalesce-assign.php index 28e611b04f6..df69c39680e 100644 --- a/tests/PHPStan/Rules/Variables/data/coalesce-assign.php +++ b/tests/PHPStan/Rules/Variables/data/coalesce-assign.php @@ -1,6 +1,39 @@ = 7.4 -function coalesceAssign () +namespace CoalesceAssignRule; + +class FooCoalesce +{ + /** @var string|null */ + public static $staticStringOrNull = null; + + /** @var string */ + public static $staticString = ''; + + /** @var null */ + public static $staticAlwaysNull; + + /** @var string|null */ + public $stringOrNull = null; + + /** @var string */ + public $string = ''; + + /** @var null */ + public $alwaysNull; + + /** @var FooCoalesce|null */ + public $fooCoalesceOrNull; + + /** @var FooCoalesce */ + public $fooCoalesce; + + public function thisCoalesce() { + echo $this->string ??= null; + } +} + +function coalesce() { $scalar = 3; @@ -17,7 +50,7 @@ function coalesceAssign () echo $doesNotExist ??= 0; - if(rand() > 0.5) { + if (rand() > 0.5) { $maybeVariable = 3; } @@ -44,12 +77,41 @@ function coalesceAssign () // Always set, then never set echo $fixedDimArray['dim-empty']['b'] ??= 0; + +// echo rand() ??= 0; // not valid for assignment + +// echo preg_replace('', '', '') ??= 0; // not valid for assignment + + $foo = new FooCoalesce(); + + echo $foo->stringOrNull ??= ''; + + echo $foo->string ??= ''; + + echo $foo->alwaysNull ??= ''; + + echo $foo->fooCoalesce->string ??= ''; + + echo $foo->fooCoalesceOrNull->string ??= ''; + + echo FooCoalesce::$staticStringOrNull ??= ''; + + echo FooCoalesce::$staticString ??= ''; + + echo FooCoalesce::$staticAlwaysNull ??= ''; } /** * @param array $array */ -function coalesceAssignStringOffset(array $array) +function coalesceStringOffset(array $array) { echo $array['string'] ??= 0; } + +function alwaysNullCoalesce (?string $a): void +{ + if (!is_string($a)) { + echo $a ??= 'foo'; + } +} diff --git a/tests/PHPStan/Rules/Variables/data/coalesce.php b/tests/PHPStan/Rules/Variables/data/coalesce.php index cc8e29004ea..8f9af52a120 100644 --- a/tests/PHPStan/Rules/Variables/data/coalesce.php +++ b/tests/PHPStan/Rules/Variables/data/coalesce.php @@ -1,5 +1,38 @@ string ?? null; + } +} + function coalesce() { @@ -17,7 +50,7 @@ function coalesce() echo $doesNotExist ?? 0; - if(rand() > 0.5) { + if (rand() > 0.5) { $maybeVariable = 3; } @@ -44,6 +77,28 @@ function coalesce() // Always set, then never set echo $fixedDimArray['dim-empty']['b'] ?? 0; + + echo rand() ?? 0; + + echo preg_replace('', '', '') ?? 0; + + $foo = new FooCoalesce(); + + echo $foo->stringOrNull ?? ''; + + echo $foo->string ?? ''; + + echo $foo->alwaysNull ?? ''; + + echo $foo->fooCoalesce->string ?? ''; + + echo $foo->fooCoalesceOrNull->string ?? ''; + + echo FooCoalesce::$staticStringOrNull ?? ''; + + echo FooCoalesce::$staticString ?? ''; + + echo FooCoalesce::$staticAlwaysNull ?? ''; } /** @@ -53,3 +108,10 @@ function coalesceStringOffset(array $array) { echo $array['string'] ?? 0; } + +function alwaysNullCoalesce (?string $a): void +{ + if (!is_string($a)) { + echo $a ?? 'foo'; + } +}