Repository navigation
Report ternary operators that always give one of their branches - #6894
SanderMuller wants to merge 6 commits into
Conversation
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) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
453d403 to
5cb3ce4
Compare
| * 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 |
There was a problem hiding this comment.
what I don't like about the PR is
- isPure() function impl / don't we have this logic somewhere else already?
- use of NodeTraverser which can be slow
There was a problem hiding this comment.
The engine already knows this. The cond, if and else results have impure points, but a rule on Ternary cannot read them. Since #6418, NullCoalesceRule reads them from CoalesceExpressionNode, which CoalesceHandler emits after it processes both sides. To do the same here, TernaryHandler has to emit a new virtual node, and Turbo shadows it, so it also needs a C++ port.
Impure points alone are not enough. A possibly undefined offset and a property fetch on a nullable have none, but both give a warning. So the rule would still need some checks on top, like NullCoalesceRule has.
About the speed: on self-analysis the rule checked 2023 ternaries in 66 ms in total, out of 141 s of CPU. The clone ran 500 times and took 5.6 ms of that.
| 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; | ||
| } |
There was a problem hiding this comment.
do we miss handling for methods?
pure callables?
There was a problem hiding this comment.
Right, method calls, static calls and pure callables are not reported now. The rule skips them, so this is a missing report, not a wrong one.
The impure points from a virtual node would cover them. I checked this with NullCoalesceRule, which reports a pure method, a pure static method and a pure-callable on its right side. Do you want me to switch to that, with the C++ port? The smaller option is to add MethodCall and StaticCall to isPure() through hasSideEffects().
There was a problem hiding this comment.
You're right, the rule now treats pure method calls, static calls and pure callables as pure. See 65ad8b0.
…rnary 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds a bleeding edge rule for a ternary that always gives one of its branches, including the short
?:form.It stays quiet when:
===,!==,!,&&,||,instanceofand calls to functions without side effects count as pure. Property fetches do not, because of__getand hooks.$s === false ? 0 : strlen($s). Self-analysis found this one.-0.0 ?: 0.0gives0.0.SimpleXMLElement. The rule usestoBoolean()for this, so?Countable ?: nullis still reported untiltoBoolean()returnsboolfor these interfaces. ReturnboolfromtoBoolean()ofobject,object{}and interfaces thatSimpleXMLElement,GMPorBcMath\Numbercan implement #6892 proposes that.On a 4.5k-file real-world project it reports one true positive, with no measurable CPU cost (113.9 s on, 113.1 s off, medians of 3).
Closes phpstan/phpstan#15608
🤖 Generated with Claude Code