diff --git a/src/Analyser/ExprHandler/AssignOpHandler.php b/src/Analyser/ExprHandler/AssignOpHandler.php index 186c792b0b8..4f8e0a92bd2 100644 --- a/src/Analyser/ExprHandler/AssignOpHandler.php +++ b/src/Analyser/ExprHandler/AssignOpHandler.php @@ -139,7 +139,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex } if ($condResult !== null) { - $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??='), $beforeScope, $storage, $context); + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, $valueResult, 'on left side of ??='), $beforeScope, $storage, $context); } return $this->expressionResultFactory->create( diff --git a/src/Analyser/ExprHandler/CoalesceHandler.php b/src/Analyser/ExprHandler/CoalesceHandler.php index c0a54c5bee3..980498eafc2 100644 --- a/src/Analyser/ExprHandler/CoalesceHandler.php +++ b/src/Analyser/ExprHandler/CoalesceHandler.php @@ -136,7 +136,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $scope->filterByTruthyValue(new Expr\Isset_([$expr->left]))->mergeWith($rightResult->getScope()); } - $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??'), $beforeScope, $storage, $context); + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, $rightResult, 'on left side of ??'), $beforeScope, $storage, $context); return $this->expressionResultFactory->create( $scope, diff --git a/src/Node/CoalesceExpressionNode.php b/src/Node/CoalesceExpressionNode.php index fc5e1ff4942..0431876a21d 100644 --- a/src/Node/CoalesceExpressionNode.php +++ b/src/Node/CoalesceExpressionNode.php @@ -21,6 +21,7 @@ final class CoalesceExpressionNode extends NodeAbstract implements VirtualNode public function __construct( private Expr $originalExpr, private ExpressionResult $subjectResult, + private ExpressionResult $rightResult, private string $operatorDescription, ) { @@ -37,6 +38,12 @@ public function getSubjectResult(): ExpressionResult return $this->subjectResult; } + /** Result of the right side - the operand that's only evaluated when the left side is null. */ + public function getRightResult(): ExpressionResult + { + return $this->rightResult; + } + public function getOperatorDescription(): string { return $this->operatorDescription; diff --git a/src/Rules/Variables/NullCoalesceRule.php b/src/Rules/Variables/NullCoalesceRule.php index 9aff47daa6f..b27ed154ce2 100644 --- a/src/Rules/Variables/NullCoalesceRule.php +++ b/src/Rules/Variables/NullCoalesceRule.php @@ -3,7 +3,9 @@ namespace PHPStan\Rules\Variables; use PhpParser\Node; +use PhpParser\NodeFinder; use PHPStan\Analyser\CollectedDataEmitter; +use PHPStan\Analyser\InternalThrowPoint; use PHPStan\Analyser\NodeCallbackInvoker; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\AutowiredParameter; @@ -15,6 +17,7 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Type; +use function array_filter; use function sprintf; /** @@ -96,6 +99,20 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop return null; } + // Dropping the coalesce also drops the evaluation of the right side, which + // only happens when the left side is null. That's only observationally + // equivalent when evaluating the right side cannot do anything but produce + // its null value. + $rightResult = $node->getRightResult(); + if ( + $rightResult->getImpurePoints() !== [] + || array_filter($rightResult->getThrowPoints(), static fn (InternalThrowPoint $throwPoint): bool => $throwPoint->isExplicit()) !== [] + || $rightResult->hasYield() + || $this->containsAssign($right) + ) { + return null; + } + // The coalesce only changes the result when the left side is undefined. // If the left side is always set, `?? null` (or `??= null`) never changes // anything, so the whole coalesce is redundant. @@ -109,4 +126,19 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop )->identifier('nullCoalesce.unnecessary')->build(); } + /** + * Writes to variables, properties and offsets are not impure points, but they + * still make the right side worth keeping around. + */ + private function containsAssign(Node\Expr $expr): bool + { + return (new NodeFinder())->findFirst([$expr], static fn (Node $node): bool => $node instanceof Node\Expr\Assign + || $node instanceof Node\Expr\AssignRef + || $node instanceof Node\Expr\AssignOp + || $node instanceof Node\Expr\PostInc + || $node instanceof Node\Expr\PreInc + || $node instanceof Node\Expr\PostDec + || $node instanceof Node\Expr\PreDec) !== null; + } + } diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index 42f5d5aaa1a..304e46b4a58 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -505,6 +505,41 @@ public function testBug4337(): void ]); } + public function testBug15134(): void + { + $this->analyse([__DIR__ . '/data/bug-15134.php'], []); + } + + public function testUnnecessaryNullCoalesceSideEffects(): void + { + $this->analyse([__DIR__ . '/data/unnecessary-null-coalesce-side-effects.php'], [ + [ + 'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.', + 111, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 118, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 123, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 128, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 135, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 140, + ], + ]); + } + public function testBug12179(): void { $this->analyse([__DIR__ . '/data/bug-12179.php'], [ diff --git a/tests/PHPStan/Rules/Variables/data/bug-15134.php b/tests/PHPStan/Rules/Variables/data/bug-15134.php new file mode 100644 index 00000000000..0786f32b66f --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-15134.php @@ -0,0 +1,30 @@ += 8.2 + +declare(strict_types = 1); + +namespace Bug15134; + +use LogicException; + +interface Node {} + +class A implements Node {} + +abstract class Parser { + + protected function parseExpressionChild(bool $value): ?Node { + return $this->parseNumber($value) + ?? $this->parseSpace($value); + } + + abstract protected function parseNumber(bool $value): ?A; + + protected function parseSpace(bool $value): null { + if ($value === false) { + throw new LogicException('The string is not a mathematical expression.'); + } + + return null; + } + +} diff --git a/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php new file mode 100644 index 00000000000..5de1a355d23 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php @@ -0,0 +1,178 @@ +stringOrNull ?? returnsNull($name !== null); +} + +function methodCallOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? $foo->returnsNull(); +} + +function staticCallOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? Foo::staticReturnsNull(); +} + +function invokeOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? $foo(); +} + +function closureCallOnRightSide(Foo $foo): ?string +{ + $closure = static function () { + echo 'side effect'; + + return null; + }; + + return $foo->stringOrNull ?? $closure(); +} + +function assignOnRightSide(Foo $foo): ?string +{ + $result = $foo->stringOrNull ?? $x = null; + echo $x; + + return $result; +} + +function assignOpOnRightSide(Foo $foo, ?string $name): ?string +{ + $x = $name; + $x ??= $foo->returnsNull(); + + return $x; +} + +function assignOpPureOnRightSide(?string $name): ?string +{ + $x = $name; + $x ??= null; + + return $x; +} + +function pureFuncCallOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? pureReturnsNull(); +} + +function constantOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? NULL_CONSTANT; +} + +function classConstantOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? Foo::NULL_CONSTANT; +} + +function nullVariableOnRightSide(Foo $foo): ?string +{ + $null = null; + + return $foo->stringOrNull ?? $null; +} + +function nullPropertyOnRightSide(Foo $foo, Foo $bar): ?string +{ + return $foo->stringOrNull ?? $bar->alwaysNull; +} + +function assignRefOnRightSide(Foo $foo): ?string +{ + $null = null; + + return $foo->stringOrNull ?? $ref = &$null; +} + +/** @param array $nulls */ +function assignOpInOffsetOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[$i += 1]; +} + +/** @param array $nulls */ +function postIncOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[$i++]; +} + +/** @param array $nulls */ +function preIncOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[++$i]; +} + +/** @param array $nulls */ +function postDecOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[$i--]; +} + +/** @param array $nulls */ +function preDecOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[--$i]; +}