Skip to content

Commit

Permalink
Isset rule for checking always-defined/never-defined properties, arra…
Browse files Browse the repository at this point in the history
…y offsets etc. (level 4) - bleeding edge
  • Loading branch information
ondrejmirtes committed Mar 1, 2020
1 parent 70ee0a4 commit 25b61d9
Show file tree
Hide file tree
Showing 10 changed files with 315 additions and 12 deletions.
5 changes: 5 additions & 0 deletions conf/config.level4.neon
Original file line number Diff line number Diff line change
Expand Up @@ -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%

Expand Down Expand Up @@ -121,5 +123,8 @@ services:
tags:
- phpstan.rules.rule

-
class: PHPStan\Rules\Variables\IssetRule

-
class: PHPStan\Rules\Variables\NullCoalesceRule
5 changes: 5 additions & 0 deletions src/Reflection/ResolvedPropertyReflection.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
12 changes: 8 additions & 4 deletions src/Rules/IssetCheck.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -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
Expand Down
17 changes: 14 additions & 3 deletions src/Rules/Properties/FoundPropertyReflection.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
}

}
42 changes: 42 additions & 0 deletions src/Rules/Variables/IssetRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Variables;

use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Rules\IssetCheck;

/**
* @implements \PHPStan\Rules\Rule<Node\Expr\Isset_>
*/
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;
}

}
4 changes: 2 additions & 2 deletions src/Rules/Variables/NullCoalesceRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 [];
}
Expand Down
84 changes: 84 additions & 0 deletions tests/PHPStan/Rules/Variables/IssetRuleTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Variables;

use PHPStan\Rules\IssetCheck;
use PHPStan\Rules\Properties\PropertyDescriptor;
use PHPStan\Rules\Properties\PropertyReflectionFinder;
use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;

/**
* @extends RuleTestCase<IssetRule>
*/
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,
],
]);
}

}
10 changes: 7 additions & 3 deletions tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public function testCoalesceRule(): void
79,
],
[
'Left side of ?? is not nullable.',
'Expression on left side of ?? is not nullable.',
81,
],
[
Expand Down Expand Up @@ -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,
],
[
Expand All @@ -89,6 +89,10 @@ public function testCoalesceRule(): void
'Static property CoalesceRule\FooCoalesce::$staticString (string) on left side of ?? is not nullable.',
131,
],
[
'Property ReflectionClass<object>::$name (class-string<object>) on left side of ?? is not nullable.',
136,
],
]);
}

Expand Down
Loading

0 comments on commit 25b61d9

Please sign in to comment.