Skip to content

Commit

Permalink
Consider LastConditionVisitor in ConstantConditionRuleHelper::getBool…
Browse files Browse the repository at this point in the history 
…eanType() using rules
  • Loading branch information
@staabm @ondrejmirtes
staabm authored and ondrejmirtes committed Jan 9, 2023
1 parent 6f6e9d3 commit b2a1743
Show file tree
Hide file tree
Showing 13 changed files with 179 additions and 32 deletions.
22 changes: 13 additions & 9 deletions src/Rules/Comparison/BooleanAndConstantConditionRule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Node\BooleanAndNode;
use PHPStan\Parser\LastConditionVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantBooleanType;
Expand Down Expand Up @@ -53,19 +54,22 @@ public function processNode(

return $ruleErrorBuilder->tip($tipText);
};
$errors[] = $addTipLeft(RuleErrorBuilder::message(sprintf(
'Left side of %s is always %s.',
$nodeText,
$leftType->getValue() ? 'true' : 'false',
)))->line($originalNode->left->getLine())->build();

if ($leftType->getValue() === false || $originalNode->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
$errors[] = $addTipLeft(RuleErrorBuilder::message(sprintf(
'Left side of %s is always %s.',
$nodeText,
$leftType->getValue() ? 'true' : 'false',
)))->line($originalNode->left->getLine())->build();
}
}

$rightScope = $node->getRightScope();
$rightType = $this->helper->getBooleanType(
$rightScope,
$originalNode->right,
);
if ($rightType instanceof ConstantBooleanType) {
if ($rightType instanceof ConstantBooleanType && !$scope->isInFirstLevelStatement()) {
$addTipRight = function (RuleErrorBuilder $ruleErrorBuilder) use ($rightScope, $originalNode, $tipText): RuleErrorBuilder {
if (!$this->treatPhpDocTypesAsCertain) {
return $ruleErrorBuilder;
Expand All @@ -82,7 +86,7 @@ public function processNode(
return $ruleErrorBuilder->tip($tipText);
};

if (!$scope->isInFirstLevelStatement()) {
if ($rightType->getValue() === false || $originalNode->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
$errors[] = $addTipRight(RuleErrorBuilder::message(sprintf(
'Right side of %s is always %s.',
$nodeText,
Expand All @@ -91,7 +95,7 @@ public function processNode(
}
}

if (count($errors) === 0) {
if (count($errors) === 0 && !$scope->isInFirstLevelStatement()) {
$nodeType = $this->treatPhpDocTypesAsCertain ? $scope->getType($originalNode) : $scope->getNativeType($originalNode);
if ($nodeType instanceof ConstantBooleanType) {
$addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $originalNode, $tipText): RuleErrorBuilder {
Expand All @@ -107,7 +111,7 @@ public function processNode(
return $ruleErrorBuilder->tip($tipText);
};

if (!$scope->isInFirstLevelStatement()) {
if ($nodeType->getValue() === false || $originalNode->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
$errors[] = $addTip(RuleErrorBuilder::message(sprintf(
'Result of %s is always %s.',
$nodeText,
Expand Down
15 changes: 9 additions & 6 deletions src/Rules/Comparison/BooleanNotConstantConditionRule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Parser\LastConditionVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantBooleanType;
Expand Down Expand Up @@ -47,12 +48,14 @@ public function processNode(
return $ruleErrorBuilder->tip('Because the type is coming from a PHPDoc, you can turn off this check by setting <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%configurationFile%</>.');
};

return [
$addTip(RuleErrorBuilder::message(sprintf(
'Negated boolean expression is always %s.',
$exprType->getValue() ? 'false' : 'true',
)))->line($node->expr->getLine())->build(),
];
if ($exprType->getValue() === true || $node->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
return [
$addTip(RuleErrorBuilder::message(sprintf(
'Negated boolean expression is always %s.',
$exprType->getValue() ? 'false' : 'true',
)))->line($node->expr->getLine())->build(),
];
}
}

return [];
Expand Down
22 changes: 13 additions & 9 deletions src/Rules/Comparison/BooleanOrConstantConditionRule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Node\BooleanOrNode;
use PHPStan\Parser\LastConditionVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantBooleanType;
Expand Down Expand Up @@ -53,19 +54,22 @@ public function processNode(

return $ruleErrorBuilder->tip($tipText);
};
$messages[] = $addTipLeft(RuleErrorBuilder::message(sprintf(
'Left side of %s is always %s.',
$nodeText,
$leftType->getValue() ? 'true' : 'false',
)))->line($originalNode->left->getLine())->build();

if ($leftType->getValue() === false || $originalNode->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
$messages[] = $addTipLeft(RuleErrorBuilder::message(sprintf(
'Left side of %s is always %s.',
$nodeText,
$leftType->getValue() ? 'true' : 'false',
)))->line($originalNode->left->getLine())->build();
}
}

$rightScope = $node->getRightScope();
$rightType = $this->helper->getBooleanType(
$rightScope,
$originalNode->right,
);
if ($rightType instanceof ConstantBooleanType) {
if ($rightType instanceof ConstantBooleanType && !$scope->isInFirstLevelStatement()) {
$addTipRight = function (RuleErrorBuilder $ruleErrorBuilder) use ($rightScope, $originalNode, $tipText): RuleErrorBuilder {
if (!$this->treatPhpDocTypesAsCertain) {
return $ruleErrorBuilder;
Expand All @@ -82,7 +86,7 @@ public function processNode(
return $ruleErrorBuilder->tip($tipText);
};

if (!$scope->isInFirstLevelStatement()) {
if ($rightType->getValue() === false || $originalNode->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
$messages[] = $addTipRight(RuleErrorBuilder::message(sprintf(
'Right side of %s is always %s.',
$nodeText,
Expand All @@ -91,7 +95,7 @@ public function processNode(
}
}

if (count($messages) === 0) {
if (count($messages) === 0 && !$scope->isInFirstLevelStatement()) {
$nodeType = $this->treatPhpDocTypesAsCertain ? $scope->getType($originalNode) : $scope->getNativeType($originalNode);
if ($nodeType instanceof ConstantBooleanType) {
$addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $originalNode, $tipText): RuleErrorBuilder {
Expand All @@ -107,7 +111,7 @@ public function processNode(
return $ruleErrorBuilder->tip($tipText);
};

if (!$scope->isInFirstLevelStatement()) {
if ($nodeType->getValue() === false || $originalNode->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) !== true) {
$messages[] = $addTip(RuleErrorBuilder::message(sprintf(
'Result of %s is always %s.',
$nodeText,
Expand Down
9 changes: 5 additions & 4 deletions src/Rules/Comparison/ConstantConditionRuleHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ public function __construct(

public function shouldReportAlwaysTrueByDefault(Expr $expr): bool
{
if ($expr->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) === true) {
return false;
}

return $expr instanceof Expr\BooleanNot
|| $expr instanceof Expr\BinaryOp\BooleanOr
|| $expr instanceof Expr\BinaryOp\BooleanAnd
Expand Down Expand Up @@ -54,6 +50,11 @@ public function shouldSkip(Scope $scope, Expr $expr): bool
return true;
}

if ($expr->getAttribute(LastConditionVisitor::ATTRIBUTE_NAME) === true) {
// always-true should not be reported because last condition
return true;
}

if (
$expr instanceof FuncCall
|| $expr instanceof MethodCall
Expand Down
8 changes: 8 additions & 0 deletions tests/PHPStan/Rules/Comparison/BooleanAndConstantConditionRuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ public function testRule(): void
'Right side of && is always true.',
147,
],
[
'Left side of && is always true.',
178,
],
[
'Right side of && is always true.',
178,
],
]);
}

Expand Down
4 changes: 4 additions & 0 deletions tests/PHPStan/Rules/Comparison/BooleanNotConstantConditionRuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ public function testRule(): void
'Negated boolean expression is always false.',
50,
],
[
'Negated boolean expression is always true.',
67,
],
]);
}

Expand Down
8 changes: 8 additions & 0 deletions tests/PHPStan/Rules/Comparison/BooleanOrConstantConditionRuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ public function testRule(): void
'Right side of || is always true.',
85,
],
[
'Left side of || is always true.',
101,
],
[
'Right side of || is always true.',
110,
],
]);
}

Expand Down
8 changes: 4 additions & 4 deletions tests/PHPStan/Rules/Comparison/ElseIfConstantConditionRuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public function testRule(): void
$this->analyse([__DIR__ . '/data/elseif-condition.php'], [
[
'Elseif condition is always true.',
18,
56,
],
]);
}
Expand All @@ -52,7 +52,7 @@ public function testDoNotReportPhpDoc(): void
$this->analyse([__DIR__ . '/data/elseif-condition-not-phpdoc.php'], [
[
'Elseif condition is always true.',
18,
46,
],
]);
}
Expand All @@ -63,11 +63,11 @@ public function testReportPhpDoc(): void
$this->analyse([__DIR__ . '/data/elseif-condition-not-phpdoc.php'], [
[
'Elseif condition is always true.',
18,
46,
],
[
'Elseif condition is always true.',
24,
56,
'Because the type is coming from a PHPDoc, you can turn off this check by setting <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%configurationFile%</>.',
],
]);
Expand Down
16 changes: 16 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/boolean-and.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,19 @@ function bug1924() {
if (isset($arr['a']) && isset($arr['b'])) {
}
}

class ConditionalAlwaysTrue
{
public function sayHello(int $i): void
{
$one = 1;
if ($i < 5) {
} elseif ($one && $i) { // always-true should not be reported because last condition
}

if ($i < 5) {
} elseif ($one && $i) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}
}
}
19 changes: 19 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/boolean-not.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,22 @@ public function nestedIfConditions($mixed): void
}
}
}

class ConditionalAlwaysTrue
{
public function doFoo(int $i)
{
$zero = 0;
if ($i < 0) {
} elseif (!$zero) { // always-true should not be reported because last condition
}

$zero = 0;
if ($i < 0) {
} elseif (!$zero) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}
}

}

26 changes: 26 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/boolean-or.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,29 @@ public function orInIfCondition($mixed, int $i): void
}
}
}

class ConditionalAlwaysTrue
{
public function doFoo(int $i, $x)
{
$one = 1;
if ($x) {
} elseif ($one || $i) { // always-true should not be reported because last condition
}

if ($x) {
} elseif ($one || $i) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}

if ($x) {
} elseif ($i || $one) { // always-true should not be reported because last condition
}

if ($x) {
} elseif ($i || $one) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}
}

}
34 changes: 34 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/elseif-condition-not-phpdoc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,37 @@ public function doFoo(
}

}

class ConditionalAlwaysTrue
{
/**
* @param object $object
*/
public function doFoo(
self $self,
$object
): void
{
if (rand(0, 1)) {
} elseif ($self) { // always-true should not be reported because last condition
}

if (rand(0, 1)) {
} elseif ($self) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}


if (rand(0, 1)) {
} elseif ($object) { // always-true should not be reported because last condition
}

if (rand(0, 1)) {
} elseif ($object) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}

}

}

20 changes: 20 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/elseif-condition.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,23 @@ public function doFoo(int $i, \stdClass $std, $union, $intersection)
}

}

class ConditionalAlwaysTrue
{
/**
* @param int $i
* @param \stdClass $std
*/
public function doFoo(int $i, \stdClass $std)
{
if ($i) {
} elseif ($std) { // always-true should not be reported because last condition
}

if ($i) {
} elseif ($std) { // always-true should be reported, because another condition below
} elseif (rand(0,1)) {
}
}

}

0 comments on commit b2a1743

Please sign in to comment.