From 54248e03b834f9c512f0ab0fcd2e84fb5d58ef54 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Sat, 31 Oct 2026 18:49:36 +0100 Subject: [PATCH 1/6] Report ternary operators that always give one of their branches The rule is behind the unnecessaryTernary toggle, which bleeding edge turns on. It covers the short ?: form too. Co-Authored-By: Claude Opus 5.5 (1M context) --- conf/bleedingEdge.neon | 1 + conf/config.neon | 1 + conf/parametersSchema.neon | 1 + .../UnnecessaryTernaryOperatorRule.php | 292 ++++++++++++++++++ .../UnnecessaryTernaryOperatorRuleTest.php | 166 ++++++++++ .../Comparison/data/unnecessary-ternary.php | 221 +++++++++++++ 6 files changed, 682 insertions(+) create mode 100644 src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php create mode 100644 tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php create mode 100644 tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 7098877fb44..01e20e44486 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -23,6 +23,7 @@ parameters: unusedLabel: true newOnNonObject: true unnecessaryNullCoalesce: true + unnecessaryTernary: true finiteTypesInHaystack: true switchConditionAlwaysFalse: true checkImportedClassNameCase: true diff --git a/conf/config.neon b/conf/config.neon index 5174d60f881..aa85a2d050c 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -55,6 +55,7 @@ parameters: unusedLabel: false newOnNonObject: false unnecessaryNullCoalesce: false + unnecessaryTernary: false finiteTypesInHaystack: false switchConditionAlwaysFalse: false checkImportedClassNameCase: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index fbf06496f89..09a323da997 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -52,6 +52,7 @@ parametersSchema: unusedLabel: bool() newOnNonObject: bool() unnecessaryNullCoalesce: bool() + unnecessaryTernary: bool() finiteTypesInHaystack: bool() switchConditionAlwaysFalse: bool() checkImportedClassNameCase: bool() diff --git a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php new file mode 100644 index 00000000000..f16b831c052 --- /dev/null +++ b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php @@ -0,0 +1,292 @@ + + */ +#[RegisteredRule(level: 4)] +final class UnnecessaryTernaryOperatorRule implements Rule +{ + + public function __construct( + private ConstantConditionRuleHelper $helper, + private ConstantConditionInTraitHelper $constantConditionInTraitHelper, + private ReflectionProvider $reflectionProvider, + private ExprPrinter $exprPrinter, + #[AutowiredParameter(ref: '%featureToggles.unnecessaryTernary%')] + private bool $unnecessaryTernary, + #[AutowiredParameter] + private bool $treatPhpDocTypesAsCertain, + #[AutowiredParameter(ref: '%tips.treatPhpDocTypesAsCertain%')] + private bool $treatPhpDocTypesAsCertainTip, + ) + { + } + + public function getNodeType(): string + { + return Node\Expr\Ternary::class; + } + + public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array + { + if (!$this->unnecessaryTernary) { + return []; + } + + $error = $this->check($node, $scope); + if ($error === null) { + $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $node); + return []; + } + + if ($scope->isInTrait()) { + $this->constantConditionInTraitHelper->emitError(self::class, $scope, $node, true, $error); + return []; + } + + return [$error]; + } + + private function check(Node\Expr\Ternary $node, Scope $scope): ?IdentifierRuleError + { + // TernaryOperatorConstantConditionRule already reports these + $conditionType = $this->helper->getBooleanType($scope, $node->cond); + if ($conditionType->isTrue()->yes() || $conditionType->isFalse()->yes()) { + return null; + } + + $result = $this->findResult($node, $scope, !$this->treatPhpDocTypesAsCertain); + if ($result === null) { + return null; + } + + $ruleErrorBuilder = RuleErrorBuilder::message(sprintf( + $node->if === null + ? 'Short ternary operator ?: is unnecessary because it always results in %s.' + : 'Ternary operator is unnecessary because it always results in %s.', + $this->exprPrinter->printExpr($result), + ))->identifier('ternary.unnecessary'); + + if ( + $this->treatPhpDocTypesAsCertain + && $this->treatPhpDocTypesAsCertainTip + && $this->findResult($node, $scope, true) === null + ) { + $ruleErrorBuilder = $ruleErrorBuilder->treatPhpDocTypesAsCertainTip(); + } + + return $ruleErrorBuilder->build(); + } + + /** + * The branch the ternary always results in, or null. + */ + private function findResult(Node\Expr\Ternary $node, Scope $scope, bool $native): ?Expr + { + // Without the ternary, the result is evaluated every time, and the + // condition and the other branch are not evaluated at all. + if (!$this->isPure($node->else, $scope)) { + return null; + } + + if (!$this->isTruthinessNarrowedExactly($node->cond, $scope, $native)) { + return null; + } + + if ($node->if === null) { + if (!$this->isAlwaysResult($scope->filterByFalseyValue($node->cond), $node->cond, $scope, $node->else, $native)) { + return null; + } + + return $node->cond; + } + + if (!$this->isPure($node->cond, $scope) || !$this->isPure($node->if, $scope)) { + return null; + } + + // The result then also runs in the scope of the other branch, where a + // function call can get an argument it does not accept, for example + // `$s === false ? 0 : strlen($s)`. + if (!$this->containsCall($node->if) && $this->isAlwaysResult($scope->filterByFalseyValue($node->cond), $node->if, $scope, $node->else, $native)) { + return $node->if; + } + if (!$this->containsCall($node->else) && $this->isAlwaysResult($scope->filterByTruthyValue($node->cond), $node->else, $scope, $node->if, $native)) { + return $node->else; + } + + return null; + } + + /** + * Whether $result always has the single value of $other in $branchScope, + * the scope in which the ternary picks $other. + */ + private function isAlwaysResult(Scope $branchScope, Expr $result, Scope $scope, Expr $other, bool $native): bool + { + // a fresh copy, because the walked node keeps the type it had in its own branch + $result = (new NodeTraverser(new CloningVisitor()))->traverse([$result])[0]; + if (!$result instanceof Expr) { + return false; + } + + if ($native) { + $resultType = $branchScope->getNativeType($result); + $otherType = $scope->getNativeType($other); + } else { + $resultType = $branchScope->getType($result); + $otherType = $scope->getType($other); + } + + // enum cases are not constant values, but each of them is a single value + if (!$otherType->isConstantValue()->yes() && count($otherType->getFiniteTypes()) !== 1) { + return false; + } + if (!$resultType->equals($otherType)) { + return false; + } + + // 0.0 === -0.0, so `$f ?: 0.0` turns -0.0 into 0.0 + return $otherType->isFloat()->no(); + } + + /** + * Narrowing by truthiness treats every object as truthy, but an empty + * SimpleXMLElement or a zero GMP is falsy. Then the scope of the other + * branch does not hold all values that reach it. + */ + private function isTruthinessNarrowedExactly(Expr $cond, Scope $scope, bool $native): bool + { + if ($cond instanceof Expr\BooleanNot) { + return $this->isTruthinessNarrowedExactly($cond->expr, $scope, $native); + } + + if ( + $cond instanceof Expr\BinaryOp\BooleanAnd + || $cond instanceof Expr\BinaryOp\BooleanOr + || $cond instanceof Expr\BinaryOp\LogicalAnd + || $cond instanceof Expr\BinaryOp\LogicalOr + ) { + return $this->isTruthinessNarrowedExactly($cond->left, $scope, $native) && $this->isTruthinessNarrowedExactly($cond->right, $scope, $native); + } + + $type = $native ? $scope->getNativeType($cond) : $scope->getType($cond); + $objectType = new ObjectWithoutClassType(); + if ($objectType->isSuperTypeOf($type)->no()) { + return true; + } + + return TypeCombinator::intersect($type, $objectType)->toBoolean()->isTrue()->yes(); + } + + private function containsCall(Expr $expr): bool + { + return (new NodeFinder())->findFirstInstanceOf([$expr], Expr\FuncCall::class) !== null; + } + + /** + * Whether evaluating the expression can only produce its value, so that + * dropping it or evaluating it once more does not change anything. + */ + private function isPure(Expr $expr, Scope $scope): bool + { + if ($expr instanceof Expr\Variable) { + return is_string($expr->name); + } + + if ($expr instanceof Node\Scalar\Int_ || $expr instanceof Node\Scalar\Float_ || $expr instanceof Node\Scalar\String_) { + return true; + } + + if ($expr instanceof Expr\ConstFetch) { + return true; + } + + if ($expr instanceof Expr\ClassConstFetch) { + return $expr->class instanceof Node\Name && $expr->name instanceof Node\Identifier; + } + + if ($expr instanceof Expr\Array_) { + foreach ($expr->items as $item) { + if ($item->unpack || $item->byRef) { + return false; + } + if ($item->key !== null && !$this->isPure($item->key, $scope)) { + return false; + } + if (!$this->isPure($item->value, $scope)) { + return false; + } + } + + return true; + } + + if ( + $expr instanceof Expr\BinaryOp\Identical + || $expr instanceof Expr\BinaryOp\NotIdentical + || $expr instanceof Expr\BinaryOp\BooleanAnd + || $expr instanceof Expr\BinaryOp\BooleanOr + || $expr instanceof Expr\BinaryOp\LogicalAnd + || $expr instanceof Expr\BinaryOp\LogicalOr + ) { + return $this->isPure($expr->left, $scope) && $this->isPure($expr->right, $scope); + } + + if ($expr instanceof Expr\BooleanNot) { + return $this->isPure($expr->expr, $scope); + } + + if ($expr instanceof Expr\Instanceof_) { + return $expr->class instanceof Node\Name && $this->isPure($expr->expr, $scope); + } + + if ($expr instanceof Expr\FuncCall) { + if (!$expr->name instanceof Node\Name || $expr->isFirstClassCallable()) { + return false; + } + if (!$this->reflectionProvider->hasFunction($expr->name, $scope)) { + return false; + } + if (!$this->reflectionProvider->getFunction($expr->name, $scope)->hasSideEffects()->no()) { + return false; + } + foreach ($expr->getArgs() as $arg) { + if ($arg->unpack || $arg->byRef || !$this->isPure($arg->value, $scope)) { + return false; + } + } + + return true; + } + + return false; + } + +} diff --git a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php new file mode 100644 index 00000000000..1a45beaecb0 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php @@ -0,0 +1,166 @@ + + */ +class UnnecessaryTernaryOperatorRuleTest extends RuleTestCase +{ + + private bool $unnecessaryTernary = true; + + private bool $treatPhpDocTypesAsCertain = true; + + protected function getRule(): Rule + { + // @phpstan-ignore argument.type + return new CompositeRule([ + new UnnecessaryTernaryOperatorRule( + self::getContainer()->getByType(ConstantConditionRuleHelper::class), + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + self::createReflectionProvider(), + self::getContainer()->getByType(ExprPrinter::class), + $this->unnecessaryTernary, + $this->treatPhpDocTypesAsCertain, + true, + ), + new ConstantConditionInTraitRule(), + ]); + } + + protected function shouldTreatPhpDocTypesAsCertain(): bool + { + return $this->treatPhpDocTypesAsCertain; + } + + #[RequiresPhp('>= 8.1')] + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/unnecessary-ternary.php'], [ + [ + 'Ternary operator is unnecessary because it always results in $i.', + 21, + ], + [ + 'Ternary operator is unnecessary because it always results in $i.', + 31, + ], + [ + 'Ternary operator is unnecessary because it always results in $i.', + 36, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 41, + ], + [ + 'Ternary operator is unnecessary because it always results in $i.', + 46, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 54, + 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 59, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $b.', + 64, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $a.', + 73, + ], + [ + 'Ternary operator is unnecessary because it always results in $s.', + 78, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 151, + ], + [ + 'Ternary operator is unnecessary because it always results in $s.', + 190, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 195, + ], + ]); + } + + #[RequiresPhp('>= 8.1')] + public function testRuleWithoutTreatingPhpDocTypesAsCertain(): void + { + $this->treatPhpDocTypesAsCertain = false; + $this->analyse([__DIR__ . '/data/unnecessary-ternary.php'], [ + [ + 'Ternary operator is unnecessary because it always results in $i.', + 21, + ], + [ + 'Ternary operator is unnecessary because it always results in $i.', + 31, + ], + [ + 'Ternary operator is unnecessary because it always results in $i.', + 36, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 41, + ], + [ + 'Ternary operator is unnecessary because it always results in $i.', + 46, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 59, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $b.', + 64, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $a.', + 73, + ], + [ + 'Ternary operator is unnecessary because it always results in $s.', + 78, + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 151, + ], + [ + 'Ternary operator is unnecessary because it always results in $s.', + 190, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 195, + ], + ]); + } + + #[RequiresPhp('>= 8.1')] + public function testRuleDisabled(): void + { + $this->unnecessaryTernary = false; + $this->analyse([__DIR__ . '/data/unnecessary-ternary.php'], []); + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php new file mode 100644 index 00000000000..2307a0a3e11 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php @@ -0,0 +1,221 @@ += 8.1 + +namespace UnnecessaryTernary; + +class Foo +{ + +} + +enum Suit +{ + case Hearts; + case Spades; +} + +class HelloWorld +{ + + public function getIntOrNull(?int $i): ?int + { + return $i !== null ? $i : null; + } + + public function getIntOrNullCoalesce(?int $i): ?int + { + return $i ?? null; + } + + public function identical(?int $i): ?int + { + return $i === null ? null : $i; + } + + public function nullFirst(?int $i): ?int + { + return null !== $i ? $i : null; + } + + public function instanceofCheck(?Foo $foo): ?Foo + { + return $foo instanceof Foo ? $foo : null; + } + + public function isInt(?int $i): ?int + { + return is_int($i) ? $i : null; + } + + /** + * @param Foo|null $foo + */ + public function phpDoc($foo): ?Foo + { + return $foo instanceof Foo ? $foo : null; + } + + public function shortObject(?Foo $foo): ?Foo + { + return $foo ?: null; + } + + public function shortBool(bool $b): bool + { + return $b ?: false; + } + + /** + * @param array $a + * @return array + */ + public function shortArray(array $a): array + { + return $a ?: []; + } + + public function enumCase(Suit $s): Suit + { + return $s !== Suit::Hearts ? $s : Suit::Hearts; + } + + public function shortInt(?int $i): ?int + { + // falsy $i is 0|null + return $i ?: null; + } + + public function shortString(string $s): string + { + // falsy $s is ''|'0' + return $s ?: ''; + } + + public function otherValue(?int $i): ?int + { + return $i !== null ? $i : 0; + } + + public function float(float $f): float + { + return $f !== 0.0 ? $f : 0.0; + } + + public function shortFloat(?float $f): ?float + { + return $f ?: null; + } + + public function impureCondition(): ?int + { + return $this->next() !== null ? $this->next() : null; + } + + public function impureElse(?Foo $foo): ?Foo + { + return $foo ?: $this->nothing(); + } + + public function otherVariable(?int $i, ?int $j): ?int + { + return $j !== null ? $i : null; + } + + public function next(): ?int + { + return rand(0, 1) === 1 ? 1 : null; + } + + public function nothing(): ?Foo + { + echo 'x'; + return null; + } + +} + +function strlenOfFalse(string|false $blob): int +{ + return $blob === false ? 0 : strlen($blob); +} + +function strlenOfString(string|false $blob): int +{ + return $blob !== false ? strlen($blob) : 0; +} + +trait FooTrait +{ + + public function shortObject(?Foo $foo): ?Foo + { + return $foo ?: null; + } + +} + +class UsesFooTrait +{ + + use FooTrait; + +} + +class Issets +{ + + public function maybeUndefined(): ?int + { + if (rand(0, 1)) { + $a = 1; + } + + return isset($a) ? $a : null; + } + + public function property(Bar $bar): ?int + { + return $bar->p !== null ? $bar->p : null; + } + + public function sameVariables(?int $a, ?int $b): ?int + { + return $a !== $b ? $a : $b; + } + + /** + * @param 'a'|'b' $s + */ + public function literal(string $s): string + { + return $s === 'a' ? 'a' : $s; + } + + public function booleanNot(?Foo $foo): ?Foo + { + return !$foo ? null : $foo; + } + +} + +class Bar +{ + + public ?int $p = null; + +} + +class FalsyObjects +{ + + public function simpleXml(?\SimpleXMLElement $xml): ?\SimpleXMLElement + { + // an empty SimpleXMLElement is falsy + return $xml ?: null; + } + + public function simpleXmlNot(?\SimpleXMLElement $xml): ?\SimpleXMLElement + { + return !$xml ? null : $xml; + } + +} From aa05d36264f413be051f8bb8fb06db948ae856b1 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Sat, 31 Oct 2026 20:45:34 +0100 Subject: [PATCH 2/6] Use a full version in the RequiresPhp attributes Co-Authored-By: Claude Opus 5.5 (1M context) --- .../Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php index 1a45beaecb0..3bfcb595120 100644 --- a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php @@ -40,7 +40,7 @@ protected function shouldTreatPhpDocTypesAsCertain(): bool return $this->treatPhpDocTypesAsCertain; } - #[RequiresPhp('>= 8.1')] + #[RequiresPhp('>= 8.1.0')] public function testRule(): void { $this->analyse([__DIR__ . '/data/unnecessary-ternary.php'], [ @@ -100,7 +100,7 @@ public function testRule(): void ]); } - #[RequiresPhp('>= 8.1')] + #[RequiresPhp('>= 8.1.0')] public function testRuleWithoutTreatingPhpDocTypesAsCertain(): void { $this->treatPhpDocTypesAsCertain = false; @@ -156,7 +156,7 @@ public function testRuleWithoutTreatingPhpDocTypesAsCertain(): void ]); } - #[RequiresPhp('>= 8.1')] + #[RequiresPhp('>= 8.1.0')] public function testRuleDisabled(): void { $this->unnecessaryTernary = false; From 5cb3ce4beb087b3dfd628e0fa0ed1d330edd15b0 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Sat, 31 Oct 2026 21:03:28 +0100 Subject: [PATCH 3/6] Test the constant condition and side effect checks of the ternary rule The constant condition and float checks now read the constant values, and the object check uses isObject(), so a mutant of them changes the result. New test cases cover a constant condition and a function that can have side effects. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../UnnecessaryTernaryOperatorRule.php | 16 +++++++++---- .../UnnecessaryTernaryOperatorRuleTest.php | 12 +++++----- .../Comparison/data/unnecessary-ternary.php | 23 +++++++++++++++++++ 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php index f16b831c052..6f2a4e69a1f 100644 --- a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php +++ b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php @@ -20,6 +20,7 @@ use PHPStan\Type\ObjectWithoutClassType; use PHPStan\Type\TypeCombinator; use function count; +use function is_float; use function is_string; use function sprintf; @@ -77,7 +78,7 @@ private function check(Node\Expr\Ternary $node, Scope $scope): ?IdentifierRuleEr { // TernaryOperatorConstantConditionRule already reports these $conditionType = $this->helper->getBooleanType($scope, $node->cond); - if ($conditionType->isTrue()->yes() || $conditionType->isFalse()->yes()) { + if (count($conditionType->getConstantScalarValues()) === 1) { return null; } @@ -173,7 +174,13 @@ private function isAlwaysResult(Scope $branchScope, Expr $result, Scope $scope, } // 0.0 === -0.0, so `$f ?: 0.0` turns -0.0 into 0.0 - return $otherType->isFloat()->no(); + foreach ($otherType->getConstantScalarValues() as $value) { + if (is_float($value)) { + return false; + } + } + + return true; } /** @@ -197,12 +204,11 @@ private function isTruthinessNarrowedExactly(Expr $cond, Scope $scope, bool $nat } $type = $native ? $scope->getNativeType($cond) : $scope->getType($cond); - $objectType = new ObjectWithoutClassType(); - if ($objectType->isSuperTypeOf($type)->no()) { + if ($type->isObject()->no()) { return true; } - return TypeCombinator::intersect($type, $objectType)->toBoolean()->isTrue()->yes(); + return TypeCombinator::intersect($type, new ObjectWithoutClassType())->toBoolean()->isTrue()->yes(); } private function containsCall(Expr $expr): bool diff --git a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php index 3bfcb595120..45f16eb2654 100644 --- a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php @@ -87,15 +87,15 @@ public function testRule(): void ], [ 'Short ternary operator ?: is unnecessary because it always results in $foo.', - 151, + 162, ], [ 'Ternary operator is unnecessary because it always results in $s.', - 190, + 201, ], [ 'Ternary operator is unnecessary because it always results in $foo.', - 195, + 206, ], ]); } @@ -143,15 +143,15 @@ public function testRuleWithoutTreatingPhpDocTypesAsCertain(): void ], [ 'Short ternary operator ?: is unnecessary because it always results in $foo.', - 151, + 162, ], [ 'Ternary operator is unnecessary because it always results in $s.', - 190, + 201, ], [ 'Ternary operator is unnecessary because it always results in $foo.', - 195, + 206, ], ]); } diff --git a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php index 2307a0a3e11..6734681f76b 100644 --- a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php @@ -143,6 +143,17 @@ function strlenOfString(string|false $blob): int return $blob !== false ? strlen($blob) : 0; } +/** @return null */ +function maybeSideEffects() +{ + return null; +} + +function callWithMaybeSideEffects(?Foo $foo): ?Foo +{ + return $foo ?: maybeSideEffects(); +} + trait FooTrait { @@ -219,3 +230,15 @@ public function simpleXmlNot(?\SimpleXMLElement $xml): ?\SimpleXMLElement } } + +class ConstantCondition +{ + + public function alwaysFalse(): int + { + // TernaryOperatorConstantConditionRule reports this + $i = 0; + return $i ?: 0; + } + +} From 65ad8b0e980ecec8639f94271801ba2137e4ef0b Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Tue, 3 Nov 2026 21:52:19 +0100 Subject: [PATCH 4/6] Treat pure method calls, static calls and callables as pure in the ternary rule The rule only accepted function calls without side effects, so it did not report a ternary with a pure method, static method or callable call. These calls also count for the check that a result with a call is not evaluated in the scope of the other branch. Co-Authored-By: Claude Opus 5.5 --- .../UnnecessaryTernaryOperatorRule.php | 72 +++++++++++++-- .../UnnecessaryTernaryOperatorRuleTest.php | 22 +++++ .../data/unnecessary-ternary-calls.php | 91 +++++++++++++++++++ 3 files changed, 176 insertions(+), 9 deletions(-) create mode 100644 tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php diff --git a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php index 6f2a4e69a1f..5d434f52c6c 100644 --- a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php +++ b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php @@ -17,7 +17,9 @@ use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; +use PHPStan\Type\ObjectType; use PHPStan\Type\ObjectWithoutClassType; +use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; use function count; use function is_float; @@ -213,7 +215,9 @@ private function isTruthinessNarrowedExactly(Expr $cond, Scope $scope, bool $nat private function containsCall(Expr $expr): bool { - return (new NodeFinder())->findFirstInstanceOf([$expr], Expr\FuncCall::class) !== null; + return (new NodeFinder())->findFirst([$expr], static fn (Node $node): bool => $node instanceof Expr\FuncCall + || $node instanceof Expr\MethodCall + || $node instanceof Expr\StaticCall) !== null; } /** @@ -274,25 +278,75 @@ private function isPure(Expr $expr, Scope $scope): bool } if ($expr instanceof Expr\FuncCall) { - if (!$expr->name instanceof Node\Name || $expr->isFirstClassCallable()) { + if ($expr->isFirstClassCallable() || !$this->areArgsPure($expr, $scope)) { return false; } - if (!$this->reflectionProvider->hasFunction($expr->name, $scope)) { + if ($expr->name instanceof Node\Name) { + return $this->reflectionProvider->hasFunction($expr->name, $scope) + && $this->reflectionProvider->getFunction($expr->name, $scope)->hasSideEffects()->no(); + } + + return $this->isPure($expr->name, $scope) && $this->isPureCallable($scope->getType($expr->name), $scope); + } + + if ($expr instanceof Expr\MethodCall) { + if (!$expr->name instanceof Node\Identifier || $expr->isFirstClassCallable() || !$this->isPure($expr->var, $scope) || !$this->areArgsPure($expr, $scope)) { return false; } - if (!$this->reflectionProvider->getFunction($expr->name, $scope)->hasSideEffects()->no()) { + + return $this->isPureMethod($scope->getType($expr->var), $expr->name->toString(), $scope); + } + + if ($expr instanceof Expr\StaticCall) { + if (!$expr->class instanceof Node\Name || !$expr->name instanceof Node\Identifier || $expr->isFirstClassCallable() || !$this->areArgsPure($expr, $scope)) { return false; } - foreach ($expr->getArgs() as $arg) { - if ($arg->unpack || $arg->byRef || !$this->isPure($arg->value, $scope)) { - return false; - } + $className = $scope->resolveName($expr->class); + if (!$this->reflectionProvider->hasClass($className)) { + return false; } - return true; + return $this->isPureMethod(new ObjectType($className), $expr->name->toString(), $scope); } return false; } + private function areArgsPure(Expr\CallLike $call, Scope $scope): bool + { + foreach ($call->getArgs() as $arg) { + if ($arg->unpack || $arg->byRef || !$this->isPure($arg->value, $scope)) { + return false; + } + } + + return true; + } + + private function isPureMethod(Type $calledOnType, string $methodName, Scope $scope): bool + { + if (!$calledOnType->canCallMethods()->yes() || !$calledOnType->hasMethod($methodName)->yes()) { + return false; + } + + $method = $scope->getMethodReflection($calledOnType, $methodName); + + return $method !== null && $method->hasSideEffects()->no(); + } + + private function isPureCallable(Type $type, Scope $scope): bool + { + if (!$type->isCallable()->yes()) { + return false; + } + + foreach ($type->getCallableParametersAcceptors($scope) as $acceptor) { + if (!$acceptor->isPure()->yes()) { + return false; + } + } + + return true; + } + } diff --git a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php index 45f16eb2654..14aaf149588 100644 --- a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php @@ -40,6 +40,28 @@ protected function shouldTreatPhpDocTypesAsCertain(): bool return $this->treatPhpDocTypesAsCertain; } + #[RequiresPhp('>= 8.0.0')] + public function testCalls(): void + { + $this->analyse([__DIR__ . '/data/unnecessary-ternary-calls.php'], [ + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 43, + 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 48, + 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + ], + [ + 'Short ternary operator ?: is unnecessary because it always results in $foo.', + 82, + 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', + ], + ]); + } + #[RequiresPhp('>= 8.1.0')] public function testRule(): void { diff --git a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php new file mode 100644 index 00000000000..d02c18c6095 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php @@ -0,0 +1,91 @@ += 8.0 + +namespace UnnecessaryTernaryCalls; + +class Foo +{ + + /** + * @phpstan-pure + * @return null + */ + public function pureNull() + { + return null; + } + + /** + * @phpstan-pure + * @return null + */ + public static function staticPureNull() + { + return null; + } + + /** @return null */ + public function maybeImpureNull() + { + return null; + } + + /** + * @phpstan-pure + * @return ($s is string ? int<1, max> : 0) + */ + public function size(string $s): int + { + return strlen($s) + 1; + } + + public function pureMethod(?Foo $foo): ?Foo + { + return $foo ?: $this->pureNull(); + } + + public function pureStaticMethod(?Foo $foo): ?Foo + { + return $foo ?: self::staticPureNull(); + } + + public function maybeImpureMethod(?Foo $foo): ?Foo + { + return $foo ?: $this->maybeImpureNull(); + } + + public function sizeOfFalse(string|false $s): int + { + return $s === false ? 0 : $this->size($s); + } + + public static function staticSizeOfFalse(string|false $s): int + { + return $s === false ? 0 : self::staticSize($s); + } + + /** + * @phpstan-pure + * @return ($s is string ? int<1, max> : 0) + */ + public static function staticSize(string $s): int + { + return strlen($s) + 1; + } + +} + +/** + * @param pure-callable(): null $cb + */ +function pureCallable(?Foo $foo, callable $cb): ?Foo +{ + return $foo ?: $cb(); +} + +/** + * @param callable(): null $cb + */ +function impureCallable(?Foo $foo, callable $cb): ?Foo +{ + return $foo ?: $cb(); +} From 808193296b4ecaba31641bfcf2123a53a08c6bc5 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Tue, 3 Nov 2026 22:13:15 +0100 Subject: [PATCH 5/6] Test a method call on a nullable receiver in the ternary rule hasMethod() is only yes when every type of the receiver has the method, so the canCallMethods() check before it changed nothing. Co-Authored-By: Claude Opus 5.5 --- src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php | 2 +- .../Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php | 2 +- .../Rules/Comparison/data/unnecessary-ternary-calls.php | 5 +++++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php index 5d434f52c6c..f50f818e6f3 100644 --- a/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php +++ b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php @@ -325,7 +325,7 @@ private function areArgsPure(Expr\CallLike $call, Scope $scope): bool private function isPureMethod(Type $calledOnType, string $methodName, Scope $scope): bool { - if (!$calledOnType->canCallMethods()->yes() || !$calledOnType->hasMethod($methodName)->yes()) { + if (!$calledOnType->hasMethod($methodName)->yes()) { return false; } diff --git a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php index 14aaf149588..5feeb1b1a04 100644 --- a/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php @@ -56,7 +56,7 @@ public function testCalls(): void ], [ 'Short ternary operator ?: is unnecessary because it always results in $foo.', - 82, + 87, 'Because the type is coming from a PHPDoc, you can turn off this check by setting treatPhpDocTypesAsCertain: false in your %configurationFile%.', ], ]); diff --git a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php index d02c18c6095..1ea71a3acce 100644 --- a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php @@ -53,6 +53,11 @@ public function maybeImpureMethod(?Foo $foo): ?Foo return $foo ?: $this->maybeImpureNull(); } + public function nullableReceiver(?Foo $foo, ?Foo $other): ?Foo + { + return $foo ?: $other->pureNull(); + } + public function sizeOfFalse(string|false $s): int { return $s === false ? 0 : $this->size($s); From 902f1fba078155c5f62cba56f43c7a80a0f33e44 Mon Sep 17 00:00:00 2001 From: Sander Muller Date: Tue, 3 Nov 2026 22:35:46 +0100 Subject: [PATCH 6/6] Test a callable that can be null in the ternary rule Co-Authored-By: Claude Opus 5.5 --- .../Rules/Comparison/data/unnecessary-ternary-calls.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php index 1ea71a3acce..9e452670cb5 100644 --- a/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php @@ -94,3 +94,11 @@ function impureCallable(?Foo $foo, callable $cb): ?Foo { return $foo ?: $cb(); } + +/** + * @param (pure-callable(): null)|null $cb + */ +function nullableCallable(?Foo $foo, $cb): ?Foo +{ + return $foo ?: $cb(); +}