Skip to content

Commit

Permalink
BooleanOrConstantConditionRule - check LogicalOr (bleeding edge), use…
Browse files Browse the repository at this point in the history
… virtual node for more correct result
  • Loading branch information
ondrejmirtes committed Jan 11, 2021
1 parent 40a76e8 commit ae9a558
Show file tree
Hide file tree
Showing 7 changed files with 93 additions and 18 deletions.
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ parameters:
dateTimeInstantiation: true
detectDuplicateStubFiles: true
checkLogicalAndConstantCondition: true
checkLogicalOrConstantCondition: true
1 change: 1 addition & 0 deletions conf/config.level4.neon
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ services:
class: PHPStan\Rules\Comparison\BooleanOrConstantConditionRule
arguments:
treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain%
checkLogicalOrConstantCondition: %featureToggles.checkLogicalOrConstantCondition%
tags:
- phpstan.rules.rule

Expand Down
4 changes: 3 additions & 1 deletion conf/config.neon
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ parameters:
dateTimeInstantiation: false
detectDuplicateStubFiles: false
checkLogicalAndConstantCondition: false
checkLogicalOrConstantCondition: false
fileExtensions:
- php
checkAlwaysTrueCheckTypeFunctionCall: false
Expand Down Expand Up @@ -170,7 +171,8 @@ parametersSchema:
readComposerPhpVersion: bool(),
dateTimeInstantiation: bool(),
detectDuplicateStubFiles: bool(),
checkLogicalAndConstantCondition: bool()
checkLogicalAndConstantCondition: bool(),
checkLogicalOrConstantCondition: bool()
])
fileExtensions: listOf(string())
checkAlwaysTrueCheckTypeFunctionCall: bool()
Expand Down
3 changes: 3 additions & 0 deletions src/Analyser/NodeScopeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand Down
55 changes: 55 additions & 0 deletions src/Node/BooleanOrNode.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php declare(strict_types = 1);

namespace PHPStan\Node;

use PhpParser\Node\Expr\BinaryOp\BooleanOr;
use PhpParser\Node\Expr\BinaryOp\LogicalOr;
use PhpParser\NodeAbstract;
use PHPStan\Analyser\Scope;

class BooleanOrNode extends NodeAbstract implements VirtualNode
{

/** @var BooleanOr|LogicalOr */
private $originalNode;

private Scope $rightScope;

/**
* @param BooleanOr|LogicalOr $originalNode
* @param Scope $rightScope
*/
public function __construct($originalNode, Scope $rightScope)
{
parent::__construct($originalNode->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 [];
}

}
44 changes: 28 additions & 16 deletions src/Rules/Comparison/BooleanOrConstantConditionRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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<BooleanOrNode>
*/
class BooleanOrConstantConditionRule implements \PHPStan\Rules\Rule
{
Expand All @@ -15,35 +17,44 @@ 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(
\PhpParser\Node $node,
\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 <fg=cyan>treatPhpDocTypesAsCertain: false</> in your <fg=cyan>%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;
}
Expand All @@ -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;
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ protected function getRule(): \PHPStan\Rules\Rule
),
$this->treatPhpDocTypesAsCertain
),
$this->treatPhpDocTypesAsCertain
$this->treatPhpDocTypesAsCertain,
true
);
}

Expand Down

0 comments on commit ae9a558

Please sign in to comment.