From ae9a558484dc4f330873e962b8dc1b2d18f0c9b7 Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Mon, 11 Jan 2021 14:06:31 +0100 Subject: [PATCH] BooleanOrConstantConditionRule - check LogicalOr (bleeding edge), use virtual node for more correct result --- conf/bleedingEdge.neon | 1 + conf/config.level4.neon | 1 + conf/config.neon | 4 +- src/Analyser/NodeScopeResolver.php | 3 + src/Node/BooleanOrNode.php | 55 +++++++++++++++++++ .../BooleanOrConstantConditionRule.php | 44 +++++++++------ .../BooleanOrConstantConditionRuleTest.php | 3 +- 7 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 src/Node/BooleanOrNode.php diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 8d087677c4..b6413b2183 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -10,3 +10,4 @@ parameters: dateTimeInstantiation: true detectDuplicateStubFiles: true checkLogicalAndConstantCondition: true + checkLogicalOrConstantCondition: true diff --git a/conf/config.level4.neon b/conf/config.level4.neon index 2415631a53..8dd3eb2315 100644 --- a/conf/config.level4.neon +++ b/conf/config.level4.neon @@ -50,6 +50,7 @@ services: class: PHPStan\Rules\Comparison\BooleanOrConstantConditionRule arguments: treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain% + checkLogicalOrConstantCondition: %featureToggles.checkLogicalOrConstantCondition% tags: - phpstan.rules.rule diff --git a/conf/config.neon b/conf/config.neon index 55aa9f1efd..27c08c7667 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -23,6 +23,7 @@ parameters: dateTimeInstantiation: false detectDuplicateStubFiles: false checkLogicalAndConstantCondition: false + checkLogicalOrConstantCondition: false fileExtensions: - php checkAlwaysTrueCheckTypeFunctionCall: false @@ -170,7 +171,8 @@ parametersSchema: readComposerPhpVersion: bool(), dateTimeInstantiation: bool(), detectDuplicateStubFiles: bool(), - checkLogicalAndConstantCondition: bool() + checkLogicalAndConstantCondition: bool(), + checkLogicalOrConstantCondition: bool() ]) fileExtensions: listOf(string()) checkAlwaysTrueCheckTypeFunctionCall: bool() diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 44cbd2aef7..9ca153cb87 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -50,6 +50,7 @@ use PHPStan\File\FileHelper; use PHPStan\File\FileReader; use PHPStan\Node\BooleanAndNode; +use PHPStan\Node\BooleanOrNode; use PHPStan\Node\ClassConstantsNode; use PHPStan\Node\ClassMethodsNode; use PHPStan\Node\ClassPropertiesNode; @@ -1923,6 +1924,8 @@ static function () use ($leftMergedWithRightScope, $expr): MutatingScope { $rightResult = $this->processExprNode($expr->right, $leftResult->getFalseyScope(), $nodeCallback, $context); $leftMergedWithRightScope = $leftResult->getScope()->mergeWith($rightResult->getScope()); + $nodeCallback(new BooleanOrNode($expr, $leftResult->getFalseyScope()), $scope); + return new ExpressionResult( $leftMergedWithRightScope, $leftResult->hasYield() || $rightResult->hasYield(), diff --git a/src/Node/BooleanOrNode.php b/src/Node/BooleanOrNode.php new file mode 100644 index 0000000000..0682f66bb2 --- /dev/null +++ b/src/Node/BooleanOrNode.php @@ -0,0 +1,55 @@ +getAttributes()); + $this->originalNode = $originalNode; + $this->rightScope = $rightScope; + } + + /** + * @return BooleanOr|LogicalOr + */ + public function getOriginalNode() + { + return $this->originalNode; + } + + public function getRightScope(): Scope + { + return $this->rightScope; + } + + public function getType(): string + { + return 'PHPStan_Node_BooleanOrNode'; + } + + /** + * @return string[] + */ + public function getSubNodeNames(): array + { + return []; + } + +} diff --git a/src/Rules/Comparison/BooleanOrConstantConditionRule.php b/src/Rules/Comparison/BooleanOrConstantConditionRule.php index b079e3f8b8..4b29a14d4d 100644 --- a/src/Rules/Comparison/BooleanOrConstantConditionRule.php +++ b/src/Rules/Comparison/BooleanOrConstantConditionRule.php @@ -2,11 +2,13 @@ namespace PHPStan\Rules\Comparison; +use PhpParser\Node\Expr\BinaryOp\BooleanOr; +use PHPStan\Node\BooleanOrNode; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Constant\ConstantBooleanType; /** - * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\BinaryOp\BooleanOr> + * @implements \PHPStan\Rules\Rule */ class BooleanOrConstantConditionRule implements \PHPStan\Rules\Rule { @@ -15,18 +17,22 @@ class BooleanOrConstantConditionRule implements \PHPStan\Rules\Rule private bool $treatPhpDocTypesAsCertain; + private bool $checkLogicalOrConstantCondition; + public function __construct( ConstantConditionRuleHelper $helper, - bool $treatPhpDocTypesAsCertain + bool $treatPhpDocTypesAsCertain, + bool $checkLogicalOrConstantCondition ) { $this->helper = $helper; $this->treatPhpDocTypesAsCertain = $treatPhpDocTypesAsCertain; + $this->checkLogicalOrConstantCondition = $checkLogicalOrConstantCondition; } public function getNodeType(): string { - return \PhpParser\Node\Expr\BinaryOp\BooleanOr::class; + return BooleanOrNode::class; } public function processNode( @@ -34,16 +40,21 @@ public function processNode( \PHPStan\Analyser\Scope $scope ): array { + $originalNode = $node->getOriginalNode(); + if (!$originalNode instanceof BooleanOr && !$this->checkLogicalOrConstantCondition) { + return []; + } + $messages = []; - $leftType = $this->helper->getBooleanType($scope, $node->left); + $leftType = $this->helper->getBooleanType($scope, $originalNode->left); $tipText = 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.'; if ($leftType instanceof ConstantBooleanType) { - $addTipLeft = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $tipText): RuleErrorBuilder { + $addTipLeft = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $originalNode, $tipText): RuleErrorBuilder { if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } - $booleanNativeType = $this->helper->getNativeBooleanType($scope, $node->left); + $booleanNativeType = $this->helper->getNativeBooleanType($scope, $originalNode->left); if ($booleanNativeType instanceof ConstantBooleanType) { return $ruleErrorBuilder; } @@ -53,22 +64,23 @@ public function processNode( $messages[] = $addTipLeft(RuleErrorBuilder::message(sprintf( 'Left side of || is always %s.', $leftType->getValue() ? 'true' : 'false' - )))->line($node->left->getLine())->build(); + )))->line($originalNode->left->getLine())->build(); } + $rightScope = $node->getRightScope(); $rightType = $this->helper->getBooleanType( - $scope->filterByFalseyValue($node->left), - $node->right + $rightScope, + $originalNode->right ); if ($rightType instanceof ConstantBooleanType) { - $addTipRight = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $tipText): RuleErrorBuilder { + $addTipRight = function (RuleErrorBuilder $ruleErrorBuilder) use ($rightScope, $originalNode, $tipText): RuleErrorBuilder { if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } $booleanNativeType = $this->helper->getNativeBooleanType( - $scope->doNotTreatPhpDocTypesAsCertain()->filterByFalseyValue($node->left), - $node->right + $rightScope->doNotTreatPhpDocTypesAsCertain(), + $originalNode->right ); if ($booleanNativeType instanceof ConstantBooleanType) { return $ruleErrorBuilder; @@ -79,18 +91,18 @@ public function processNode( $messages[] = $addTipRight(RuleErrorBuilder::message(sprintf( 'Right side of || is always %s.', $rightType->getValue() ? 'true' : 'false' - )))->line($node->right->getLine())->build(); + )))->line($originalNode->right->getLine())->build(); } if (count($messages) === 0) { - $nodeType = $scope->getType($node); + $nodeType = $scope->getType($originalNode); if ($nodeType instanceof ConstantBooleanType) { - $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $node, $tipText): RuleErrorBuilder { + $addTip = function (RuleErrorBuilder $ruleErrorBuilder) use ($scope, $originalNode, $tipText): RuleErrorBuilder { if (!$this->treatPhpDocTypesAsCertain) { return $ruleErrorBuilder; } - $booleanNativeType = $scope->doNotTreatPhpDocTypesAsCertain()->getType($node); + $booleanNativeType = $scope->doNotTreatPhpDocTypesAsCertain()->getType($originalNode); if ($booleanNativeType instanceof ConstantBooleanType) { return $ruleErrorBuilder; } diff --git a/tests/PHPStan/Rules/Comparison/BooleanOrConstantConditionRuleTest.php b/tests/PHPStan/Rules/Comparison/BooleanOrConstantConditionRuleTest.php index bf0cffe434..f02dc93cbb 100644 --- a/tests/PHPStan/Rules/Comparison/BooleanOrConstantConditionRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/BooleanOrConstantConditionRuleTest.php @@ -23,7 +23,8 @@ protected function getRule(): \PHPStan\Rules\Rule ), $this->treatPhpDocTypesAsCertain ), - $this->treatPhpDocTypesAsCertain + $this->treatPhpDocTypesAsCertain, + true ); }