Emit SwitchConditionNode and report always-false switch case comparisons#5854
Emit SwitchConditionNode and report always-false switch case comparisons#5854phpstan-bot wants to merge 3 commits into
Conversation
- Add `SwitchConditionNode` virtual node, emitted by `NodeScopeResolver` for every non-default `case`, pairing the switch subject with the case condition. - Add `SwitchConditionRule` (level 4, gated behind the bleeding-edge `switchConditionAlwaysFalse` feature toggle) which inspects the loose `==` comparison the `switch` performs in the scope captured at the case condition. That scope already excludes the values matched by earlier (terminating) cases, so the rule reports type-incompatible cases (phpstan/phpstan#712), exhausted unions/enums, integer-range/literal cases and duplicate int/enum/bool cases as `switch.alwaysFalse`. This mirrors how `MatchExpressionRule` reports `match.alwaysFalse`. - Fix `InitializerExprTypeResolver::resolveEqualType()` to return `false` when either operand is `NeverType`, mirroring the existing handling in `resolveIdenticalType()`. Without this, `never == X` returned `bool`, so exhausted switch subjects (narrowed to `never`) were not detected. - Update `nsrt/bcmath-number.php` expectations: `never == X` / `never != X` are now `false` / `true` (previously the inconsistent `bool`), matching `===`/`!==`.
| if (!$this->isConstantBoolean($conditionType)) { | ||
| $this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $conditionExpr); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
this rule is doing some trait magic, and it seems to miss tests for it
There was a problem hiding this comment.
I've addressed the reviewer's comment. The SwitchConditionRule delegated in-trait handling to ConstantConditionInTraitHelper (emitting collected data instead of reporting directly when $scope->isInTrait()), but that path had no test.
What I did, mirroring MatchExpressionRuleTest:
-
tests/PHPStan/Rules/Comparison/SwitchConditionRuleTest.php— wrapped the rule inCompositeRuletogether withConstantConditionInTraitRule(theCollectedDataNoderule that consumes the emitted trait data), and added atestInTrait()method. -
tests/PHPStan/Rules/Comparison/data/switch-condition-in-trait.php(new) — aFooTraitused by two classes:doFoo()—case $this->doBar()wheredoBar()returnsfalsein one class butboolin another. The case is always-false in only one context, so it's correctly not reported.doFoo2()—case $this->doBar2()wheredoBar2()returnsfalsein both classes, so it's always-false everywhere and is reported once at thecaseline.
This exercises both branches of the trait magic: the emitNoError suppression (inconsistent across contexts) and the emitError dedup-and-report-once path. Tests pass (5 tests, 5 assertions) and make phpstan is clean. The commit has been pushed.
VincentLanglet
left a comment
There was a problem hiding this comment.
To be consistent with MatchExpressionRule I think the SwitchConditionRule should also report always-true
Mirror MatchExpressionRule's trait handling test: combine the rule with ConstantConditionInTraitRule via CompositeRule and assert that an always-false switch case inside a trait is reported once when constant in every using class, while a case that is constant in only some contexts is not reported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirror MatchExpressionRule by also reporting switch cases whose loose `==` comparison against the subject can never be false. A case is flagged `switch.alwaysTrue` when the subject is narrowed (by earlier terminating cases) to exactly match it, unless it is the last case of the switch — in which case the always-true comparison makes nothing unreachable. As with match, once a case is always-true the subsequent (now dead) cases are suppressed instead of being reported as always-false. To track this across cases, SwitchConditionNode is now emitted once per switch carrying every non-default case together with the scope captured at that case (like MatchExpressionNode), and SwitchConditionRule iterates them with cross-case dead-case tracking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done. Here's a summary of how I addressed VincentLanglet's review ("To be consistent with MatchExpressionRule I think the SwitchConditionRule should also report always-true"). What changed
To get true consistency I had to address a structural difference. Reporting always-true correctly requires cross-case state (when a case is always-true, the cases below it become dead and must be suppressed rather than reported as always-false — exactly what
A welcome side effect: reading each case's scope at the settled point (like match does) produces more precise subject types in messages (e.g. Behavior, verified against matchFor the idiomatic enum- Tests
|
|
We need to think about what to do about those "always true" last cases (or last before default). Match expression is supposed to be exhaustive. We're steering people away from having Switch statements do not have to be exhaustive. We should not steer people away from having Should the new rule for switch statement follow https://phpstan.org/config-reference#reportalwaystrueinlastcondition? And consider even the last non-default case before |
I'm fine with this strategy ; we could still change later. |
VincentLanglet
left a comment
There was a problem hiding this comment.
Do not report always-true error if the only remaining case is the default one.
(Basically a case which is only followed by default could be considered as last too)
Summary
PHPStan already reports always-false
matcharm comparisons (match.alwaysFalse) by tracking types inScope, but had no equivalent forswitch. This adds that diagnostic forswitchstatements: acasewhose loose==comparison against the switch subject can never be true is now reported asswitch.alwaysFalse.This catches type-incompatible cases (e.g.
switch ($int) { case 'foo': }— phpstan/phpstan#712), cases on already-exhausted subjects (unions/enums narrowed away by earlier cases), and duplicateint/enum/boolcases. As withmatch, generalstringsubjects that PHPStan cannot narrow are intentionally not flagged.Changes
src/Node/SwitchConditionNode.php(new): virtual node pairing the switch subject with a case condition.src/Analyser/NodeScopeResolver.php: emit aSwitchConditionNodefor every non-defaultcase, using the scope captured right after the case condition is processed (which already excludes values matched by earlier terminating cases).src/Rules/Comparison/SwitchConditionRule.php(new): builds the looseEqual(subject, caseCondition)comparison and reportsswitch.alwaysFalsewhen it is constantlyfalse. MirrorsMatchExpressionRule's native-type /treatPhpDocTypesAsCertain, boolean-subject and in-trait handling.conf/config.neon,conf/parametersSchema.neon,conf/bleedingEdge.neon,conf/config.level4.neon: newswitchConditionAlwaysFalsefeature toggle (off by default, on under bleeding edge), registering the rule at level 4.src/Reflection/InitializerExprTypeResolver.php:resolveEqualType()now returnsfalsewhen either operand isNeverType, mirroringresolveIdenticalType().Root cause
switchhad no type-based always-false detection at all — only the never-merged syntacticDuplicateCasesInSwitchRule(#5849) approached the problem, and only for syntactic duplicates. The scope-tracking approach reuses the narrowing PHPStan already performs between cases.The accompanying type-system bug is an instance of "the same bug in adjacent code":
resolveIdenticalType()short-circuitedNeverTypeoperands tofalse, but its siblingresolveEqualType()did not and fell through tolooseCompare(), yieldingbool. As a result a switch subject narrowed tonever(all cases exhausted) producednever == X === bool, hiding the always-false case. FixingresolveEqualType()makes loose and strict comparison consistent fornever.Test
tests/PHPStan/Rules/Comparison/SwitchConditionRuleTest.phpwith data files:switch-condition-always-false.php— the samples from phpstan-src#5849, asserting the different errors the scope-based approach produces (duplicateintcase, duplicatetrue/nullonmixed).switch-condition-always-false-enum.php— duplicate enum case.switch-condition-always-false-impossible.php— type mismatch (More precise dirname signature #712), exhausted literal-int / string unions, integer ranges, and a non-constant case on an exhausted (never) subject that specifically exercises theresolveEqualTypefix.switch-condition-always-false-native.php—treatPhpDocTypesAsCertain = falsemode.tests/PHPStan/Analyser/nsrt/bcmath-number.phpupdated:never == X/never != Xnow inferfalse/true, locking in theresolveEqualTypefix.Fixes phpstan/phpstan#14815