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..f50f818e6f3 --- /dev/null +++ b/src/Rules/Comparison/UnnecessaryTernaryOperatorRule.php @@ -0,0 +1,352 @@ + + */ +#[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 (count($conditionType->getConstantScalarValues()) === 1) { + 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 + foreach ($otherType->getConstantScalarValues() as $value) { + if (is_float($value)) { + return false; + } + } + + return true; + } + + /** + * 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); + if ($type->isObject()->no()) { + return true; + } + + return TypeCombinator::intersect($type, new ObjectWithoutClassType())->toBoolean()->isTrue()->yes(); + } + + private function containsCall(Expr $expr): bool + { + return (new NodeFinder())->findFirst([$expr], static fn (Node $node): bool => $node instanceof Expr\FuncCall + || $node instanceof Expr\MethodCall + || $node instanceof Expr\StaticCall) !== 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->isFirstClassCallable() || !$this->areArgsPure($expr, $scope)) { + return false; + } + 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; + } + + 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; + } + $className = $scope->resolveName($expr->class); + if (!$this->reflectionProvider->hasClass($className)) { + return false; + } + + 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->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 new file mode 100644 index 00000000000..5feeb1b1a04 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/UnnecessaryTernaryOperatorRuleTest.php @@ -0,0 +1,188 @@ + + */ +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.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.', + 87, + '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 + { + $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.', + 162, + ], + [ + 'Ternary operator is unnecessary because it always results in $s.', + 201, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 206, + ], + ]); + } + + #[RequiresPhp('>= 8.1.0')] + 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.', + 162, + ], + [ + 'Ternary operator is unnecessary because it always results in $s.', + 201, + ], + [ + 'Ternary operator is unnecessary because it always results in $foo.', + 206, + ], + ]); + } + + #[RequiresPhp('>= 8.1.0')] + public function testRuleDisabled(): void + { + $this->unnecessaryTernary = false; + $this->analyse([__DIR__ . '/data/unnecessary-ternary.php'], []); + } + +} 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..9e452670cb5 --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary-calls.php @@ -0,0 +1,104 @@ += 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 nullableReceiver(?Foo $foo, ?Foo $other): ?Foo + { + return $foo ?: $other->pureNull(); + } + + 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(); +} + +/** + * @param (pure-callable(): null)|null $cb + */ +function nullableCallable(?Foo $foo, $cb): ?Foo +{ + return $foo ?: $cb(); +} 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..6734681f76b --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/unnecessary-ternary.php @@ -0,0 +1,244 @@ += 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; +} + +/** @return null */ +function maybeSideEffects() +{ + return null; +} + +function callWithMaybeSideEffects(?Foo $foo): ?Foo +{ + return $foo ?: maybeSideEffects(); +} + +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; + } + +} + +class ConstantCondition +{ + + public function alwaysFalse(): int + { + // TernaryOperatorConstantConditionRule reports this + $i = 0; + return $i ?: 0; + } + +}