diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 89001077f9..f6cc07dab3 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -30,6 +30,7 @@ parameters: alwaysCheckTooWideReturnTypeFinalMethods: true duplicateStubs: true logicalXor: true + betterNoop: true invarianceComposition: true alwaysTrueAlwaysReported: true disableUnreachableBranchesRules: true diff --git a/conf/config.level4.neon b/conf/config.level4.neon index f9bb627f91..030e7bb02d 100644 --- a/conf/config.level4.neon +++ b/conf/config.level4.neon @@ -27,6 +27,8 @@ conditionalTags: phpstan.rules.rule: %featureToggles.notAnalysedTrait% PHPStan\Rules\Comparison\LogicalXorConstantConditionRule: phpstan.rules.rule: %featureToggles.logicalXor% + PHPStan\Rules\DeadCode\BetterNoopRule: + phpstan.rules.rule: %featureToggles.betterNoop% PHPStan\Rules\TooWideTypehints\TooWideFunctionParameterOutTypeRule: phpstan.rules.rule: %featureToggles.paramOutType% PHPStan\Rules\TooWideTypehints\TooWideMethodParameterOutTypeRule: @@ -74,7 +76,7 @@ services: - class: PHPStan\Rules\DeadCode\NoopRule arguments: - logicalXor: %featureToggles.logicalXor% + better: %featureToggles.betterNoop% tags: - phpstan.rules.rule @@ -142,6 +144,9 @@ services: treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain% reportAlwaysTrueInLastCondition: %reportAlwaysTrueInLastCondition% + - + class: PHPStan\Rules\DeadCode\BetterNoopRule + - class: PHPStan\Rules\Comparison\MatchExpressionRule arguments: diff --git a/conf/config.neon b/conf/config.neon index b56449389f..f4ad8d3e59 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -65,6 +65,7 @@ parameters: alwaysCheckTooWideReturnTypeFinalMethods: false duplicateStubs: false logicalXor: false + betterNoop: false invarianceComposition: false alwaysTrueAlwaysReported: false disableUnreachableBranchesRules: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index b1c55c8161..f2831fb577 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -60,6 +60,7 @@ parametersSchema: alwaysCheckTooWideReturnTypeFinalMethods: bool() duplicateStubs: bool() logicalXor: bool() + betterNoop: bool() invarianceComposition: bool() alwaysTrueAlwaysReported: bool() disableUnreachableBranchesRules: bool() diff --git a/src/Analyser/ImpurePoint.php b/src/Analyser/ImpurePoint.php index 33d4574baf..7f4ad423ba 100644 --- a/src/Analyser/ImpurePoint.php +++ b/src/Analyser/ImpurePoint.php @@ -6,7 +6,7 @@ use PHPStan\Node\VirtualNode; /** - * @phpstan-type ImpurePointIdentifier = 'echo'|'die'|'exit'|'propertyAssign'|'methodCall'|'new'|'functionCall'|'include'|'require'|'print'|'eval'|'superglobal' + * @phpstan-type ImpurePointIdentifier = 'echo'|'die'|'exit'|'propertyAssign'|'methodCall'|'new'|'functionCall'|'include'|'require'|'print'|'eval'|'superglobal'|'yield'|'yieldFrom' * @api */ class ImpurePoint diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 2c64d9ed30..94cc3460e3 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -106,6 +106,7 @@ use PHPStan\Node\MatchExpressionNode; use PHPStan\Node\MethodCallableNode; use PHPStan\Node\MethodReturnStatementsNode; +use PHPStan\Node\NoopExpressionNode; use PHPStan\Node\PropertyAssignNode; use PHPStan\Node\ReturnStatement; use PHPStan\Node\StaticMethodCallableNode; @@ -746,6 +747,17 @@ private function processStmtNode( } elseif ($stmt instanceof Node\Stmt\Expression) { $earlyTerminationExpr = $this->findEarlyTerminatingExpr($stmt->expr, $scope); $result = $this->processExprNode($stmt, $stmt->expr, $scope, $nodeCallback, ExpressionContext::createTopLevel()); + $throwPoints = array_filter($result->getThrowPoints(), static fn ($throwPoint) => $throwPoint->isExplicit()); + if ( + count($result->getImpurePoints()) === 0 + && count($throwPoints) === 0 + && !$stmt->expr instanceof Expr\PostInc + && !$stmt->expr instanceof Expr\PreInc + && !$stmt->expr instanceof Expr\PostDec + && !$stmt->expr instanceof Expr\PreDec + ) { + $nodeCallback(new NoopExpressionNode($stmt->expr), $scope); + } $scope = $result->getScope(); $scope = $scope->filterBySpecifiedTypes($this->typeSpecifier->specifyTypesInCondition( $scope, @@ -2991,6 +3003,13 @@ static function (): void { $throwPoints = $result->getThrowPoints(); $throwPoints[] = ThrowPoint::createImplicit($scope, $expr); $impurePoints = $result->getImpurePoints(); + $impurePoints[] = new ImpurePoint( + $scope, + $expr, + 'yieldFrom', + 'yield from', + true, + ); $hasYield = true; $scope = $result->getScope(); @@ -3226,12 +3245,20 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void { $throwPoints = [ ThrowPoint::createImplicit($scope, $expr), ]; - $impurePoints = []; + $impurePoints = [ + new ImpurePoint( + $scope, + $expr, + 'yield', + 'yield', + true, + ), + ]; if ($expr->key !== null) { $keyResult = $this->processExprNode($stmt, $expr->key, $scope, $nodeCallback, $context->enterDeep()); $scope = $keyResult->getScope(); $throwPoints = $keyResult->getThrowPoints(); - $impurePoints = $keyResult->getImpurePoints(); + $impurePoints = array_merge($impurePoints, $keyResult->getImpurePoints()); } if ($expr->value !== null) { $valueResult = $this->processExprNode($stmt, $expr->value, $scope, $nodeCallback, $context->enterDeep()); diff --git a/src/Node/NoopExpressionNode.php b/src/Node/NoopExpressionNode.php new file mode 100644 index 0000000000..191fddb144 --- /dev/null +++ b/src/Node/NoopExpressionNode.php @@ -0,0 +1,34 @@ +originalExpr->getAttributes()); + } + + public function getOriginalExpr(): Expr + { + return $this->originalExpr; + } + + public function getType(): string + { + return 'PHPStan_Node_NoopExpressionNode'; + } + + /** + * @return string[] + */ + public function getSubNodeNames(): array + { + return []; + } + +} diff --git a/src/Rules/DeadCode/BetterNoopRule.php b/src/Rules/DeadCode/BetterNoopRule.php new file mode 100644 index 0000000000..2fe271515e --- /dev/null +++ b/src/Rules/DeadCode/BetterNoopRule.php @@ -0,0 +1,110 @@ + + */ +class BetterNoopRule implements Rule +{ + + public function __construct(private ExprPrinter $exprPrinter) + { + } + + public function getNodeType(): string + { + return NoopExpressionNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $expr = $node->getOriginalExpr(); + if ($expr instanceof Node\Expr\BinaryOp\LogicalXor) { + return [ + RuleErrorBuilder::message( + 'Unused result of "xor" operator.', + )->line($expr->getStartLine()) + ->tip('This operator has unexpected precedence, try disambiguating the logic with parentheses ().') + ->identifier('logicalXor.resultUnused') + ->build(), + ]; + } + if ($expr instanceof Node\Expr\BinaryOp\LogicalAnd || $expr instanceof Node\Expr\BinaryOp\LogicalOr) { + $identifierType = $expr instanceof Node\Expr\BinaryOp\LogicalAnd ? 'logicalAnd' : 'logicalOr'; + + return [ + RuleErrorBuilder::message(sprintf( + 'Unused result of "%s" operator.', + $expr->getOperatorSigil(), + ))->line($expr->getStartLine()) + ->tip('This operator has unexpected precedence, try disambiguating the logic with parentheses ().') + ->identifier(sprintf('%s.resultUnused', $identifierType)) + ->build(), + ]; + } + + if ($expr instanceof Node\Expr\BinaryOp\BooleanAnd || $expr instanceof Node\Expr\BinaryOp\BooleanOr) { + $identifierType = $expr instanceof Node\Expr\BinaryOp\BooleanAnd ? 'booleanAnd' : 'booleanOr'; + + return [ + RuleErrorBuilder::message(sprintf( + 'Unused result of "%s" operator.', + $expr->getOperatorSigil(), + ))->line($expr->getStartLine()) + ->identifier(sprintf('%s.resultUnused', $identifierType)) + ->build(), + ]; + } + + if ($expr instanceof Node\Expr\Ternary) { + return [ + RuleErrorBuilder::message('Unused result of ternary operator.') + ->line($expr->getStartLine()) + ->identifier('ternary.resultUnused') + ->build(), + ]; + } + + if ( + $expr instanceof Node\Expr\FuncCall + || $expr instanceof Node\Expr\NullsafeMethodCall + || $expr instanceof Node\Expr\MethodCall + || $expr instanceof Node\Expr\New_ + || $expr instanceof Node\Expr\StaticCall + ) { + // handled by *WithoutSideEffectsRule rules + return []; + } + + if ( + $expr instanceof Node\Expr\Assign + || $expr instanceof Node\Expr\AssignOp + || $expr instanceof Node\Expr\AssignRef + ) { + return []; + } + + if ($expr instanceof Node\Expr\Closure) { + return []; + } + + return [ + RuleErrorBuilder::message(sprintf( + 'Expression "%s" on a separate line does not do anything.', + $this->exprPrinter->printExpr($expr), + ))->line($expr->getStartLine()) + ->identifier('expr.resultUnused') + ->build(), + ]; + } + +} diff --git a/src/Rules/DeadCode/NoopRule.php b/src/Rules/DeadCode/NoopRule.php index 8105463cc0..b05eb401f9 100644 --- a/src/Rules/DeadCode/NoopRule.php +++ b/src/Rules/DeadCode/NoopRule.php @@ -15,7 +15,7 @@ class NoopRule implements Rule { - public function __construct(private ExprPrinter $exprPrinter, private bool $logicalXor) + public function __construct(private ExprPrinter $exprPrinter, private bool $better) { } @@ -26,6 +26,10 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { + if ($this->better) { + // disabled in bleeding edge + return []; + } $originalExpr = $node->expr; $expr = $originalExpr; if ( @@ -36,70 +40,7 @@ public function processNode(Node $node, Scope $scope): array ) { $expr = $expr->expr; } - if ($this->logicalXor) { - if ($expr instanceof Node\Expr\BinaryOp\LogicalXor) { - return [ - RuleErrorBuilder::message( - 'Unused result of "xor" operator.', - )->line($expr->getStartLine()) - ->tip('This operator has unexpected precedence, try disambiguating the logic with parentheses ().') - ->identifier('logicalXor.resultUnused') - ->build(), - ]; - } - if ($expr instanceof Node\Expr\BinaryOp\LogicalAnd || $expr instanceof Node\Expr\BinaryOp\LogicalOr) { - if (!$this->isNoopExpr($expr->right)) { - return []; - } - - $identifierType = $expr instanceof Node\Expr\BinaryOp\LogicalAnd ? 'logicalAnd' : 'logicalOr'; - - return [ - RuleErrorBuilder::message(sprintf( - 'Unused result of "%s" operator.', - $expr->getOperatorSigil(), - ))->line($expr->getStartLine()) - ->tip('This operator has unexpected precedence, try disambiguating the logic with parentheses ().') - ->identifier(sprintf('%s.resultUnused', $identifierType)) - ->build(), - ]; - } - - if ($expr instanceof Node\Expr\BinaryOp\BooleanAnd || $expr instanceof Node\Expr\BinaryOp\BooleanOr) { - if (!$this->isNoopExpr($expr->right)) { - return []; - } - $identifierType = $expr instanceof Node\Expr\BinaryOp\BooleanAnd ? 'booleanAnd' : 'booleanOr'; - - return [ - RuleErrorBuilder::message(sprintf( - 'Unused result of "%s" operator.', - $expr->getOperatorSigil(), - ))->line($expr->getStartLine()) - ->identifier(sprintf('%s.resultUnused', $identifierType)) - ->build(), - ]; - } - - if ($expr instanceof Node\Expr\Ternary) { - $if = $expr->if; - if ($if === null) { - $if = $expr->cond; - } - - if (!$this->isNoopExpr($if) || !$this->isNoopExpr($expr->else)) { - return []; - } - - return [ - RuleErrorBuilder::message('Unused result of ternary operator.') - ->line($expr->getStartLine()) - ->identifier('ternary.resultUnused') - ->build(), - ]; - } - } if (!$this->isNoopExpr($expr)) { return []; } diff --git a/tests/PHPStan/Analyser/data/conditional-expression-infinite-loop.php b/tests/PHPStan/Analyser/data/conditional-expression-infinite-loop.php index 8500424864..9dda0d65b1 100644 --- a/tests/PHPStan/Analyser/data/conditional-expression-infinite-loop.php +++ b/tests/PHPStan/Analyser/data/conditional-expression-infinite-loop.php @@ -5,7 +5,7 @@ class test { public function test2(bool $isFoo, bool $isBar): void { - match (true) { + $a = match (true) { $isFoo && $isBar => $foo = 1, $isFoo || $isBar => $foo = 2, default => $foo = null, diff --git a/tests/PHPStan/Rules/DeadCode/BetterNoopRuleTest.php b/tests/PHPStan/Rules/DeadCode/BetterNoopRuleTest.php new file mode 100644 index 0000000000..e5103a7554 --- /dev/null +++ b/tests/PHPStan/Rules/DeadCode/BetterNoopRuleTest.php @@ -0,0 +1,134 @@ + + */ +class BetterNoopRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new BetterNoopRule(new ExprPrinter(new Printer())); + } + + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/noop.php'], [ + [ + 'Expression "$arr" on a separate line does not do anything.', + 9, + ], + [ + 'Expression "$arr[\'test\']" on a separate line does not do anything.', + 10, + ], + [ + 'Expression "$foo::$test" on a separate line does not do anything.', + 11, + ], + [ + 'Expression "$foo->test" on a separate line does not do anything.', + 12, + ], + [ + 'Expression "\'foo\'" on a separate line does not do anything.', + 14, + ], + [ + 'Expression "1" on a separate line does not do anything.', + 15, + ], + [ + 'Expression "@\'foo\'" on a separate line does not do anything.', + 17, + ], + [ + 'Expression "+1" on a separate line does not do anything.', + 18, + ], + [ + 'Expression "-1" on a separate line does not do anything.', + 19, + ], + [ + 'Expression "isset($test)" on a separate line does not do anything.', + 25, + ], + [ + 'Expression "empty($test)" on a separate line does not do anything.', + 26, + ], + [ + 'Expression "true" on a separate line does not do anything.', + 27, + ], + [ + 'Expression "\DeadCodeNoop\Foo::TEST" on a separate line does not do anything.', + 28, + ], + [ + 'Expression "(string) 1" on a separate line does not do anything.', + 30, + ], + [ + 'Unused result of "xor" operator.', + 32, + 'This operator has unexpected precedence, try disambiguating the logic with parentheses ().', + ], + [ + 'Unused result of "and" operator.', + 35, + 'This operator has unexpected precedence, try disambiguating the logic with parentheses ().', + ], + [ + 'Unused result of "or" operator.', + 38, + 'This operator has unexpected precedence, try disambiguating the logic with parentheses ().', + ], + [ + 'Unused result of ternary operator.', + 40, + ], + [ + 'Unused result of ternary operator.', + 41, + ], + [ + 'Unused result of "||" operator.', + 46, + ], + [ + 'Unused result of "&&" operator.', + 49, + ], + ]); + } + + public function testNullsafe(): void + { + $this->analyse([__DIR__ . '/data/nullsafe-property-fetch-noop.php'], [ + [ + 'Expression "$ref?->name" on a separate line does not do anything.', + 10, + ], + ]); + } + + public function testRuleImpurePoints(): void + { + $this->analyse([__DIR__ . '/data/noop-impure-points.php'], [ + [ + 'Unused result of "&&" operator.', + 10, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php b/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php index 48864332f9..edffaa5b9a 100644 --- a/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php +++ b/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php @@ -15,7 +15,7 @@ class NoopRuleTest extends RuleTestCase protected function getRule(): Rule { - return new NoopRule(new ExprPrinter(new Printer()), true); + return new NoopRule(new ExprPrinter(new Printer()), false); } public function testRule(): void @@ -77,37 +77,6 @@ public function testRule(): void 'Expression "(string) 1" on a separate line does not do anything.', 30, ], - [ - 'Unused result of "xor" operator.', - 32, - 'This operator has unexpected precedence, try disambiguating the logic with parentheses ().', - ], - [ - 'Unused result of "and" operator.', - 35, - 'This operator has unexpected precedence, try disambiguating the logic with parentheses ().', - ], - [ - 'Unused result of "or" operator.', - 38, - 'This operator has unexpected precedence, try disambiguating the logic with parentheses ().', - ], - [ - 'Unused result of ternary operator.', - 40, - ], - [ - 'Unused result of ternary operator.', - 41, - ], - [ - 'Unused result of "||" operator.', - 46, - ], - [ - 'Unused result of "&&" operator.', - 49, - ], ]); } diff --git a/tests/PHPStan/Rules/DeadCode/data/noop-impure-points.php b/tests/PHPStan/Rules/DeadCode/data/noop-impure-points.php new file mode 100644 index 0000000000..c44c018d36 --- /dev/null +++ b/tests/PHPStan/Rules/DeadCode/data/noop-impure-points.php @@ -0,0 +1,36 @@ +doBar(); + $b && $this->doBaz(); + $b && $this->doLorem(); + } + + /** + * @phpstan-pure + */ + public function doBar(): bool + { + return true; + } + + /** + * @phpstan-impure + */ + public function doBaz(): bool + { + return true; + } + + public function doLorem(): bool + { + return true; + } + +}