From c35860b692a0888d0719ee793b739c141262fffc Mon Sep 17 00:00:00 2001 From: "l.gersen@visymo.com" Date: Tue, 26 Nov 2019 11:19:41 +0100 Subject: [PATCH 1/5] Add rule checking arguments to unset (#2605) --- src/Rules/Functions/UnsetRule.php | 74 +++++++++++++++++++ .../PHPStan/Rules/Functions/UnsetRuleTest.php | 46 ++++++++++++ tests/PHPStan/Rules/Functions/data/unset.php | 31 ++++++++ 3 files changed, 151 insertions(+) create mode 100644 src/Rules/Functions/UnsetRule.php create mode 100644 tests/PHPStan/Rules/Functions/UnsetRuleTest.php create mode 100644 tests/PHPStan/Rules/Functions/data/unset.php diff --git a/src/Rules/Functions/UnsetRule.php b/src/Rules/Functions/UnsetRule.php new file mode 100644 index 0000000000..18cfc44c7c --- /dev/null +++ b/src/Rules/Functions/UnsetRule.php @@ -0,0 +1,74 @@ +checkMaybeUndefinedVariables = $checkMaybeUndefinedVariables; + } + + public function getNodeType(): string + { + return Node\Stmt\Unset_::class; + } + + public function processNode(Node $node, Scope $scope): array + { + /** @var Node\Stmt\Unset_ $node */ + $functionArguments = $node->vars; + $messages = []; + + foreach ($functionArguments as $argument) { + $this->canBeUnset($argument, $scope, $messages); + } + + return $messages; + } + + private function canBeUnset(Node $node, Scope $scope, array &$messages): void + { + if ($node instanceof Node\Expr\Variable) { + $scopeHasVariable = $scope->hasVariableType($node->name); + + if ($scopeHasVariable->no()) { + $messages[] = RuleErrorBuilder::message( + sprintf('Call to function unset() contains undefined variable $%s.', $node->name) + )->line($node->getLine())->build(); + } elseif ($this->checkMaybeUndefinedVariables && $scopeHasVariable->maybe()) { + $messages[] = RuleErrorBuilder::message( + sprintf('Call to function unset() contains possibly undefined variable $%s.', $node->name) + )->line($node->getLine())->build(); + } + } + + if ($node instanceof Node\Expr\ArrayDimFetch) { + $type = $scope->getType($node->var); + $dimType = $scope->getType($node->dim); + + $isInaccessibleIterable = $type instanceof IterableType && $type->getIterableKeyType()->isSuperTypeOf($dimType)->no(); + + if ($isInaccessibleIterable || $type->hasOffsetValueType($dimType)->no()) { + $messages[] = RuleErrorBuilder::message( + sprintf( + 'Cannot unset offset %s on %s.', + $dimType->describe(VerbosityLevel::value()), + $type->describe(VerbosityLevel::value()) + ) + )->line($node->getLine())->build(); + } + } + } + +} diff --git a/tests/PHPStan/Rules/Functions/UnsetRuleTest.php b/tests/PHPStan/Rules/Functions/UnsetRuleTest.php new file mode 100644 index 0000000000..fd8f5c6423 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/UnsetRuleTest.php @@ -0,0 +1,46 @@ +analyse([__DIR__ . '/data/unset.php'], [ + [ + 'Call to function unset() contains undefined variable $notSetVariable.', + 3, + ], + [ + 'Cannot unset offset \'a\' on 3.', + 7, + ], + [ + 'Cannot unset offset \'b\' on 1.', + 11, + ], + [ + 'Cannot unset offset \'c\' on 1.', + 15, + ], + [ + 'Cannot unset offset \'b\' on 1.', + 15, + ], + [ + 'Call to function unset() contains possibly undefined variable $maybeSet.', + 21, + ], + [ + 'Cannot unset offset \'string\' on iterable.', + 30, + ], + ]); + } +} diff --git a/tests/PHPStan/Rules/Functions/data/unset.php b/tests/PHPStan/Rules/Functions/data/unset.php new file mode 100644 index 0000000000..1c660c5225 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/unset.php @@ -0,0 +1,31 @@ + 1]; + +unset($singleDimArray['a']['b']); + +$multiDimArray = ['a' => ['b' => 1]]; + +unset($multiDimArray['a']['b']['c'], $scalar, $singleDimArray['a']['b']); + +if (rand()) { + $maybeSet = 1; +} + +unset($maybeSet); + +/** @param iterable $iterable */ +function unsetOnMaybeIterable(iterable $iterable) { + unset($iterable['string']); +} + +/** @param iterable $iterable */ +function unsetOnYesIterable(iterable $iterable) { + unset($iterable['string']); +} From f0994b9acf1cada4cac030ed238c209f75c3e4f2 Mon Sep 17 00:00:00 2001 From: "l.gersen@visymo.com" Date: Tue, 26 Nov 2019 13:50:29 +0100 Subject: [PATCH 2/5] Satisfy build checks --- src/Rules/Functions/UnsetRule.php | 18 ++++++++++++++---- .../PHPStan/Rules/Functions/UnsetRuleTest.php | 3 +++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/Rules/Functions/UnsetRule.php b/src/Rules/Functions/UnsetRule.php index 18cfc44c7c..f96cab4284 100644 --- a/src/Rules/Functions/UnsetRule.php +++ b/src/Rules/Functions/UnsetRule.php @@ -8,6 +8,9 @@ use PHPStan\Type\IterableType; use PHPStan\Type\VerbosityLevel; +/** + * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Stmt\Unset_> + */ class UnsetRule implements \PHPStan\Rules\Rule { @@ -26,6 +29,10 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { + if (!$node instanceof Node\Stmt\Unset_) { + return []; + } + /** @var Node\Stmt\Unset_ $node */ $functionArguments = $node->vars; $messages = []; @@ -37,9 +44,14 @@ public function processNode(Node $node, Scope $scope): array return $messages; } + /** + * @param Node $node + * @param Scope $scope + * @param string[] $messages + */ private function canBeUnset(Node $node, Scope $scope, array &$messages): void { - if ($node instanceof Node\Expr\Variable) { + if ($node instanceof Node\Expr\Variable && is_string($node->name)) { $scopeHasVariable = $scope->hasVariableType($node->name); if ($scopeHasVariable->no()) { @@ -51,9 +63,7 @@ private function canBeUnset(Node $node, Scope $scope, array &$messages): void sprintf('Call to function unset() contains possibly undefined variable $%s.', $node->name) )->line($node->getLine())->build(); } - } - - if ($node instanceof Node\Expr\ArrayDimFetch) { + } elseif ($node instanceof Node\Expr\ArrayDimFetch && $node->dim !== null) { $type = $scope->getType($node->var); $dimType = $scope->getType($node->dim); diff --git a/tests/PHPStan/Rules/Functions/UnsetRuleTest.php b/tests/PHPStan/Rules/Functions/UnsetRuleTest.php index fd8f5c6423..2a8f902b04 100644 --- a/tests/PHPStan/Rules/Functions/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Functions/UnsetRuleTest.php @@ -2,6 +2,9 @@ namespace PHPStan\Rules\Functions; +/** + * @extends \PHPStan\Testing\RuleTestCase + */ class UnsetRuleTest extends \PHPStan\Testing\RuleTestCase { protected function getRule(): \PHPStan\Rules\Rule From bb447920d28d728ea0c18a4c29c6d72db99a1371 Mon Sep 17 00:00:00 2001 From: "l.gersen@visymo.com" Date: Tue, 26 Nov 2019 13:56:42 +0100 Subject: [PATCH 3/5] phpcs corrections --- src/Rules/Functions/UnsetRule.php | 2 +- tests/PHPStan/Rules/Functions/UnsetRuleTest.php | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Rules/Functions/UnsetRule.php b/src/Rules/Functions/UnsetRule.php index f96cab4284..d746228e3a 100644 --- a/src/Rules/Functions/UnsetRule.php +++ b/src/Rules/Functions/UnsetRule.php @@ -1,4 +1,4 @@ - Date: Tue, 26 Nov 2019 15:34:20 +0100 Subject: [PATCH 4/5] Apply review comments --- src/Rules/Functions/UnsetRule.php | 47 +++++++------------ .../PHPStan/Rules/Functions/UnsetRuleTest.php | 18 +++---- tests/PHPStan/Rules/Functions/data/unset.php | 27 ++++++----- 3 files changed, 37 insertions(+), 55 deletions(-) diff --git a/src/Rules/Functions/UnsetRule.php b/src/Rules/Functions/UnsetRule.php index d746228e3a..c1ba237d02 100644 --- a/src/Rules/Functions/UnsetRule.php +++ b/src/Rules/Functions/UnsetRule.php @@ -5,7 +5,6 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; use PHPStan\Rules\RuleErrorBuilder; -use PHPStan\Type\IterableType; use PHPStan\Type\VerbosityLevel; /** @@ -14,14 +13,6 @@ class UnsetRule implements \PHPStan\Rules\Rule { - /** @var bool */ - private $checkMaybeUndefinedVariables; - - public function __construct(bool $checkMaybeUndefinedVariables) - { - $this->checkMaybeUndefinedVariables = $checkMaybeUndefinedVariables; - } - public function getNodeType(): string { return Node\Stmt\Unset_::class; @@ -29,56 +20,50 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { - if (!$node instanceof Node\Stmt\Unset_) { - return []; - } - - /** @var Node\Stmt\Unset_ $node */ $functionArguments = $node->vars; $messages = []; foreach ($functionArguments as $argument) { - $this->canBeUnset($argument, $scope, $messages); + $message = $this->canBeUnset($argument, $scope); + + if (!$message) { + continue; + } + + $messages[] = $message; } return $messages; } - /** - * @param Node $node - * @param Scope $scope - * @param string[] $messages - */ - private function canBeUnset(Node $node, Scope $scope, array &$messages): void + private function canBeUnset(Node $node, Scope $scope): ?string { if ($node instanceof Node\Expr\Variable && is_string($node->name)) { $scopeHasVariable = $scope->hasVariableType($node->name); if ($scopeHasVariable->no()) { - $messages[] = RuleErrorBuilder::message( + return RuleErrorBuilder::message( sprintf('Call to function unset() contains undefined variable $%s.', $node->name) - )->line($node->getLine())->build(); - } elseif ($this->checkMaybeUndefinedVariables && $scopeHasVariable->maybe()) { - $messages[] = RuleErrorBuilder::message( - sprintf('Call to function unset() contains possibly undefined variable $%s.', $node->name) - )->line($node->getLine())->build(); + )->line($node->getLine())->build()->getMessage(); } } elseif ($node instanceof Node\Expr\ArrayDimFetch && $node->dim !== null) { $type = $scope->getType($node->var); $dimType = $scope->getType($node->dim); - $isInaccessibleIterable = $type instanceof IterableType && $type->getIterableKeyType()->isSuperTypeOf($dimType)->no(); + $isOffsetAccessible = $type->isOffsetAccessible() && $type->getIterableKeyType()->isSuperTypeOf($dimType)->no(); - if ($isInaccessibleIterable || $type->hasOffsetValueType($dimType)->no()) { - $messages[] = RuleErrorBuilder::message( + if ($isOffsetAccessible || $type->hasOffsetValueType($dimType)->no()) { + return RuleErrorBuilder::message( sprintf( 'Cannot unset offset %s on %s.', $dimType->describe(VerbosityLevel::value()), $type->describe(VerbosityLevel::value()) ) - )->line($node->getLine())->build(); + )->line($node->getLine())->build()->getMessage(); } } + + return null; } } diff --git a/tests/PHPStan/Rules/Functions/UnsetRuleTest.php b/tests/PHPStan/Rules/Functions/UnsetRuleTest.php index b9c957854f..407ca72a59 100644 --- a/tests/PHPStan/Rules/Functions/UnsetRuleTest.php +++ b/tests/PHPStan/Rules/Functions/UnsetRuleTest.php @@ -10,7 +10,7 @@ class UnsetRuleTest extends \PHPStan\Testing\RuleTestCase protected function getRule(): \PHPStan\Rules\Rule { - return new UnsetRule(true); + return new UnsetRule(); } public function testUnsetRule(): void @@ -19,31 +19,27 @@ public function testUnsetRule(): void $this->analyse([__DIR__ . '/data/unset.php'], [ [ 'Call to function unset() contains undefined variable $notSetVariable.', - 3, + 6, ], [ 'Cannot unset offset \'a\' on 3.', - 7, + 10, ], [ 'Cannot unset offset \'b\' on 1.', - 11, + 14, ], [ 'Cannot unset offset \'c\' on 1.', - 15, + 18, ], [ 'Cannot unset offset \'b\' on 1.', - 15, - ], - [ - 'Call to function unset() contains possibly undefined variable $maybeSet.', - 21, + 18, ], [ 'Cannot unset offset \'string\' on iterable.', - 30, + 31, ], ]); } diff --git a/tests/PHPStan/Rules/Functions/data/unset.php b/tests/PHPStan/Rules/Functions/data/unset.php index 1c660c5225..501555b1c0 100644 --- a/tests/PHPStan/Rules/Functions/data/unset.php +++ b/tests/PHPStan/Rules/Functions/data/unset.php @@ -1,31 +1,32 @@ 1]; + unset($scalar['a']); -unset($singleDimArray['a']['b']); + $singleDimArray = ['a' => 1]; -$multiDimArray = ['a' => ['b' => 1]]; + unset($singleDimArray['a']['b']); -unset($multiDimArray['a']['b']['c'], $scalar, $singleDimArray['a']['b']); + $multiDimArray = ['a' => ['b' => 1]]; -if (rand()) { - $maybeSet = 1; -} + unset($multiDimArray['a']['b']['c'], $scalar, $singleDimArray['a']['b']); -unset($maybeSet); +} /** @param iterable $iterable */ -function unsetOnMaybeIterable(iterable $iterable) { +function unsetOnMaybeIterable(iterable $iterable) +{ unset($iterable['string']); } /** @param iterable $iterable */ -function unsetOnYesIterable(iterable $iterable) { +function unsetOnYesIterable(iterable $iterable) +{ unset($iterable['string']); } From 894c08c928f29f06676316b44f32fc413bd48643 Mon Sep 17 00:00:00 2001 From: "l.gersen@visymo.com" Date: Wed, 27 Nov 2019 08:20:59 +0100 Subject: [PATCH 5/5] Correct isOffsetAccessible check --- src/Rules/Functions/UnsetRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Rules/Functions/UnsetRule.php b/src/Rules/Functions/UnsetRule.php index c1ba237d02..cbaef6539b 100644 --- a/src/Rules/Functions/UnsetRule.php +++ b/src/Rules/Functions/UnsetRule.php @@ -50,7 +50,7 @@ private function canBeUnset(Node $node, Scope $scope): ?string $type = $scope->getType($node->var); $dimType = $scope->getType($node->dim); - $isOffsetAccessible = $type->isOffsetAccessible() && $type->getIterableKeyType()->isSuperTypeOf($dimType)->no(); + $isOffsetAccessible = !$type->isOffsetAccessible()->no() && $type->getIterableKeyType()->isSuperTypeOf($dimType)->no(); if ($isOffsetAccessible || $type->hasOffsetValueType($dimType)->no()) { return RuleErrorBuilder::message(