Skip to content

Commit

Permalink
Bleeding edge - NoopRule - take advantage of impure points
Browse files Browse the repository at this point in the history
  • Loading branch information
ondrejmirtes committed Apr 3, 2024
1 parent d7579c4 commit a647052
Show file tree
Hide file tree
Showing 13 changed files with 360 additions and 101 deletions.
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ parameters:
alwaysCheckTooWideReturnTypeFinalMethods: true
duplicateStubs: true
logicalXor: true
betterNoop: true
invarianceComposition: true
alwaysTrueAlwaysReported: true
disableUnreachableBranchesRules: true
Expand Down
7 changes: 6 additions & 1 deletion conf/config.level4.neon
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -74,7 +76,7 @@ services:
-
class: PHPStan\Rules\DeadCode\NoopRule
arguments:
logicalXor: %featureToggles.logicalXor%
better: %featureToggles.betterNoop%
tags:
- phpstan.rules.rule

Expand Down Expand Up @@ -142,6 +144,9 @@ services:
treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain%
reportAlwaysTrueInLastCondition: %reportAlwaysTrueInLastCondition%

-
class: PHPStan\Rules\DeadCode\BetterNoopRule

-
class: PHPStan\Rules\Comparison\MatchExpressionRule
arguments:
Expand Down
1 change: 1 addition & 0 deletions conf/config.neon
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ parameters:
alwaysCheckTooWideReturnTypeFinalMethods: false
duplicateStubs: false
logicalXor: false
betterNoop: false
invarianceComposition: false
alwaysTrueAlwaysReported: false
disableUnreachableBranchesRules: false
Expand Down
1 change: 1 addition & 0 deletions conf/parametersSchema.neon
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ parametersSchema:
alwaysCheckTooWideReturnTypeFinalMethods: bool()
duplicateStubs: bool()
logicalXor: bool()
betterNoop: bool()
invarianceComposition: bool()
alwaysTrueAlwaysReported: bool()
disableUnreachableBranchesRules: bool()
Expand Down
2 changes: 1 addition & 1 deletion src/Analyser/ImpurePoint.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 29 additions & 2 deletions src/Analyser/NodeScopeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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());
Expand Down
34 changes: 34 additions & 0 deletions src/Node/NoopExpressionNode.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php declare(strict_types = 1);

namespace PHPStan\Node;

use PhpParser\Node\Expr;
use PhpParser\NodeAbstract;

class NoopExpressionNode extends NodeAbstract implements VirtualNode
{

public function __construct(private Expr $originalExpr)
{
parent::__construct($this->originalExpr->getAttributes());
}

public function getOriginalExpr(): Expr
{
return $this->originalExpr;
}

public function getType(): string
{
return 'PHPStan_Node_NoopExpressionNode';
}

/**
* @return string[]
*/
public function getSubNodeNames(): array
{
return [];
}

}
110 changes: 110 additions & 0 deletions src/Rules/DeadCode/BetterNoopRule.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\DeadCode;

use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Node\NoopExpressionNode;
use PHPStan\Node\Printer\ExprPrinter;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use function sprintf;

/**
* @implements Rule<NoopExpressionNode>
*/
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(),
];
}

}
69 changes: 5 additions & 64 deletions src/Rules/DeadCode/NoopRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}

Expand All @@ -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 (
Expand All @@ -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 [];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit a647052

Please sign in to comment.